-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add mustache templating for agent prompts #1249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Test & Lint
Tests: 661 passed, 11 skipped |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for wiring in the Mustache helper and the associated tests—those pieces look solid. I did spot one blocker on the agent side: the new tools context is built via runtime duck-typing and silent fallbacks, which violates the FunctionTool invariants and can hide real issues. Please build that context directly from the typed FunctionTool surface instead, and keep the mustache render result typed as a string. Once that’s cleaned up I think we’ll be good to go.
| source: ToolSource; | ||
| }; | ||
|
|
||
| const extractToolPrompt = (tool: unknown): { name: string; prompt: string } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] This helper is doing runtime duck-typing on the tools and silently falls back to empty strings. Inside we only deal with real instances, which already expose , , and with strong typing. If any of those invariants are broken we need to surface it, not hide it in the rendered system prompt. Please drop the defensive checks and build the context straight from the API (e.g. ).
| source: ToolSource; | ||
| }; | ||
|
|
||
| const extractToolPrompt = (tool: unknown): { name: string; prompt: string } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] This helper is doing runtime duck-typing on the tools and silently falls back to empty strings. Inside AgentNode we only deal with real FunctionTool instances, which already expose name, description, and definition() with strong typing. If any of those invariants are broken we need to surface it, not hide it in the rendered system prompt. Please drop the defensive checks and build the context straight from the FunctionTool API (e.g. tools.map(tool => ({ name: tool.name, prompt: tool.description }))).
| for (const tool of tools as ReadonlyArray<unknown>) { | ||
| toolContext.push(extractToolPrompt(tool)); | ||
| } | ||
| const renderedPrompt: unknown = renderMustache(systemTemplate, { tools: toolContext }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] renderMustache already returns a string. Converting it to unknown and then back to string just obscures the types and would hide regressions if the helper ever changed. Please keep the return value typed as a string (e.g. const systemPrompt = renderMustache(...)).
Update
Test & Lint
Tests: 661 passed, 11 skipped |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the tooling context issue. The agent now builds the Mustache data directly from the typed surface, and the render helper stays strongly typed. Tests and helpers look good—approving.
Update
Test & Lint
Tests: 375 passed |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the UI work—love the prompt preview idea. Right now the preview never receives any agent context because doesn’t pass the new , , or props into . That means always ends up empty and the Mustache render is effectively no-op. Please wire the selected node data from down so the preview reflects the actual graph; once that’s in place I’ll take another look.
| ); | ||
|
|
||
| const agentsContext = useMemo<ManageAgentContext[]>(() => { | ||
| const currentNodeId = typeof nodeId === 'string' && nodeId.length > 0 ? nodeId : null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] This preview logic depends on nodeId, graphNodes, and graphEdges, but in the real app we never pass those props. GraphLayout still renders <NodePropertiesSidebar … /> without any of the new fields, so at runtime currentNodeId is always null. The result is that the preview will always be rendered with an empty agents list, which means the UI change does not actually deliver the templating context. Please wire the selected node’s id plus the graph nodes/edges into NodePropertiesSidebar (and down into this view) so the preview matches the live graph.
Update
Test & Lint
Tests: 375 passed |
Update
Test & Lint
Tests: 376 passed |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preview refactor looks good. Optional keeps the markdown components backward compatible, and both the split/preview tabs now render the transformed content while the editor stays on the raw template. Manage tool wiring still feeds from the graph, and the updated tests cover both the fullscreen preview transform and the manage tool scenario. Approving.
Test & Lint
✅ 377 tests passed (0 failed / 0 skipped) |
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. The system prompt preview now builds the context straight from graph connections (with sane fallbacks for tool names/descriptions), still behaves when no tools are present, and only the preview tab sees the rendered output—the editor keeps the raw template. Tests cover the agent scenario end-to-end. Approving.
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for expanding the prompt metadata support. One blocker: the Agent system-prompt preview builds its context using different fallbacks than the runtime. On the server we return , but the UI derives these fields from node titles/canonical names. That means the editor preview can diverge from what the agent actually receives. Please mirror the server fallback order so the preview stays faithful.
|
|
||
| const targetNode = nodesById.get(targetId); | ||
| if (!targetNode || targetNode.kind !== 'Tool') continue; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] The client-side context needs to match the runtime shape. On the server we now build each entry as . Here we’re deriving , , , and using different fallbacks (config name → canonical name → template title, etc.), so the preview can diverge from what the agent actually receives. Please mirror the exact server fallback order—use (from the FunctionTool) for , fall back to the same template metadata for , and ensure / use the same config/description chain. Otherwise the preview will show values that never appear at runtime.
Test & Lint Summary
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for aligning the preview logic. The agent UI now uses the same fallbacks as the runtime, so preview and live contexts stay in sync. Everything else (prompt fields, template fallbacks, tests) continues to look good. Approving.
Local QA
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tightening up the nested prompt resolution—the cycle handling and shared resolver look solid. One regression left: the Manage tool’s function metadata no longer renders the configured prompt, so the LLM never sees the template output. Please wire the getter back through so still surfaces in the tool description. Once that’s restored we should be good.
| get schema() { | ||
| return ManageInvocationSchema; | ||
| } | ||
| get description() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] We still need to surface the Manage prompt to the LLM. In the previous revision this getter rendered the configured template ( / agent context). Now it always returns , so any prompt is ignored at runtime—the parent agent will only ever see the static fallback string. The new helper already handles memoization and cycle detection; please delegate to it here (e.g. ) so config.prompt continues to work.
| get schema() { | ||
| return ManageInvocationSchema; | ||
| } | ||
| get description() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] We still need to surface the Manage prompt to the LLM. In the previous revision this getter rendered the configured template ( / agent context). Now it always returns , so any prompt is ignored at runtime—the parent agent will only ever see the static fallback string. The new helper already handles memoization and cycle detection; please delegate to it here so continues to work.
| get schema() { | ||
| return ManageInvocationSchema; | ||
| } | ||
| get description() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[major] We still need to surface the Manage prompt to the LLM. In the previous revision this getter rendered the configured template (renderMustache + agent context). Now it always returns getFallbackDescription(), so any { agents } prompt is ignored at runtime—the parent agent will only ever see the static fallback string. The new ManageToolNode.resolvePrompt() helper already handles memoization and cycle detection; please delegate to it here so config.prompt continues to work.
|
Updates after prompt resolution fix:
|
noa-lucent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick follow-up. now delegates to , so the prompt flows back to the LLM while keeping the memoized cycle-safe resolver in play. Updated tests cover the templated and fallback paths. With that blocker cleared and the rest of the prompt plumbing still solid, I’m good to approve.
Summary
Testing
Resolves #1248