From 5b5932886ce6477a0f4a3d25ca465392288d5126 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 2 Aug 2026 23:19:56 +0530 Subject: [PATCH] fix(background-review): inherit prefill messages + OpenRouter provider pins on non-routed forks Completes the cache-parity bug class from #76938: the parent's request body diverges from the fork's not only at the ephemeral system prompt but also at prefill messages (inserted right after the system message at API-call time) and, on OpenRouter, at upstream-provider selection (prompt caches live per upstream; an unpinned fork can be routed to a different upstream and miss a byte-identical prefix). Also hardens the tests: pairwise asserts instead of re-implementing the production prompt join, and routed-path omission guards for the whole gated kwarg family. --- agent/background_review.py | 31 ++++++++ .../test_background_review_cache_parity.py | 77 ++++++++++++++++--- 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index 92dbf8003a026..01820cfffdf22 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -18,6 +18,7 @@ for invariants and PR review criteria. from __future__ import annotations +import copy import json import logging import os @@ -752,6 +753,36 @@ def _run_review_in_thread( _fork_kwargs["ephemeral_system_prompt"] = getattr( agent, "ephemeral_system_prompt", None ) + # Prefill messages are inserted immediately after the system + # message at API-call time (chat_completion_helpers.py / + # conversation_loop.py), so a parent with prefill configured + # (gateway prefill_messages_file) would otherwise diverge + # from the warm prefix at message index 1 — same bug class + # as the ephemeral prompt above, one position later. + # Deep copy: the unicode-error recovery path mutates + # prefill entries IN PLACE (_sanitize_messages_surrogates + # via conversation_loop), so sharing dicts would let a + # fork-side sanitize rewrite the parent's prefill bytes. + _parent_prefill = copy.deepcopy( + getattr(agent, "prefill_messages", None) or [] + ) + if _parent_prefill: + _fork_kwargs["prefill_messages"] = _parent_prefill + # OpenRouter provider-routing pins: prompt caches live per + # UPSTREAM provider, so a fork without the parent's pins can + # be routed to a different upstream and miss the warm cache + # even with byte-identical prompt/tools bytes. + for _pref_attr in ( + "providers_allowed", + "providers_ignored", + "providers_order", + "provider_sort", + "provider_require_parameters", + "provider_data_collection", + ): + _pref_val = getattr(agent, _pref_attr, None) + if _pref_val: + _fork_kwargs[_pref_attr] = _pref_val 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 d050a148ba717..da5f1dcb61404 100644 --- a/tests/run_agent/test_background_review_cache_parity.py +++ b/tests/run_agent/test_background_review_cache_parity.py @@ -47,6 +47,16 @@ def _make_agent_stub(agent_cls): # 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"} + # Non-empty so tests catch prefill/provider-routing NOT being inherited — + # prefills sit right after the system message in the request body, and + # OpenRouter provider pins decide WHICH upstream's cache gets hit. + agent.prefill_messages = [{"role": "user", "content": "prefill turn"}] + agent.providers_allowed = ["anthropic"] + agent.providers_ignored = None + agent.providers_order = None + agent.provider_sort = "throughput" + agent.provider_require_parameters = False + agent.provider_data_collection = None return agent @@ -181,17 +191,49 @@ def test_review_fork_inherits_parent_ephemeral_system_prompt(): review_skills=False, ) - parent_effective = ( - getattr(agent, "_cached_system_prompt") - + "\n\n" - + getattr(agent, "ephemeral_system_prompt") - ).strip() - fork_effective = ( - captured["_cached_system_prompt"] - + "\n\n" - + captured["ephemeral_system_prompt"] - ).strip() - assert fork_effective == parent_effective + # Pairwise asserts: stronger than comparing a locally re-joined + # "effective" prompt (which would re-implement the production join and + # silently keep passing if the separator ever changed — and would compare + # equal for cached="A\n\nB"/ephemeral="" vs cached="A"/ephemeral="B"). + assert captured["_cached_system_prompt"] == agent._cached_system_prompt + assert captured["ephemeral_system_prompt"] == agent.ephemeral_system_prompt + + +def test_review_fork_inherits_prefill_and_provider_routing(): + """Non-routed fork must inherit prefill messages and OpenRouter pins. + + Prefill messages are inserted right after the system message at + API-call time, so omitting them diverges the fork's request body from + the parent's warm prefix at message index 1. OpenRouter provider pins + (providers_allowed/order/sort/...) decide which UPSTREAM provider serves + the request — prompt caches live per upstream, so an unpinned fork can + be routed to a different upstream and miss even a byte-identical prefix. + """ + 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): + agent._spawn_background_review( + messages_snapshot=[], + review_memory=True, + review_skills=False, + ) + + init_kwargs = captured.get("init_kwargs", {}) + assert init_kwargs.get("prefill_messages") == agent.prefill_messages + # Must be a DEEP copy: the fork's unicode-error recovery + # (_sanitize_messages_surrogates) mutates prefill dicts in place, so + # aliased dicts would let the fork rewrite the parent's prefill bytes + # — silently breaking the parent's own warm prefix. + assert ( + init_kwargs["prefill_messages"][0] is not agent.prefill_messages[0] + ), "fork prefill aliases the parent's dicts (needs deepcopy)" + assert init_kwargs.get("providers_allowed") == agent.providers_allowed + assert init_kwargs.get("provider_sort") == agent.provider_sort def test_review_fork_pins_session_start_and_session_id(): @@ -285,3 +327,16 @@ def test_routed_review_fork_does_not_inherit_reasoning_config(): "be invalid for the routed model/provider — it must be omitted so " "the fork uses provider defaults." ) + # The whole cache-parity kwarg family shares the same ``not _routed`` + # gate — a future refactor hoisting any of them out of the gate must + # fail here, not silently ship parent-only context to a foreign model. + for _gated in ( + "ephemeral_system_prompt", + "prefill_messages", + "providers_allowed", + "provider_sort", + ): + assert _gated not in init_kwargs, ( + f"Routed review fork was passed parent-only kwarg {_gated!r}; " + "cache-parity inheritance must stay behind the not-routed gate." + )