diff --git a/agent/background_review.py b/agent/background_review.py index c2ea87bd94e2..0e2274394520 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -687,9 +687,37 @@ def _bg_review_auto_deny(command, description, **kwargs): # parent below so memory(action="add") writes from # the review still land on disk; the review just # has zero side effects on external providers. - # Match parent's toolset config so ``tools[]`` is byte-identical - # in the request body — Anthropic's cache key includes it. - # (The runtime whitelist below still restricts dispatch.) + # Toolset config for the fork. For cache-backed providers we match + # the parent's toolsets so ``tools[]`` is byte-identical in the + # request body — Anthropic's cache key includes it — and rely on + # the runtime whitelist below to restrict dispatch. + # + # For a LOCAL review endpoint there is no prefix cache to preserve, + # so we narrow the *advertised* schema to the review's real + # permissions instead. Otherwise a weaker local model imitates the + # snapshot history (full of write_file/read_file/terminal calls) + # and burns turns hitting the dispatch deny-wall — pure waste, + # since advertising the full schema buys nothing without a cache. + # Classification uses the RESOLVED review runtime (_rt), so an + # auxiliary.background_review route is judged by the endpoint that + # actually serves the review, not the parent's. The narrowed set + # mirrors the runtime whitelist's memory gate below: a profile + # with memory/user-profile disabled must not get the memory tool + # re-advertised at schema level. The runtime whitelist still + # applies as a belt-and-suspenders net. + from agent.model_metadata import is_local_endpoint + + _review_base_url = _rt.get("base_url") or None + if _review_base_url and is_local_endpoint(_review_base_url): + _review_enabled_toolsets = ["skills"] + if getattr(agent, "_memory_enabled", False) or getattr( + agent, "_user_profile_enabled", False + ): + _review_enabled_toolsets.insert(0, "memory") + _review_disabled_toolsets = None + else: + _review_enabled_toolsets = getattr(agent, "enabled_toolsets", None) + _review_disabled_toolsets = getattr(agent, "disabled_toolsets", None) _fork_kwargs: Dict[str, Any] = {} if isinstance(_rt.get("max_tokens"), int): _fork_kwargs["max_tokens"] = _rt["max_tokens"] @@ -716,13 +744,13 @@ def _bg_review_auto_deny(command, description, **kwargs): platform=agent.platform, provider=_rt.get("provider") or agent.provider, api_mode=_rt.get("api_mode"), - base_url=_rt.get("base_url") or None, + base_url=_review_base_url, api_key=_rt.get("api_key") or None, credential_pool=_rt.get("credential_pool"), request_overrides=_rt.get("request_overrides") or {}, parent_session_id=agent.session_id, - enabled_toolsets=getattr(agent, "enabled_toolsets", None), - disabled_toolsets=getattr(agent, "disabled_toolsets", None), + enabled_toolsets=_review_enabled_toolsets, + disabled_toolsets=_review_disabled_toolsets, skip_memory=True, **_fork_kwargs, ) diff --git a/tests/run_agent/test_background_review_toolset_restriction.py b/tests/run_agent/test_background_review_toolset_restriction.py index f8d6559da5f5..43e80384ab4e 100644 --- a/tests/run_agent/test_background_review_toolset_restriction.py +++ b/tests/run_agent/test_background_review_toolset_restriction.py @@ -227,3 +227,161 @@ def _no_init(self, *args, **kwargs): ) assert "memory" in captured["whitelist"] + + +def _capture_init_kwargs(captured): + def _init(self, *args, **kwargs): + captured["enabled_toolsets"] = kwargs.get("enabled_toolsets", "UNSET") + captured["disabled_toolsets"] = kwargs.get("disabled_toolsets", "UNSET") + captured["base_url"] = kwargs.get("base_url", "UNSET") + raise RuntimeError("stop after capturing init args") + + return _init + + +def test_background_review_narrows_toolset_for_local_endpoint(): + """Local endpoints have no prefix cache to preserve, so the fork advertises + only memory/skills instead of the parent's full schema — a weak local model + otherwise imitates the snapshot history and thrashes against the dispatch + deny-wall. + """ + import run_agent + + agent = _make_agent_stub(run_agent.AIAgent) + agent._current_main_runtime = lambda: { + "model": "Qwen3-Coder-Next-4bit", + "provider": "custom", + "base_url": "http://127.0.0.1:8149/v1", + "api_key": "k", + "api_mode": "", + } + captured = {} + + with patch.object(run_agent.AIAgent, "__init__", _capture_init_kwargs(captured)), \ + patch("threading.Thread", _SyncThread): + agent._spawn_background_review( + messages_snapshot=[], + review_memory=True, + review_skills=True, + ) + + assert captured.get("enabled_toolsets") == ["memory", "skills"], captured + assert captured.get("disabled_toolsets") is None, captured + + +def test_background_review_keeps_parent_toolset_for_remote_endpoint(): + """Cache-backed (remote) endpoints still mirror the parent's toolsets so + the ``tools[]`` payload stays byte-identical for the prefix cache. + """ + import run_agent + + agent = _make_agent_stub(run_agent.AIAgent) + agent._current_main_runtime = lambda: { + "model": "claude-sonnet-4-6", + "provider": "anthropic", + "base_url": "https://api.anthropic.com/v1", + "api_key": "k", + "api_mode": "", + } + captured = {} + + with patch.object(run_agent.AIAgent, "__init__", _capture_init_kwargs(captured)), \ + patch("threading.Thread", _SyncThread): + agent._spawn_background_review( + messages_snapshot=[], + review_memory=True, + review_skills=True, + ) + + assert captured.get("enabled_toolsets") == agent.enabled_toolsets, captured + assert captured.get("disabled_toolsets") == agent.disabled_toolsets, captured + + +def test_background_review_local_narrowing_respects_memory_gate(): + """The narrowed schema must honor the memory gate: a memory-disabled + profile gets only the skills toolset advertised, mirroring the runtime + whitelist's conditional memory grant (no schema-level re-grant). + """ + import run_agent + + agent = _make_agent_stub(run_agent.AIAgent) + agent._memory_enabled = False + agent._user_profile_enabled = False + agent._current_main_runtime = lambda: { + "model": "Qwen3-Coder-Next-4bit", + "provider": "custom", + "base_url": "http://127.0.0.1:8149/v1", + "api_key": "k", + "api_mode": "", + } + captured = {} + + with patch.object(run_agent.AIAgent, "__init__", _capture_init_kwargs(captured)), \ + patch("threading.Thread", _SyncThread): + agent._spawn_background_review( + messages_snapshot=[], + review_memory=False, + review_skills=True, + ) + + assert captured.get("enabled_toolsets") == ["skills"], captured + assert captured.get("disabled_toolsets") is None, captured + + +def test_background_review_routed_endpoint_classified_by_review_runtime(): + """An auxiliary.background_review route must be classified by the RESOLVED + review runtime's endpoint, not the parent's: remote parent + local aux + route still narrows, and the fork is constructed on the routed base_url. + """ + import run_agent + + agent = _make_agent_stub(run_agent.AIAgent) + agent.provider = "anthropic" + agent.model = "claude-sonnet-4-6" + agent._current_main_runtime = lambda: { + "model": "claude-sonnet-4-6", + "provider": "anthropic", + "base_url": "https://api.anthropic.com/v1", + "api_key": "k", + "api_mode": "", + } + captured = {} + routed_url = "http://127.0.0.1:8150/v1" + + def _fake_load_config(): + return { + "auxiliary": { + "background_review": {"provider": "custom", "model": "qwen-local"} + } + } + + def _fake_resolve_runtime_provider(**kwargs): + return { + "provider": "custom", + "model": "qwen-local", + "api_key": "k2", + "base_url": routed_url, + "api_mode": "", + "credential_pool": None, + "request_overrides": {}, + "max_output_tokens": None, + "command": None, + "args": [], + } + + with patch.object(run_agent.AIAgent, "__init__", _capture_init_kwargs(captured)), \ + patch("hermes_cli.config.load_config", _fake_load_config), \ + patch( + "hermes_cli.runtime_provider.resolve_runtime_provider", + _fake_resolve_runtime_provider, + ), \ + patch("threading.Thread", _SyncThread): + agent._spawn_background_review( + messages_snapshot=[], + review_memory=True, + review_skills=True, + ) + + assert captured.get("base_url") == routed_url, captured + assert captured.get("enabled_toolsets") == ["memory", "skills"], captured + assert captured.get("disabled_toolsets") is None, captured