feat(tool_search): minimal core-tool deferral + skills compact_categories (v2.0) - #67457
feat(tool_search): minimal core-tool deferral + skills compact_categories (v2.0)#67457ardhaecosystem wants to merge 6 commits into
Conversation
…ries Re-application of the core-tool deferral patch against v0.18.2, stripped to the minimal viable change. Drops the three additive features (hot-tools promotion, per-toolset deferral, available_tool_names tracking) that triggered hermes-sweeper review concerns on PR NousResearch#63844. Sidesteps all three by omission. ## Tool deferral (tools/tool_search.py + 3 callers) Stock v0.18.2 only defers MCP/plugin tools; verbose built-ins (terminal, patch, memory, browser_*, kanban_*, ha_*) always load. With 33 tools, schemas consume ~59 KB / ~14,700 tokens per turn — 45% of context on a 1M-token model. Changes: - ToolSearchConfig gains defer_core_tools (bool, default False) and auto_token_threshold (int, default 0) fields - DEFAULT_DEFERRABLE_CORE_TOOLS: static allowlist of 39 verbose core tools. Foundational tools (read_file, write_file, search_files, web_search, web_extract, process, todo, clarify, skill_view, skills_list) excluded by construction — verified by self-check. - is_deferrable_tool_name(name, config=None) checks the allowlist when config.defer_core_tools is True. Config param optional for backward compat. - should_activate() honors auto_token_threshold as primary gate - classify_tools() and scoped_deferrable_names() accept config param - model_tools.py + agent/tool_executor.py: thread config to scoped_deferrable_names - agent/conversation_loop.py: auto-route direct calls to deferred tools through tool_call bridge. Fast-path skips when all tool calls are valid. Backward compatible: defer_core_tools defaults to False, stock behavior unchanged. All 39 existing tool_search tests pass. All 29 repair_tool_call tests pass. ## Skills compact_categories (agent/system_prompt.py + agent/prompt_builder.py) The existing compact_categories rendering in build_skills_system_prompt() demotes skill categories to names-only in the index. Previously gated on coding_context: focus mode, which only fires on interactive coding surfaces (CLI/TUI/ACP/desktop) — not on Telegram/Discord/CLI. Changes: - agent/system_prompt.py: merge skills.compact_categories config (list or str) into the _compact_cats frozenset, unioned with the coding-posture result. - agent/prompt_builder.py: genericize the hidden_note text (was 'coding context', now 'to keep the prompt lean'). Backward compatible: empty config = current behavior. ## Measured impact (v0.18.2, glm-5.2, 33 tools, ecc-imports skills) | Metric | Before | After | Change | |---|---|---|---| | Tool schemas | 59,357 B (33 tools) | 21,760 B (24 tools) | -63% | | Skills block | 47,159 B | 16,053 B | -66% | | System prompt total | 64,725 B | 31,761 B | -51% | Config: tools: tool_search: enabled: on defer_core_tools: true auto_token_threshold: 8000 skills: compact_categories: ecc-imports Refs NousResearch#6839, NousResearch#58838, NousResearch#2045, NousResearch#22620 Supersedes NousResearch#63844 (minimal re-application without the additive features) (ponytail: minimal diff, 140 lines across 6 files, no new abstractions, no module-global mutable state, no mid-conversation schema mutation. Cache-stable by design — tool list computed once at init, reused for session.)
22 tests covering: - defer_core_tools config field + bool/string/int coercion - auto_token_threshold gate in should_activate - DEFAULT_DEFERRABLE_CORE_TOOLS excludes foundational tools - is_deferrable_tool_name honors config - classify_tools threads config - backward compat (default config = stock behavior) - auto-route logic simulation (deferred→bridge, valid→direct, None args, malformed JSON) All 90 tests pass (39 existing + 22 new + 29 repair_tool_call).
…w": ...}
Claude Code review (kimi-k2.7-code) flagged the _wrapped = {"_raw": ...}
fallback as a ship-blocker: wrapping malformed args as {"_raw": "<malformed>"}
breaks the underlying tool's schema — it receives arguments it doesn't
recognize instead of its expected shape.
Fix: fall back to the existing _repair_tool_call_arguments() helper (already
imported in conversation_loop.py:40 from agent.message_sanitization) which
applies common repairs (unescaped control chars, Python None, trailing
commas). If repair also fails, default to {} — the tool call proceeds with
empty args and the model agent-corrects from the tool's error response,
which is the normal Hermes behavior for bad arguments.
Updated test_tool_search_v2.py: replaced test_malformed_json_falls_back_to_raw
with test_malformed_json_falls_back_to_repair_then_empty verifying the new
fallback path.
90/90 tests still pass.
|
Thank you for reducing the scope and preserving a static, opt-in configuration path. Problems
Suggested changes
Automated hermes-sweeper review. |
hermes-sweeper (teknium1) on PR NousResearch#67457 flagged that config wasn't propagated through the whole bridge — the auto-route could rewrite terminal() to tool_call(), but the bridge couldn't resolve it because dispatch_tool_search, dispatch_tool_describe, and resolve_underlying_call all called classify_tools/is_deferrable_tool_name without config. Deferred core tools were rejected at every checkpoint. Fix: thread config through all four missing call sites. - tools/tool_search.py: dispatch_tool_search, dispatch_tool_describe, and resolve_underlying_call all accept and use config param. - model_tools.py: pass config=_ts_mod.load_config() to resolve_underlying_call. - agent/tool_executor.py: pass config=_ts.load_config() to resolve_underlying_call (both call sites: :403 and :1077). Also added 8 real bridge E2E tests (TestBridgeE2E class) covering: - resolve_underlying_call accepts/rejects deferred core tools with/without opt-in - resolve_underlying_call rejects foundational tools even with opt-in - resolve_underlying_call rejects bridge tools and malformed JSON - dispatch_tool_describe serves/rejects deferred core tools - full round trip: auto-route → bridge resolution → underlying tool The test_full_auto_route_to_bridge_resolution test is the specific E2E test teknium1 asked for — verifies the complete path from model emitting terminal() directly to the bridge resolving it back to terminal(). 98/98 tests pass (was 90, +8 bridge E2E).
|
Thanks for the review — all three points addressed in 1. Config not propagated through the bridge — FIXEDThe missing config threading is the real bug. The auto-route rewrites Fixed: config now threads through all four missing call sites:
Verified end-to-end: 2. Simulated test — REPLACED with real bridge E2EReplaced the simulator with
98/98 tests pass (was 90, +8 bridge E2E). 3. Design decision — core tools always-direct contractYou're right that the original commit
The isolated-cron concern is real — if a cron job runs with Happy to update the docs in this PR if the design direction is approved, or split it into a separate docs PR. |
Two changes from Opus review round 3 on PR NousResearch#67457. ## 1. Move auto-route BEFORE repair (ship-blocker) Opus found that _repair_tool_call ran before the auto-route, so when the model emits a deferred tool's exact name (e.g. ), repair's fuzzy matcher (cutoff=0.7) could silently rewrite it to a ≥0.7-similar visible tool and execute the wrong tool. The auto-route then never saw the original name. Low probability with stock toolsets (no ≥0.7 collision exists among stock names), but a plugin registering a similarly-named visible tool makes it live. High blast radius: silent wrong-tool execution. Fix: swap the order. Auto-route claims exact matches on deferred tools first (rewriting them to tool_call), then repair only sees actual typos. The fast-path (_all_valid) still skips both blocks on the common path. ## 2. Deferred-tool manifest (architectural pivot) The auto-route only fires when the model *emits* a direct call. Isolated cron/subagent turns that need a deferred tool but never saw its name in the prompt will never emit a direct call — the NousResearch#84141 silent-dropout regression class the original tool_search commit (369075d) was designed to prevent. Fix: emit a compact manifest of deferred tools into the system prompt. The model always sees every tool name → always can emit a direct call → auto-route fires even in cron. We drop schemas, not names. The manifest costs ~500 tokens for 39 deferred tools — a fraction of the ~14K tokens of schemas saved. This reconciles our bridge approach with issue NousResearch#6839's two-pass proposal: keep bridge-level schema savings, restore the always-see-every-name property. The 'should core tools ever defer?' objection dissolves because we're no longer dropping names from the model's view — only schemas. Implementation: - agent/agent_init.py: stash _pre_assembly_tool_defs on the agent at init (the pre-assembly view, before deferral strips schemas). Computed once, reused for the session — cache-stable. - agent/system_prompt.py: build the manifest from _pre_assembly_tool_defs + is_deferrable_tool_name check. Gated on defer_core_tools. Wrapped in try/except. Uses load_config_readonly() to skip the deepcopy cost. ## 3. Thread config through dispatch_tool_search/describe (Q5 nit) model_tools.py now passes config explicitly to dispatch_tool_search and dispatch_tool_describe (was using internal default). Symmetry with the tool_call branch. Single load_config() call shared across all three. 98/98 tests still pass. System prompt: 31.8KB → 32.8KB (+1KB for manifest). Net savings vs baseline: 64.7KB → 32.8KB (49% reduction).
|
Round 3 review (Claude Opus, 40 turns) landed two findings, both addressed in 1. Repair-before-auto-route hijack — FIXED (ship-blocker)Opus found that
Fix: swapped the order. Auto-route claims exact matches on deferred tools first (rewriting them to 2. Deferred-tool manifest — ADDED (architectural pivot)This is the bigger one. Opus's architectural review:
The auto-route only fires when the model emits a direct call. Isolated cron/subagent turns that need a deferred tool but never saw its name in the prompt will never emit a direct call — exactly the #84141 silent-dropout that commit Fix: emit a compact This reconciles our bridge approach with issue #6839's two-pass proposal: keep bridge-level schema savings, restore the always-see-every-name property. The "should core tools ever defer?" objection largely dissolves because we're no longer dropping names from the model's view — only schemas. Implementation:
3. Q5 nit — config threading symmetry
What Opus also verified (no action needed)
Measured impact (with manifest)
98/98 tests still pass. The manifest adds ~260 tokens to restore the invariant the original |
Opus round 4 found the manifest fired on the config flag alone, not on whether deferral actually happened. In configs like: - enabled: off + defer_core_tools: true - enabled: auto + defer_core_tools: true, below threshold ...nothing is deferred (tools stay visible), but the manifest still listed every core tool as 'loaded on demand, routes through bridge' — misleading and token-wasting. Tools appeared twice (schema + manifest prose). Fix: exclude anything still in valid_tool_names. If assembly didn't activate, everything's visible → _deferred empty → no manifest. Clean. One line added: _visible = agent.valid_tool_names; filter n not in _visible. 98/98 tests still pass.
|
Round 4 review (Opus, 25 turns) — one blocker found, fixed in The bugThe manifest fired on the config flag alone (
Result: tools appeared twice (real schema in tools array + name in manifest prose) with misleading "call them directly, routes through bridge" text — when they're directly callable normally. Token waste + model confusion. Not wrong execution (the auto-route's The fixOne line: if getattr(_ts_cfg, "defer_core_tools", False):
_visible = getattr(agent, "valid_tool_names", None) or set()
_deferred = [
...
if n and n not in _visible and _ts.is_deferrable_tool_name(n, config=_ts_cfg)
]98/98 tests still pass. Numbers unchanged: system prompt 32.8 KB, tool schemas 21.2 KB, skills 15.7 KB. What Opus also verified
StatusOpus verdict: mergeable after the one-line gate fix. Applied. This is the 4th review round — each found something the prior didn't. The pattern: tests verify behavior, Opus verifies architecture. 98/98 tests pass, but each round caught a real defect tests couldn't. 6 commits on the PR. Ready for maintainer review. |
What
Minimal re-application of core-tool deferral against v0.18.2, plus skills
compact_categoriesconfig for non-coding surfaces. Stripped to the smallest viable change — drops the three additive features from PR #63844 that triggered hermes-sweeper review concerns.Problem
Hermes injects full JSON schemas for ALL enabled tools on every API call. On a stock v0.18.2 install with 33 tools, schemas consume ~59 KB / ~14,700 tokens per turn — 45% of context on a 1M-token model. Stock
tool_searchonly defers MCP/plugin tools; verbose built-ins (terminal,patch,memory,browser_*,kanban_*,ha_*) always load.Separately, the
<available_skills>skills index is gated to demote only undercoding_context: focusmode, which fires on interactive coding surfaces (CLI/TUI/ACP/desktop) — not Telegram/Discord/CLI. Users with 400+ installed skills carry ~47 KB of skill metadata per turn.Solution
Tool deferral (4 files, 79 lines in tool_search.py)
Widen the existing
is_deferrable_tool_name()to honor an opt-indefer_core_toolsflag + a static allowlist. The bridge, classification, dispatch, and config plumbing already exist for MCP tools — we just extend eligibility. No new mechanisms.ToolSearchConfiggainsdefer_core_tools: bool = Falseandauto_token_threshold: int = 0DEFAULT_DEFERRABLE_CORE_TOOLS: static frozenset of 39 verbose core tools. The 10 foundational tools (read_file,write_file,search_files,web_search,web_extract,process,todo,clarify,skill_view,skills_list) are excluded by construction — verified by self-check.is_deferrable_tool_name(name, config=None)checks the allowlist whenconfig.defer_core_toolsis True. Config param optional for backward compat.should_activate()honorsauto_token_thresholdas primary gate (in addition to existing % gate).classify_tools()andscoped_deferrable_names()accept config param.agent/conversation_loop.py: auto-route direct calls to deferred tools throughtool_callbridge. Fast-path skips when all tool calls are valid (zero cost on common path).Skills compact_categories (2 files, 27 lines in system_prompt.py)
Reuse the existing
compact_categoriesrendering inbuild_skills_system_prompt(). Unbundle it from thecoding_context: focusgate so Telegram/Discord/CLI users can opt in viaskills.compact_categories: <category>config.What was dropped from PR #63844 (and why)
The original PR included three additive features. All three triggered hermes-sweeper review concerns (teknium1, Jul 16). This v2.0 re-application drops all three:
model_tools.py:279-284) shared across concurrent gateway sessions;agent.toolssnapshotted at init, mid-conversation promotion doesn't reach itdefer_toolsets)tools/tool_search.py:265-266) despite always-direct contractavailable_tool_namestrackingThe minimal version sidesteps all three concerns by omission. No module-global mutable state. No mid-conversation schema mutation. No
defer_toolsetsthat can hide foundational tools. Foundational tools are excluded fromDEFAULT_DEFERRABLE_CORE_TOOLSby construction.Backward compatibility
defer_core_toolsdefaults toFalse— stock behavior unchanged.is_deferrable_tool_name(name)andclassify_tools(tool_defs)work without config param.skills.compact_categoriesempty/absent = current behavior.tests/tools/test_tool_search.pypass.tests/run_agent/test_repair_tool_call_name.pypass.Measured impact (v0.18.2, glm-5.2, 33 tools, ecc-imports skills)
Tool count: 33 → 24 (10 direct + 3 bridge + 11 MCP/plugin). 9 verbose core tools deferred.
Test coverage
test_tool_search.pypasstest_tool_search_v2.pypass (config coercion, allowlist, classify_tools, should_activate, auto-route simulation, backward compat)test_repair_tool_call_name.pypassConfig
```yaml
tools:
tool_search:
enabled: on # must be "on" (not auto) for core deferral
defer_core_tools: true # opt-in
auto_token_threshold: 8000 # primary gate
threshold_pct: 10 # secondary % gate
skills:
compact_categories: ecc-imports # str or list
```
Cache stability
Tool list is computed once at agent init and reused for the entire session. No mid-conversation schema mutation.
load_config()is cached on mtime/size. The auto-route inconversation_loop.pyonly fires when a tool name is NOT invalid_tool_names(the error path), and has a fast-path that skips the whole block when all tool calls are valid.Maintainer advice incorporated
compact_categoriesreuse instead of a newlist_skills()tool (less invasive).ecc-importsblock is a single category, demotion solves it without embeddings.Refs
Supersedes #63844 (minimal re-application without the additive features).
Builds on #58838 (same
defer_core_toolsconfig key + tool_search-bridge mechanism).Addresses #6839, #2045, #22620.