Repository navigation
fix(tooldefer): a keyword query for a deferred tool no longer disables client mode - #934
Conversation
…s client mode (#898, #900) Claude Code's own ToolSearch indexes every tool it has, so a keyword query for a proxy-deferred tool works (#871). Remove the non_select_query_for_our_tool disable reason. Such a query now passes through untouched. Assisted-By: Claude Opus 5.5 Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
amiddavid
left a comment
There was a problem hiding this comment.
| Reviewer | Kind | Started by | Context | Review | Round | Head | Verdict |
|---|---|---|---|---|---|---|---|
| Reviewer 2 (bob, user of @amiddavid) | IBM Bob Shell 1.0.3 | offload-review skill, @amiddavid's session | fresh | 2 of 2 | 1 of 2 | daff76465f5e |
approve |
Verdict: approve
Findings
None.
Non-blocking
- docs/tool-search-matrix.md:75 — The safety net explanation could be clearer. The phrase "has nothing to add today" might confuse readers who don't know the history. Consider: "Claude Code's search already indexes all tools, including deferred ones, so it finds them without the proxy's help. The proxy's
tool_referenceaddition is a safety mechanism that currently adds nothing (client_loadsstays 0)."
What I checked
- Change scope: The PR removes the
non_select_query_for_our_tooldisable reason. A keyword query that names a deferred tool now passes through untouched instead of disabling client-search mode. - Rationale: Claude Code's own search indexes every tool it has, including deferred ones, so it answers keyword queries itself (#871, comment 6642059429). The proxy no longer needs to detect and block these queries.
- Code changes:
tooldefer/toolsearch.go: RemovednamesOursfunction andAugment.Badfield. TheAugmentSearchesfunction no longer checks if non-select queries name deferred tools.tooldefer/apply.go: Removed the check fora.Badthat would disable client-search mode.ReadCallsnow executes unconditionally afterAugmentSearches.tooldefer/gate.go: RemovedDisabledNonSelcounter andReasonNonSelectQueryconstant.proxy/promexport.goandproxy/tooldefer.go: Removed the metric and stats field for this reason.tooldefer/toolsearch_test.go: Renamed test fromTestClientModeNonSelectQueryDisablestoTestClientModeKeywordQueryPassesThrough. Test now verifies keyword queries pass through and nothing is disabled.
- Documentation: Updated
docs/tool-search-matrix.mdanddocs/reference/presets.mdto reflect the new behavior. The disable reasons table now lists onlyunknown_descriptionandbreak. - Fail-open behavior: Preserved. The change removes a disable trigger, making the system more permissive. When a keyword query passes through, Claude Code's search handles it. If that fails, the model sees the error, which is correct behavior.
- Test coverage: The renamed test verifies the new behavior: keyword queries pass through, the hash is not marked disabled, and both the live thread and new threads continue deferring. This covers the critical path.
- Consistency: All references to
non_select_query_for_our_toolare removed. The two remaining disable reasons (unknown_descriptionandbreak) are consistently documented and implemented. - Metrics and observability: The metric
cg_tooldefer_client_disabled_totalnow has two labels instead of three. The/statsfieldclient_search_disablednow has two keys. Both changes are backward-compatible (removing a label/key does not break queries). - Edge cases:
- A keyword query that names a deferred tool: passes through (new behavior, tested).
- A select query: still augmented with
tool_referenceblocks (unchanged). - An error result: still passes through untouched (unchanged, tested in line 112).
- A mixed result: still augmented correctly (unchanged, tested in line 104).
- Hard boundaries: No violation. The change is in
tooldefer/, which is library code. No sidecar-plugin code is touched. Fail-open is preserved. No behavior change for users who don't useultrahighpreset. - Data dependence: No change. The feature still operates on metadata only (tool names, query strings, result shapes).
- Security: No issue. The change removes a restriction, making the system more permissive in a safe direction (letting Claude Code's own search handle more queries).
amiddavid
left a comment
There was a problem hiding this comment.
| Reviewer | Kind | Started by | Context | Review | Round | Head | Verdict |
|---|---|---|---|---|---|---|---|
| Reviewer 1 (bob, user of @amiddavid) | IBM Bob Shell 1.0.3 | offload-review skill, @amiddavid's session | fresh | 1 of 2 | 1 of 2 | daff76465f5e |
approve |
Verdict: approve
Findings
None.
Non-blocking
- The change relies on Claude Code's search indexing all tools, including deferred ones. The hash-based versioning (
knownDescHashes) protects against format changes, but a behavioral change in Claude Code's search (e.g., it stops indexing deferred tools) would not be caught until a break occurs. This is acceptable given the fail-open design and the break detection that remains in place.
What I checked
-
Core logic change: Removed the
non_select_query_for_our_tooldisable trigger. Non-select queries that name deferred tools now pass through untouched. The assumption is that Claude Code's own search indexes all tools and handles these queries itself (verified in #871). -
Fail-open compliance: The change makes the system more permissive by removing a disable trigger. This aligns with fail-open principles. The remaining disable triggers (
unknown_descriptionandbreak) still protect against actual failures. -
Code consistency: All references to
ReasonNonSelectQueryandDisabledNonSelwere removed cleanly across:tooldefer/gate.go: Removed constant and countertooldefer/apply.go: Removed thea.Badchecktooldefer/toolsearch.go: RemovednamesOurs()function andBadfield fromAugmentproxy/tooldefer.go: Removed from stats mapproxy/promexport.go: Removed from metrics
-
Test coverage:
TestClientModeNonSelectQueryDisableswas renamed toTestClientModeKeywordQueryPassesThroughand updated to verify the new behavior:- Keyword queries pass through unchanged
- Queries naming deferred tools pass through without disabling
- The hash is not marked disabled
- Both live and new threads continue deferring
-
Break detection: The
ReasonBreaktrigger remains unchanged. A deferred tool whose result says it is not loaded still disables client-search mode. This is the safety net if Claude Code's search fails to load a tool. -
Documentation: All docs updated consistently:
docs/reference/presets.md: Updatedultrahighdescription to reflect the new behaviordocs/tool-search-matrix.md: Clarified that non-select queries pass through and disable nothing
-
Metrics and observability: The metric
cg_tooldefer_client_disabled_total{reason="non_select_query_for_our_tool"}and the corresponding stats field were removed. The remaining reasons (unknown_descriptionandbreak) are sufficient for monitoring. -
Data flow: The
AugmentSearches()function no longer checks for non-select queries naming deferred tools. It only processes exactselect:queries and addstool_referenceblocks for them. All other queries pass through to Claude Code's search. -
Reversibility: Not applicable. This change is about when to disable client-search mode, not about lossy operations.
The disable metric now has 2 reasons, unknown_description and break. T4 counts break only. Releases up to v0.6.7 still send the old label; the monitor ignores it. Assisted-By: Claude Opus 5.5 Signed-off-by: DAVID AMID <DAVIDA@il.ibm.com>
Behavior change (approved by the owner on 2026-11-05)
ultrahigh, Claude Code withToolSearchon.ultrahigh.non_select_query_for_our_toolis removed fromcg_tooldefer_client_disabled_totaland from/statstool_defer.client_search_disabled.Summary
A test on #871 (comment 6642059429) showed that Claude Code's
ToolSearchindexes every tool, also the ones the proxy defers. A keyword query for such a tool works. The old disable punished a query that works.What changed
non_select_query_for_our_toolfrom the code, the gate, the metric,/stats, tests and docs.select:queries only.unknown_descriptionandbreak.docs/reference/presets.md. Noted indocs/tool-search-matrix.mdthat Claude Code's search already finds the deferred tools, so the proxy's addition is a safety net with nothing to add (client_loadsstays 0).How it was tested
On box 2, from a fresh sync, with
-race:./tooldefer/...ok./apply/...ok./proxy/...okgofmtandgo vetclean.TestClientModeKeywordQueryPassesThroughfails. Restored, it passes.Technical notes
ReasonNonSelectQuery,Gate.DisabledNonSel,Augment.Bad,namesOurs./statscomment now list two reasons.🤖 Generated with Claude Code