From 8ef006933ec05eacae74c01ef9bf4f3d7f21bb0d Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:05:17 +0530 Subject: [PATCH] fix(background_review): gate reasoning_config inheritance on not-routed + dedupe recorder stubs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up to the reasoning_config cache-parity fix: - Only inherit the parent's reasoning_config when the fork runs on the parent's model (not routed). On the routed aux path (auxiliary.background_review.{provider,model}) the cache is cold regardless, so parity buys nothing, and the parent's effort vocabulary can be invalid for the routed model/provider: OpenRouter extra_body.reasoning.effort is forwarded unclamped (chat_completions.py) and codex_responses only maps max/ultra for gpt-5.6 — an exotic parent effort routed to a strict provider could 400 the review. Mirrors the existing 'not _routed' gate on _cached_system_prompt / session_start three lines below. - Add a routed-path regression test asserting reasoning_config is omitted from the fork kwargs when _resolve_review_runtime returns routed=True. - Extract the four copy-pasted recorder stubs in test_background_review_cache_parity.py into a single _make_recorder_class() factory so a new fork attribute needs one stub edit, not four. --- agent/background_review.py | 11 +- .../test_background_review_cache_parity.py | 234 +++++++++--------- 2 files changed, 132 insertions(+), 113 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index e1d4cbf1e89c..c2ea87bd94e2 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -699,7 +699,16 @@ def _run_review_in_thread( # Match parent's reasoning config so the fork's ``thinking`` / # ``output_config`` are byte-identical in the request body — # Anthropic's cache key is namespaced by ``thinking`` presence. - _fork_kwargs["reasoning_config"] = getattr(agent, "reasoning_config", None) + # Same-model path only: when routed to a different aux model the + # cache is cold regardless (parity buys nothing) and the parent's + # effort vocabulary may not be valid for the routed model/provider + # (e.g. OpenRouter ``extra_body.reasoning.effort`` is forwarded + # unclamped; codex_responses passes ``max``/``ultra`` through + # unmapped except on gpt-5.6/xAI). Let the routed fork use + # provider defaults — matching the ``not _routed`` gate on + # _cached_system_prompt below. + if not _routed: + _fork_kwargs["reasoning_config"] = getattr(agent, "reasoning_config", None) review_agent = AIAgent( model=_rt.get("model") or agent.model, max_iterations=16, diff --git a/tests/run_agent/test_background_review_cache_parity.py b/tests/run_agent/test_background_review_cache_parity.py index fd7ef4ff58b5..b7e27245d265 100644 --- a/tests/run_agent/test_background_review_cache_parity.py +++ b/tests/run_agent/test_background_review_cache_parity.py @@ -58,28 +58,52 @@ class _SyncThread: self._target() -class _ReviewAgentRecorder: - """Stand-in for the review-fork AIAgent that records the prompt assignment.""" +def _make_recorder_class(captured=None, record_on_run=()): + """Build a Recorder class standing in for the review-fork AIAgent. - def __init__(self, *args, **kwargs): - self._cached_system_prompt = None - self._memory_write_origin = None - self._memory_write_context = None - self._memory_store = None - self._memory_enabled = None - self._user_profile_enabled = None - self._memory_nudge_interval = None - self._skill_nudge_interval = None - self.suppress_status_output = None + Keeps the stub attribute list in ONE place: when + ``_spawn_background_review`` starts touching a new fork attribute, only + this factory needs the extra stub — not one copy per test. - def run_conversation(self, *args, **kwargs): - raise RuntimeError("stop after recording state — don't actually call the API") + ``captured`` (dict): if given, ``__init__`` stores the full constructor + kwargs under ``captured["init_kwargs"]`` so tests can assert on both + kwarg values and kwarg *presence*. + ``record_on_run``: instance attribute names copied into ``captured`` when + ``run_conversation`` fires — for values the production code assigns + after construction. + """ - def shutdown_memory_provider(self): - pass + class _Recorder: + def __init__(self, *args, **kwargs): + if captured is not None: + captured["init_kwargs"] = dict(kwargs) + self._cached_system_prompt = None + self._memory_write_origin = None + self._memory_write_context = None + self._memory_store = None + self._memory_enabled = None + self._user_profile_enabled = None + self._memory_nudge_interval = None + self._skill_nudge_interval = None + self.suppress_status_output = None + self.session_start = None + self.session_id = None - def close(self): - pass + def run_conversation(self, *args, **kwargs): + if captured is not None: + for _name in record_on_run: + captured[_name] = getattr(self, _name) + raise RuntimeError( + "stop after recording — don't actually call the API" + ) + + def shutdown_memory_provider(self): + pass + + def close(self): + pass + + return _Recorder def test_review_fork_inherits_parent_cached_system_prompt(): @@ -97,27 +121,21 @@ def test_review_fork_inherits_parent_cached_system_prompt(): captured = {} parent_prompt = agent._cached_system_prompt - # Hook the assignment site: record what gets put on the review agent. - real_recorder_init = _ReviewAgentRecorder.__init__ + _Recorder = _make_recorder_class() - def _recorder_init(self, *args, **kwargs): - real_recorder_init(self, *args, **kwargs) - # The actual production code assigns _cached_system_prompt AFTER __init__, - # so we need to capture it on attribute set. Use a property-style sentinel - # via __setattr__ on this instance. - - with patch.object(run_agent, "AIAgent", _ReviewAgentRecorder), \ + with patch.object(run_agent, "AIAgent", _Recorder), \ patch("threading.Thread", _SyncThread): - # Wrap the recorder's __setattr__ so we can see the _cached_system_prompt - # write that _spawn_background_review performs after construction. - orig_setattr = _ReviewAgentRecorder.__setattr__ + # The production code assigns _cached_system_prompt AFTER __init__, + # so wrap the recorder's __setattr__ to see that post-construction + # write from _spawn_background_review. + orig_setattr = _Recorder.__setattr__ def _spy_setattr(self, name, value): if name == "_cached_system_prompt": captured["written_prompt"] = value orig_setattr(self, name, value) - with patch.object(_ReviewAgentRecorder, "__setattr__", _spy_setattr): + with patch.object(_Recorder, "__setattr__", _spy_setattr): agent._spawn_background_review( messages_snapshot=[], review_memory=True, @@ -147,31 +165,9 @@ def test_review_fork_pins_session_start_and_session_id(): agent = _make_agent_stub(run_agent.AIAgent) captured = {} - - class _Recorder: - def __init__(self, *args, **kwargs): - self._cached_system_prompt = None - self._memory_write_origin = None - self._memory_write_context = None - self._memory_store = None - self._memory_enabled = None - self._user_profile_enabled = None - self._memory_nudge_interval = None - self._skill_nudge_interval = None - self.suppress_status_output = None - self.session_start = None - self.session_id = None - - def run_conversation(self, *args, **kwargs): - captured["session_start"] = self.session_start - captured["session_id"] = self.session_id - raise RuntimeError("stop after recording") - - def shutdown_memory_provider(self): - pass - - def close(self): - pass + _Recorder = _make_recorder_class( + captured, record_on_run=("session_start", "session_id") + ) with patch.object(run_agent, "AIAgent", _Recorder), \ patch("threading.Thread", _SyncThread): @@ -198,31 +194,7 @@ def test_review_fork_inherits_parent_toolset_config(): agent = _make_agent_stub(run_agent.AIAgent) captured = {} - - class _Recorder: - def __init__(self, *args, **kwargs): - captured["enabled_toolsets"] = kwargs.get("enabled_toolsets") - captured["disabled_toolsets"] = kwargs.get("disabled_toolsets") - self._cached_system_prompt = None - self._memory_write_origin = None - self._memory_write_context = None - self._memory_store = None - self._memory_enabled = None - self._user_profile_enabled = None - self._memory_nudge_interval = None - self._skill_nudge_interval = None - self.suppress_status_output = None - self.session_start = None - self.session_id = None - - def run_conversation(self, *args, **kwargs): - raise RuntimeError("stop after recording — don't actually call the API") - - def shutdown_memory_provider(self): - pass - - def close(self): - pass + _Recorder = _make_recorder_class(captured) with patch.object(run_agent, "AIAgent", _Recorder), \ patch("threading.Thread", _SyncThread): @@ -232,47 +204,30 @@ def test_review_fork_inherits_parent_toolset_config(): review_skills=False, ) - assert captured.get("enabled_toolsets") == agent.enabled_toolsets, ( - f"enabled_toolsets mismatch: {captured.get('enabled_toolsets')!r} " + init_kwargs = captured.get("init_kwargs", {}) + assert init_kwargs.get("enabled_toolsets") == agent.enabled_toolsets, ( + f"enabled_toolsets mismatch: {init_kwargs.get('enabled_toolsets')!r} " f"vs expected {agent.enabled_toolsets!r}" ) - assert captured.get("disabled_toolsets") == agent.disabled_toolsets, ( - f"disabled_toolsets mismatch: {captured.get('disabled_toolsets')!r} " + assert init_kwargs.get("disabled_toolsets") == agent.disabled_toolsets, ( + f"disabled_toolsets mismatch: {init_kwargs.get('disabled_toolsets')!r} " f"vs expected {agent.disabled_toolsets!r}" ) def test_review_fork_inherits_parent_reasoning_config(): - """``reasoning_config`` parity: fork must inherit parent's value so the request body's ``thinking`` / ``output_config`` match (Anthropic cache is namespaced by ``thinking`` presence).""" + """``reasoning_config`` parity on the default (non-routed) path. + + The fork must inherit the parent's value so the request body's + ``thinking`` / ``output_config`` match — Anthropic's cache is + namespaced by ``thinking`` presence. + """ import run_agent agent = _make_agent_stub(run_agent.AIAgent) captured = {} - - class _Recorder: - def __init__(self, *args, **kwargs): - captured["reasoning_config"] = kwargs.get("reasoning_config") - self._cached_system_prompt = None - self._memory_write_origin = None - self._memory_write_context = None - self._memory_store = None - self._memory_enabled = None - self._user_profile_enabled = None - self._memory_nudge_interval = None - self._skill_nudge_interval = None - self.suppress_status_output = None - self.session_start = None - self.session_id = None - - def run_conversation(self, *args, **kwargs): - raise RuntimeError("stop after recording — don't actually call the API") - - def shutdown_memory_provider(self): - pass - - def close(self): - pass + _Recorder = _make_recorder_class(captured) with patch.object(run_agent, "AIAgent", _Recorder), \ patch("threading.Thread", _SyncThread): @@ -282,7 +237,62 @@ def test_review_fork_inherits_parent_reasoning_config(): review_skills=False, ) - assert captured.get("reasoning_config") == agent.reasoning_config, ( - f"reasoning_config mismatch: {captured.get('reasoning_config')!r} " + init_kwargs = captured.get("init_kwargs", {}) + assert init_kwargs.get("reasoning_config") == agent.reasoning_config, ( + f"reasoning_config mismatch: {init_kwargs.get('reasoning_config')!r} " f"vs expected {agent.reasoning_config!r}" ) + + +def test_routed_review_fork_does_not_inherit_reasoning_config(): + """Routed aux path: the fork must NOT inherit the parent's reasoning_config. + + When ``auxiliary.background_review.{provider,model}`` routes the review + to a different model, cache parity is moot (the cache is cold on that + model regardless) and the parent's effort vocabulary may be invalid for + the routed model/provider (OpenRouter ``extra_body.reasoning.effort`` is + forwarded unclamped; codex_responses passes ``max``/``ultra`` through + unmapped except on gpt-5.6/xAI). The routed fork must fall back to + provider defaults, mirroring the ``not _routed`` gate on + ``_cached_system_prompt`` inheritance. + """ + import run_agent + import agent.background_review as bg_review + + agent_stub = _make_agent_stub(run_agent.AIAgent) + + captured = {} + _Recorder = _make_recorder_class(captured) + + routed_runtime = { + "provider": "openrouter", + "model": "aux-cheap-model", + "api_key": "test-key", + "base_url": None, + "api_mode": None, + "credential_pool": None, + "request_overrides": {}, + "max_tokens": None, + "command": None, + "args": [], + "routed": True, + } + + with patch.object(run_agent, "AIAgent", _Recorder), \ + patch.object(bg_review, "_resolve_review_runtime", + return_value=routed_runtime), \ + patch("threading.Thread", _SyncThread): + agent_stub._spawn_background_review( + messages_snapshot=[], + review_memory=True, + review_skills=False, + ) + + init_kwargs = captured.get("init_kwargs", {}) + assert "reasoning_config" not in init_kwargs, ( + f"Routed review fork was passed the parent's reasoning_config " + f"({init_kwargs.get('reasoning_config')!r}). On the routed path the " + "cache is cold (no parity benefit) and the parent's effort value may " + "be invalid for the routed model/provider — it must be omitted so " + "the fork uses provider defaults." + )