fix(agent): defer verbose core tool schemas - #58838
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (not blocking)
New feature/feature gate: defers verbose core tool schemas to reduce per-call overhead. The diff touches 9 files across the tool dispatch and MOA relay layers. New allowlist-based deferral logic with comprehensive test coverage.
What looks good:
- Well-tested (new tests added for deferral conditions, thresholds, and gating)
- Clear allowlist vs. denylist distinction for which tools defer
- Respects the "core tool schema is sent on every call" cost concern from AGENTS.md
Note: This is a behavioral change to the tool dispatch surface. The tests appear thorough, but as a non-trivial change to what the agent sees per call, a human reviewer should confirm the deferral logic correctly handles all edge cases before merge.
|
Rebased onto current Local verification:
|
03734b7 to
358c54a
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the measured default-schema cost. The premise remains present on current main: tools/tool_search.py:5-11 and model_tools.py:537-563 explicitly preserve core schemas.
Problems
agent/agent_init.py:1072-1076records the full scope inavailable_tool_names, but the unchanged validator inagent/conversation_loop.py:4567-4575rejects names outsidevalid_tool_names. Deferred names are absent from that visible set, so a direct deferred call is rejected rather than bridged.tools/tool_search.py:662-667keeps plugins direct, whiletools/tool_search.py:741-742still builds the bridge catalog from all deferrable tools. This exposes plugins twice and permits bridge calls despite the stated direct-only behavior.website/docs/user-guide/features/tool-search.md:82-86mis-indents the new YAML keys; it also namesskill_manageas deferrable although it is absent fromDEFAULT_DEFERRABLE_CORE_TOOLSattools/tool_search.py:65-103.
Suggested changes
- Route exact, in-scope deferred direct calls through
tool_callbefore validation, with sequential and concurrent regressions. - Keep the bridge catalog consistent with the hidden subset, then correct the documentation.
Automated hermes-sweeper review.
| quiet_mode=True, | ||
| skip_tool_search_assembly=True, | ||
| ) | ||
| agent.available_tool_names = { |
There was a problem hiding this comment.
valid_tool_names remains the model-facing set, while conversation_loop.py rejects any direct function name outside it before execution. A deferred terminal direct call therefore cannot reach the bridge. Please route exact in-scope deferred names to tool_call before that validator and cover it with an end-to-end execution-path regression.
| limit = max(1, min(config.max_search_limit, _safe_int(raw_limit, config.search_default_limit))) | ||
|
|
||
| _, deferrable = classify_tools(current_tool_defs) | ||
| _, deferrable = classify_tools(current_tool_defs, config=config) |
There was a problem hiding this comment.
This catalog still contains plugin tools because classify_tools() marks them deferrable, even though assembly retains them directly at lines 662-667. Filter dispatch catalogs to the same hidden subset so direct plugins are not duplicated as bridge targets.
| threshold_pct: 10 # percentage of context — only used in auto mode | ||
| search_default_limit: 5 | ||
| max_search_limit: 20 | ||
| threshold_pct: 10 # percentage of context — capped by auto_token_threshold |
There was a problem hiding this comment.
These keys need four-space indentation under tools.tool_search; as written they are siblings of tool_search under tools and will not configure the fields described below.
|
Thanks for the detailed review. The exact direct-call bridge must rewrite both the tool name and arguments before May I widen this PR's scope to include that pre-validation normalizer and its run-loop regressions? With that approval, I will also apply the in-scope bridge-catalog and documentation corrections from this review in the same update. |
Extends Tool Search so default sessions stop sending verbose built-in tool manuals on every request while keeping the core direct-tool substrate available.
Shared root cause
model_tools.get_tool_definitions()always returning full schemas for every default core tool, whiletools.tool_searchonly deferred MCP/plugin tools. A defaulthermes-cliassembly in this worktree measured about 14,746 estimated schema tokens before assembly.terminal,memory,skill_manage,browser_*,execute_code,delegate_task,session_search, etc.) while keeping foundational tools such asread_file,write_file,search_files,web_search,web_extract,process, andtododirect.tools.tool_search.auto_token_thresholdcaps the auto gate so large-context models still benefit once deferrable schemas exceed the fixed token ceiling;defer_core_tools: falserestores the previous MCP/plugin-only behavior.How this fixes each issue
hermes-clischemas dropped from about 14,746 estimated tokens to about 2,432 after assembly.tool_search,tool_describe, andtool_call, instead of permanently including all heavy default schemas on every request.How to test
/opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/tools/test_tool_search.py -q' shpassed: 44 passed./opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/tools/test_tool_search.py tests/test_get_tool_definitions_cache_isolation.py tests/test_model_tools.py -q' shpassed: 81 passed./opt/homebrew/bin/timeout -k 30 480 sh -c 'pytest tests/ -q -x --timeout=60 "$@"' shwas attempted and aborted during collection because this environment lacks FastAPI and lazy dependency installation is blocked by externally managed Python (ModuleNotFoundError: No module named 'fastapi', then PEP 668 pip failure).ruff check.python -m py_compilepassed for changed Python files.python scripts/check-windows-footguns.py agent/agent_init.py agent/agent_runtime_helpers.py agent/memory_manager.py agent/tool_executor.py model_tools.py tools/tool_search.py tests/tools/test_tool_search.pypassed.What platforms tested on
This coordinated PR bundles a fix that spans several issues. Happy to split it back into focused per-issue PRs if you'd prefer to review them separately.
Refs #13983
Refs #57044