Deferred (lazy) tool loading: tool_search + auto threshold for large MCP tool sets#148
Merged
Conversation
4 tasks
Contributor
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
…MCP tool sets Adds deferred tool loading so a large MCP tool set no longer sends every tool's full JSON schema each turn. When the number of visible MCP tools crosses a threshold (config tools.deferThreshold, default 10; 0 disables), their schemas are withheld and advertised compactly in a per-turn system-reminder; the model pulls a tool's full schema on demand via a new tool_search tool. Strictly additive: below the threshold behaviour is byte-identical to today. No new module dependencies. - Eligibility: MCP tools opt in via an optional Deferred() bool interface (tools.IsDeferred); built-ins stay eager. No change to the core Tool interface. - Formatting (internal/tools/deferred.go): compact one-liners wrapped in a <system-reminder> by BuildDeferredReminder; server label via MCPServerName(). - tool_search (internal/tools/tool_search.go): SideEffectNone/PermissionAllow/AdvertiseInAuto; select:Name1,Name2 (exact) or keyword (ranked); returns full schemas and signals the loop via Meta["load_tools"]. Operator-filter-aware (never lists tools hidden by --enabled-tools/--disabled-tools). - Agent loop: partitionTools is the single source of truth; eligible counts only ToolVisible deferred tools; Meta["load_tools"] lifts to ToolResult.LoadedTools and unions into a per-run loaded-set advertised next turn. Reminder appended only to the per-turn request copy (never persisted), including both reactive-compaction retry paths. tool_search stays reachable when deferral is active (exempt from the EnabledTools allowlist). - Config + wiring: tools.deferThreshold (default 10; 0 disables; negative rejected); tool_search registered and DeferThreshold threaded in exec and TUI, gated on the same visible-deferred count the loop uses. Recreated on top of current main; resolves the SideEffectNone clash with #146 (escalate_model). Supersedes #147.
gnanam1990
force-pushed
the
deferred-tool-loading
branch
from
June 9, 2026 04:52
e59a867 to
d3560db
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds configurable deferred MCP-tool loading with a DeferThreshold, a new ChangesDeferred MCP tool loading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/tools/tool_search.go (1)
98-102: ⚡ Quick winDead code:
deferredToolsappears unused.The method
deferredTools()is not called anywhere in this file, and static analysis flags it as unused. If it's not needed for external callers, consider removing it to reduce maintenance surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tools/tool_search.go` around lines 98 - 102, The method deferredTools on type toolSearchTool is dead code; either remove it or make it intentionally used/exported—delete the deferredTools() function body and its declaration if there are no external callers, or if it was intended for external use, rename/export it and update callers to use tool.visibleDeferredTools(nil, nil) (reference symbols: deferredTools, toolSearchTool, visibleDeferredTools) so there is no unused function left in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/deferred_loop_test.go`:
- Around line 700-735: Update
TestDisabledToolSearchStaysHiddenAndRejectedWhenActive to assert the
eager/inactive fallback when "tool_search" is explicitly disabled: call
partitionTools (used in this test) and assert there is no "tool_search" loader
definition, and that the deferred tools (fakeDeferredTool names
"mcp__srv__alpha" and "mcp__srv__beta") are present in the exposed set; then
change the executeToolCall expectations so we do NOT assert that a disabled
tool_search call remains hidden/filtered — remove the DenialFiltered check and
instead assert that calls operate under the eager/inactive behavior (no loader
definition present and deferred tools are accessible) by ensuring
executeToolCall returns with no unexpected abortErr and that access to the
actual deferred tools succeeds rather than being blocked.
In `@internal/agent/loop.go`:
- Around line 939-998: The deferral path is enabled solely by the deferred count
(active) even when the tool_search loader is missing/disabled/unrunnable; change
the active gate to also require that tool_search is actually registered and
usable before hiding tools or emitting the reminder. Concretely, when computing
active (and before using exposedNames/hiddenLines), call
registry.Get(tools.ToolSearchToolName) and ensure it exists and is not disabled
via containsToolName(options.DisabledTools, tools.ToolSearchToolName) (and if
you have a runtime check for usability/runnable on the returned loader, include
that too); only set active true if both the defer-threshold condition and the
loader availability/usability check pass so the code falls back to the
eager/inactive path when tool_search cannot run (this prevents telling the model
to call tool_search while executeToolCall will later reject it).
In `@internal/cli/exec.go`:
- Around line 227-234: Compute a single effective defer threshold and force it
to 0 when "tool_search" is present in options.disabledTools, then use that
effective value everywhere instead of the raw resolved.Tools.DeferThreshold;
specifically, create a local variable (e.g., effectiveDeferThreshold)
initialized to resolved.Tools.DeferThreshold, set it to 0 if
options.disabledTools contains "tool_search", pass effectiveDeferThreshold into
registerToolSearchIfEligible(registry, ..., permissionMode,
options.enabledTools, options.disabledTools) and assign/use it for
agent.Options.DeferThreshold (and anywhere else the raw DeferThreshold is passed
into agent.Run or related registration/partition gates) so the run and
registration gate share the same threshold and deferred loading is disabled when
tool_search is explicitly disabled.
---
Nitpick comments:
In `@internal/tools/tool_search.go`:
- Around line 98-102: The method deferredTools on type toolSearchTool is dead
code; either remove it or make it intentionally used/exported—delete the
deferredTools() function body and its declaration if there are no external
callers, or if it was intended for external use, rename/export it and update
callers to use tool.visibleDeferredTools(nil, nil) (reference symbols:
deferredTools, toolSearchTool, visibleDeferredTools) so there is no unused
function left in the file.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b73782-3110-4582-bc21-ae26ac9d3345
📒 Files selected for processing (20)
internal/agent/deferred_loop_test.gointernal/agent/loop.gointernal/agent/types.gointernal/cli/app.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_tools.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/config/types_test.gointernal/mcp/registry.gointernal/mcp/registry_test.gointernal/tools/deferred.gointernal/tools/deferred_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/tool_search.gointernal/tools/tool_search_test.gointernal/tools/types.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/agent/loop.go (1)
939-989:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeactivate deferral unless
tool_searchis actually runnable.Line 939 enables deferral purely by deferred-count. If
tool_searchis missing, explicitly disabled, or not runnable in current mode, Lines 963-998 can still hide deferred tools and emit reminders, while calls are rejected later (see Line 422), creating a dead-end.Suggested fix
- active := options.DeferThreshold > 0 && eligible >= options.DeferThreshold + loader, loaderFound := registry.Get(tools.ToolSearchToolName) + loaderUsable := loaderFound && + !containsToolName(options.DisabledTools, tools.ToolSearchToolName) && + ToolAdvertised(loader, permissionMode) + active := options.DeferThreshold > 0 && eligible >= options.DeferThreshold && loaderUsable @@ - if active && !exposedNames[tools.ToolSearchToolName] && !containsToolName(options.DisabledTools, tools.ToolSearchToolName) { - if loader, ok := registry.Get(tools.ToolSearchToolName); ok { + if active && !exposedNames[tools.ToolSearchToolName] { + if loaderUsable { definitions = append(definitions, zeroruntime.ToolDefinition{ Name: loader.Name(), Description: loader.Description(), Parameters: schemaToRuntimeMap(loader.Parameters()), }) } }
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/agent/loop.go`:
- Around line 906-913: Remove the unused shim function toolDefinitions: delete
the toolDefinitions function and update any callers to call
partitionTools(registry, permissionMode, options, map[string]bool{}) directly
(preserving the same semantics/exposed return) and clean up any now-unused
imports; reference the toolDefinitions symbol for removal and partitionTools for
the replacement call.
In `@internal/tools/tool_search.go`:
- Around line 98-102: Remove the unused helper deferredTools defined on type
toolSearchTool; delete the function named deferredTools() that simply calls
visibleDeferredTools(nil, nil) since golangci-lint flags it as dead code. Search
for the method signature "func (tool toolSearchTool) deferredTools() []Tool" and
remove that entire method; ensure no other code references deferredTools (if
there are callers, replace them to call visibleDeferredTools directly with the
appropriate nil,nil args). After removal, run linters/tests to confirm no
remaining references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f2d091b-34a1-4146-b9b9-aabb79f53063
📒 Files selected for processing (20)
internal/agent/deferred_loop_test.gointernal/agent/loop.gointernal/agent/types.gointernal/cli/app.gointernal/cli/deferred_wiring_test.gointernal/cli/exec.gointernal/cli/exec_tools.gointernal/config/resolver.gointernal/config/resolver_test.gointernal/config/types.gointernal/config/types_test.gointernal/mcp/registry.gointernal/mcp/registry_test.gointernal/tools/deferred.gointernal/tools/deferred_test.gointernal/tools/registry.gointernal/tools/registry_test.gointernal/tools/tool_search.gointernal/tools/tool_search_test.gointernal/tools/types.go
✅ Files skipped from review due to trivial changes (1)
- internal/tools/types.go
🚧 Files skipped from review as they are similar to previous changes (15)
- internal/mcp/registry.go
- internal/cli/exec_tools.go
- internal/agent/types.go
- internal/cli/app.go
- internal/cli/exec.go
- internal/config/resolver.go
- internal/mcp/registry_test.go
- internal/config/types_test.go
- internal/config/resolver_test.go
- internal/tools/deferred.go
- internal/config/types.go
- internal/cli/deferred_wiring_test.go
- internal/tools/tool_search_test.go
- internal/tools/deferred_test.go
- internal/agent/deferred_loop_test.go
Vasanthdev2004
requested changes
Jun 9, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Verdict: Request changes
Findings:
-
P1
internal/agent/loop.go:939: deferral activates fromDeferThreshold+ eligible count alone, without requiring a runnabletool_search. Iftool_searchis disabled or absent,partitionToolscan hide deferred tools and emit a reminder, whileexecuteToolCalllater rejects the loader. That leaves the model with neither the deferred schemas nor a usable loader. Please gateactiveonregistry.Get("tool_search"), not disabled, and advertised/runnable in the current permission mode; otherwise fall back to eager exposure. -
P1 test coverage
internal/agent/deferred_loop_test.go:701:TestDisabledToolSearchStaysHiddenAndRejectedWhenActivecurrently locks in the dead-end behavior above. It should assert eager/inactive fallback whentool_searchis explicitly disabled: no loader definition, empty reminder, and the deferred tools exposed normally. -
Cleanup
internal/agent/loop.go:910,internal/tools/tool_search.go:100:toolDefinitionsandtoolSearchTool.deferredToolsare unused shim/helper functions. Remove them or wire callers directly throughpartitionTools(...)/visibleDeferredTools(...)so linter/static analysis stays clean.
Validation I ran locally on the PR worktree:
git diff --check origin/main...HEADgo test ./internal/agent ./internal/tools ./internal/cli ./internal/mcp
Both passed, but the behavioral blocker above still needs a fix before merge.
…drop dead deferredTools() Addresses #148 review: - partitionTools gates 'active' on a registered, non-disabled, mode-advertised tool_search (mirrors the executeToolCall dispatch gate). When the loader is unavailable (explicitly disabled, missing, or not advertised e.g. spec-draft), fall back to the eager/inactive path instead of hiding deferred tools behind a loader the model can never call (the dead-end). - Flip TestDisabledToolSearch... to assert the eager fallback; register a real tool_search in the active-path tests that previously relied on activation without a usable loader. - Remove unused toolSearchTool.deferredTools().
…rop unused toolDefinitions shim Further #148 review: - exec computes one effective DeferThreshold, forced to 0 when tool_search is in --disabled-tools, and feeds it to both registerToolSearchIfEligible and agent.Options.DeferThreshold — so the registration gate and the loop partition gate agree, and a disabled loader never enables deferral. - Remove the unused toolDefinitions shim (golangci-lint unused; no callers).
Collaborator
Author
|
@coderabbitai review — all 5 review findings addressed across 26ab40b and 962a5ab (loop gating on a runnable tool_search, exec effective-threshold parity, test flipped to eager fallback, dead code removed). Full suite + -race + Windows build green locally. |
|
✅ Action performedReview finished.
|
Vasanthdev2004
approved these changes
Jun 9, 2026
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
Collaborator
There was a problem hiding this comment.
Rereviewed the latest head 962a5ab.
The previous blockers are resolved:
tool_searchdeferral now only activates when the loader is registered, not disabled, and advertised/runnable for the current permission mode.execnow uses a single effective defer threshold and forces it to0whentool_searchis disabled.- The disabled-
tool_searchregression test now asserts eager fallback with deferred tools exposed normally. - The unused helper/shim cleanup is done.
Validation run locally on the PR worktree:
git diff --check b83367e9d3f0d3f19d4c5bf659ec73f60d0f510b...HEADgo test ./internal/agent ./internal/tools ./internal/cli ./internal/mcpgo test ./...
All passed. Approving.
5 tasks
This was referenced Jun 11, 2026
Merged
This was referenced Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds deferred (lazy) tool loading so a large MCP tool set no longer sends every tool's full JSON schema on every turn. When the number of visible MCP tools crosses a threshold, their schemas are withheld, advertised compactly in a per-turn
<system-reminder>, and the model pulls a tool's full schema on demand via atool_searchtool. Strictly additive — below the threshold (default 10) behaviour is byte-identical to today. No new module dependencies.internal/tools,internal/mcp): MCP tools are deferred-eligible via an optionalDeferred() boolinterface (tools.IsDeferred); built-ins never implement it, so they stay eager. No change to the coreToolinterface.internal/tools/deferred.go): compact one-liners —name: <short-desc> | server: <X> | inputs (* required): a (string)*, …; +N more— wrapped in a<system-reminder>block byBuildDeferredReminder. Server label uses the MCP server name directly (MCPServerName()), falling back to a name parse.internal/tools/tool_search.go):tool_search(SideEffectNone/PermissionAllow/AdvertiseInAuto). Queryselect:Name1,Name2(exact) or keywords (ranked); returns each loaded tool's full schema and signals the loop viaMeta["load_tools"]. It is operator-filter-aware — it never resolves, ranks, or lists a tool hidden by--enabled-tools/--disabled-tools.internal/agent):partitionToolsis the single source of truth —eligiblecounts onlyToolVisibledeferred tools; wheneligible >= DeferThreshold, unloaded deferred tools are withheld into the reminder whiletool_search+ built-ins + already-loaded tools stay exposed.Meta["load_tools"]is lifted toToolResult.LoadedToolsand unioned into a per-run loaded-set so the next turn advertises them. The reminder is appended only to the per-turn request copy (never persisted history), including on both reactive-compaction retry paths.tool_searchis always reachable when deferral is active (exempt from theEnabledToolsallowlist) so an allowlist of deferred tools can never strand the loader.internal/config,internal/cli):tools.deferThreshold(default 10;0disables; negative rejected; explicit0preserved vs unset).tool_searchis registered — andDeferThresholdthreaded — in exec and the TUI, gated on the same visible-deferred count the loop uses, so registration and activation never disagree.Loaded-set scope is within a single
agent.Run(v1, fully covers headlessexec); TUI cross-turn persistence is a documented follow-up.Test Plan
go build ./.../go vet ./...clean;gofmtclean (touched files)go test ./...green (new tests acrosstools,mcp,agent,config,cli)go test -race ./internal/{tools,mcp,agent,config,cli}/...greenGOOS=windows GOARCH=amd64 go build ./...greenReviewer focus
partitionToolsinactive output isreflect.DeepEqualto the legacytoolDefinitions(onlytool_searchdropped, which isn't registered when inactive).ToolVisibledeferred population — prompt-gated MCP tools that aren't advertised in auto mode don't spuriously registertool_search.--enabled-toolsallowlist of deferred tools keepstool_searchreachable (regression-tested); a deferred tool hidden by--disabled-toolsis invisible totool_search.Note
Recreated on top of current
main. PR #146 (model escalation) has since merged; itstools.SideEffectNoneconstant (forescalate_model) and this branch'sSideEffectNone(fortool_search) are reconciled here into a single declaration. Supersedes #147.Summary by CodeRabbit
New Features
Behavior Changes
Tests