From 31e17052712d2427684a81e48a1b2d11afd6e6d9 Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Thu, 13 Aug 2026 03:33:10 +0000 Subject: [PATCH 1/6] fix(agent): withhold web_search when there is no key to search with 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 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 --- raven/agent/loop/main.py | 13 ++- raven/agent/subagent/manager.py | 7 +- raven/agent/tools/web.py | 12 ++- tests/test_agent_loop_tool_search.py | 4 + tests/test_agent_loop_web_tools.py | 152 +++++++++++++++++++++++++++ tests/test_provider_auth_method.py | 6 ++ 6 files changed, 189 insertions(+), 5 deletions(-) create mode 100644 tests/test_agent_loop_web_tools.py diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index ba7fbfcd..80d76a86 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -848,7 +848,18 @@ def _register_default_tools(self) -> None: extra_deny_patterns=self.exec_config.extra_deny_patterns, ) ) - self.tools.register(WebSearchTool(api_key=self.brave_api_key, proxy=self.web_proxy)) + # web_search needs a Serper key it does not have by default, and offering + # it anyway is worse than withholding it: the model reaches for it, the + # call fails, and the error text -- naming a config file and an env var -- + # gets relayed to whoever is on the other end of the channel. Ask the tool + # rather than the config, because it resolves the key at call time from + # either source; gating on `brave_api_key` alone would withdraw the tool + # from a deploy that only exports SERPER_API_KEY. + web_search = WebSearchTool(api_key=self.brave_api_key, proxy=self.web_proxy) + if web_search.api_key: + self.tools.register(web_search) + # web_fetch is unconditional by contrast: it works without a key, and the + # Jina one only upgrades the extraction. self.tools.register(WebFetchTool(api_key=self.jina_api_key, proxy=self.web_proxy)) # Media tools (image/speech/video) are opt-in: a tool is registered only # when the user configured it (a model or apiKey under tools.media.), diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index 2d4f74ba..ba7c4d79 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -202,7 +202,12 @@ async def _run_subagent_inner( extra_deny_patterns=self.exec_config.extra_deny_patterns, ) ) - tools.register(WebSearchTool(api_key=self.brave_api_key, proxy=self.web_proxy)) + # Withheld without a key, same as the main loop: a sub-agent that + # reaches for a search it cannot run reports the failure to its + # caller, and that text ends up in the parent turn. + web_search = WebSearchTool(api_key=self.brave_api_key, proxy=self.web_proxy) + if web_search.api_key: + tools.register(web_search) tools.register(WebFetchTool(api_key=self.jina_api_key, proxy=self.web_proxy)) system_prompt = self._build_subagent_prompt() diff --git a/raven/agent/tools/web.py b/raven/agent/tools/web.py index 622b308e..8d7bafcf 100644 --- a/raven/agent/tools/web.py +++ b/raven/agent/tools/web.py @@ -37,10 +37,16 @@ def api_key(self) -> str: async def execute(self, query: str, count: int | None = None, **kwargs: Any) -> str: if not self.api_key: + # Reachable only if the key goes away after registration, since the + # loops withhold this tool when there is none. Name the file actually + # in force: hard-coding ~/.raven/config.json sent anyone running with + # --config to edit a file the process never reads. + from raven.config.loader import get_config_path + return ( - "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." + f"Error: Serper API key not configured. Set it in {get_config_path()} " + "under tools.web.search.apiKey (or export SERPER_API_KEY), " + "then restart the gateway." ) try: diff --git a/tests/test_agent_loop_tool_search.py b/tests/test_agent_loop_tool_search.py index 10529df9..3800aa0d 100644 --- a/tests/test_agent_loop_tool_search.py +++ b/tests/test_agent_loop_tool_search.py @@ -61,6 +61,10 @@ def _make_loop(workspace: Path, cfg, strategies=None) -> AgentLoop: restrict_to_workspace=True, tool_search_config=cfg, strategies=strategies, + # web_search is the cataloged domain tool these tests fold away, and the + # loop only registers it when a search key resolves. Supplying one keeps + # the subject of the test present for the right reason. + brave_api_key="test-serper-key", ) diff --git a/tests/test_agent_loop_web_tools.py b/tests/test_agent_loop_web_tools.py new file mode 100644 index 00000000..54a2e596 --- /dev/null +++ b/tests/test_agent_loop_web_tools.py @@ -0,0 +1,152 @@ +"""Registration rules for the web tools inside ``_register_default_tools``. + +``web_search`` needs a Serper key. Registering it without one let the model +reach for a search it could not run, and the tool's error -- naming a config +file and an env var -- was relayed to whoever was on the other end of the +channel. It is withheld instead, on the same terms as the media tools right +below it. + +``web_fetch`` is the contrast and is asserted alongside: it works with no key, +so it is registered unconditionally and must stay that way. +""" + +from __future__ import annotations + +import tempfile +from pathlib import Path + +import pytest + +from raven.agent.loop import AgentLoop +from raven.agent.subagent.manager import SubagentManager +from raven.agent.tools.registry import ToolRegistry +from raven.agent.tools.web import WebSearchTool +from raven.providers.base import LLMProvider, LLMResponse + + +class _StubProvider(LLMProvider): + def __init__(self) -> None: + super().__init__(api_key="test") + + async def chat( + self, + messages, + tools=None, + model=None, + max_tokens=4096, + temperature=0.7, + reasoning_effort=None, + tool_choice=None, + ): + return LLMResponse(content="stub", finish_reason="stop") + + def get_default_model(self) -> str: + return "stub" + + +@pytest.fixture +def workspace(): + with tempfile.TemporaryDirectory() as td: + yield Path(td) + + +@pytest.fixture(autouse=True) +def _no_ambient_serper_key(monkeypatch: pytest.MonkeyPatch) -> None: + """The tool falls back to ``SERPER_API_KEY``, so a developer who exports one + would otherwise see these tests pass for the wrong reason.""" + monkeypatch.delenv("SERPER_API_KEY", raising=False) + + +def _loop(workspace: Path, **kw) -> AgentLoop: + return AgentLoop(provider=_StubProvider(), workspace=workspace, model="stub", **kw) + + +async def _noop(*_a, **_kw) -> None: + return None + + +def test_web_search_is_withheld_without_a_key(workspace) -> None: + loop = _loop(workspace) + + assert not loop.tools.has("web_search"), ( + "offering a search that cannot run makes the model relay the tool's setup error to the user" + ) + assert loop.tools.has("web_fetch"), "web_fetch needs no key and must stay unconditional" + + +def test_a_configured_key_registers_web_search(workspace) -> None: + loop = _loop(workspace, brave_api_key="sk-serper") + + assert loop.tools.has("web_search") + + +def test_the_env_var_alone_registers_web_search(workspace, monkeypatch: pytest.MonkeyPatch) -> None: + # The tool resolves its key at call time from the config value *or* + # SERPER_API_KEY, so a gate that reads only the config would withdraw the + # tool from a deploy that exports the variable and configures nothing. + monkeypatch.setenv("SERPER_API_KEY", "sk-from-env") + + loop = _loop(workspace) + + assert loop.tools.has("web_search") + + +def test_the_subagent_surface_applies_the_same_rule(workspace, monkeypatch: pytest.MonkeyPatch) -> None: + # A sub-agent reaching for a search it cannot run reports the failure to its + # caller, and that text lands in the parent turn. + # + # The manager builds its registry inside the run and keeps no reference, so + # the names are observed as they are registered. The collector opens before + # the manager is constructed and drops nothing on the floor: were a + # registration ever to happen outside a window, it would land in the previous + # run's list and be caught, rather than vanishing and leaving an assertion + # that passes over an empty list. + registered: list[list[str]] = [] + real = ToolRegistry.register + + def _spy(self, tool): # noqa: ANN001, ANN202 + real(self, tool) + assert registered, f"{tool.name} was registered outside a collection window" + registered[-1].append(tool.name) + + monkeypatch.setattr(ToolRegistry, "register", _spy) + # The run announces its result through the spine, which is not wired here and + # is not what this is about. + monkeypatch.setattr(SubagentManager, "_announce_result", _noop) + + async def _names(**kw) -> list[str]: + registered.append([]) + manager = SubagentManager(provider=_StubProvider(), workspace=workspace, model="stub", **kw) + await manager._run_subagent_inner("t1", "task", "label", {}, None, manager.provider, manager.model) + return registered[-1] + + import asyncio + + without = asyncio.run(_names()) + with_key = asyncio.run(_names(brave_api_key="sk-serper")) + + # Baselines first: an empty list would satisfy the "not in" assertion below + # without proving anything about the gate. + assert "read_file" in without and "web_fetch" in without, without + assert "read_file" in with_key and "web_fetch" in with_key, with_key + assert "web_search" not in without + assert "web_search" in with_key + + +@pytest.mark.asyncio +async def test_the_unconfigured_error_names_the_config_actually_in_force(tmp_path: Path) -> None: + """Reachable only if the key disappears after registration, but the message + used to hard-code ``~/.raven/config.json`` and send anyone running with + ``--config`` to edit a file the process never reads.""" + from raven.config.loader import get_config_path, set_config_path + + before = get_config_path() + chosen = tmp_path / "elsewhere.json" + try: + set_config_path(chosen) + out = await WebSearchTool().execute(query="anything") + finally: + set_config_path(before) + + assert str(chosen) in out + assert "~/.raven/config.json" not in out diff --git a/tests/test_provider_auth_method.py b/tests/test_provider_auth_method.py index 8d13f50c..87f9500e 100644 --- a/tests/test_provider_auth_method.py +++ b/tests/test_provider_auth_method.py @@ -436,6 +436,12 @@ def test_only_the_auth_module_decides_configuredness_from_a_key() -> None: # Not an LLM provider section: a tool's own key (deep research, media # generation, web search), the router's, or EverOS's. "raven/agent/loop/main.py", + # The sub-agent surface asks the same question the main loop does, about + # the same tool: whether web_search resolved a Serper key, so an unusable + # search is withheld rather than offered and failed. It asks the built + # tool rather than the config because the tool resolves from either the + # constructor value or SERPER_API_KEY. + "raven/agent/subagent/manager.py", "raven/agent/tools/deep_research.py", "raven/agent/tools/media_gen.py", "raven/agent/tools/web.py", From d80571b88447aa2c1a9d286f808816cf6ce83f07 Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:14:32 +0000 Subject: [PATCH 2/6] feat(agent): let each tool answer whether it is configured, and list 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) --- raven/agent/tools/capabilities.py | 173 +++++++++++++++++++++++++ raven/agent/tools/media_gen.py | 17 +++ raven/agent/tools/web.py | 12 ++ tests/test_provider_auth_method.py | 6 + tests/test_tool_capabilities.py | 199 +++++++++++++++++++++++++++++ 5 files changed, 407 insertions(+) create mode 100644 raven/agent/tools/capabilities.py create mode 100644 tests/test_tool_capabilities.py diff --git a/raven/agent/tools/capabilities.py b/raven/agent/tools/capabilities.py new file mode 100644 index 00000000..7f8aaf51 --- /dev/null +++ b/raven/agent/tools/capabilities.py @@ -0,0 +1,173 @@ +"""What each credential-bearing tool needs, in one place. + +Providers have had a declarative credential model for a while -- +``providers.auth`` names what each connection method requires and +``credential_status`` is the single authority on whether a provider is usable. +An AST invariant enforces that authority, because six surfaces once answered +the same question six ways and each looked reasonable alone. + +Tools never got the equivalent. Three rules decide whether a tool is offered to +the model, one per family, each a different shape: + + web_search a resolved key, asked of the built tool + web_fetch nothing -- always registered, a key only improves extraction + media x3 an api_key *or* a model, either one counting as configured + +Each rule is defensible where it sits. What is missing is anywhere to *read* +them. A deployer cannot ask what this install lacks, and since an unconfigured +tool stopped being registered there is no surface at all saying the capability +exists: the model is not offered it, no document lists it, and ``raven doctor`` +reports on providers and memory but has never mentioned tools. + +This module is that surface. It describes the rules rather than replacing them +-- ``is_configured`` mirrors the loop's judgement instead of inventing a second +one -- and ``tests/test_tool_capabilities.py`` pins the description against what +the loop actually registers, so the two cannot drift apart quietly. Drifting +descriptions of the same fact is the failure this exists to avoid repeating. + +``deep_research`` is deliberately absent: it is moving to the sub-agent surface +and its tool is going away. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from enum import Enum +from typing import TYPE_CHECKING, Any + +if TYPE_CHECKING: + from raven.config.schema import Config + + +class Need(Enum): + """What stands between a capability and working.""" + + #: Usable as shipped. A credential, if any, only improves it. + NOTHING = "nothing" + #: A credential this deployment already holds elsewhere; the capability + #: needs switching on, not a new account. + OWN_CREDENTIAL = "own_credential" + #: An account and key the deployer has to go and obtain. + NEW_ACCOUNT = "new_account" + + +@dataclass(frozen=True) +class Capability: + """One credential-bearing tool, described for whoever decides what to set up.""" + + #: The name the model sees, so a doctor row can be matched to a transcript. + tool: str + #: One line in the deployer's terms rather than the code's. + summary: str + need: Need + #: Dotted config path the deployer would edit to switch this on. + config_path: str = "" + #: Environment variable accepted instead of the config field, if any. + env_var: str = "" + #: Where to go when ``need`` is NEW_ACCOUNT. + obtain_from: str = "" + #: Attribute on ``tools.media`` backing this tool, for the media family. + media_attr: str = "" + #: Stated before the deployer switches it on rather than after: these cost + #: money per call, and one cannot run at all without prepaid credit. + cost_note: str = "" + + +#: Ordered by how much the deployer has to do, least first, because that is the +#: order the question "what am I missing" wants answering in. +CAPABILITIES: tuple[Capability, ...] = ( + Capability( + tool="web_fetch", + summary="Read a web page the agent already has the URL for", + need=Need.NOTHING, + config_path="tools.web.jinaApiKey", + ), + Capability( + tool="image_generate", + summary="Generate an image", + need=Need.OWN_CREDENTIAL, + config_path="tools.media.image.model", + media_attr="image", + cost_note="Billed per image.", + ), + Capability( + tool="text_to_speech", + summary="Generate speech from text", + need=Need.OWN_CREDENTIAL, + config_path="tools.media.speech.model", + media_attr="speech", + cost_note="Billed per call.", + ), + Capability( + tool="video_generate", + summary="Generate a video", + need=Need.OWN_CREDENTIAL, + config_path="tools.media.video.model", + media_attr="video", + cost_note="Billed per call; needs prepaid OpenRouter credit to run at all.", + ), + Capability( + tool="web_search", + summary="Search the web", + need=Need.NEW_ACCOUNT, + config_path="tools.web.search.apiKey", + env_var="SERPER_API_KEY", + obtain_from="https://serper.dev", + ), +) + + +def _resolved_media(cap: Capability, config: "Config") -> Any: + """This tool's media section with the OpenRouter borrow already applied.""" + return getattr(config.effective_media_config(), cap.media_attr) + + +def is_configured(cap: Capability, config: "Config") -> bool: + """Whether this capability would be offered to the model right now. + + Delegates to the tool rather than deciding here. The rule for each family + lives with the tool that owns the credential, so this module cannot become + a second opinion about configured-ness -- which is the divergence + ``providers.auth`` exists to prevent on the provider side, and the reason + an AST invariant guards it there. + """ + if cap.need is Need.NOTHING: + return True + if cap.media_attr: + from raven.agent.tools.media_gen import _OpenRouterMediaTool + + return _OpenRouterMediaTool.is_configured(_resolved_media(cap, config)) + + from raven.agent.tools.web import WebSearchTool + + return WebSearchTool.is_configured(config.tools.web.search.api_key) + + +def configured_from(cap: Capability, config: "Config") -> str: + """Where a satisfied capability got its credential, for a doctor row. + + Reads keys to *report* on them, never to rule on whether anything is set + up -- :func:`is_configured` answers that, and it asks the tools. The + distinction matters to a deployer: "reusing the OpenRouter key you already + have" and "needs a key" are different instructions, and a row that cannot + tell them apart sends someone to create an account they already have. + + Empty when the capability is unconfigured, and for the ones needing nothing + -- there is no credential to report on either. + """ + if cap.need is Need.NOTHING or not is_configured(cap, config): + return "" + if cap.media_attr: + # effective_media_config resolves the borrow, so a key present after it + # but absent in the raw section came from the provider entry. + raw = getattr(config.tools.media, cap.media_attr) + if not raw.api_key and _resolved_media(cap, config).api_key: + return "providers.openrouter.apiKey (borrowed)" + return cap.config_path + if config.tools.web.search.api_key: + return cap.config_path + return cap.env_var if os.environ.get(cap.env_var) else "" + + +__all__ = ["CAPABILITIES", "Capability", "Need", "configured_from", "is_configured"] diff --git a/raven/agent/tools/media_gen.py b/raven/agent/tools/media_gen.py index 6f64baeb..49ea59f3 100644 --- a/raven/agent/tools/media_gen.py +++ b/raven/agent/tools/media_gen.py @@ -90,6 +90,23 @@ def api_key(self) -> str: cfg_key = getattr(self._config, "api_key", "") if self._config else "" return cfg_key or os.environ.get("OPENROUTER_API_KEY", "") + @classmethod + def is_configured(cls, config: "MediaToolConfig | None") -> bool: + """Whether this deployment asked for the tool at all. + + A model *or* a key, matching what ``AgentLoop`` registers on: the key + alone would let an OpenRouter credential set for chat quietly switch on + three tools that bill per call, and the model alone would miss the + deployment that names no model and relies on ``default_model``. + + A classmethod because the caller deciding whether to offer the tool has + a config and no instance, and because the answer has to be askable + without building one. + """ + if config is None: + return False + return bool(config.api_key or config.model) + @property def api_base(self) -> str: cfg_base = getattr(self._config, "api_base", "") if self._config else "" diff --git a/raven/agent/tools/web.py b/raven/agent/tools/web.py index 8d7bafcf..8d981f4d 100644 --- a/raven/agent/tools/web.py +++ b/raven/agent/tools/web.py @@ -35,6 +35,18 @@ def api_key(self) -> str: """Resolve API key at call time so env/config changes are picked up.""" return self._init_api_key or os.environ.get("SERPER_API_KEY", "") + @classmethod + def is_configured(cls, config_key: str | None) -> bool: + """Whether a search key resolves, from the config value or the + environment. + + Asked of the tool rather than of the config because those are two + sources and only the tool consults both: a deployment that exports + ``SERPER_API_KEY`` and configures nothing is configured, and a caller + reading ``tools.web.search.apiKey`` alone would say otherwise. + """ + return bool(cls(api_key=config_key or None).api_key) + async def execute(self, query: str, count: int | None = None, **kwargs: Any) -> str: if not self.api_key: # Reachable only if the key goes away after registration, since the diff --git a/tests/test_provider_auth_method.py b/tests/test_provider_auth_method.py index 87f9500e..0cf02b37 100644 --- a/tests/test_provider_auth_method.py +++ b/tests/test_provider_auth_method.py @@ -456,6 +456,12 @@ def test_only_the_auth_module_decides_configuredness_from_a_key() -> None: # for display, rotate it -- rather than to rule on whether a provider is # set up. "raven/config/schema.py", + # Reports which source supplied a tool's key, so a deployer is told + # "reusing the OpenRouter key you already have" rather than "needs a + # key" and does not go and create an account twice. It rules on nothing: + # `is_configured` asks each tool, which is where that family's rule + # already lives, so this file cannot become a second opinion. + "raven/agent/tools/capabilities.py", "raven/config/update_providers.py", "raven/providers/litellm_provider.py", "raven/cli/_helpers.py", diff --git a/tests/test_tool_capabilities.py b/tests/test_tool_capabilities.py new file mode 100644 index 00000000..03905315 --- /dev/null +++ b/tests/test_tool_capabilities.py @@ -0,0 +1,199 @@ +"""The capability table must describe what AgentLoop actually does. + +A second description of an existing rule is only worth having while it stays +true. These drive a real ``AgentLoop`` and compare what it registered against +what the table predicts, for every combination that changes an answer -- so the +table cannot quietly become a third opinion about which tools are available. +""" + +from __future__ import annotations + +import tempfile +from pathlib import Path + +import pytest + +from raven.agent.loop import AgentLoop +from raven.agent.tools.capabilities import CAPABILITIES, Need, configured_from, is_configured +from raven.config.loader import load_config +from raven.providers.base import LLMProvider, LLMResponse + + +class _StubProvider(LLMProvider): + def __init__(self) -> None: + super().__init__(api_key="test") + + async def chat( + self, + messages, + tools=None, + model=None, + max_tokens=4096, + temperature=0.7, + reasoning_effort=None, + tool_choice=None, + ): + return LLMResponse(content="stub", finish_reason="stop") + + def get_default_model(self) -> str: + return "stub" + + +@pytest.fixture +def workspace(): + with tempfile.TemporaryDirectory() as td: + yield Path(td) + + +@pytest.fixture(autouse=True) +def _no_ambient_serper_key(monkeypatch: pytest.MonkeyPatch) -> None: + """web_search resolves its key from the environment too, so a developer who + exported one would otherwise see these pass for the wrong reason.""" + monkeypatch.delenv("SERPER_API_KEY", raising=False) + + +def _config(tmp_path: Path): + """A real Config with genuine defaults, independent of whoever runs this.""" + return load_config(tmp_path / "absent.json") + + +def _resolved(config, attr): + return getattr(config.effective_media_config(), attr) + + +def _loop(workspace: Path, config, **kw) -> AgentLoop: + return AgentLoop( + provider=_StubProvider(), + workspace=workspace, + model="stub", + media_config=config.effective_media_config(), + **kw, + ) + + +def test_the_table_names_exactly_the_credential_gated_tools(workspace, tmp_path: Path) -> None: + """Derived rather than listed: the tools that appear only once credentials + are supplied *are* the credential-gated ones, so this catches both halves -- + a table entry for a tool that no longer exists, and a newly gated tool whose + author forgot the table. The second is the one that puts a deployer back to + guessing what is missing.""" + bare = _config(tmp_path) + loop_bare = _loop(workspace, bare) + + full = _config(tmp_path) + full.tools.web.search.api_key = "sk-serper" + for attr in ("image", "speech", "video"): + getattr(full.tools.media, attr).model = "some/model" + full.providers.openrouter.api_key = "sk-or-test" + loop_full = _loop(workspace, full, brave_api_key="sk-serper") + + gated = set(loop_full.tools.tool_names) - set(loop_bare.tools.tool_names) + declared = {c.tool for c in CAPABILITIES if c.need is not Need.NOTHING} + + assert gated == declared, ( + f"gated but undeclared: {sorted(gated - declared)}; declared but not gated: {sorted(declared - gated)}" + ) + + +@pytest.mark.parametrize("cap", CAPABILITIES, ids=lambda c: c.tool) +def test_the_table_agrees_with_the_loop_when_unconfigured(cap, workspace, tmp_path: Path) -> None: + config = _config(tmp_path) + loop = _loop(workspace, config) + + assert is_configured(cap, config) is loop.tools.has(cap.tool), ( + f"{cap.tool}: table says configured={is_configured(cap, config)}, loop registered={loop.tools.has(cap.tool)}" + ) + + +def test_a_configured_search_key_agrees_on_both_sides(workspace, tmp_path: Path) -> None: + config = _config(tmp_path) + config.tools.web.search.api_key = "sk-serper" + loop = _loop(workspace, config, brave_api_key="sk-serper") + + cap = next(c for c in CAPABILITIES if c.tool == "web_search") + assert is_configured(cap, config) and loop.tools.has("web_search") + assert configured_from(cap, config) == "tools.web.search.apiKey" + + +def test_the_env_var_alone_agrees_on_both_sides(workspace, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The half of the deploys a config-only reading would answer wrong.""" + monkeypatch.setenv("SERPER_API_KEY", "sk-from-env") + config = _config(tmp_path) + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.tool == "web_search") + assert is_configured(cap, config) and loop.tools.has("web_search") + assert configured_from(cap, config) == "SERPER_API_KEY" + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_a_media_model_alone_agrees_on_both_sides(attr, tool, workspace, tmp_path: Path) -> None: + """A model with no key is the switch-on case: the key is borrowed.""" + config = _config(tmp_path) + getattr(config.tools.media, attr).model = "some/model" + config.providers.openrouter.api_key = "sk-or-test" + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert is_configured(cap, config) and loop.tools.has(tool) + assert configured_from(cap, config) == "providers.openrouter.apiKey (borrowed)" + + +def test_an_openrouter_key_alone_switches_nothing_on(workspace, tmp_path: Path) -> None: + """The property that makes the media rule "model *or* key" rather than + "resolved key": a chat credential must not silently enable three paid tools.""" + config = _config(tmp_path) + config.providers.openrouter.api_key = "sk-or-test" + loop = _loop(workspace, config) + + for cap in (c for c in CAPABILITIES if c.media_attr): + assert not is_configured(cap, config), cap.tool + assert not loop.tools.has(cap.tool), cap.tool + + +def test_the_free_capability_needs_no_credential(workspace, tmp_path: Path) -> None: + config = _config(tmp_path) + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.need is Need.NOTHING) + assert is_configured(cap, config) and loop.tools.has(cap.tool) + assert configured_from(cap, config) == "", "nothing was required, so nothing supplied it" + + +def test_every_entry_carries_what_a_deployer_has_to_act_on() -> None: + """The table is read by someone deciding what to go and do, so the fields + that tell them are not optional for the needs that require action.""" + for cap in CAPABILITIES: + assert cap.summary and cap.tool + if cap.need is Need.NEW_ACCOUNT: + assert cap.obtain_from, f"{cap.tool}: no url for an account the deployer must create" + assert cap.config_path, f"{cap.tool}: no config path to put the key in" + if cap.need is Need.OWN_CREDENTIAL: + assert cap.cost_note, f"{cap.tool}: switched on without saying it bills per call" + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_a_media_model_with_no_key_to_borrow_still_counts(attr, tool, workspace, tmp_path: Path) -> None: + """The half of the media rule the borrow hides. + + With an OpenRouter key present, `effective_media_config` fills `api_key` in, + so a rule reading only the key still answers correctly and a test that sets + both proves nothing about the `or model` half. With no key to borrow, the + model is the only thing making this configured -- and the loop registers it, + so the table must agree. + """ + config = _config(tmp_path) + getattr(config.tools.media, attr).model = "some/model" + assert not config.providers.openrouter.api_key, "this case needs nothing to borrow" + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert not _resolved(config, attr).api_key, "nothing should have been borrowed" + assert is_configured(cap, config) and loop.tools.has(tool) + assert configured_from(cap, config) == cap.config_path From 5a0f752ca5e55d6c97cc9beae011fc2e3e046b4c Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Thu, 13 Aug 2026 07:15:01 +0000 Subject: [PATCH 3/6] feat(cli): report tool capabilities in doctor, configured or not `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) --- raven/cli/doctor_commands.py | 109 ++++++++++++++++++++++++++++++ tests/test_cli_doctor_commands.py | 71 +++++++++++++++++++ 2 files changed, 180 insertions(+) diff --git a/raven/cli/doctor_commands.py b/raven/cli/doctor_commands.py index 3c2d8cc9..7fc7147b 100644 --- a/raven/cli/doctor_commands.py +++ b/raven/cli/doctor_commands.py @@ -27,6 +27,7 @@ from pathlib import Path from raven.config.raven import RavenConfig + from raven.config.schema import Config console = Console() @@ -132,6 +133,39 @@ class ConfigHealth: applied: list[str] = field(default_factory=list) +@dataclass +class ToolCapabilityInfo: + """One credential-bearing tool, as the deployer needs to see it. + + Reported whether or not it is configured, which is the point: an + unconfigured tool is not registered, so nothing else in the running system + mentions that the capability exists at all. + """ + + tool: str + summary: str + #: ``nothing`` / ``own_credential`` / ``new_account`` -- how much the + #: deployer has to do, which is a different question from configured-ness. + need: str + configured: bool + #: Where a configured one got its credential; empty when unconfigured or + #: when none was needed. + source: str = "" + config_path: str = "" + env_var: str = "" + obtain_from: str = "" + cost_note: str = "" + + +@dataclass +class ToolsInfo: + capabilities: list[ToolCapabilityInfo] = field(default_factory=list) + + @property + def unconfigured(self) -> list[ToolCapabilityInfo]: + return [c for c in self.capabilities if not c.configured] + + @dataclass class DoctorReport: version: int = 1 @@ -141,6 +175,7 @@ class DoctorReport: features: Optional[FeaturesInfo] = None gateway: Optional[GatewayInfo] = None memory: Optional[MemoryInfo] = None + tools: Optional[ToolsInfo] = None probe: Optional[ProbeResult] = None config_health: Optional[ConfigHealth] = None @@ -278,6 +313,34 @@ def _write_config_preserving_mode(path: "Path", raw: dict) -> None: _os.replace(tmp, path) +def _gather_tools(config: "Config") -> ToolsInfo: + """Every credential-bearing tool and whether this install can use it. + + Reads the capability table rather than re-deriving the rules: three + families decide registration three different ways, and a fourth opinion + here is how the answers drift apart. See + ``raven/agent/tools/capabilities.py``. + """ + from raven.agent.tools.capabilities import CAPABILITIES, configured_from, is_configured + + return ToolsInfo( + capabilities=[ + ToolCapabilityInfo( + tool=cap.tool, + summary=cap.summary, + need=cap.need.value, + configured=is_configured(cap, config), + source=configured_from(cap, config), + config_path=cap.config_path, + env_var=cap.env_var, + obtain_from=cap.obtain_from, + cost_note=cap.cost_note, + ) + for cap in CAPABILITIES + ] + ) + + def _gather_static_checks() -> DoctorReport: """Inspect config / routing / features. Strictly zero-network.""" from raven.config.loader import get_config_path, load_config @@ -350,6 +413,8 @@ def _gather_static_checks() -> DoctorReport: skill_forge_enabled=skill_forge_on, ) + report.tools = _gather_tools(config) + from raven.cli._gateway_lock import read_status info = read_status(now=time.time()) @@ -488,6 +553,46 @@ def _render_memory_capabilities(memory: MemoryInfo) -> None: console.print(f" [dim]Check the server log: {_server_log_hint()}[/dim]") +def _render_tool_capabilities(tools: ToolsInfo) -> None: + """List every credential-bearing tool, configured or not. + + An unconfigured tool is not registered, so the agent never offers it and no + other surface says it exists -- this is the only place a deployer can learn + the capability is available at all. Ordered by how much they would have to + do, so what is one edit away reads before what needs an account. + + Not a fault: an install with no image generation is a choice, so nothing + here moves the exit code. + """ + for cap in tools.capabilities: + label = f" {cap.tool + ':':<17}" + if cap.configured: + where = f" [dim]({cap.source})[/dim]" if cap.source else "" + console.print(f"{label}[green]✓[/green] {cap.summary}{where}") + continue + console.print(f"{label}[dim]- {cap.summary}[/dim]") + # One fact per line rather than one 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 this row is for. + indent = f"{'':<19}" + if cap.need == "own_credential": + console.print(f"{indent}[dim]switch on:[/dim] {cap.config_path}") + console.print(f"{indent}[dim]key: borrowed from providers.openrouter[/dim]") + elif cap.need == "new_account": + console.print(f"{indent}[dim]set:[/dim] {cap.config_path}") + if cap.env_var: + console.print(f"{indent}[dim]or env:[/dim] {cap.env_var}") + console.print(f"{indent}[dim]key from:[/dim] {cap.obtain_from}") + if cap.cost_note: + console.print(f"{indent}[dim]{cap.cost_note}[/dim]") + + if not tools.unconfigured: + return + console.print( + f" [dim]{len(tools.unconfigured)} capability(s) available but not set up; the agent is not offered them.[/dim]" + ) + + def _degradation_note(section: str) -> str: """What is lost by leaving an optional role unconfigured. @@ -590,6 +695,10 @@ def _render_human_output(report: DoctorReport) -> None: console.print(" Retrieval: [dim]unknown (the server you run is not answering)[/dim]") _render_memory_capabilities(memory) + if report.tools is not None: + console.print("\n[bold]Tool capabilities[/bold]") + _render_tool_capabilities(report.tools) + if report.probe is not None: console.print("\n[bold]LLM Probe[/bold]") if routing: diff --git a/tests/test_cli_doctor_commands.py b/tests/test_cli_doctor_commands.py index 305de0de..02480048 100644 --- a/tests/test_cli_doctor_commands.py +++ b/tests/test_cli_doctor_commands.py @@ -919,3 +919,74 @@ def test_doctor_prints_what_the_fix_applied(tmp_path, capsys) -> None: out = capsys.readouterr().out assert "fixed" in out assert "raven doctor --fix" not in out, "nothing left to apply, so nothing to advertise" + + +# ------------------------------------------------------- tool capabilities + + +@pytest.fixture(autouse=True) +def _no_ambient_serper_key(monkeypatch: pytest.MonkeyPatch) -> None: + """web_search resolves its key from the environment too, so a developer who + exported one would see these assert the wrong branch.""" + monkeypatch.delenv("SERPER_API_KEY", raising=False) + + +def test_doctor_lists_a_capability_that_is_not_configured(healthy_config: Path) -> None: + """The reason this section exists: an unconfigured tool is not registered, + so without it nothing in the running system says the capability is there.""" + result = runner.invoke(app, ["doctor"]) + + assert "Tool capabilities" in result.stdout + assert "web_search" in result.stdout + assert "serper.dev" in result.stdout, "a deployer cannot act without being told where to go" + assert "tools.web.search.apiKey" in result.stdout + + +def test_doctor_says_a_paid_capability_bills_before_it_is_switched_on(healthy_config: Path) -> None: + result = runner.invoke(app, ["doctor"]) + + assert "Billed per image." in result.stdout + assert "prepaid OpenRouter credit" in result.stdout, "video cannot run at all without it" + + +def test_doctor_reports_a_configured_capability_and_where_its_key_came_from(tmp_config: Path, tmp_path: Path) -> None: + cfg = Config() + cfg.agents.defaults.model = "anthropic/claude-sonnet-4-5" + cfg.agents.defaults.workspace = str(tmp_path / "workspace") + cfg.providers.anthropic.api_key = "sk-fake" + cfg.providers.openrouter.api_key = "sk-or-fake" + cfg.tools.media.image.model = "some/model" + save_config(cfg) + + result = runner.invoke(app, ["doctor"]) + + assert "image_generate" in result.stdout + assert "borrowed" in result.stdout, "the deployer should see it reused a key, not that it needs one" + + +def test_an_unconfigured_capability_is_not_a_failure(healthy_config: Path) -> None: + """An install without image generation is a choice, not a fault.""" + result = runner.invoke(app, ["doctor"]) + + assert result.exit_code == 0 + + +def test_tool_capabilities_reach_the_json_output(healthy_config: Path) -> None: + result = runner.invoke(app, ["doctor", "--json"]) + + payload = json.loads(result.stdout) + tools = payload["tools"]["capabilities"] + by_name = {c["tool"]: c for c in tools} + assert "web_search" in by_name and "web_fetch" in by_name + assert by_name["web_search"]["configured"] is False + assert by_name["web_search"]["obtain_from"] == "https://serper.dev" + assert by_name["web_fetch"]["configured"] is True + + +def test_a_config_path_is_never_split_across_lines(healthy_config: Path) -> None: + """These rows exist to be copied. A key wrapped mid-path is unusable, which + is why each fact is printed on its own line rather than in a sentence.""" + result = runner.invoke(app, ["doctor"]) + + for path in ("tools.web.search.apiKey", "tools.media.image.model", "SERPER_API_KEY"): + assert path in result.stdout, f"{path} was broken across a line wrap" From e36dad2684192a0442f7878a04c8bb4cc40854a9 Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Thu, 13 Aug 2026 12:54:18 +0000 Subject: [PATCH 4/6] fix(cli): stop telling deployers to reuse a key they do not have 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) --- raven/agent/tools/capabilities.py | 103 +++++++++++++++++++++--- raven/agent/tools/media_gen.py | 28 ++++++- raven/cli/doctor_commands.py | 57 +++++++++++--- tests/test_cli_doctor_commands.py | 58 +++++++++++++- tests/test_provider_auth_method.py | 12 +-- tests/test_tool_capabilities.py | 121 +++++++++++++++++++++++++++-- 6 files changed, 341 insertions(+), 38 deletions(-) diff --git a/raven/agent/tools/capabilities.py b/raven/agent/tools/capabilities.py index 7f8aaf51..546ad981 100644 --- a/raven/agent/tools/capabilities.py +++ b/raven/agent/tools/capabilities.py @@ -13,6 +13,13 @@ web_fetch nothing -- always registered, a key only improves extraction media x3 an api_key *or* a model, either one counting as configured +For the media family, being offered to the model and being usable are two +different questions: a section naming only a model is registered, because a +model alone counts as asking for the tool, and then every call fails on a +missing key. :func:`is_configured` answers the first and the tools' ``has_key`` +answers the second -- collapsing them is how a report ends up ticking a +capability that cannot run. + Each rule is defensible where it sits. What is missing is anywhere to *read* them. A deployer cannot ask what this install lacks, and since an unconfigured tool stopped being registered there is no surface at all saying the capability @@ -39,6 +46,10 @@ if TYPE_CHECKING: from raven.config.schema import Config +#: The credential the media family falls back to, named once so what a doctor +#: row tells the deployer to set cannot drift from what is read here. +_OPENROUTER_KEY = "providers.openrouter.apiKey" + class Need(Enum): """What stands between a capability and working.""" @@ -61,9 +72,11 @@ class Capability: #: One line in the deployer's terms rather than the code's. summary: str need: Need - #: Dotted config path the deployer would edit to switch this on. + #: Dotted config path the deployer would edit to switch this on. For the + #: media family this is the *model* field, not a credential -- see + #: :attr:`key_path`. config_path: str = "" - #: Environment variable accepted instead of the config field, if any. + #: Environment variable accepted instead of this capability's credential. env_var: str = "" #: Where to go when ``need`` is NEW_ACCOUNT. obtain_from: str = "" @@ -73,6 +86,16 @@ class Capability: #: money per call, and one cannot run at all without prepaid credit. cost_note: str = "" + @property + def key_path(self) -> str: + """Dotted path to this capability's own credential field. + + Distinct from :attr:`config_path`, which for the media family is the + model field: naming a model path as where a key came from sends the + deployer to edit a line that holds no credential. + """ + return f"tools.media.{self.media_attr}.apiKey" if self.media_attr else self.config_path + #: Ordered by how much the deployer has to do, least first, because that is the #: order the question "what am I missing" wants answering in. @@ -89,6 +112,7 @@ class Capability: need=Need.OWN_CREDENTIAL, config_path="tools.media.image.model", media_attr="image", + env_var="OPENROUTER_API_KEY", cost_note="Billed per image.", ), Capability( @@ -97,6 +121,7 @@ class Capability: need=Need.OWN_CREDENTIAL, config_path="tools.media.speech.model", media_attr="speech", + env_var="OPENROUTER_API_KEY", cost_note="Billed per call.", ), Capability( @@ -105,6 +130,7 @@ class Capability: need=Need.OWN_CREDENTIAL, config_path="tools.media.video.model", media_attr="video", + env_var="OPENROUTER_API_KEY", cost_note="Billed per call; needs prepaid OpenRouter credit to run at all.", ), Capability( @@ -144,6 +170,28 @@ def is_configured(cap: Capability, config: "Config") -> bool: return WebSearchTool.is_configured(config.tools.web.search.api_key) +def has_credential(cap: Capability, config: "Config") -> bool: + """Whether a credential actually resolves for this capability. + + A different question from :func:`is_configured`, which answers whether the + deployment asked for the tool. They come apart for the media family: a + section naming only a model is registered and offered to the model, and + every call then fails on a missing key. A report that collapses the two + ticks a capability that cannot run, which is the one thing it must not do. + + The same answer twice for the other families -- web_search is configured by + a resolved key and nothing else, and web_fetch needs none -- so this only + ever diverges where the rule itself does. + """ + if cap.need is Need.NOTHING: + return True + if cap.media_attr: + from raven.agent.tools.media_gen import _OpenRouterMediaTool + + return _OpenRouterMediaTool.has_key(_resolved_media(cap, config)) + return is_configured(cap, config) + + def configured_from(cap: Capability, config: "Config") -> str: """Where a satisfied capability got its credential, for a doctor row. @@ -153,21 +201,54 @@ def configured_from(cap: Capability, config: "Config") -> str: have" and "needs a key" are different instructions, and a row that cannot tell them apart sends someone to create an account they already have. - Empty when the capability is unconfigured, and for the ones needing nothing - -- there is no credential to report on either. + Empty when the capability is unconfigured, when it needs nothing, and -- + for the media family only -- when it is registered with no credential at + all: a section naming just a model is offered to the model and fails on + every call, so there is no source to name. + + Names a source; it does not rule on whether one exists. Callers wanting that + answer ask :func:`has_credential`, which asks the tools -- inferring it from + an empty string here would make the two facts one, and they are not. """ if cap.need is Need.NOTHING or not is_configured(cap, config): return "" if cap.media_attr: + if getattr(config.tools.media, cap.media_attr).api_key: + return cap.key_path # effective_media_config resolves the borrow, so a key present after it # but absent in the raw section came from the provider entry. - raw = getattr(config.tools.media, cap.media_attr) - if not raw.api_key and _resolved_media(cap, config).api_key: - return "providers.openrouter.apiKey (borrowed)" - return cap.config_path - if config.tools.web.search.api_key: - return cap.config_path + if _resolved_media(cap, config).api_key: + return f"borrowed: {_OPENROUTER_KEY}" + elif config.tools.web.search.api_key: + return cap.key_path + return cap.env_var if os.environ.get(cap.env_var) else "" + + +def borrowable_credential(cap: Capability, config: "Config") -> str: + """Where an unconfigured media capability would get its key once switched on. + + "Reuse the OpenRouter key you already have" and "get a key as well" are + different instructions, and only this tells them apart. Stating the first + unconditionally is worse than saying nothing: the deployer sets a model, + the tool is registered because a model alone counts, and every call then + fails on a credential they were told they already had. + + Empty for the other families, whose own rows already name what to set. + """ + if not cap.media_attr: + return "" + openrouter = config.providers.get("openrouter") + if openrouter and openrouter.api_key: + return _OPENROUTER_KEY return cap.env_var if os.environ.get(cap.env_var) else "" -__all__ = ["CAPABILITIES", "Capability", "Need", "configured_from", "is_configured"] +__all__ = [ + "CAPABILITIES", + "Capability", + "Need", + "borrowable_credential", + "configured_from", + "has_credential", + "is_configured", +] diff --git a/raven/agent/tools/media_gen.py b/raven/agent/tools/media_gen.py index 49ea59f3..25971918 100644 --- a/raven/agent/tools/media_gen.py +++ b/raven/agent/tools/media_gen.py @@ -85,10 +85,21 @@ def __init__( # ── config resolution (at call time, so env/config edits are picked up) ── + @staticmethod + def _resolve_key(config: "MediaToolConfig | None") -> str: + """The credential chain in one place: this tool's section, then the + shared environment variable. + + A static method so :meth:`has_key` can ask it without an instance -- + this base is abstract, and the callers deciding whether a capability + works hold a config and no tool. + """ + cfg_key = getattr(config, "api_key", "") if config else "" + return cfg_key or os.environ.get("OPENROUTER_API_KEY", "") + @property def api_key(self) -> str: - cfg_key = getattr(self._config, "api_key", "") if self._config else "" - return cfg_key or os.environ.get("OPENROUTER_API_KEY", "") + return self._resolve_key(self._config) @classmethod def is_configured(cls, config: "MediaToolConfig | None") -> bool: @@ -107,6 +118,19 @@ def is_configured(cls, config: "MediaToolConfig | None") -> bool: return False return bool(config.api_key or config.model) + @classmethod + def has_key(cls, config: "MediaToolConfig | None") -> bool: + """Whether a credential resolves for this tool. + + A different question from :meth:`is_configured`, which answers whether + the deployment asked for the tool at all: a section naming only a model + is registered and offered to the model, and then every call returns + :meth:`_no_key_error`. Asked of the tool because only the tool consults + both the section and ``OPENROUTER_API_KEY``, so a caller reading the + config alone answers wrong for every deployment that exports it. + """ + return bool(cls._resolve_key(config)) + @property def api_base(self) -> str: cfg_base = getattr(self._config, "api_base", "") if self._config else "" diff --git a/raven/cli/doctor_commands.py b/raven/cli/doctor_commands.py index 7fc7147b..49120092 100644 --- a/raven/cli/doctor_commands.py +++ b/raven/cli/doctor_commands.py @@ -151,7 +151,17 @@ class ToolCapabilityInfo: #: Where a configured one got its credential; empty when unconfigured or #: when none was needed. source: str = "" + #: Whether a credential resolves at all, which for the media family is not + #: the same as ``configured``: a model with no key is registered and fails + #: on every call. + has_credential: bool = True config_path: str = "" + #: Where this capability's own credential goes, which for the media family + #: is not ``config_path`` -- that one names the model. + key_path: str = "" + #: The credential an unconfigured one would pick up on being switched on; + #: empty when there is none to pick up, so the row can say so. + borrowable: str = "" env_var: str = "" obtain_from: str = "" cost_note: str = "" @@ -321,7 +331,13 @@ def _gather_tools(config: "Config") -> ToolsInfo: here is how the answers drift apart. See ``raven/agent/tools/capabilities.py``. """ - from raven.agent.tools.capabilities import CAPABILITIES, configured_from, is_configured + from raven.agent.tools.capabilities import ( + CAPABILITIES, + borrowable_credential, + configured_from, + has_credential, + is_configured, + ) return ToolsInfo( capabilities=[ @@ -331,7 +347,10 @@ def _gather_tools(config: "Config") -> ToolsInfo: need=cap.need.value, configured=is_configured(cap, config), source=configured_from(cap, config), + has_credential=has_credential(cap, config), config_path=cap.config_path, + key_path=cap.key_path, + borrowable=borrowable_credential(cap, config), env_var=cap.env_var, obtain_from=cap.obtain_from, cost_note=cap.cost_note, @@ -561,23 +580,43 @@ def _render_tool_capabilities(tools: ToolsInfo) -> None: the capability is available at all. Ordered by how much they would have to do, so what is one edit away reads before what needs an account. - Not a fault: an install with no image generation is a choice, so nothing - here moves the exit code. + A capability registered with no credential is warned about rather than + ticked, because that one is not a choice: the agent is offered a tool whose + every call returns a missing-key error. + + Still not a fault, though: an install with no image generation is a choice, + and the half-finished one fails loudly where it happens rather than + silently, so nothing here moves the exit code. """ + # One fact per line rather than one 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 these rows are for. + indent = f"{'':<19}" for cap in tools.capabilities: label = f" {cap.tool + ':':<17}" if cap.configured: where = f" [dim]({cap.source})[/dim]" if cap.source else "" - console.print(f"{label}[green]✓[/green] {cap.summary}{where}") + # The marker carries the answer too: a green tick above a line + # saying every call fails is the same misreport in miniature. + mark = "[green]✓[/green]" if cap.has_credential else "[yellow]![/yellow]" + console.print(f"{label}{mark} {cap.summary}{where}") + if not cap.has_credential: + console.print(f"{indent}[yellow]no key resolves; calls will fail[/yellow]") + console.print(f"{indent}[dim]set:[/dim] {cap.key_path}") + console.print(f"{indent}[dim]or env:[/dim] {cap.env_var}") continue console.print(f"{label}[dim]- {cap.summary}[/dim]") - # One fact per line rather than one 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 this row is for. - indent = f"{'':<19}" if cap.need == "own_credential": console.print(f"{indent}[dim]switch on:[/dim] {cap.config_path}") - console.print(f"{indent}[dim]key: borrowed from providers.openrouter[/dim]") + if cap.borrowable: + # "reusing" rather than "borrowed from" because the same line + # covers a provider entry and an exported variable. + console.print(f"{indent}[dim]key: reusing[/dim] {cap.borrowable}") + else: + # Claiming the borrow with nothing to borrow sends the deployer + # to set a model and land in the case flagged above. + console.print(f"{indent}[dim]also set:[/dim] {cap.key_path}") + console.print(f"{indent}[dim]or env:[/dim] {cap.env_var}") elif cap.need == "new_account": console.print(f"{indent}[dim]set:[/dim] {cap.config_path}") if cap.env_var: diff --git a/tests/test_cli_doctor_commands.py b/tests/test_cli_doctor_commands.py index 02480048..a2094ba0 100644 --- a/tests/test_cli_doctor_commands.py +++ b/tests/test_cli_doctor_commands.py @@ -925,10 +925,16 @@ def test_doctor_prints_what_the_fix_applied(tmp_path, capsys) -> None: @pytest.fixture(autouse=True) -def _no_ambient_serper_key(monkeypatch: pytest.MonkeyPatch) -> None: - """web_search resolves its key from the environment too, so a developer who - exported one would see these assert the wrong branch.""" - monkeypatch.delenv("SERPER_API_KEY", raising=False) +def _no_ambient_keys(monkeypatch: pytest.MonkeyPatch) -> None: + """These credentials resolve from the environment too, so a developer who + exported one would see these assert the wrong branch. + + ``OPENROUTER_API_KEY`` counts as much as the search key: it is what the + media family falls back to, so exporting it turns the "nothing to borrow" + rows into "borrowing from the environment" rows. + """ + for var in ("SERPER_API_KEY", "OPENROUTER_API_KEY"): + monkeypatch.delenv(var, raising=False) def test_doctor_lists_a_capability_that_is_not_configured(healthy_config: Path) -> None: @@ -964,6 +970,47 @@ def test_doctor_reports_a_configured_capability_and_where_its_key_came_from(tmp_ assert "borrowed" in result.stdout, "the deployer should see it reused a key, not that it needs one" +def test_doctor_does_not_offer_a_borrow_it_cannot_make(healthy_config: Path) -> None: + """This config has no OpenRouter key, so "reuse the one you have" is false. + + It is false in the expensive direction: acting on it means setting a model, + getting a registered tool -- a model alone counts -- and every call failing + on a credential the deployer was told they already had. + """ + result = runner.invoke(app, ["doctor"]) + + assert "image_generate" in result.stdout + assert "borrow" not in result.stdout, "claimed a reuse with nothing to reuse" + assert "tools.media.image.apiKey" in result.stdout, "must name the key it actually needs" + assert "OPENROUTER_API_KEY" in result.stdout + + +def test_doctor_flags_a_capability_registered_without_a_key(tmp_config: Path, tmp_path: Path) -> None: + """A media model with no key from any source is offered to the model and + fails on every call. A satisfied row is the one thing that must not say.""" + cfg = Config() + cfg.agents.defaults.model = "anthropic/claude-sonnet-4-5" + cfg.agents.defaults.workspace = str(tmp_path / "workspace") + cfg.providers.anthropic.api_key = "sk-fake" + cfg.tools.media.image.model = "some/model" + save_config(cfg) + + result = runner.invoke(app, ["doctor"]) + + assert "no key resolves" in result.stdout + assert "tools.media.image.apiKey" in result.stdout + # Warned, not failed. Unlike a memory role the server could not build, this + # failure is loud where it happens -- the tool returns its missing-key error + # to the model -- so the section stays advisory, as the rest of it is. + assert result.exit_code == 0 + + # Two fields rather than one, because collapsing them is what hid this + # state: registered *and* unusable is not reachable through either alone. + payload = json.loads(runner.invoke(app, ["doctor", "--json"]).stdout) + image = next(c for c in payload["tools"]["capabilities"] if c["tool"] == "image_generate") + assert image["configured"] is True and image["has_credential"] is False + + def test_an_unconfigured_capability_is_not_a_failure(healthy_config: Path) -> None: """An install without image generation is a choice, not a fault.""" result = runner.invoke(app, ["doctor"]) @@ -981,6 +1028,9 @@ def test_tool_capabilities_reach_the_json_output(healthy_config: Path) -> None: assert by_name["web_search"]["configured"] is False assert by_name["web_search"]["obtain_from"] == "https://serper.dev" assert by_name["web_fetch"]["configured"] is True + assert by_name["image_generate"]["key_path"] == "tools.media.image.apiKey", ( + "the key path is not the model path this row switches on" + ) def test_a_config_path_is_never_split_across_lines(healthy_config: Path) -> None: diff --git a/tests/test_provider_auth_method.py b/tests/test_provider_auth_method.py index 0cf02b37..4f7a8caa 100644 --- a/tests/test_provider_auth_method.py +++ b/tests/test_provider_auth_method.py @@ -456,11 +456,13 @@ def test_only_the_auth_module_decides_configuredness_from_a_key() -> None: # for display, rotate it -- rather than to rule on whether a provider is # set up. "raven/config/schema.py", - # Reports which source supplied a tool's key, so a deployer is told - # "reusing the OpenRouter key you already have" rather than "needs a - # key" and does not go and create an account twice. It rules on nothing: - # `is_configured` asks each tool, which is where that family's rule - # already lives, so this file cannot become a second opinion. + # Reports which source supplied a tool's key, and whether one is there + # to reuse, so a deployer is told "reusing the OpenRouter key you + # already have" rather than "needs a key" and does not go and create an + # account twice -- or, when nothing is there, is not told to reuse a + # credential that does not exist. It rules on nothing: `is_configured` + # and `has_credential` both ask the tools, which is where each family's + # rule already lives, so this file cannot become a second opinion. "raven/agent/tools/capabilities.py", "raven/config/update_providers.py", "raven/providers/litellm_provider.py", diff --git a/tests/test_tool_capabilities.py b/tests/test_tool_capabilities.py index 03905315..847aa4fe 100644 --- a/tests/test_tool_capabilities.py +++ b/tests/test_tool_capabilities.py @@ -14,7 +14,14 @@ import pytest from raven.agent.loop import AgentLoop -from raven.agent.tools.capabilities import CAPABILITIES, Need, configured_from, is_configured +from raven.agent.tools.capabilities import ( + CAPABILITIES, + Need, + borrowable_credential, + configured_from, + has_credential, + is_configured, +) from raven.config.loader import load_config from raven.providers.base import LLMProvider, LLMResponse @@ -46,10 +53,17 @@ def workspace(): @pytest.fixture(autouse=True) -def _no_ambient_serper_key(monkeypatch: pytest.MonkeyPatch) -> None: - """web_search resolves its key from the environment too, so a developer who - exported one would otherwise see these pass for the wrong reason.""" - monkeypatch.delenv("SERPER_API_KEY", raising=False) +def _no_ambient_keys(monkeypatch: pytest.MonkeyPatch) -> None: + """Every credential here also resolves from the environment, so a developer + who exported one would otherwise see these pass for the wrong reason. + + ``OPENROUTER_API_KEY`` matters as much as the search key: it is a third + source for the media family, behind the section and the borrow, and a case + asserting that nothing was available to borrow silently stops testing that + on any machine that exports it. + """ + for var in ("SERPER_API_KEY", "OPENROUTER_API_KEY"): + monkeypatch.delenv(var, raising=False) def _config(tmp_path: Path): @@ -139,7 +153,7 @@ def test_a_media_model_alone_agrees_on_both_sides(attr, tool, workspace, tmp_pat cap = next(c for c in CAPABILITIES if c.tool == tool) assert is_configured(cap, config) and loop.tools.has(tool) - assert configured_from(cap, config) == "providers.openrouter.apiKey (borrowed)" + assert configured_from(cap, config) == "borrowed: providers.openrouter.apiKey" def test_an_openrouter_key_alone_switches_nothing_on(workspace, tmp_path: Path) -> None: @@ -173,6 +187,11 @@ def test_every_entry_carries_what_a_deployer_has_to_act_on() -> None: assert cap.config_path, f"{cap.tool}: no config path to put the key in" if cap.need is Need.OWN_CREDENTIAL: assert cap.cost_note, f"{cap.tool}: switched on without saying it bills per call" + # The row that says "you already have this key" has to name where, + # and the doctor prints both unguarded -- a blank one renders as an + # instruction with the answer missing. + assert cap.env_var, f"{cap.tool}: reuses a credential without naming the variable" + assert cap.key_path != cap.config_path, f"{cap.tool}: model path doubling as the key path" @pytest.mark.parametrize( @@ -196,4 +215,92 @@ def test_a_media_model_with_no_key_to_borrow_still_counts(attr, tool, workspace, cap = next(c for c in CAPABILITIES if c.tool == tool) assert not _resolved(config, attr).api_key, "nothing should have been borrowed" assert is_configured(cap, config) and loop.tools.has(tool) - assert configured_from(cap, config) == cap.config_path + # Registered, and unusable: no key resolves from the section, the borrow, or + # the environment, so every call returns the tool's missing-key error. There + # is no source to name, and naming the model path -- the only path this + # capability has -- would tell the deployer a key sits somewhere it does not. + assert configured_from(cap, config) == "" + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_a_media_key_is_reported_at_its_own_path_not_the_model_path(attr, tool, workspace, tmp_path: Path) -> None: + """``config_path`` names the model for this family, so reusing it as the + credential source sends the deployer to edit a line holding no key.""" + config = _config(tmp_path) + getattr(config.tools.media, attr).api_key = "sk-tool-own" + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert is_configured(cap, config) and loop.tools.has(tool) + assert configured_from(cap, config) == f"tools.media.{attr}.apiKey" + assert configured_from(cap, config) != cap.config_path + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_a_media_key_from_the_environment_is_a_source_like_any_other( + attr, tool, monkeypatch: pytest.MonkeyPatch, workspace, tmp_path: Path +) -> None: + """The tool resolves ``OPENROUTER_API_KEY`` at call time, so a report that + consults only config answers "no credential" for a working install.""" + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-from-env") + config = _config(tmp_path) + getattr(config.tools.media, attr).model = "some/model" + loop = _loop(workspace, config) + + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert is_configured(cap, config) and loop.tools.has(tool) + assert configured_from(cap, config) == "OPENROUTER_API_KEY" + # The half that decides whether the row carries a warning. This install + # works, so calling it keyless would send a deployer to fix what is not + # broken -- and only the tool's chain reaches the variable to know that. + assert has_credential(cap, config) + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_no_borrow_is_claimed_when_there_is_nothing_to_borrow(attr, tool, tmp_path: Path) -> None: + """The instruction "reuse the key you already have" is wrong when no key + exists, and wrong in the expensive direction: it is the reason a deployer + sets a model, gets a registered tool, and sees every call fail.""" + config = _config(tmp_path) + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert not config.providers.openrouter.api_key, "this case needs nothing to borrow" + + assert borrowable_credential(cap, config) == "" + + config.providers.openrouter.api_key = "sk-or-test" + assert borrowable_credential(cap, config) == "providers.openrouter.apiKey" + + +@pytest.mark.parametrize( + ("attr", "tool"), + [("image", "image_generate"), ("speech", "text_to_speech"), ("video", "video_generate")], +) +def test_an_exported_key_is_reusable_too(attr, tool, monkeypatch: pytest.MonkeyPatch, tmp_path: Path) -> None: + """A deployment that exports the key and configures nothing needs the model + and nothing else, so telling it to go and set a key is the same misreport in + the other direction.""" + monkeypatch.setenv("OPENROUTER_API_KEY", "sk-from-env") + config = _config(tmp_path) + cap = next(c for c in CAPABILITIES if c.tool == tool) + assert not config.providers.openrouter.api_key, "the environment must be the only source" + + assert borrowable_credential(cap, config) == "OPENROUTER_API_KEY" + + +def test_only_the_media_family_borrows(tmp_path: Path) -> None: + """web_search cannot reuse anything -- its row already names what to get -- + and web_fetch needs nothing at all.""" + config = _config(tmp_path) + config.providers.openrouter.api_key = "sk-or-test" + + for cap in (c for c in CAPABILITIES if not c.media_attr): + assert borrowable_credential(cap, config) == "", cap.tool From 669db655a085a7625dbf8cf1152ed5e195c44b28 Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Thu, 13 Aug 2026 14:11:47 +0000 Subject: [PATCH 5/6] test(agent): derive the gated media tools instead of listing them 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) --- tests/test_tool_capabilities.py | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/test_tool_capabilities.py b/tests/test_tool_capabilities.py index 847aa4fe..df122b24 100644 --- a/tests/test_tool_capabilities.py +++ b/tests/test_tool_capabilities.py @@ -75,6 +75,20 @@ def _resolved(config, attr): return getattr(config.effective_media_config(), attr) +def _media_attrs() -> list[str]: + """Every media tool the schema declares, asked of the schema. + + Listing them here instead would make the coverage test below blind to the + one case it exists to catch: a media tool added to the schema and to + registration but not to the table. A hardcoded list never configures the new + tool, so it never appears among the gated ones, so the assertion holds while + the table is already wrong. + """ + from raven.config.schema import MediaGenConfig, MediaToolConfig + + return [n for n, f in MediaGenConfig.model_fields.items() if f.annotation is MediaToolConfig] + + def _loop(workspace: Path, config, **kw) -> AgentLoop: return AgentLoop( provider=_StubProvider(), @@ -96,7 +110,7 @@ def test_the_table_names_exactly_the_credential_gated_tools(workspace, tmp_path: full = _config(tmp_path) full.tools.web.search.api_key = "sk-serper" - for attr in ("image", "speech", "video"): + for attr in _media_attrs(): getattr(full.tools.media, attr).model = "some/model" full.providers.openrouter.api_key = "sk-or-test" loop_full = _loop(workspace, full, brave_api_key="sk-serper") From 3fd13c9a680c2d573b08c3396a72913ee4ae55ba Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:37:17 +0000 Subject: [PATCH 6/6] fix(cli): tell a switched-off capability apart from an unconfigured one `is_configured` answered the credential gate while its own docstring claimed to answer whether the tool "would be offered to the model right now". Those come apart: `tools.disabledTools` is applied after registration, in `AgentLoop._apply_disabled_tools`. A deployment with a Serper key and `disabledTools: ["web_search"]` therefore got a green doctor row naming `tools.web.search.apiKey` for a tool the agent does not hold -- the report claiming a capability is on offer when Raven has explicitly removed it. Folding it into `configured` would be the wrong repair. A switched-off tool usually has its credential set, so calling it unconfigured sends the deployer to set a key that is already there. It is its own state and it reads as one: `is_disabled` beside `is_configured`, and `is_offered` for the question the registry actually answers. The doctor row says which decision hid the tool, and where that decision lives, so it can be undone in the one place that made it. The parametrised test is the guard that matters: every capability, switched on and then off by name, compared against a real AgentLoop's final registry. Writing it caught a second thing worth knowing -- the loop is told what is disabled through an argument, not by reading the config, so a test that only sets the field proves nothing. Co-authored-by: Claude (claude-opus-5) --- raven/agent/tools/capabilities.py | 29 ++++++++++- raven/cli/doctor_commands.py | 22 ++++++++- tests/test_cli_doctor_commands.py | 81 +++++++++++++++++++++++++++++++ tests/test_tool_capabilities.py | 48 ++++++++++++++++++ 4 files changed, 178 insertions(+), 2 deletions(-) diff --git a/raven/agent/tools/capabilities.py b/raven/agent/tools/capabilities.py index 546ad981..baef56e4 100644 --- a/raven/agent/tools/capabilities.py +++ b/raven/agent/tools/capabilities.py @@ -150,13 +150,17 @@ def _resolved_media(cap: Capability, config: "Config") -> Any: def is_configured(cap: Capability, config: "Config") -> bool: - """Whether this capability would be offered to the model right now. + """Whether this capability's credential gate is satisfied. Delegates to the tool rather than deciding here. The rule for each family lives with the tool that owns the credential, so this module cannot become a second opinion about configured-ness -- which is the divergence ``providers.auth`` exists to prevent on the provider side, and the reason an AST invariant guards it there. + + Not the same question as "is it offered", which :func:`is_offered` answers: + a deployment can switch a fully credentialed tool off. See + :func:`is_disabled`. """ if cap.need is Need.NOTHING: return True @@ -170,6 +174,29 @@ def is_configured(cap: Capability, config: "Config") -> bool: return WebSearchTool.is_configured(config.tools.web.search.api_key) +def is_disabled(cap: Capability, config: "Config") -> bool: + """Whether the deployment has switched this tool off by name. + + A separate state from unconfigured, and reported as one: a switched-off + tool usually has its credential set, and calling it unconfigured would send + the deployer to set a key that is already there. + + ``tools.disabledTools`` is applied after registration + (``AgentLoop._apply_disabled_tools``), so this is the only thing standing + between a satisfied credential gate and a tool the agent actually holds. + """ + return cap.tool in (config.tools.disabled_tools or []) + + +def is_offered(cap: Capability, config: "Config") -> bool: + """Whether the agent ends up holding this tool: credentialed and not off. + + The predicate that matches the final registry, which is what a report about + available capabilities has to agree with. + """ + return is_configured(cap, config) and not is_disabled(cap, config) + + def has_credential(cap: Capability, config: "Config") -> bool: """Whether a credential actually resolves for this capability. diff --git a/raven/cli/doctor_commands.py b/raven/cli/doctor_commands.py index 49120092..5e34bbff 100644 --- a/raven/cli/doctor_commands.py +++ b/raven/cli/doctor_commands.py @@ -155,6 +155,9 @@ class ToolCapabilityInfo: #: the same as ``configured``: a model with no key is registered and fails #: on every call. has_credential: bool = True + #: Switched off by name in ``tools.disabledTools``, which happens after + #: registration -- so this row is configured and still not offered. + disabled: bool = False config_path: str = "" #: Where this capability's own credential goes, which for the media family #: is not ``config_path`` -- that one names the model. @@ -337,6 +340,7 @@ def _gather_tools(config: "Config") -> ToolsInfo: configured_from, has_credential, is_configured, + is_disabled, ) return ToolsInfo( @@ -348,6 +352,7 @@ def _gather_tools(config: "Config") -> ToolsInfo: configured=is_configured(cap, config), source=configured_from(cap, config), has_credential=has_credential(cap, config), + disabled=is_disabled(cap, config), config_path=cap.config_path, key_path=cap.key_path, borrowable=borrowable_credential(cap, config), @@ -599,13 +604,28 @@ def _render_tool_capabilities(tools: ToolsInfo) -> None: # The marker carries the answer too: a green tick above a line # saying every call fails is the same misreport in miniature. mark = "[green]✓[/green]" if cap.has_credential else "[yellow]![/yellow]" + if cap.disabled: + # Not a tick and not a fault: switched off is a decision + # someone made, and the row says whose decision it was so it + # can be undone in the one place that made it. + mark = "[dim]x[/dim]" console.print(f"{label}{mark} {cap.summary}{where}") + if cap.disabled: + console.print(f"{indent}[dim]switched off in[/dim] tools.disabledTools") + continue if not cap.has_credential: console.print(f"{indent}[yellow]no key resolves; calls will fail[/yellow]") console.print(f"{indent}[dim]set:[/dim] {cap.key_path}") console.print(f"{indent}[dim]or env:[/dim] {cap.env_var}") continue - console.print(f"{label}[dim]- {cap.summary}[/dim]") + glyph = "x" if cap.disabled else "-" + console.print(f"{label}[dim]{glyph} {cap.summary}[/dim]") + if cap.disabled: + # First, and outside the credential advice below: the two are + # independent decisions, and setup instructions that leave the off + # switch unsaid send someone to set a key, restart, and find the + # tool still gone. + console.print(f"{indent}[dim]switched off in[/dim] tools.disabledTools") if cap.need == "own_credential": console.print(f"{indent}[dim]switch on:[/dim] {cap.config_path}") if cap.borrowable: diff --git a/tests/test_cli_doctor_commands.py b/tests/test_cli_doctor_commands.py index a2094ba0..d66fbb2e 100644 --- a/tests/test_cli_doctor_commands.py +++ b/tests/test_cli_doctor_commands.py @@ -1040,3 +1040,84 @@ def test_a_config_path_is_never_split_across_lines(healthy_config: Path) -> None for path in ("tools.web.search.apiKey", "tools.media.image.model", "SERPER_API_KEY"): assert path in result.stdout, f"{path} was broken across a line wrap" + + +def _search_config(tmp_path: Path, *, key: bool, off: bool) -> None: + """One cell of the web_search state matrix, persisted. + + ``key`` and ``off`` are independent in production -- a deployment can set + neither, either, or both -- so they are independent here. + """ + cfg = Config() + cfg.agents.defaults.model = "anthropic/claude-sonnet-4-5" + cfg.agents.defaults.workspace = str(tmp_path / "workspace") + cfg.providers.anthropic.api_key = "sk-fake" + if key: + cfg.tools.web.search.api_key = "sk-serper" + if off: + cfg.tools.disabled_tools = ["web_search"] + save_config(cfg) + + +def _switched_off_search(tmp_path: Path) -> None: + """A credentialed web_search that the deployment has switched off by name.""" + _search_config(tmp_path, key=True, off=True) + + +def test_doctor_says_a_capability_is_switched_off_rather_than_ticking_it(tmp_config: Path, tmp_path: Path) -> None: + """A key plus `disabledTools` used to print a green tick for a tool the + agent does not hold -- the report claiming a capability is on offer when + Raven has removed it.""" + _switched_off_search(tmp_path) + + result = runner.invoke(app, ["doctor"]) + + assert "tools.disabledTools" in result.stdout + # Not the unconfigured path: the key is set, and telling them to set it + # again is how a report sends someone in a circle. + assert "switch on:" not in result.stdout.split("web_search")[-1][:200] + + +def test_the_switched_off_state_reaches_the_json_output(tmp_config: Path, tmp_path: Path) -> None: + _switched_off_search(tmp_path) + + result = runner.invoke(app, ["doctor", "--json"]) + payload = json.loads(result.stdout) + + row = next(c for c in payload["tools"]["capabilities"] if c["tool"] == "web_search") + assert row["configured"] is True, "the key is set; calling it unconfigured is the wrong repair" + assert row["disabled"] is True + + +@pytest.mark.parametrize("key", [True, False], ids=["keyed", "keyless"]) +def test_the_off_switch_is_named_whether_or_not_a_key_is_set(key: bool, tmp_config: Path, tmp_path: Path) -> None: + """The cell the first version of this rendering got wrong. + + With no key, the row used to print only the credential advice -- so a + deployer could set `tools.web.search.apiKey`, restart, and still not have + search, because `_apply_disabled_tools` removes it either way. + """ + _search_config(tmp_path, key=key, off=True) + + result = runner.invoke(app, ["doctor"]) + + assert "tools.disabledTools" in result.stdout + + +@pytest.mark.parametrize( + ("key", "off", "configured", "disabled"), + [(True, True, True, True), (True, False, True, False), (False, True, False, True), (False, False, False, False)], + ids=["keyed-off", "keyed-on", "keyless-off", "keyless-on"], +) +def test_the_json_row_reports_the_two_states_independently( + key: bool, off: bool, configured: bool, disabled: bool, tmp_config: Path, tmp_path: Path +) -> None: + """Both flags, all four combinations. Collapsing either into the other is + what made the report tell a deployer to set a key that was already set.""" + _search_config(tmp_path, key=key, off=off) + + payload = json.loads(runner.invoke(app, ["doctor", "--json"]).stdout) + row = next(c for c in payload["tools"]["capabilities"] if c["tool"] == "web_search") + + assert row["configured"] is configured + assert row["disabled"] is disabled diff --git a/tests/test_tool_capabilities.py b/tests/test_tool_capabilities.py index df122b24..05fd1878 100644 --- a/tests/test_tool_capabilities.py +++ b/tests/test_tool_capabilities.py @@ -21,6 +21,8 @@ configured_from, has_credential, is_configured, + is_disabled, + is_offered, ) from raven.config.loader import load_config from raven.providers.base import LLMProvider, LLMResponse @@ -318,3 +320,49 @@ def test_only_the_media_family_borrows(tmp_path: Path) -> None: for cap in (c for c in CAPABILITIES if not c.media_attr): assert borrowable_credential(cap, config) == "", cap.tool + + +def test_a_switched_off_tool_is_configured_and_still_not_offered(workspace, tmp_path: Path) -> None: + """The combination the report used to get wrong. + + A key is set, so the credential gate is satisfied and saying "unconfigured" + would send the deployer to set it again. What decides whether the agent + holds the tool is `tools.disabledTools`, applied after registration -- so + the predicate that has to agree with the registry is `is_offered`, not + `is_configured`. + """ + config = _config(tmp_path) + config.tools.web.search.api_key = "sk-serper" + config.tools.disabled_tools = ["web_search"] + # Passed in, the way the CLI entry points do: the loop takes the list as an + # argument rather than reading the config. + loop = _loop(workspace, config, brave_api_key="sk-serper", disabled_tools=config.tools.disabled_tools) + cap = next(c for c in CAPABILITIES if c.tool == "web_search") + + assert loop.tools.has("web_search") is False + assert is_configured(cap, config) is True + assert is_disabled(cap, config) is True + assert is_offered(cap, config) is loop.tools.has("web_search") + + +@pytest.mark.parametrize("cap", CAPABILITIES, ids=lambda c: c.tool) +def test_being_offered_matches_the_registry_for_every_capability(cap, workspace, tmp_path: Path) -> None: + """Every entry, switched on and then off by name, against the real loop. + + Parametrised rather than written for web_search alone: the disabled list + covers any tool, and a family that stops agreeing is exactly the drift the + rest of this file exists to catch. + """ + config = _config(tmp_path) + config.tools.web.search.api_key = "sk-serper" + for attr in _media_attrs(): + getattr(config.tools.media, attr).model = "some/model" + config.providers.openrouter.api_key = "sk-or-test" + + on = _loop(workspace, config, brave_api_key="sk-serper") + assert is_offered(cap, config) is on.tools.has(cap.tool) + + config.tools.disabled_tools = [cap.tool] + off = _loop(workspace, config, brave_api_key="sk-serper", disabled_tools=config.tools.disabled_tools) + assert is_offered(cap, config) is off.tools.has(cap.tool) + assert off.tools.has(cap.tool) is False