Repository navigation
feat(*): fold a large tool catalog by default, with the meta-pair reserved - #616
Conversation
3bf2976 to
4fd35bb
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: the connected-MCP notice must not promise meta-tools that the actual iteration omits.
Reviewed the PR diff and the surrounding registry, capability/hook, strategy, MCP lifecycle, packaged-agent, config, and RPC paths. I also checked AGENTS.md/CONTEXT.md constraints, relevant history and callers, backward compatibility, architecture boundaries, and whether the tests weakened prior coverage.
Verification: the canonical Python 3.12 + dev-extra run passed all 527 affected tests; Ruff check and format passed on all 15 touched Python files. An initial run without the dev extra stopped at 176 passed / 1 failed because the local raven_ppt workspace package was not installed; rerunning with the repository's canonical setup resolved that environment failure. The inline reproduction still demonstrates the blocking mismatch.
|
CI note: the one red check, unit (py3.12 / ubuntu-latest / 1/4), is not from this branch. tests/test_subagent_probe.py::TestAutomaticSnapshotVerification::test_a_snapshot_missing_the_model_choices_key_is_reverified resolves whatever ACP presets the host has, so it fails on the runner and passes on a developer machine; #618 restores the preset stub the replay dropped. This branch touches no probe, preset or registry code. It will be rebased onto main once that lands, to re-run on a green base. |
tools.disabled_tools could name tool_search and tool_call, and their absence from an array is how the fold reads "this request has no search route": it answers by shipping every schema. So an off switch there did not slim a folded request, it unfolded it -- and mid-turn, because the switch is read once per assembled array, which is the first segment of the prompt-cache prefix. One measured turn went from 12 tools to 61 between two model calls. The pair joins the MCP resource and prompt meta-tools in the reserved set: names the loop owns for its lifetime, reported once and ignored where the array is built. tools.tool_search.enabled is the switch that speaks for both. The settings page needs no change: ext.list already reports both as builtin, and the page draws a fixed control for those. The four packaged agents drop their now-inert rows, so tool_call joins their faces and the ledger tests record it; each also pins toolSearch.enabled to false, a no-op against today's default that keeps a curated face off the fold. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
A folded request carries the core tools and the meta-pair; every other schema is withheld. Nothing in the prompt says what is in the catalog: the runtime context named an MCP server only while it awaited authorization or was still connecting, so a connected one read as nothing at all and a model that did not think to search reported a capability it held as missing. One line per connected server, with its tool count, only while tool_search actually reaches the request -- below the fold those tools are in the array and the sentence would be false. That gate is ToolSearchController.search_visible, made public for it. The strategy also stops assuming the array's shape. Replay feeds a recorded array back through the same chain, so a corrupt recording arrived as an entry whose "function" is a string and ended the turn with a TypeError instead of reaching replay's own backstop. What it cannot read, it now counts as a tool and passes through untouched. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
tools.tool_search.enabled ships on. It costs a deploy under the threshold nothing -- the strategy drops tool_search from every request while the catalog fits -- so the switch only starts deciding anything once a deploy carries more tools than one request should, which is where injecting every schema burns context that scales with tool count and moves the prompt-cache prefix on every MCP connect. The two prerequisites are in: neither meta-tool can be switched off, and a folded request now names the connected MCP servers whose schemas it withheld. "No config" stops being a third state: a host that wires none takes the schema's, which folds. The premise tests that read the old default -- the DAG controls' route, the token_wise socket's strategy list -- say so, and the controls' route is pinned in both states now that the switch has two. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
…ered The connected-server notice took its count from the manager snapshot, which counts every wrapper a server registered. Two of those are not hidden by the fold: one the operator switched off, which search() filters out and tool_call refuses, and one named in toolSearch.always_visible, whose schema is in the array already. A server with every tool switched off was announced as a searchable catalog that cannot answer -- the false capability metadata this notice exists to prevent, inverted. The count is now the server's names that this turn's array carries and the fold withholds, and a server with none of them gets no line. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The case is about a tool_search the loop never registered still reaching the settings page, and it asserted the fixture was in that state. With the fold shipping on, the fixture no longer was -- so it now switches the fold off rather than leaning on the default, and the guard assertion keeps standing. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
The line told the model its schemas were folded away and named tool_search and tool_call as the way back. Neither claim is the runtime-context block's to make: it is rendered once per turn, while the fold is decided per model call against an array a before_iteration hook may narrow first (HookDecision.modified_tools) -- drop the meta-pair there and the strategy ships every schema, leaving the block directing the model to tools the request does not carry. What survives that timing is an inventory: this server is connected and offers N tools. The count is the server's tools this turn's array carries, so a switched -off one is still out of it, and the fold gate stays as noise control rather than as a claim. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
4fd35bb to
e7ffd8e
Compare
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the current PR diff after the rebase and the fix delta, including repository rules and runtime terminology, registry/capability/hook/strategy/MCP callers, relevant history, backward compatibility, architecture boundaries, and test-coverage changes. The prior blocker is resolved without overriding per-iteration tool narrowing: the notice is now stable turn-level inventory and makes no claim about the final request or an available route. No new correctness findings remain.
Verification: uv run --frozen --python 3.12 --extra dev pytest over the 11 affected test modules passed all 527 tests; Ruff check and format passed on all 15 touched Python files. The previous narrowed-array reproduction now yields the inventory notice while the request has no meta-tools, confirming the notice no longer promises them.
|
Not a blocker -- two small things beside the thread on schema.py, and one candidate I measured and dropped. (1) The new reserved-tool warning offers a remedy that does not work for half the names it fires on. wiring.py:124-131 ends "The entry has no effect; remove it, or turn the fold off with that setting." For a For the record, (2) No CHANGELOG entry, for two user-visible changes: the fold ships on, and I want to be straight about how much weight that carries, because I went looking for a rule and did not find one. Of the last eight commits on main that changed a default in (3) DROPPED, measured the other way: I expected the default flip to change what a normal install sends, and it does not. I built a bare loop and counted rather than reading the registration list: 32 catalog tools on a bare wiring, 33 with every switchable tool the bare wiring can add, against ALSO CHECKED AND SOUND: the reservation is a loud warning plus an ignore, not a silent drop, and it names the switch; |
|
Blocking: trajectory replay must preserve the recorded post-fold tool array. The warning observation also holds but does not add a blocker: disabling the fold does not unregister |
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: trajectory replay must preserve the recorded post-fold tool array.
The prior clean stance is superseded by new, independently reproduced evidence. run_replay constructs the now-default ToolSearchStrategy and then replaces only loop.tools; when a recording already contains the post-fold array, the strategy reads that small recorded array as below threshold and removes tool_search. The replay comparator then reports a tools divergence, fatal in strict mode.
Verification: a construction-level reproduction produced recorded=12 tools, live=11 tools, with tool_search missing and compare_llm_request returning field tools. uv run --frozen --python 3.12 --extra dev pytest tests/test_trajectory_replay.py tests/test_trajectory_regressions.py -x passed all 121 tests, demonstrating the coverage gap rather than disproving the reproduced failure. The reserved-warning wording is also misleading for tool_call, but remains nonblocking; the CHANGELOG suggestion is optional under current repository rules.
They pinned tools.toolSearch.enabled to false, which was written to keep four curated faces byte-identical while the default flipped. That reasoning only holds while their catalogs stay small, and it is not their config that decides the size: each serves over `raven acp`, so a dispatcher can hand it per-session MCP servers, and an operator can add tools.mcpServers at any time. The pin would have held the fold open at exactly the catalog size it exists for. The faces record tool_search instead. It registers with the default and the strategy drops it from every request while the catalog fits, so it is in the registry these fixtures read and in no request the model sees. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
Blocking: trajectory replay must preserve the recorded post-fold tool array.
Reviewed the full current diff and the new packaged-agent delta, including repository rules and runtime terminology, packaged-agent configuration/callers, registry-versus-request visibility, relevant history, backward compatibility, architecture boundaries, and test changes. The new commit correctly lets the four packaged products inherit the fold and updates their registry-face ledgers; it introduces no additional finding.
The existing replay blocker is untouched: the independent construction still reports recorded=12, live=11, missing tool_search, with compare_llm_request mismatching on tools. Verification: the 13 affected/replay test modules passed all 648 tests; Ruff check and format passed on the newly touched Python files. The passing tests do not cover a recorded post-fold array.
A replay reproduces a request; it does not assemble one. The harness builds its loop with no tool-search config, so the new default reached it -- and a recording made above the threshold already IS the folded array, core plus the meta-pair. Served through the strategy again it counts as a catalog under the threshold, tool_search is dropped from what the replay sends, and every strict regression case halts on a divergence in the offered tool names before the first recorded reply. Measured through the harness's own construction: 11 tools live against 12 recorded, tool_search missing. The harness now spells the switch out, which is what it always effectively had, and a case replays a recording carrying tool_search so the next default that leaks in is caught here rather than by a reader. Two more from the same review. The reserved-tool warning offered one remedy for two names that are not owned the same way: turning the fold off does stop tool_search registering and leaves tool_call exactly where it was, so an operator who wrote tool_call was sent to a setting that changes nothing for it. It now answers per name. And the changelog records both user-visible changes: the fold shipping on, and disabledTools no longer reaching the pair. Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the full current diff and the replay-fix delta, including repository rules and runtime terminology, replay construction and comparison, tool-strategy wiring, packaged-agent configuration, warning behavior, relevant callers/history, backward compatibility, architecture boundaries, and test changes. The replay harness now explicitly preserves recorded request arrays, and the regression test covers the folded recording that previously lost tool_search. No new correctness findings remain.
Verification: 650 affected and trajectory tests passed; the focused folded-recording regression passed; Ruff check and format passed on the newly touched Python files; git merge-tree --write-tree HEAD github/main completed without conflicts.
| return _Target(tool, None) | ||
|
|
||
| def _search_visible(self) -> bool: | ||
| def search_visible(self) -> bool: |
There was a problem hiding this comment.
Blocker - route the host's other two claims about the fold through this gate, or derive them from the post-fold array.
This method was made public for exactly this job. Its own docstring says it is "the honest gate on anything the host says about the fold", and _mcp_tool_notices now uses it. Two other host-side surfaces make a claim about the fold and do not. IdentitySegmentBuilder.build calls render.live_dispatch_tools(self._get_tool_definitions) (context_engine/segments/identity.py:85), and ActiveSkillsSegmentBuilder._require_tools calls filter_by_required_tools(skills, render.collect_tool_names(self._get_tool_definitions)) (context_engine/segments/active_skills.py:63). Both bindings are self.tools.get_definitions (agent/loop/main.py:438, :644) -- the registry, read during context assembly, before ToolSearchStrategy.before_llm_call folds anything.
run_subagent_dag is deliberately absent from DEFAULT_ALWAYS_VISIBLE (tool_search.py:62-73), so above compaction_threshold its schema is withheld while it stays registered. Both surfaces therefore still name it. The ## Delegation block renders the imperative "Hand the whole task over with spawn, or run_subagent_dag when several are involved" (context_engine/segments/render.py:314-320), and the DAG guide passes its requires.tools gate and is injected into every system prompt, since its frontmatter declares always: true, inject: description.
Reproduced. I built a main-agent loop with ToolSearchConfig(enabled=True), registered filler tools until the catalog reached 53, and ran ToolSearchStrategy.before_llm_call over the registry definitions:
post-fold array size : 12
spawn in request : True
run_subagent_dag in req : False
live_dispatch_tools() : ('spawn', 'run_subagent_dag')
always-skill gate, against the REGISTRY : ['subagent-dag-orchestration']
always-skill gate, against the REQUEST : withheld
The model is not stranded: run_subagent_dag is not schema-hidden, so tool_search finds it and tool_call invokes it. What it gets is a prompt carrying an imperative that names a tool absent from its array -- the class of defect the MCP notice was rewritten to avoid one commit earlier in this same PR.
The mechanism is pre-existing and this PR does not touch it: render.py:125, identity.py:85 and active_skills.py:63 are byte-identical at d50a13c. What changed is ToolSearchConfig.enabled: False -> True, which turns a dormant visibility decision into the default for every main agent whose catalog exceeds 50. Packaged products are unaffected, since all four list spawn and run_subagent_dag in tools.disabledTools, and sub-agents are unaffected, since WITHHELD_FROM_SUBAGENT covers both.
One line to correct either way: _require_tools's docstring states the set is "checked here, per turn, against the definitions actually being sent". That was true at d50a13c and is not true with the fold on.
No test pairs the fold with either surface. No test file referencing live_dispatch_tools or filter_by_required_tools mentions ToolSearchConfig.
Two directions close it and they are not equivalent. Gate both surfaces on the fold state the way the MCP notice now is, or add run_subagent_dag to DEFAULT_ALWAYS_VISIBLE and accept the schema cost. The first keeps the design the tuple's own comment argues for; the second abandons it. That choice is a design call rather than a patch, so I am not prescribing one.
| withheld.update(self.tools.resolve_configured(entry)) | ||
| withheld.update(self._unconfigured_tool_names()) | ||
| return frozenset(withheld - (RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES)) | ||
| return frozenset(withheld - (RESOURCE_TOOL_NAMES | PROMPT_TOOL_NAMES | META_TOOL_NAMES)) |
There was a problem hiding this comment.
Question - is this reservation meant to be one-sided, or does the charter path need it too?
This subtraction closes the tools.disabled_tools route to withholding the meta-pair, and the docstring above gives the reason: their absence from the array is how ToolSearchStrategy reads "this request has no search route", so an off switch would unfold the array rather than slim it.
ToolRegistry.withheld_names (agent/tools/registry.py:428-448) unions three sources, not one -- the loop's _withheld hook, which is this method, plus _turn_withheld and _charter_withheld. narrowed_tools (agent/subagent/charter.py:254-270) is allowlist-shaped, frozenset(name for name in offered if name not in keep), so a charter whose tools list does not name tool_search / tool_call withholds them, and this subtraction does not reach that route.
I want to be straight that I did not reproduce this and that the resulting behaviour looks correct rather than broken. A charter-narrowed turn hits the META_TOOL_NAMES <= present guard and takes the "expose everything rather than strand" branch, which is the right answer when the model has no search route. The array is already narrowed to keep as well, so unfolding it costs nothing unless a charter admits more than compaction_threshold tools while excluding the pair.
So this is a question about intent rather than a claim of defect. If a charter is expected to be able to take the pair away, the docstring's "an off switch here would not slim the array, it would unfold it" is worth qualifying, because that is precisely what the one remaining route does.
|
Blocking: 1 finding needs correction before this revision can merge; see the inline notes. Scope: I reviewed only the I examined the in-scope diff, the tool-search controller and strategy, the meta-tool reservation and both of its readers ( Confirmed findings:
Verification:
Checked and deliberately not reported:
|
…erved (#616) ## Summary Make progressive tool disclosure safe to ship on, then ship it on. `tools.toolSearch.enabled` now defaults to true. Below the compaction threshold that costs a deploy nothing (the strategy drops `tool_search` from every request while the catalog fits); above it, most schemas are withheld and reached through `tool_search` / `tool_call`, so context stops scaling with tool count and an MCP connect stops moving the prompt-cache prefix. Two things had to land first, and both are here: 1. **The meta-pair is reserved from `tools.disabledTools`.** Their absence from an array is how the fold reads "this request has no search route", and it answers by shipping every schema. So an off switch there did not slim a folded request, it unfolded it -- mid-turn, since the switch is read once per assembled array. A measured turn went from 12 tools to 61 between two model calls, discarding the whole cached prefix. They join the MCP resource and prompt meta-tools as names the loop owns for its lifetime; `tools.toolSearch.enabled` speaks for both. The settings page needs no change: `ext.list` already reports them as builtin and the page draws a fixed control for those. 2. **A folded request names the connected MCP servers whose schemas it withheld.** The runtime context named a server only while it awaited authorization or was still connecting, so a connected one read as nothing at all and a model that did not think to search reported a capability it held as missing. One line per server, counting the tools this turn's array carries and the fold removes -- not the manager's registered count, which includes a switched-off tool that search filters out and an always-visible one whose schema is present. Knock-on changes: the four packaged agents drop their now-inert disable rows, so `tool_call` joins their faces and the ledger tests record it; each pins `toolSearch.enabled` to false, which keeps a curated face off the fold. The strategy also stops assuming the array's shape -- replay feeds recorded arrays back through the same chain, and a corrupt recording used to end the turn with a TypeError instead of reaching replay's own backstop. This replaces #609, which was cut from the pre-rebuild main. The commits are the same four plus one test fixture adjustment; the only dropped hunk is an edit to `ui-web/src/live/090-extensions.js`, which the frontend rebuild deleted and whose behaviour the rebuilt settings page already has. ## Type - [ ] Fix - [x] Feature - [ ] Docs - [ ] CI / tooling - [ ] Refactor - [ ] Other ## Verification Run on the branch tip, against github/main at 52eb09b: - `uv run --frozen --python 3.12 --extra dev pytest -q` with SERPER_API_KEY unset: 24325 passed, 121 skipped, 0 failed. - `uv run ruff check raven/ tests/` and `ruff format --check` on every touched Python file: clean. - Both new guards were reverse-verified: with the reservation removed `test_disabled_tools_cannot_withhold_the_meta_tools` fails; with the notice filter removed `test_the_count_is_what_the_fold_withholds_not_what_the_server_registered` fails. - Note for reviewers: the packaged-agent face tests read SERPER_API_KEY from the environment despite being hermetic, so `web_search` joins the face and they fail on a machine that has one. That predates this branch and is untouched here. - [x] Relevant tests pass locally - [x] Relevant lint / type checks pass locally - [ ] User-facing docs or screenshots are updated when needed ## Risk User-visible: a deploy whose catalog passes `tools.toolSearch.compactionThreshold` (50 by default) now sends most tool schemas only on demand. A config file that spells `enabled: false` keeps today's behaviour with no action. Rollback is that one setting, or `compactionThreshold` raised past the catalog size. Also user-visible: `tools.disabledTools` entries naming `tool_search` or `tool_call` stop taking effect and are reported once at startup, and the four packaged agents now offer `tool_call`. - [x] Security impact considered - [x] Backward compatibility considered - [x] Rollback path is clear for risky changes ## Related Issues Replaces #609. N/A otherwise. --------- Co-authored-by: Claude (claude-opus-5[1m]) <noreply@anthropic.com>
Summary
Make progressive tool disclosure safe to ship on, then ship it on.
tools.toolSearch.enablednow defaults to true. Below the compaction thresholdthat costs a deploy nothing (the strategy drops
tool_searchfrom every requestwhile the catalog fits); above it, most schemas are withheld and reached through
tool_search/tool_call, so context stops scaling with tool count and an MCPconnect stops moving the prompt-cache prefix.
Two things had to land first, and both are here:
tools.disabledTools. Their absence from anarray is how the fold reads "this request has no search route", and it answers
by shipping every schema. So an off switch there did not slim a folded request,
it unfolded it -- mid-turn, since the switch is read once per assembled array.
A measured turn went from 12 tools to 61 between two model calls, discarding
the whole cached prefix. They join the MCP resource and prompt meta-tools as
names the loop owns for its lifetime;
tools.toolSearch.enabledspeaks forboth. The settings page needs no change:
ext.listalready reports them asbuiltin and the page draws a fixed control for those.
The runtime context named a server only while it awaited authorization or was
still connecting, so a connected one read as nothing at all and a model that
did not think to search reported a capability it held as missing. One line per
server, counting the tools this turn's array carries and the fold removes --
not the manager's registered count, which includes a switched-off tool that
search filters out and an always-visible one whose schema is present.
Knock-on changes: the four packaged agents drop their now-inert disable rows, so
tool_calljoins their faces and the ledger tests record it; each pinstoolSearch.enabledto false, which keeps a curated face off the fold. Thestrategy also stops assuming the array's shape -- replay feeds recorded arrays
back through the same chain, and a corrupt recording used to end the turn with a
TypeError instead of reaching replay's own backstop.
This replaces #609, which was cut from the pre-rebuild main. The commits are the
same four plus one test fixture adjustment; the only dropped hunk is an edit to
ui-web/src/live/090-extensions.js, which the frontend rebuild deleted and whosebehaviour the rebuilt settings page already has.
Type
Verification
Run on the branch tip, against github/main at 52eb09b:
uv run --frozen --python 3.12 --extra dev pytest -qwith SERPER_API_KEY unset:24325 passed, 121 skipped, 0 failed.
uv run ruff check raven/ tests/andruff format --checkon every touchedPython file: clean.
Both new guards were reverse-verified: with the reservation removed
test_disabled_tools_cannot_withhold_the_meta_toolsfails; with the noticefilter removed
test_the_count_is_what_the_fold_withholds_not_what_the_server_registeredfails.
Note for reviewers: the packaged-agent face tests read SERPER_API_KEY from the
environment despite being hermetic, so
web_searchjoins the face and they failon a machine that has one. That predates this branch and is untouched here.
Relevant tests pass locally
Relevant lint / type checks pass locally
User-facing docs or screenshots are updated when needed
Risk
User-visible: a deploy whose catalog passes
tools.toolSearch.compactionThreshold(50 by default) now sends most tool schemas only on demand. A config file that
spells
enabled: falsekeeps today's behaviour with no action. Rollback is thatone setting, or
compactionThresholdraised past the catalog size.Also user-visible:
tools.disabledToolsentries namingtool_searchortool_callstop taking effect and are reported once at startup, and the fourpackaged agents now offer
tool_call.Related Issues
Replaces #609. N/A otherwise.