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.
This commit is contained in:
parent
857926cceb
commit
5b5932886c
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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."
|
||||
)
|
||||
|
|
|
|||
Loading…
Reference in New Issue