fix(background_review): inherit parent's reasoning_config for Anthropic cache parity (salvage #30532)

agent/background_review.py

@@ -696,6 +696,19 @@ def _bg_review_auto_deny(command, description, **kwargs):
             if isinstance(_rt.get("command"), str) and _rt["command"]:
                 _fork_kwargs["acp_command"] = _rt["command"]
                 _fork_kwargs["acp_args"] = _rt.get("args") or []
+            # 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.
+            # 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,

tests/run_agent/test_background_review_cache_parity.py

@@ -41,6 +41,9 @@ def _make_agent_stub(agent_cls):
     # Non-None so the test catches a missing-kwarg regression.
     agent.enabled_toolsets = ["memory", "skills", "terminal"]
     agent.disabled_toolsets = ["spotify", "feishu_doc"]
+    # Non-None so the test catches reasoning_config NOT being inherited —
+    # which would put the fork into a different Anthropic cache namespace.
+    agent.reasoning_config = {"enabled": True, "effort": "medium"}
     return agent
 
 
@@ -55,28 +58,52 @@ def start(self):
             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.
+    """
+
+    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 shutdown_memory_provider(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 close(self):
-        pass
+        def shutdown_memory_provider(self):
+            pass
+
+        def close(self):
+            pass
+
+    return _Recorder
 
 
 def test_review_fork_inherits_parent_cached_system_prompt():
@@ -94,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,
@@ -144,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):
@@ -195,31 +194,40 @@ def test_review_fork_inherits_parent_toolset_config():
     agent = _make_agent_stub(run_agent.AIAgent)
 
     captured = {}
+    _Recorder = _make_recorder_class(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
+    with patch.object(run_agent, "AIAgent", _Recorder), \
+         patch("threading.Thread", _SyncThread):
+        agent._spawn_background_review(
+            messages_snapshot=[],
+            review_memory=True,
+            review_skills=False,
+        )
 
-        def run_conversation(self, *args, **kwargs):
-            raise RuntimeError("stop after recording — don't actually call the API")
+    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 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 shutdown_memory_provider(self):
-            pass
 
-        def close(self):
-            pass
+def test_review_fork_inherits_parent_reasoning_config():
+    """``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 = {}
+    _Recorder = _make_recorder_class(captured)
 
     with patch.object(run_agent, "AIAgent", _Recorder), \
          patch("threading.Thread", _SyncThread):
@@ -229,11 +237,62 @@ def close(self):
             review_skills=False,
         )
 
-    assert captured.get("enabled_toolsets") == agent.enabled_toolsets, (
-        f"enabled_toolsets mismatch: {captured.get('enabled_toolsets')!r} "
-        f"vs expected {agent.enabled_toolsets!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}"
     )
-    assert captured.get("disabled_toolsets") == agent.disabled_toolsets, (
-        f"disabled_toolsets mismatch: {captured.get('disabled_toolsets')!r} "
-        f"vs expected {agent.disabled_toolsets!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."
     )