Skip to content

Commit 3fd13c9

Browse files
Handsome-wzwclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 669db65 commit 3fd13c9

4 files changed

Lines changed: 178 additions & 2 deletions

File tree

‎raven/agent/tools/capabilities.py‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,13 +150,17 @@ def _resolved_media(cap: Capability, config: "Config") -> Any:
150150

151151

152152
def is_configured(cap: Capability, config: "Config") -> bool:
153-
"""Whether this capability would be offered to the model right now.
153+
"""Whether this capability's credential gate is satisfied.
154154
155155
Delegates to the tool rather than deciding here. The rule for each family
156156
lives with the tool that owns the credential, so this module cannot become
157157
a second opinion about configured-ness -- which is the divergence
158158
``providers.auth`` exists to prevent on the provider side, and the reason
159159
an AST invariant guards it there.
160+
161+
Not the same question as "is it offered", which :func:`is_offered` answers:
162+
a deployment can switch a fully credentialed tool off. See
163+
:func:`is_disabled`.
160164
"""
161165
if cap.need is Need.NOTHING:
162166
return True
@@ -170,6 +174,29 @@ def is_configured(cap: Capability, config: "Config") -> bool:
170174
return WebSearchTool.is_configured(config.tools.web.search.api_key)
171175

172176

177+
def is_disabled(cap: Capability, config: "Config") -> bool:
178+
"""Whether the deployment has switched this tool off by name.
179+
180+
A separate state from unconfigured, and reported as one: a switched-off
181+
tool usually has its credential set, and calling it unconfigured would send
182+
the deployer to set a key that is already there.
183+
184+
``tools.disabledTools`` is applied after registration
185+
(``AgentLoop._apply_disabled_tools``), so this is the only thing standing
186+
between a satisfied credential gate and a tool the agent actually holds.
187+
"""
188+
return cap.tool in (config.tools.disabled_tools or [])
189+
190+
191+
def is_offered(cap: Capability, config: "Config") -> bool:
192+
"""Whether the agent ends up holding this tool: credentialed and not off.
193+
194+
The predicate that matches the final registry, which is what a report about
195+
available capabilities has to agree with.
196+
"""
197+
return is_configured(cap, config) and not is_disabled(cap, config)
198+
199+
173200
def has_credential(cap: Capability, config: "Config") -> bool:
174201
"""Whether a credential actually resolves for this capability.
175202

‎raven/cli/doctor_commands.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,9 @@ class ToolCapabilityInfo:
155155
#: the same as ``configured``: a model with no key is registered and fails
156156
#: on every call.
157157
has_credential: bool = True
158+
#: Switched off by name in ``tools.disabledTools``, which happens after
159+
#: registration -- so this row is configured and still not offered.
160+
disabled: bool = False
158161
config_path: str = ""
159162
#: Where this capability's own credential goes, which for the media family
160163
#: is not ``config_path`` -- that one names the model.
@@ -337,6 +340,7 @@ def _gather_tools(config: "Config") -> ToolsInfo:
337340
configured_from,
338341
has_credential,
339342
is_configured,
343+
is_disabled,
340344
)
341345

