feat: per-MCP-server tool_injection control (description_only mode) - #66826
feat: per-MCP-server tool_injection control (description_only mode)#66826WeilaiSun wants to merge 9 commits into
tool_injection control (description_only mode)#66826Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused per-server proposal. The global-threshold premise is real on current main (model_tools.py:548-563), but this implementation needs scope and lifecycle work before it is safe to salvage.
Problems
agent/system_prompt.pyenumerates the process-global marked set, not the agent's filtered toolset. This conflicts with the restricted-session isolation contract documented intests/tools/test_tool_search.py:419-428.- The marking added in
tools/mcp_tool.pycovers onlyserver._tools; resource/prompt utility schemas are registered separately attools/mcp_tool.py:5088-5123and remain eager. - Current
model_tools.py:550skips assembly when global tool search is off, while the new inventory still instructs the model to usetool_search. - The PR adds no tests despite changing MCP registration, tool assembly, and cached prompt construction.
Suggested changes
- Derive the inventory from agent-scoped pre-assembly definitions, define utility-tool semantics, and add coverage for scoped sessions, tool-search-off, late MCP registration, and bridge dispatch.
Automated hermes-sweeper review.
| if agent.valid_tool_names: | ||
| try: | ||
| from tools.tool_search import get_description_only_tool_names | ||
| _do_names = get_description_only_tool_names() |
There was a problem hiding this comment.
get_description_only_tool_names() is process-global. This inventory is not filtered through the agent's enabled/disabled toolsets, so a restricted session can be told about description-only tools from another MCP server. Please derive the inventory from the same session-scoped pre-assembly definitions used by the bridge catalog.
| @@ -4799,6 +4811,13 @@ def _should_register(tool_name: str) -> bool: | |||
| _track_mcp_tool_server(tool_name_prefixed, name) | |||
| registered_names.append(tool_name_prefixed) | |||
|
|
|||
| # Description-only tools: mark for always-deferred treatment | |||
There was a problem hiding this comment.
This marker is applied only in the server-native server._tools registration loop. The resource/prompt utility schemas registered below remain eager, so the advertised per-server mode does not apply to every tool from this server. Please either cover those registrations or narrow and document the policy.
Review feedback fixes (3 commits)Addressed all 4 problems identified by @teknium1 in sweeper review: P1: Session-scoped inventory ✅
P2: Utility tool marking ✅
P3: tool_search=off gate ✅
P4: Test coverage ✅
50/50 tests pass | Changes match suggested approach: agent-scoped definitions, utility-tool semantics, comprehensive coverage. |
|
Hey @teknium1, gentle ping — all 4 issues from the sweeper review have been addressed and pushed (see previous comment). Summary of fixes:
Ready for re-review when you have a chance. Thanks! |
Adds tool_injection config option for MCP servers, supporting two modes: - full (default, backwards-compatible): all tools loaded eagerly - description_only: tools always deferred, discovered via tool_search Modeled after Claude Code's defer_loading pattern and Hermes's own Skill frontmatter discovery (name+desc in prompt, body on demand). Changes: - tools/tool_search.py: mark_description_only_tool() registry + force-defer in assemble_tools() even when under threshold - tools/mcp_tool.py: read tool_injection config, mark tools during registration - agent/system_prompt.py: inject MCP tool inventory (name+desc) into stable tier for description-only servers, mirroring the Skill index block Closes NousResearch#66736 Refs: NousResearch#6839 (Lazy Tool Schema Loading), Writer Harness paper (arXiv 2607.06906)
…earch gate, tests Fixes all 4 issues from @teknium1's review: 1. Session scoping: filter description_only inventory against agent.valid_tool_names instead of exposing the process-global set. Prevents cross-session tool leakage. 2. Utility tools: mark MCP Resources/Prompts utility tools as description_only when the server's tool_injection is set to description_only mode. 3. tool_search-off gate: skip MCP inventory injection when tool_search is disabled globally, preventing misleading instructions to use tool_search. 4. Tests: add TestDescriptionOnly class with 8 tests covering mark/is roundtrip, classification, assembly force-bridge, session scoping, and duplicate handling. Test results: 263/264 passed (1 pre-existing Windows env test failure)
…dge tests P1: Description-only MCP tool inventory in the system prompt now derives from pre-assembly tool names (agent._pre_assembly_tool_names) instead of agent.valid_tool_names. agent.valid_tool_names is the post-tool_search- assembly visible set — description_only tools are deferred behind bridge tools, so the intersection was always empty and the inventory block was never generated. agent._pre_assembly_tool_names captures the full session-granted tool set before tool_search deferral, so the inventory correctly lists description_only tools while still being scoped to the agent's enabled toolsets. P4: Added two test scenarios: - test_lazy_mcp_registration_marking_persists: verifies description_only marking + is_deferrable_tool_name + classify_tools for tools registered after agent init (lazy MCP server discovery). - test_bridge_dispatch_finds_description_only_tool: verifies tool_search catalog + tool_describe schema retrieval for description_only tools via the bridge dispatch path. P2 + P3 were already addressed in the parent fix commit; this commit completes the remediation. All 49 test_tool_search.py tests pass.
…4 scenario 1) OpenCode (deepseek-v4-pro) added test_description_only_inventory_in_system_prompt to verify the system prompt correctly lists description_only tools with descriptions but excludes full JSON parameter schemas. Completes all 5 P4 test scenarios. All 50 tests pass.
…ache hits _get_last_resolved_tool_names_ flips semantics with cache state: fresh compute stores pre-assembly names, a quiet_mode cache hit overwrites it with the cached POST-assembly list (description_only tools already collapsed behind the bridge). agent_init captured that global into agent._pre_assembly_tool_names, so the 2nd+ session with the same toolset key got an empty intersection and the system-prompt inventory silently vanished (gateway/TUI/cron all build agents with quiet_mode=True). - model_tools: new _last_pre_assembly_tool_names global, snapshot at the pre-assembly point in _compute_tool_definitions; cache value now carries (final_list, pre_assembly_names) and the cache-hit path restores both - agent_init: capture the pre-assembly global (was _last_resolved_tool_names) - mcp_tool refresh_agent_mcp_tools: publish _pre_assembly_tool_names atomically with the snapshot so late-registered description_only servers reach the inventory after /reload-mcp or lazy refresh - tool_search: unmark_description_only_tool; mcp_tool _deregister_tools now unmarks on server unload (no stale marks); tool_injection config validated (full|description_only, warn+fallback); system_prompt except now logs - tests: P1 regression (cache hit keeps pre-assembly names + inventory), tool_search-off no-inventory real path, rewrote 3 weak tests to real paths (_register_server_tools/refresh, build_system_prompt_parts, model_tools)
…arch#66826 P2) When tool_search assembly is already active, a late-registered description_only server keeps the POST-assembly name set unchanged (its tools are bridged away), so refresh_agent_mcp_tools early-returns without publishing. The pre-assembly view must still be published, or the system-prompt description_only inventory silently misses the new server's tools. - refresh_agent_mcp_tools: publish agent._pre_assembly_tool_names on the new_names == current early-return path - test: P2 regression (baseline refresh -> late register -> refresh early-return still tracks the new tool in pre-assembly); RED-verified (fails without the fix, passes with it) Refs: review finding on 13dfba6 (GO 84/100, 1 P2 residual)
c97b032 to
a0d526f
Compare
SummaryOne PR addresses issue #66736. #66826 implements the requested per-server Related pull requests
Suggested consolidationKeep #66826 open with a salvage path: preserve its per-server policy, lifecycle handling, prompt inventory, and test coverage, while replacing the process-global pre-assembly handoff with state returned directly from tool-definition assembly or otherwise bound to the receiving agent, then add a concurrent-agent isolation regression test. There are no duplicate PRs to close. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I66736(["issue #66736 (open)"])
P66826["PR #66826 (open)"]
P66826 -->|best fix| I66736
class I66736 open
class P66826 open
class P66826 best
class P66826 target
click I66736 "https://github.com/NousResearch/hermes-agent/issues/66736"
click P66826 "https://github.com/NousResearch/hermes-agent/pull/66826"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 1 pull request and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 49 kB of PR diffs, 6 kB of issue/PR text, 6 kB of discussion (7 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…ssion test - tests/run_agent/conftest.py: autouse fixture 将 with_meta 转发到 legacy get_tool_definitions mock(33 文件一次性适配,防未来漏) - tests/test_copilot_initiator.py: 手动补 with_meta mock - tools/mcp_tool.py: refresh_agent_mcp_tools 改 with_meta 解包直绑, 消除读进程全局的同类 race - tests/test_model_tools.py: +agent 级交错 init 回归测试 + finally deregister - 连带适配 3 个 mcp 相关测试文件
|
Thanks for the triage review @GottZ — the race is fixed. What changed (3 commits on top of the P1/P2 fixes):
Verified: Happy to adjust if you'd prefer the state bound differently. |
Summary
Adds per-MCP-server
tool_injectionconfig option, supporting"full"(default, backwards-compatible) and"description_only"modes. Description-only tools are always deferred and discovered viatool_search, mirroring Claude Code'sdefer_loadingpattern — the agent sees tool names + descriptions in the system prompt (stable tier, cached prefix) while full schemas are loaded on demand.Closes #66736 · Inspired by #6839 (Lazy Tool Schema Loading) · Addresses Writer Harness paper critique (arXiv 2607.06906: Hermes tools not in cached prefix)
Motivation
Currently, all MCP tool schemas are loaded into every API call irrespective of frequency. For low-frequency MCP servers (e.g., firecrawl's 26 tools used 2-3 times per session), this wastes tokens on every turn. The
tool_searchmechanism already supports deferral, but only at a global threshold (>10% context) — there's no per-server control.Real-world impact: one user moved 5 low-frequency MCP servers (64 tools) to mcporter CLI workarounds, saving ~60+ tool schemas from the tools array per turn. This feature makes that pattern native.
Design
Mirrors Claude Code's three-tier tool pool and Hermes's own Skill frontmatter discovery:
Flow:
tool_search,tool_describe,tool_call)tool_search→tool_describe→tool_callto use deferred toolsChanges (3 files, +90/-1 lines)
tools/tool_search.py:mark_description_only_tool()registry + force-defer inassemble_tools()even under thresholdtools/mcp_tool.py: readtool_injectionconfig, mark tools during registrationagent/system_prompt.py: inject MCP tool inventory into stable tier, mirroring Skill index blockBackwards Compatibility
tool_injectiondefaults to"full"— no behavior change for existing configstool_searchmust be enabled (tools.tool_search) fordescription_onlytools to be usableTesting
mark_description_only_tool/is_description_only_toolround-tripclassify_toolscorrectly routes description_only tools to deferrableassemble_toolsforces bridge injection when description_only tools existtool_search→tool_describe→tool_callworks end-to-endReferences