fix(tool-search): parallel-execution barrier, listing truncation, source indexing, availability-cache staleness - #92693
alt-glitch wants to merge 7 commits into
Conversation
…isting truncation, source indexing, check_fn memo staleness, docs
1. The batch planner classified the literal name tool_call as a
sequential barrier, so supports_parallel_tool_calls stopped working
the moment the bridge activated (every deferred call arrives
wrapped). The planner now peels the bridge and decides admission on
the underlying tool; tool_search/tool_describe lookups are
parallel-safe read-only calls.
2. _short_desc cut at the first period anywhere, so 'e.g.', 'v1.2',
and 'api.github.com' truncated catalog listing lines to fragments.
Sentence detection now requires the terminator to be followed by
whitespace/end and not to close a known abbreviation.
3. The BM25 document didn't include a tool's source, so a query naming
the service ('linear') missed tools whose own name omits it. The
source label is indexed; the dead shared mcp__ prefix token is
stripped.
4. The substring-fallback docstring documented a zero-IDF case that
cannot occur with the Lucene IDF variant (strictly positive for
df <= N). Corrected to what the fallback actually covers: total
token misses.
5. get_tool_definitions' memo was keyed on the registry generation
only, so a check_fn verdict flip (credential lands, daemon starts)
without a registry mutation served a stale tool list indefinitely.
The memo key now includes a TTL-cached snapshot of check_fn
verdicts (no_cache probes excluded — config fingerprint covers
them).
6. enabled: auto is documented as an alias of on at all three
surfaces (dataclass, config defaults, docs) with the reserved
future semantics stated.
Tests: tests/tools/test_deferral_fixes.py — 19 behavior-level cases;
10 fail on unfixed code (verified by stash run), 9 pin invariants
that must hold on both sides.
Round-1 findings from two independent external reviewers, all verified against the code before acting: - The verdict snapshot ran every availability probe on every get_tool_definitions call — including the previously-free cache-hit path — and a probe in its failure-grace window (which deliberately re-probes when uncached) was live-driven once per tools rebuild. The snapshot is now memoized on (scope, generation) for 5 s and cleared by invalidate_check_fn_cache; measured 0.08 ms per hit. - The executor's per-agent deferred-scope cache (the tool_call unwrap gate) had the same staleness class: keyed on the registry generation only, it never observed a check_fn verdict flip. Same key member added; regression test through _tool_search_scoped_names. - TOCTOU: a verdict flipping between key construction and compute stored the post-flip result under the pre-flip key, poisoning the memo for a later flip-back. Both cache sites now re-snapshot after compute and skip caching on mismatch. - Same-label probes (two '<lambda>'s) swapping verdicts inside one window produced an identical snapshot tuple; emitted elements now carry a label#index discriminator. - _short_desc was quadratic on abbreviation-dense input (25 KB of 'e.g. ' took ~5 s; MCP descriptions are third-party). The abbreviation check now uses a fixed window derived from the abbreviation list and scanning stops past the clip budget: 0.2 ms. - _check_fn_last_good gains the same hard cap as _check_fn_cache. - Planner-peel parity test: a bridged call to an opted-in MCP tool gets exactly the admission the same call gets direct — the bridge neither upgrades nor downgrades the server owner's opt-in contract. Reviewer findings rejected after verification: 'bridged MCP writers racing direct path tools' is pre-existing direct-call behavior (the server-level opt-in has never carried per-tool resource scopes; reproduced identical segments on the base commit), and per-toolset snapshot filtering would fragment the snapshot memo for negligible win.
…d guard locals, derived abbrev window, last-good cap Round-2 external review on the round-1 remediation: - The executor scope cache got the same TOCTOU guard as the get_tool_definitions memo (store skipped when the verdict snapshot moved during the rebuild) — round 1 fixed one site of the class. - The memo guard compares named locals (verdict_snapshot, key_generation), not positional cache_key slots, and only fires when the generation held steady: the first call in a process lazily registers tools (bumping the generation mid-compute), and skipping the store there left the first call permanently uncached. When the first snapshot itself triggered lazy registration, re-take once so the key and snapshot describe the same registry state. - _short_desc's abbreviation window is derived from the abbreviation list instead of a magic 16, so a longer entry can't silently outgrow it. - _check_fn_last_good gets the same hard cap as _check_fn_cache (TTL pruning alone left it unbounded under key churn inside one grace window). - Staleness docs corrected: worst case is probe TTL + snapshot TTL (~35 s); explicit invalidation clears both layers immediately.
૮ >ﻌ< ა ci reviewran on bf32ea0 — fix(tool-search): re-key the verdict-snapshot cache on regis
|
The aggregate snapshot cache was keyed on (registry, scope, probe-scope) only, so a probe that lazily registers another gated tool pinned an incomplete snapshot for the full TTL — silently defeating the post-rebuild re-take in the tool-defs memo. The registry generation joins the key: any mutation is an immediate miss. Also make the config-fingerprint scope-cache test deterministically red: assert the cache key changes across a config write (a stray cache miss recomputing the right answer no longer masks a key that omits the fingerprint), with a warm-up call to absorb lazy registrations.
c378ee8 to
bf32ea0
Compare
Overall: a thoughtful quartet of fixes — deciding parallel admission on the underlying tool after peeling the
The |
…salvage #92693, part 2) A check_fn verdict flip (credential lands, Docker daemon starts, OAuth login completes) never invalidated the get_tool_definitions memo or the executor's bridge-scope cache — the registry generation only moves on registry MUTATIONS, so the stale tool list survived for the process lifetime. Both cache sites now key on an aggregate TTL-cached snapshot of every probe's verdict (memoized in the registry; hot-path hit is one dict lookup). A flip propagates within probe TTL + snapshot TTL (~35s worst case) or immediately on invalidate_check_fn_cache(). Both sites re-check the snapshot after compute and skip the store on mismatch, so a verdict flipping mid-compute can't park a fresh result under a stale key. Note: when a tool's availability genuinely changes mid-conversation, the tool list changes with it — a one-time prompt-prefix bust for that conversation. That is the intended behavior of availability-gated tools (the alternative is the tool never appearing until restart); stable environments see zero change. Salvaged from #92693 by @alt-glitch with authorship preserved.
…dexing (salvage #92693, part 1) Four fixes to the tool-search deferral layer, split from PR #92693 (the availability-cache staleness fix ships separately): 1. The parallel batch planner now peels the tool_call bridge wrapper and decides admission on the underlying tool — supports_parallel_tool_calls works again when deferral is active. Unparseable wrappers stay sequential barriers; bridged calls get exactly the admission the same call gets direct. tool_search/tool_describe lookups batch concurrently. 2. _short_desc no longer truncates listing lines at 'e.g.', hostnames, or version strings — a sentence terminator must be followed by whitespace. 3. BM25 indexes the source label (e.g. 'linear' for mcp-linear), so service-name queries reach tools whose own name omits the service; the dead 'mcp' prefix token is stripped. 4. Substring-fallback docstring corrected (token misses, not zero-IDF). Salvaged from #92693 by @alt-glitch with authorship preserved.
Two interaction seams between the #92693 salvage (merged as #95050) and this branch: the source-label indexing test now compares in token space (the stemmer shortens 'catalogsource' to 'catalogsourc'), and the unregistered-core-name describe test forces the unregistered condition via monkeypatch instead of depending on which sibling test file imported model_tools first.
|
Closing — all five fixes were salvaged onto current main with your authorship preserved:
Your multi-query/stemming PR #92766 also landed via #95119. Excellent work across the set — the red-verified tests and live E2E evidence made these the easy kind of salvage. Thanks! |
…dexing (salvage NousResearch#92693, part 1) Four fixes to the tool-search deferral layer, split from PR NousResearch#92693 (the availability-cache staleness fix ships separately): 1. The parallel batch planner now peels the tool_call bridge wrapper and decides admission on the underlying tool — supports_parallel_tool_calls works again when deferral is active. Unparseable wrappers stay sequential barriers; bridged calls get exactly the admission the same call gets direct. tool_search/tool_describe lookups batch concurrently. 2. _short_desc no longer truncates listing lines at 'e.g.', hostnames, or version strings — a sentence terminator must be followed by whitespace. 3. BM25 indexes the source label (e.g. 'linear' for mcp-linear), so service-name queries reach tools whose own name omits the service; the dead 'mcp' prefix token is stripped. 4. Substring-fallback docstring corrected (token misses, not zero-IDF). Salvaged from NousResearch#92693 by @alt-glitch with authorship preserved.
Two interaction seams between the NousResearch#92693 salvage (merged as NousResearch#95050) and this branch: the source-label indexing test now compares in token space (the stemmer shortens 'catalogsource' to 'catalogsourc'), and the unregistered-core-name describe test forces the unregistered condition via monkeypatch instead of depending on which sibling test file imported model_tools first.
…dexing (salvage NousResearch#92693, part 1) Four fixes to the tool-search deferral layer, split from PR NousResearch#92693 (the availability-cache staleness fix ships separately): 1. The parallel batch planner now peels the tool_call bridge wrapper and decides admission on the underlying tool — supports_parallel_tool_calls works again when deferral is active. Unparseable wrappers stay sequential barriers; bridged calls get exactly the admission the same call gets direct. tool_search/tool_describe lookups batch concurrently. 2. _short_desc no longer truncates listing lines at 'e.g.', hostnames, or version strings — a sentence terminator must be followed by whitespace. 3. BM25 indexes the source label (e.g. 'linear' for mcp-linear), so service-name queries reach tools whose own name omits the service; the dead 'mcp' prefix token is stripped. 4. Substring-fallback docstring corrected (token misses, not zero-IDF). Salvaged from NousResearch#92693 by @alt-glitch with authorship preserved.
Two interaction seams between the NousResearch#92693 salvage (merged as NousResearch#95050) and this branch: the source-label indexing test now compares in token space (the stemmer shortens 'catalogsource' to 'catalogsourc'), and the unregistered-core-name describe test forces the unregistered condition via monkeypatch instead of depending on which sibling test file imported model_tools first.
Summary
Five bugs in the tool-search deferral layer, one PR. The headline:
supports_parallel_tool_callssilently stopped working the moment any MCP server was attached, because the batch planner treated the literal nametool_callas a sequential barrier. The rest: catalog listing lines truncated at "e.g.", service-name searches missing their own tools, a docstring describing impossible math, and a tool-availability cache that never noticed when a credential appeared.No new tools, no schema changes, no cache-shape changes beyond a one-time prefix bust on release (same as any tool-description edit).
Background: how deferred tool dispatch works
With many MCP servers attached, sending every tool's JSON schema to the model on every turn gets expensive. So Hermes defers them: the model sees three bridge tools instead, and loads schemas on demand.
flowchart LR subgraph model_sees [what the model sees] TS[tool_search] TD[tool_describe] TC[tool_call] end subgraph hidden [deferred catalog] L1[mcp__linear__get_issue] L2[mcp__linear__create_issue] G1[mcp__granola__get_meetings] DOTS[... 200 more] end TS -- "BM25 search" --> hidden TD -- "load one schema" --> hidden TC -- "unwrap + dispatch" --> hiddenA deferred call arrives as
tool_call(name="mcp__linear__get_issue", arguments={...}). The executor unwraps it and dispatches the real tool, so hooks, approvals, and guardrails all see the underlying name. That part worked. The problem was everything around dispatch that keyed on the literal stringtool_callinstead of unwrapping first.Fix 1: the parallel-execution barrier (the big one)
When the model emits several tool calls in one turn, a planner splits them into segments: runs of read-only/safe calls execute concurrently on a thread pool, everything else runs in order. Admission is by tool name.
The planner never unwrapped the bridge.
tool_callis not in any safe list, so it always fell through to "sequential barrier" — for every deferred tool, regardless of what was inside:flowchart TD subgraph before [before — bridge active] A1["tool_call(linear get_issue)"] --> P1{planner sees name tool_call} A2["tool_call(granola folders)"] --> P1 P1 -- "not in safe list" --> S1[sequential: one at a time] end subgraph after [after — planner peels the wrapper] B1["tool_call(linear get_issue)"] --> P2{peel to underlying tool} B2["tool_call(granola folders)"] --> P2 P2 -- "server opted in to parallel" --> R1[parallel: both at once] P2 -- "not opted in / unparseable" --> S2[sequential, as before] endThe practical effect: setting
supports_parallel_tool_calls: trueon an MCP server did nothing once deferral was active — which is always, since any MCP server activates the bridge. The config option and the bridge were mutually exclusive and nothing said so.The planner now peels the wrapper with the same parser the executor uses (
resolve_underlying_call) and decides admission on the underlying tool and its real arguments:tool_searchandtool_describeare stateless catalog reads, so a batch of them runs concurrently too.Reproduction
Live agent, two MCP servers with
supports_parallel_tool_calls: true, one prompt asking for both calls in a single batch. Tool start time = completed timestamp minus logged duration:Two tools make a small difference; a fan-out of eight I/O-bound calls pays the full serialization tax.
Fix 2: listing lines truncated at the first period
Tier-1 disclosure embeds a catalog listing: one line per tool,
name: first sentence of description. The "first sentence" cut searched for the first.,!, or?anywhere:Fetch a page from api.github.com and return the JSON body.Fetch a page from api.Create an issue (e.g. a bug report) in a repository.Create an issue (e.Upgrade to v1.2 of the schema and migrate all rows.Upgrade to v1.List repos! Supports pagination.List repos(terminator dropped)List repos!The listing is the model's only view of a deferred tool until it loads the schema, so garbled lines directly hurt tool selection. The boundary is now one regex: a terminator must be followed by whitespace or end-of-string, and
e.g.,i.e.,etc.don't count. Linear-time on hostile input (MCP descriptions are third-party), and byte-identical output on the 211 real MCP descriptions in a live install except the onee.g.case this exists to fix.Fix 3: searching for a service missed the service's tools
How the search works, in one paragraph
Each deferred tool becomes a small text document: its name split into words, its description, its parameter names. Queries score against these documents with BM25 — a term that appears in few documents is worth more than one that appears everywhere (IDF), a term matching a short document is worth more than the same match in a long one (length normalization). Top scores win.
What was wrong
Two things about how the documents were built:
create_issueliving on thelinearserver had no "linear" anywhere in its indexed text — so the obvious query"linear"returned nothing. Native MCP tools dodged this only because their names happen to embed the server (mcp__linear__create_issue).mcpprefix was indexed. Every native MCP tool document carried the tokenmcp, worth nearly zero information (it's in every document, so IDF collapses) but still able to match every tool at once and drown out the term the model actually meant.flowchart LR subgraph doc_before [indexed text — before] D1["mcp linear create issue + description + params"] end subgraph doc_after [indexed text — after] D2["linear create issue + description + params"] end Q1["query: linear"] -.->|"only matches if name contains it"| D1 Q1 ==>|"always matches via source label"| D2Now the group label (
linear, notmcp-linear) is indexed with every tool from that source — exactly once, whether or not the tool's own name already carries it, so native and plugin tools get the same term weight — and the dead prefix is stripped before tokenizing.What this deliberately does not do
No stemming, no synonyms, no field weights, no multi-query — those are a separate, larger change to the ranking itself. This fix only makes the document contain what it always should have.
Fix 4: a docstring describing impossible math
The search's substring fallback claimed to protect against "zero IDF" — a term appearing in every document scoring zero. With the IDF variant actually used,
log(1 + (N - df + 0.5)/(df + 0.5)), that cannot happen: the value is strictly positive for every df ≤ N. The fallback's real (and useful) job is total token misses —"hub"never tokenizes out ofgithub_create_issue, so nothing scores, and the substring pass catches it. The docstring and the user docs now describe the mechanism that exists.No behavior change. This matters because the wrong rationale invites a "fix" for a case that can't occur.
Fix 5: the tool list couldn't notice a credential appearing
get_tool_definitionsmemoizes its result. The cache key captured registry mutations (a counter that bumps on register/deregister) and config-file edits (mtime fingerprint) — but not the availability probes. Tools gated on external state (is Docker running? is this credential set?) run acheck_fn, and those verdicts are TTL-cached for 30 seconds precisely so they can flip when the world changes. Except a flip changed nothing in the memo key:sequenceDiagram participant W as world participant M as memo (get_tool_definitions) participant P as availability probes Note over M: before W->>M: request tool list M->>P: probe (first call, cache miss) P-->>M: unavailable → tool omitted W->>W: credential lands / daemon starts W->>M: request tool list (any time later) M-->>W: cache HIT — stale list, probes never re-run Note over M: after W->>M: request tool list M->>P: TTL-cached verdict snapshot in the key P-->>M: verdict flipped → key changed → recompute M-->>W: fresh list within ~35s worst caseThe key now includes an aggregate snapshot of every probe's verdict. The snapshot has its own short-TTL cache (keyed on registry scope, probe scope, and registry generation; cleared by
invalidate_check_fn_cache()), so a hot-path hit is one dictionary lookup and a flaky probe coalesces to at most one live re-probe per window instead of one per rebuild. A flip propagates within the probe TTL plus the snapshot TTL (~35s worst case), or immediately on explicit invalidation. Probes marked no-cache (local config reads) are excluded — their inputs are covered by the config fingerprint.The same staleness class existed at a second site: the executor's cache of which tools a session may reach through the bridge. It now keys on the verdict snapshot and the config fingerprint too. Both cache sites re-check the snapshot after computing and skip the store on mismatch, so a verdict flipping mid-compute can't park a fresh result under a stale key.
Also in this PR
enabled: autoandenabled: ondo the same thing today. That is now stated at all three surfaces (dataclass, shipped config comment, docs) along with whyautoremains the default: it reserves room for a future mode that inlines schemas when they fit and defers only when they don't, without breaking anyone who pinnedonoroff.Validation
tests/tools/test_deferral_fixes.py— 24 behavior-level tests at public seams (planner segment shapes, search results, listing lines,get_tool_definitions/ scope-cache output), not internals. Each fix's tests verified red against the unfixed code; the invariant tests (unparseable wrappers stay barriers, non-opted-in servers stay sequential, direct/bridged parity, fallback on token misses, memo still hits when verdicts are stable) hold on both sides.test_tool_search.py,test_tool_batch_segmentation.py,test_builtin_memory_disabled_surface.py,test_get_tool_definitions_cache_isolation.py,test_terminal_tool_requirements.py,test_mcp_reload_refreshes_cached_agents.py,test_discord_tool.py,test_kanban_tools.py,test_plugins_hub_perf_guard.py,test_x_search_tool.py,test_delegate_kanban_isolation.py.Cache impact
Fixes 1, 3, 4, 5 don't touch the request prefix at all (planner/dispatch/memo internals). Fix 2 changes listing bytes: one-time prefix bust on the release that ships it, identical to any listing edit; lines get slightly longer (real sentences instead of fragments), which can shift a borderline catalog's listing form — the byte-stability suite stayed green.