342346
return ToolsInfo(
@@ -348,6 +352,7 @@ def _gather_tools(config: "Config") -> ToolsInfo:
348352
configured=is_configured(cap, config),
349353
source=configured_from(cap, config),
350354
has_credential=has_credential(cap, config),
355+
disabled=is_disabled(cap, config),
351356
config_path=cap.config_path,
352357
key_path=cap.key_path,
353358
borrowable=borrowable_credential(cap, config),
@@ -599,13 +604,28 @@ def _render_tool_capabilities(tools: ToolsInfo) -> None:
599604
# The marker carries the answer too: a green tick above a line
600605
# saying every call fails is the same misreport in miniature.
601606
mark = "[green]✓[/green]" if cap.has_credential else "[yellow]![/yellow]"
607+
if cap.disabled:
608+
# Not a tick and not a fault: switched off is a decision
609+
# someone made, and the row says whose decision it was so it
610+
# can be undone in the one place that made it.
611+
mark = "[dim]x[/dim]"
602612
console.print(f"{label}{mark} {cap.summary}{where}")
613+
if cap.disabled:
614+
console.print(f"{indent}[dim]switched off in[/dim] tools.disabledTools")
615+
continue
603616
if not cap.has_credential:
604617
console.print(f"{indent}[yellow]no key resolves; calls will fail[/yellow]")
605618
console.print(f"{indent}[dim]set:[/dim] {cap.key_path}")
606619
console.print(f"{indent}[dim]or env:[/dim] {cap.env_var}")
607620
continue
608-
console.print(f"{label}[dim]- {cap.summary}[/dim]")
621+
glyph = "x" if cap.disabled else "-"
622+
console.print(f"{label}[dim]{glyph} {cap.summary}[/dim]")
623+
if cap.disabled:
624+
# First, and outside the credential advice below: the two are
625+
# independent decisions, and setup instructions that leave the off
626+
# switch unsaid send someone to set a key, restart, and find the
627+
# tool still gone.
628+
console.print(f"{indent}[dim]switched off in[/dim] tools.disabledTools")
609629
if cap.need == "own_credential":
610630
console.print(f"{indent}[dim]switch on:[/dim] {cap.config_path}")
611631
if cap.borrowable:

‎tests/test_cli_doctor_commands.py‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1040,3 +1040,84 @@ def test_a_config_path_is_never_split_across_lines(healthy_config: Path) -> None
10401040

10411041
for path in ("tools.web.search.apiKey", "tools.media.image.model", "SERPER_API_KEY"):
10421042
assert path in result.stdout, f"{path} was broken across a line wrap"
1043+
1044+
1045+
def _search_config(tmp_path: Path, *, key: bool, off: bool) -> None:
1046+
"""One cell of the web_search state matrix, persisted.
1047+
1048+
``key`` and ``off`` are independent in production -- a deployment can set
1049+
neither, either, or both -- so they are independent here.
1050+
"""
1051+
cfg = Config()
1052+
cfg.agents.defaults.model = "anthropic/claude-sonnet-4-5"
1053+
cfg.agents.defaults.workspace = str(tmp_path / "workspace")
1054+
cfg.providers.anthropic.api_key = "sk-fake"
1055+
if key:
1056+
cfg.tools.web.search.api_key = "sk-serper"
1057+
if off:
1058+
cfg.tools.disabled_tools = ["web_search"]
1059+
save_config(cfg)
1060+
1061+
1062+
def _switched_off_search(tmp_path: Path) -> None:
1063+
"""A credentialed web_search that the deployment has switched off by name."""
1064+
_search_config(tmp_path, key=True, off=True)
1065+
1066+
1067+
def test_doctor_says_a_capability_is_switched_off_rather_than_ticking_it(tmp_config: Path, tmp_path: Path) -> None:
1068+
"""A key plus `disabledTools` used to print a green tick for a tool the
1069+
agent does not hold -- the report claiming a capability is on offer when
1070+
Raven has removed it."""
1071+
_switched_off_search(tmp_path)
1072+
1073+
result = runner.invoke(app, ["doctor"])
1074+
1075+
assert "tools.disabledTools" in result.stdout
1076+
# Not the unconfigured path: the key is set, and telling them to set it
1077+
# again is how a report sends someone in a circle.
1078+
assert "switch on:" not in result.stdout.split("web_search")[-1][:200]
1079+
1080+
1081+
def test_the_switched_off_state_reaches_the_json_output(tmp_config: Path, tmp_path: Path) -> None:
1082+
_switched_off_search(tmp_path)
1083+
1084+
result = runner.invoke(app, ["doctor", "--json"])
1085+
payload = json.loads(result.stdout)
1086+
1087+
row = next(c for c in payload["tools"]["capabilities"] if c["tool"] == "web_search")
1088+
assert row["configured"] is True, "the key is set; calling it unconfigured is the wrong repair"
1089+
assert row["disabled"] is True
1090+
1091+
1092+
@pytest.mark.parametrize("key", [True, False], ids=["keyed", "keyless"])
1093+
def test_the_off_switch_is_named_whether_or_not_a_key_is_set(key: bool, tmp_config: Path, tmp_path: Path) -> None:
1094+
"""The cell the first version of this rendering got wrong.
1095+
1096+
With no key, the row used to print only the credential advice -- so a
1097+
deployer could set `tools.web.search.apiKey`, restart, and still not have
1098+
search, because `_apply_disabled_tools` removes it either way.
1099+
"""
1100+
_search_config(tmp_path, key=key, off=True)
1101+
1102+
result = runner.invoke(app, ["doctor"])
1103+
1104+
assert "tools.disabledTools" in result.stdout
1105+
1106+
1107+
@pytest.mark.parametrize(
1108+
("key", "off", "configured", "disabled"),
1109+
[(True, True, True, True), (True, False, True, False), (False, True, False, True), (False, False, False, False)],
1110+
ids=["keyed-off", "keyed-on", "keyless-off", "keyless-on"],
1111+
)
1112+
def test_the_json_row_reports_the_two_states_independently(
1113+
key: bool, off: bool, configured: bool, disabled: bool, tmp_config: Path, tmp_path: Path
1114+
) -> None:
1115+
"""Both flags, all four combinations. Collapsing either into the other is
1116+
what made the report tell a deployer to set a key that was already set."""
1117+
_search_config(tmp_path, key=key, off=off)
1118+
1119+
payload = json.loads(runner.invoke(app, ["doctor", "--json"]).stdout)
1120+
row = next(c for c in payload["tools"]["capabilities"] if c["tool"] == "web_search")
1121+
1122+
assert row["configured"] is configured
1123+
assert row["disabled"] is disabled

‎tests/test_tool_capabilities.py‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121
configured_from,
2222
has_credential,
2323
is_configured,
24+
is_disabled,
25+
is_offered,
2426
)
2527
from raven.config.loader import load_config
2628
from raven.providers.base import LLMProvider, LLMResponse
@@ -318,3 +320,49 @@ def test_only_the_media_family_borrows(tmp_path: Path) -> None:
318320

