feat(agent): withhold web_search without a key, and make tool capabilities visible - #312
Open
Handsome-wzw wants to merge 5 commits into
Open
feat(agent): withhold web_search without a key, and make tool capabilities visible#312Handsome-wzw wants to merge 5 commits into
Handsome-wzw wants to merge 5 commits into
Conversation
A WeChat user asked for a recommendation and the bot answered "the web search
tool is not configured here". That sentence was not the model's own phrasing --
it was the tool's error text, relayed outward.
`web_search` needs a Serper key and this deployment has never had one, but it
was registered unconditionally. So on every search-shaped question the model saw
the tool, reached for it, got back `Error: Serper API key not configured. Set it
in ~/.raven/config.json under tools.web.search.apiKey (or export
SERPER_API_KEY), then restart the gateway.`, and passed that on to whoever was
in the chat.
The rule was already written down one line below the offending registration, for
the media tools: *"a tool is registered only when the user configured it"*.
`web_search` sat immediately above that comment and was exempt from it. It is
now gated the same way.
**The gate asks the tool, not the config.** `WebSearchTool.api_key` resolves at
call time from the constructor value *or* `SERPER_API_KEY`, so reading
`tools.web.search.apiKey` alone would have withdrawn a working tool from any
deploy that exports the variable and configures nothing. Building the tool and
asking whether it found a key keeps one source of truth for that answer.
**The sub-agent surface had the same defect** (`subagent/manager.py`). A
sub-agent that reaches for a search it cannot run reports the failure to its
caller, and that text lands in the parent turn -- the same leak, one level down.
**The error message was also pointing at the wrong file.** It hard-coded
`~/.raven/config.json` while the gateway runs with `--config` elsewhere, so
following it meant editing a file the process never reads. It now names the path
actually in force. That text is reachable only if the key disappears after
registration, which is exactly why it should be right: it is the message for the
case the gate cannot cover.
This does not make search work -- that still needs a Serper key. It stops the
absence of one from being explained to end users in the tool's words.
`test_agent_loop_tool_search.py` asserted `loop.tools.has("web_search")` while
its fixture never supplied a key. The tool was its example of a cataloged domain
tool being folded away above the tool_search threshold, and it only qualified
because of the defect. The fixture now supplies a key, so the subject of the
test is present for the reason the test claims.
`brave_api_key` still names a Serper key across 13 sites in 6 files, left over
from Brave Search, and it does mislead -- someone will go looking for a Brave
key. The rename is mechanical but touches the sub-agent manager and three CLI
entry points while that area is being refactored, and it has nothing to do with
this defect. Worth its own change.
An offer-style stand-in, as `deep_research` uses, would be the richer answer:
a same-named tool that on a search query guides the deployer through setup
instead of vanishing. That is a design decision about a config surface, not a
defect fix, so it is left for the tool-configuration work.
- [x] Fix
- [ ] Feature
- [ ] Docs
- [ ] CI / tooling
- [ ] Refactor
- [ ] Other
```
uv run pytest 6364 passed, 32 skipped
uv run pytest tests/test_agent_loop_web_tools.py \
tests/test_agent_loop_tool_search.py 10 passed
uv run --extra dev ruff check raven tests scripts All checks passed
uv run --extra dev ruff format --check <changed> already formatted
```
Each of the three fixes was reverted in turn to confirm its test fails, and only
its test:
```
main loop registration made unconditional again
-> test_web_search_is_withheld_without_a_key FAILED (4 passed)
sub-agent registration made unconditional again
-> test_the_subagent_loop_applies_the_same_rule FAILED (4 passed)
error message hard-coded back to ~/.raven/config.json
-> test_the_unconfigured_error_names_the_config_actually_in_force FAILED (4 passed)
```
The new tests carry an autouse fixture that clears `SERPER_API_KEY`. Without it
they would pass for the wrong reason on any machine where a developer has
exported one -- which is the same class of accident as the pre-existing test
described above.
Behaviour change: on a deploy with no Serper key, `web_search` is absent from
the model's tool list rather than present and failing. A search-shaped question
now gets "I cannot search the web" in the model's own words instead of a
transcribed setup error. Deploys that do have a key -- in config or in the
environment -- are unaffected, and the env-var path has a test precisely because
it would have been the easy thing to break.
Rollback is a revert. No configuration is read differently and no state is
written.
- [x] Security impact considered
- [x] Backward compatibility considered
- [x] Rollback path is clear for risky changes
N/A
…them Five tools need an external credential and three rules decide whether they are offered, one per family, each a different shape. None of that is wrong where it sits. What is missing is anywhere to read it: an unconfigured tool is not registered, so the agent never offers it, no document lists it, and `raven doctor` reports on providers and memory but has never mentioned tools. A deployer cannot ask what this install is missing. Providers solved the same problem once. `providers.auth` declares what each connection method requires and `credential_status` is the single authority on whether one is usable, with an AST invariant enforcing that authority -- six surfaces had answered the question six ways and each looked reasonable alone. Tools never got the equivalent. Two pieces here. Each tool gains an `is_configured` classmethod, so the rule for a family lives with the credential it reads: `WebSearchTool` answers from the config value or SERPER_API_KEY, because those are two sources and only the tool consults both; the media base answers on a model *or* a key, which is what keeps an OpenRouter credential set for chat from switching on three tools that bill per call. Then `capabilities.py` describes the five for a human deciding what to set up -- what each does in one line, how much work it is, where the key goes, where to get one, and what it costs. It rules on nothing; it asks the tools. The description is pinned to the behaviour rather than trusted. The tests drive a real AgentLoop and compare what it registered against what the table predicts, and derive the gated set (tools present with credentials, absent without) instead of listing it -- so a sixth gated tool whose author forgets the table fails rather than going unnoticed. Two things the tests only caught on a mutation pass. A media case that sets both a model and an OpenRouter key proves nothing about the "or model" half, because the borrow fills the key in and a rule reading only the key still answers correctly; the case that pins it has nothing to borrow. And an autouse fixture clears SERPER_API_KEY, without which these pass for the wrong reason wherever a developer exported one. Registration still lives in the loop. Moving it onto the table is the point of this shape, but it edits a file under active change elsewhere and is worth doing on its own. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
`raven doctor` is where someone asks what this install is missing, and it has never mentioned tools. That gap became total once an unconfigured tool stopped being registered: nothing in a running Raven says the capability exists, so the only way to learn that web search is one edit and one account away was to read the source. The new section lists every credential-bearing tool whether or not it is set up, ordered by how much the deployer has to do. A configured one names where its key came from, and "borrowed from providers.openrouter" is the load-bearing half of that -- a row that cannot distinguish a reused credential from a missing one sends someone to create an account they already have. Each fact goes on its own line rather than into a sentence. The terminal wraps a long line mid-path, and a config key broken across two rows cannot be copied, which is the only thing that row is for. A test asserts the paths survive intact. Nothing here moves the exit code. An install without image generation is a choice, not a fault, and a doctor that fails on it teaches people to ignore doctor. The rows come from the capability table, which asks each tool. Doctor derives nothing itself: a fourth opinion about which tools are available is the failure the table exists to prevent. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
The media rows claimed "key: borrowed from providers.openrouter" unconditionally, including on a default install with no OpenRouter key anywhere. That instruction is wrong in the expensive direction: acting on it means setting a model, getting a registered tool -- a model alone counts as asking for one -- and watching every call fail on a credential the deployer was told they already had. The reuse line is now printed only when a key is genuinely there to pick up, from the provider entry or the environment. Three related misreports go with it. A satisfied row for a capability that cannot run. The state above is reachable today, and a green tick was the one thing it must not show, so it now renders as a warning naming the key to set. Warned rather than failed: unlike a memory role the server could not build, this failure is loud where it happens, the tool returning its missing-key error to the model on every call. A credential reported at the model's path. For this family `config_path` names the model, so reusing it as the key source pointed at a line holding no key. Capability.key_path names the credential field instead. OPENROUTER_API_KEY unseen. The tools resolve it at call time, so a report reading config alone answers "no credential" for a working install -- the same gap already fixed for SERPER_API_KEY on the search side, missed here. It is now a source like any other, in the table and in both directions of the report. Whether a key resolves is now its own fact, asked of the tools via has_key and exposed as has_credential, rather than inferred from an empty source string. Mutating the ruling to always answer yes changed no test in the first version of this fix, which is what showed the inference was load-bearing where the ruling was not: a fourth credential source would have been reported as a missing one. Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Handsome-wzw
force-pushed
the
feat/tool_capability_inventory_gh
branch
from
August 13, 2026 14:05
9182f4a to
bdbdd71
Compare
The coverage assertion claimed to catch a newly gated tool whose author had not
added it to the capability table. It did not. The configured fixture listed
image, speech and video by hand, so a fourth media tool was never switched on,
never appeared among the gated names, and the assertion held while the table was
already incomplete.
An open PR adding a MiniMax voice-clone tool is exactly that case. Merged
against this branch the assertion passed; with the list read from
MediaGenConfig it fails and names the tool:
gated but undeclared: ['voice_clone']
No behaviour change on this base, where the derivation returns the same three.
The point is that it stops returning three on its own.
Co-authored-by: Claude (claude-opus-5) <noreply@anthropic.com>
Collaborator
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
This was referenced Aug 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A WeChat user asked for a recommendation and the bot answered "the web search
tool is not configured here". That sentence was not the model's own phrasing --
it was the tool's error text, relayed outward.
web_searchneeds a Serper keyand that deployment has never had one, but the tool was registered
unconditionally, so on every search-shaped question the model saw it, reached
for it, and passed the setup error on to whoever was in the chat.
Withholding the tool fixes that and creates a second problem: an unregistered
tool is invisible. Nothing in a running Raven then says the capability exists at
all -- the model is never offered it, no document lists it, and
raven doctorreports on providers and memory but has never mentioned tools. The only way to
learn that web search is one account and one edit away was to read the source.
This does both halves, because either alone leaves a real gap: the deployer is
told what this install can and cannot do, and the model is still not offered a
tool it cannot run.
Withholding the unusable tool
web_searchis gated on a resolved key, in the main loop and in the sub-agentsurface (
subagent/manager.py) -- a sub-agent that reaches for a search itcannot run reports the failure to its caller, and that text lands in the parent
turn, the same leak one level down.
The gate asks the tool, not the config.
WebSearchTool.api_keyresolves at calltime from the constructor value or
SERPER_API_KEY, so readingtools.web.search.apiKeyalone would withdraw a working tool from any deploythat exports the variable and configures nothing.
The tool's error message also hard-coded
~/.raven/config.jsonwhile thegateway runs with
--configelsewhere, so following it meant editing a file theprocess never reads. It names the path actually in force now. That text is
reachable only if the key disappears after registration, which is exactly why it
should be right: it is the message for the case the gate cannot cover.
Three rules, and nowhere to read them
Registration is decided per family, each a different shape:
web_searchweb_fetchapi_keyor amodel, either counting as configuredFor media those are two questions, not one. A section naming only a model is
registered, because a model alone counts as asking for the tool, and then every
call returns a missing-key error. Whether a capability is offered and whether
it works come apart there, and a report treating them as one fact ticks a
capability that cannot run.
Each rule is defensible where it sits. What is missing is anywhere to read them.
Providers had the same sprawl once and answered it with
providers.auth: adeclarative table plus
credential_statusas the single authority, with an ASTinvariant enforcing that authority, because six surfaces had answered the same
question six ways and each looked reasonable alone. Tools never got the
equivalent.
What this adds
Each tool answers for itself, twice where the questions differ.
WebSearchTool.is_configuredreads the config value orSERPER_API_KEY,because those are two sources and only the tool consults both. The media base
answers
is_configuredon a model or a key -- which is what stops anOpenRouter credential set for chat from silently switching on three tools that
bill per call -- and answers
has_keyseparately on the chain it actuallyresolves at call time: its own section, the borrowed provider key, then
OPENROUTER_API_KEY. Both rules live with the credential they read.capabilities.pydescribes the five for a human deciding what to set up:what each does in one line, how much work it is (nothing / reuse a credential
you already have / obtain an account), where the key goes, where to get one, and
what it costs. It rules on nothing --
is_configuredandhas_credentialbothask the tools -- so it cannot become a second opinion.
raven doctorgrows a section, listing every capability configured or not,ordered by how much the deployer has to do:
Three deliberate details.
Naming the credential is load-bearing in both directions. A row that cannot
distinguish a reused credential from a missing one sends someone to create an
account they already have -- and a row that claims a reuse with nothing to reuse
is worse, because acting on it means setting a model, getting a registered tool,
and watching every call fail on a credential they were told they had. So the
reuse line prints only when a key is genuinely there to pick up, and a
capability already in that broken state says so outright instead of showing a
satisfied tick.
The credential is named at the path that holds it. For the media family
config_pathnames the model, so reusing it as the key source pointed thedeployer at a line with no credential in it.
And each fact is on its own line rather than in a sentence, because the terminal
wraps a long line mid-path and a config key broken across two rows cannot be
copied, which is the only thing that row is for.
Nothing here moves the exit code, including the warned row. An install without
image generation is a choice, not a fault, and a doctor that fails on it teaches
people to ignore doctor. The half-finished one is arguable -- the memory section
does exit non-zero for a role the user configured that the server could not
build -- but that failure is silent where it happens, recall just returning
nothing, whereas this one returns an error string to the model on every call.
Say the word and it becomes an exit code instead.
Not in this change
Registration still lives in
AgentLoop. Having it read the table is the pointof this shape and removes the last duplicate reader, but it edits
agent/loop/main.py, which is under active change, and it is worth doing on itsown once that settles. Until then the table is a description, and the tests
below are what keep it honest.
deep_researchis deliberately absent: it is moving to the sub-agent surfaceand its tool is going away.
Type
Mixed on purpose: the first commit is a fix and the rest are the feature it
made necessary. Splitting them would land a change that hides a capability
without landing the one that makes it discoverable.
Verification
The one failure is
test_read_file_image.py::test_an_attachment_that_cannot_be_ read_costs_a_note_not_the_turn, and it is not from this branch: it fails thesame way on
mainat1cb604awith these commits absent. The case makes a fileunreadable with
chmod 000, which does not block a root user, so it fails foranyone running the suite as root and passes in CI. This branch does not touch
that file or the code under it.
The capability tests drive a real
AgentLoopand compare what it registeredagainst what the table predicts, rather than asserting the table against itself.
The gated set is derived -- tools present once credentials are supplied, absent
without -- so a sixth gated tool whose author forgets the table fails here rather
than going unnoticed.
That last claim was false when first written, and the last commit is what makes
it true. The fixture listing the media tools by hand never switched a fourth one
on, so a new one never joined the gated set and the assertion held over an
already-incomplete table. #305, which adds a MiniMax voice-clone tool, is that
case: merged against this branch the assertion passed. Read from
MediaGenConfiginstead, it fails and names the tool --gated but undeclared: ['voice_clone'].Which means whichever of the two lands second turns this red, deliberately. The
fix is one table entry, and it needs the rule that PR settles:
voice_clonecounts as configured on
api_key or api_base or modelineffective_media_configwhile registration still gates onapi_key or model,so an
apiBase-only install with no MiniMax key is configured by one rule andwithheld by the other. That is the divergence this table exists to make visible,
and it is worth resolving there rather than papering over here.
Fifteen mutations, each caught:
Four of those tests exist only because a mutation pass found the earlier
versions insufficient, and the last rounds are why the shape changed. A media
case that sets both a model and an OpenRouter key proves nothing about the "or
model" half: the borrow fills the key in, so a rule reading only the key still
answers correctly. The case that pins it has nothing to borrow.
The sub-agent case is the same argument applied to the second call site: with
the gate present only in the main loop, every test above still passes, because
nothing was watching what the sub-agent surface registers.
More usefully, mutating
has_keyto always answer yes changed no test at all inthe first version, which said the ruling was not load-bearing: the doctor was
inferring "no credential" from an empty source string rather than from the
tool's answer, so a fourth credential source would have been reported as a
missing one.
has_credentialis now its own fact, asked of the tools, and thatmutation fails.
Separately, autouse fixtures clear
SERPER_API_KEYandOPENROUTER_API_KEY,without which several of these pass for the wrong reason on any machine where a
developer exported one.
Rendering was checked against a real config in six states -- nothing set, a
provider key present, a model with nothing to borrow, a tool with its own key, a
model plus a borrowable key, and an exported variable as the only source --
rather than only asserted on.
Risk
One behaviour change:
web_searchis no longer offered to the model when no keyresolves. That is the fix. A deployment that has a key, in config or in the
environment, is unaffected; one that has none was getting an error string in
place of an answer.
The rest is additive. No other registration logic changed, so which tools an
agent is offered is otherwise exactly what it was;
is_configuredmoved theexisting predicates onto the tools without altering them, and the mutation
results above are what pins that.
has_keyand_resolve_keyare new names forthe chain
api_keyalready resolved -- the property calls the extracted one, socallers see the same answers.
raven doctorgains a section and no new exit code. Its zero-network guaranteeholds: the table reads config and environment only.
Two allowlist entries were added to
test_only_the_auth_module_decides_configuredness_from_a_key, argued in place:subagent/manager.py, which asks the built tool whether a key resolved so anunusable search is withheld, and
capabilities.py, which reads keys to report-- which source supplied one, and whether one is there to reuse. Both ruling
halves are delegated to the tools, whose files were already listed.
Rollback is a revert. Nothing is written and no configuration is read
differently.
Related Issues
N/A