From 3628c14630b27e0c0f5c9f7e8ef3ee22983b7de0 Mon Sep 17 00:00:00 2001 From: Handsome-wzw <68996445+Handsome-wzw@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:25:17 +0000 Subject: [PATCH] fix(*): give web search its config section so maxResults takes effect `tools.web.search.maxResults` has been declared in the schema and read by nobody. Both registration sites built the tool from the key alone, so it fell back to its own default of 5 and a deployer who set the field got no effect and no warning. The cause is that web search was the one tool handed a single field instead of its config section. Every neighbour takes a section -- media_config, exec_config, sandbox_config, context_config -- and the three CLI entry points already held `config.tools.web.search` and reached into it to pull one value out. Passing the section instead is what wires the field, and it retires the `brave_api_key` parameter name in the same move: it has named a Serper key ever since the Brave backend went away, which a reviewer flagged on the capability PR as pre-existing and misleading. `WebSearchTool` keeps its own signature. It is still constructible from a bare key, which is what its unit tests and the error-path test rely on; only the callers that have a config section now pass one. The sub-agent surface gets the same treatment, and a test of its own. A first mutation pass caught the main loop dropping `max_results` and said nothing when the sub-agent did, which is the shape this area keeps taking: two registration sites, a fix applied to one. Five mutations now, all caught. Co-authored-by: Claude (claude-opus-5) --- raven/agent/loop/main.py | 25 ++++++--- raven/agent/subagent/manager.py | 12 +++-- raven/cli/agent_commands.py | 2 +- raven/cli/gateway_commands.py | 2 +- raven/cli/tui_commands.py | 2 +- tests/test_agent_loop_tool_search.py | 4 +- tests/test_agent_loop_web_tools.py | 76 +++++++++++++++++++--------- tests/test_tool_capabilities.py | 4 +- 8 files changed, 85 insertions(+), 42 deletions(-) diff --git a/raven/agent/loop/main.py b/raven/agent/loop/main.py index d768cff7..c8b96c98 100644 --- a/raven/agent/loop/main.py +++ b/raven/agent/loop/main.py @@ -76,7 +76,12 @@ RuntimeConfig, SkillForgeRouterConfig, ) - from raven.config.schema import ChannelsConfig, DeepResearchToolConfig, ExecToolConfig + from raven.config.schema import ( + ChannelsConfig, + DeepResearchToolConfig, + ExecToolConfig, + WebSearchConfig, + ) from raven.context_engine import ContextEngine from raven.memory_engine.backend import MemoryBackend from raven.proactive_engine.schedulers.cron.service import CronService @@ -292,7 +297,7 @@ def __init__( model: str | None = None, max_iterations: int = 40, context_window_tokens: int | None = None, - brave_api_key: str | None = None, + web_search_config: "WebSearchConfig | None" = None, web_proxy: str | None = None, exec_config: ExecToolConfig | None = None, cron_service: CronService | None = None, @@ -402,11 +407,11 @@ def __init__( self.context_window_tokens = context_window_tokens or effective_context_window( self.model, None, allow_fetch=False ) - self.brave_api_key = brave_api_key self.jina_api_key = jina_api_key self.web_proxy = web_proxy - from raven.config.schema import DeepResearchToolConfig, MediaGenConfig + from raven.config.schema import DeepResearchToolConfig, MediaGenConfig, WebSearchConfig + self.web_search_config = web_search_config or WebSearchConfig() self.media_config = media_config or MediaGenConfig() self.deep_research_config = deep_research_config or DeepResearchToolConfig() self.exec_config = exec_config or ExecToolConfig() @@ -534,7 +539,7 @@ def __init__( provider=provider, workspace=workspace, model=self.model, - brave_api_key=brave_api_key, + web_search_config=web_search_config, jina_api_key=jina_api_key, web_proxy=web_proxy, exec_config=self.exec_config, @@ -785,9 +790,13 @@ def _register_default_tools(self) -> None: # 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) + # either source; gating on the configured key alone would withdraw the + # tool from a deploy that only exports SERPER_API_KEY. + web_search = WebSearchTool( + api_key=self.web_search_config.api_key or None, + max_results=self.web_search_config.max_results, + 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 diff --git a/raven/agent/subagent/manager.py b/raven/agent/subagent/manager.py index b57c7926..de10e2ad 100644 --- a/raven/agent/subagent/manager.py +++ b/raven/agent/subagent/manager.py @@ -14,7 +14,7 @@ from raven.agent.tools.registry import ToolRegistry from raven.agent.tools.shell import ExecTool from raven.agent.tools.web import WebFetchTool, WebSearchTool -from raven.config.schema import ExecToolConfig +from raven.config.schema import ExecToolConfig, WebSearchConfig from raven.providers.base import LLMProvider from raven.sandbox import SandboxConfig, build_executor from raven.security.trust import wrap_untrusted @@ -38,7 +38,7 @@ def __init__( provider: LLMProvider, workspace: Path, model: str | None = None, - brave_api_key: str | None = None, + web_search_config: "WebSearchConfig | None" = None, web_proxy: str | None = None, exec_config: "ExecToolConfig | None" = None, restrict_to_workspace: bool = False, @@ -59,7 +59,7 @@ def __init__( # SUBAGENT-origin turn. self._submit = None self.model = model or provider.get_default_model() - self.brave_api_key = brave_api_key + self.web_search_config = web_search_config or WebSearchConfig() self.jina_api_key = jina_api_key self.web_proxy = web_proxy self.exec_config = exec_config or ExecToolConfig() @@ -199,7 +199,11 @@ async def _run_subagent_inner( # 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) + web_search = WebSearchTool( + api_key=self.web_search_config.api_key or None, + max_results=self.web_search_config.max_results, + 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)) diff --git a/raven/cli/agent_commands.py b/raven/cli/agent_commands.py index 715c64f4..845f5102 100644 --- a/raven/cli/agent_commands.py +++ b/raven/cli/agent_commands.py @@ -330,7 +330,7 @@ def agent( context_window_tokens=config.agents.defaults.context_window_tokens, max_concurrent_subagents=config.agents.defaults.max_concurrent_subagents, max_subagent_spawns_per_hour=config.agents.defaults.max_subagent_spawns_per_hour, - brave_api_key=config.tools.web.search.api_key or None, + web_search_config=config.tools.web.search, jina_api_key=config.tools.web.jina_api_key or None, web_proxy=config.tools.web.proxy or None, media_config=config.effective_media_config(), diff --git a/raven/cli/gateway_commands.py b/raven/cli/gateway_commands.py index 9edfccca..431bb4ea 100644 --- a/raven/cli/gateway_commands.py +++ b/raven/cli/gateway_commands.py @@ -232,7 +232,7 @@ def gateway( context_window_tokens=config.agents.defaults.context_window_tokens, max_concurrent_subagents=config.agents.defaults.max_concurrent_subagents, max_subagent_spawns_per_hour=config.agents.defaults.max_subagent_spawns_per_hour, - brave_api_key=config.tools.web.search.api_key or None, + web_search_config=config.tools.web.search, jina_api_key=config.tools.web.jina_api_key or None, web_proxy=config.tools.web.proxy or None, media_config=config.effective_media_config(), diff --git a/raven/cli/tui_commands.py b/raven/cli/tui_commands.py index 3c53367e..74778790 100644 --- a/raven/cli/tui_commands.py +++ b/raven/cli/tui_commands.py @@ -448,7 +448,7 @@ def _build_tui_agent_loop(): context_window_tokens=config.agents.defaults.context_window_tokens, max_concurrent_subagents=config.agents.defaults.max_concurrent_subagents, max_subagent_spawns_per_hour=config.agents.defaults.max_subagent_spawns_per_hour, - brave_api_key=config.tools.web.search.api_key or None, + web_search_config=config.tools.web.search, web_proxy=config.tools.web.proxy or None, media_config=config.effective_media_config(), deep_research_config=config.tools.deep_research, diff --git a/tests/test_agent_loop_tool_search.py b/tests/test_agent_loop_tool_search.py index 3800aa0d..5b7dde9d 100644 --- a/tests/test_agent_loop_tool_search.py +++ b/tests/test_agent_loop_tool_search.py @@ -14,7 +14,7 @@ import pytest from raven.agent.loop import AgentLoop -from raven.config.schema import ToolSearchConfig +from raven.config.schema import ToolSearchConfig, WebSearchConfig from raven.providers.base import LLMProvider, LLMResponse from raven.token_wise.base import TokenStrategy from raven.token_wise.registry import StrategyRegistry @@ -64,7 +64,7 @@ def _make_loop(workspace: Path, cfg, strategies=None) -> AgentLoop: # 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", + web_search_config=WebSearchConfig(api_key="test-serper-key"), ) diff --git a/tests/test_agent_loop_web_tools.py b/tests/test_agent_loop_web_tools.py index 54a2e596..c7ac4204 100644 --- a/tests/test_agent_loop_web_tools.py +++ b/tests/test_agent_loop_web_tools.py @@ -21,6 +21,7 @@ from raven.agent.subagent.manager import SubagentManager from raven.agent.tools.registry import ToolRegistry from raven.agent.tools.web import WebSearchTool +from raven.config.schema import WebSearchConfig from raven.providers.base import LLMProvider, LLMResponse @@ -75,11 +76,24 @@ def test_web_search_is_withheld_without_a_key(workspace) -> None: def test_a_configured_key_registers_web_search(workspace) -> None: - loop = _loop(workspace, brave_api_key="sk-serper") + loop = _loop(workspace, web_search_config=WebSearchConfig(api_key="sk-serper")) assert loop.tools.has("web_search") +def test_the_configured_result_count_reaches_the_tool(workspace) -> None: + """``tools.web.search.maxResults`` was declared and never wired. + + Both registration sites passed the key alone, so the tool fell back to its + own default and a deployer who set the field got no effect and no warning. + Passing the section rather than one field out of it is what closes that, and + this is the assertion that keeps it closed. + """ + loop = _loop(workspace, web_search_config=WebSearchConfig(api_key="sk-serper", max_results=9)) + + assert loop.tools.get("web_search").max_results == 9 + + 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 @@ -91,39 +105,46 @@ def test_the_env_var_alone_registers_web_search(workspace, monkeypatch: pytest.M 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]] = [] +@pytest.fixture +def subagent_run(workspace, monkeypatch: pytest.MonkeyPatch): + """Run a sub-agent and hand back the tools it registered. + + The manager builds its registry inside the run and keeps no reference, so + the tools 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. + """ + import asyncio + + runs: list[list] = [] 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) + assert runs, f"{tool.name} was registered outside a collection window" + runs[-1].append(tool) 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. + # The run announces its result through the spine, which is not wired here + # and is not what these are about. monkeypatch.setattr(SubagentManager, "_announce_result", _noop) - async def _names(**kw) -> list[str]: - registered.append([]) + def run(**kw): + runs.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] + asyncio.run(manager._run_subagent_inner("t1", "task", "label", {}, None, manager.provider, manager.model)) + return runs[-1] - import asyncio + return run - without = asyncio.run(_names()) - with_key = asyncio.run(_names(brave_api_key="sk-serper")) + +def test_the_subagent_surface_applies_the_same_rule(subagent_run) -> 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. + without = [t.name for t in subagent_run()] + with_key = [t.name for t in subagent_run(web_search_config=WebSearchConfig(api_key="sk-serper"))] # Baselines first: an empty list would satisfy the "not in" assertion below # without proving anything about the gate. @@ -133,6 +154,15 @@ async def _names(**kw) -> list[str]: assert "web_search" in with_key +def test_the_subagent_surface_gets_the_configured_result_count_too(subagent_run) -> None: + """The second registration site had the same unwired field, and a fix + applied to one of two call sites is the shape this whole area keeps taking.""" + tools = subagent_run(web_search_config=WebSearchConfig(api_key="sk-serper", max_results=7)) + + web_search = next(t for t in tools if t.name == "web_search") + assert web_search.max_results == 7 + + @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 diff --git a/tests/test_tool_capabilities.py b/tests/test_tool_capabilities.py index df122b24..d9c41f94 100644 --- a/tests/test_tool_capabilities.py +++ b/tests/test_tool_capabilities.py @@ -113,7 +113,7 @@ def test_the_table_names_exactly_the_credential_gated_tools(workspace, tmp_path: 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") + loop_full = _loop(workspace, full, web_search_config=full.tools.web.search) 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} @@ -136,7 +136,7 @@ def test_the_table_agrees_with_the_loop_when_unconfigured(cap, workspace, tmp_pa 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") + loop = _loop(workspace, config, web_search_config=config.tools.web.search) cap = next(c for c in CAPABILITIES if c.tool == "web_search") assert is_configured(cap, config) and loop.tools.has("web_search")