-
Notifications
You must be signed in to change notification settings - Fork 69
fix: inject deferred MCP tools into system prompt and lift selectSearch cap #343
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -116,6 +116,7 @@ func (a *App) ensureAgent() (*agent.Agent, error) { | |
| agent.WithHookRunner(a.hookRunner), | ||
| agent.WithExtraSystemBlocks(pluginBlocks...), | ||
| agent.WithDynamicSystemBlocksForTurn(a.workflowDynamicSystemBlock), | ||
| agent.WithDynamicSystemBlocks(func() string { return a.renderDeferredToolsBlock() }), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WithDynamicSystemBlocks replaces a.dynamicSystemBlocks instead of appending to it, so this call drops the workflowDynamicSystemBlock registered on the previous line. That removes the workflow runtime guidance, authoring rules, and prompt catalog from every turn. I reproduced this with an option-composition test: the deferred-tools block remains, but the workflow block is missing. Could we register both renderers in a single WithDynamicSystemBlocksForTurn call, or otherwise make the option composition append safely, and add a regression test that asserts both blocks are present? |
||
| agent.WithProjectMemory(a.cfg.MemoryEnabled, a.cfg.MemoryMaxChars, parseCSVList(a.cfg.MemoryFileOrder), a.workspaceRoot), | ||
| agent.WithWorktreeContext(a.worktree.Path, a.worktree.OriginalWorkspace), | ||
| agent.WithMaxParallelSubagents(a.cfg.MaxParallelSubagents), | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,9 +129,6 @@ func (c *DeferredToolCatalog) selectSearch(names string) []DeferredToolMeta { | |
| for _, t := range c.tools { | ||
| if wanted[t.Name] { | ||
| results = append(results, t) | ||
| if len(results) >= maxSearchResults { | ||
| break | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This changes exact select: queries to return an unbounded number of explicitly requested tools, but the tool_search schema still says that the tool returns up to five matching tools. Could we update that description to clarify that only keyword and must-have searches are capped, while select: returns every requested match? |
||
| } | ||
| return results | ||
|
|
||
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.
This test duplicates the truncation algorithm instead of calling renderDeferredToolsBlock, so it can stay green even if the production implementation regresses. Could we build a catalog whose rendered output exceeds the limit and assert against the actual method result? That would also let the test verify the reported omitted count and the real output-size behavior.