319321
for cap in (c for c in CAPABILITIES if not c.media_attr):
320322
assert borrowable_credential(cap, config) == "", cap.tool
323+
324+
325+
def test_a_switched_off_tool_is_configured_and_still_not_offered(workspace, tmp_path: Path) -> None:
326+
"""The combination the report used to get wrong.
327+
328+
A key is set, so the credential gate is satisfied and saying "unconfigured"
329+
would send the deployer to set it again. What decides whether the agent
330+
holds the tool is `tools.disabledTools`, applied after registration -- so
331+
the predicate that has to agree with the registry is `is_offered`, not
332+
`is_configured`.
333+
"""
334+
config = _config(tmp_path)
335+
config.tools.web.search.api_key = "sk-serper"
336+
config.tools.disabled_tools = ["web_search"]
337+
# Passed in, the way the CLI entry points do: the loop takes the list as an
338+
# argument rather than reading the config.
339+
loop = _loop(workspace, config, brave_api_key="sk-serper", disabled_tools=config.tools.disabled_tools)
340+
cap = next(c for c in CAPABILITIES if c.tool == "web_search")
341+
342+
assert loop.tools.has("web_search") is False
343+
assert is_configured(cap, config) is True
344+
assert is_disabled(cap, config) is True
345+
assert is_offered(cap, config) is loop.tools.has("web_search")
346+
347+
348+
@pytest.mark.parametrize("cap", CAPABILITIES, ids=lambda c: c.tool)
349+
def test_being_offered_matches_the_registry_for_every_capability(cap, workspace, tmp_path: Path) -> None:
350+
"""Every entry, switched on and then off by name, against the real loop.
351+
352+
Parametrised rather than written for web_search alone: the disabled list
353+
covers any tool, and a family that stops agreeing is exactly the drift the
354+
rest of this file exists to catch.
355+
"""
356+
config = _config(tmp_path)
357+
config.tools.web.search.api_key = "sk-serper"
358+
for attr in _media_attrs():
359+
getattr(config.tools.media, attr).model = "some/model"
360+
config.providers.openrouter.api_key = "sk-or-test"
361+
362+
on = _loop(workspace, config, brave_api_key="sk-serper")
363+
assert is_offered(cap, config) is on.tools.has(cap.tool)
364+
365+
config.tools.disabled_tools = [cap.tool]
366+
off = _loop(workspace, config, brave_api_key="sk-serper", disabled_tools=config.tools.disabled_tools)
367+
assert is_offered(cap, config) is off.tools.has(cap.tool)
368+
assert off.tools.has(cap.tool) is False

0 commit comments

Comments
 (0)