From 6bc0e9e6df1be528dca42b4ae720026192896662 Mon Sep 17 00:00:00 2001 From: joaomarcos Date: Sun, 13 Sep 2026 12:28:26 -0300 Subject: [PATCH] fix(agent): same-model review fork keeps the parent's affinity header and Portal conversation root (#109964) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Trimmed salvage of #110045 (deltas 1 + 2 only), stacked on the #110009 scope inheritance: - `declared_conversation_scope` treats an inherited value as a DECLARED scope only when it carries the `gwk_` prefix. A rotated CLI parent publishes no affinity scope (None → sticky key falls back to the conversation root); the fork now publishes exactly the same instead of an explicit physical lineage root. `resolve_prompt_cache_scope` honors any inherited value directly, so the body `prompt_cache_key` still matches. - `build_cache_parity_fork` snapshots `parent._conversation_root_id()` as `_cached_conversation_root`; with `_session_db=None` the fork's own walk fell back to the parent's PHYSICAL id, so after a compression rotation the review's Portal `conversation=` tag fragmented usage attribution across one logical conversation. Dropped from the original: copying `_gateway_session_key` onto the persistence-detached fork (no cache-identity consumer reads it there; the compression-boundary hooks were deliberately severed by `_detach_fork_compression`), and the defensive hasattr/callable/try wrapper around `_conversation_root_id()`. --- agent/background_review.py | 4 ++ agent/prompt_cache_scope.py | 38 +++++++++---------- run_agent.py | 3 ++ .../test_background_review_cache_parity.py | 11 +++++- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/agent/background_review.py b/agent/background_review.py index 58553c3635..5979b1134b 100644 --- a/agent/background_review.py +++ b/agent/background_review.py @@ -970,6 +970,10 @@ def build_cache_parity_fork( inherited_scope = resolve_prompt_cache_scope_safe(agent) if inherited_scope: review_agent._inherited_cache_scope = inherited_scope + # Same reason for the Portal ``conversation=`` tag: with no DB the fork's own + # _conversation_root_id() falls back to the parent's PHYSICAL id, so after a compression + # rotation the review's usage was attributed to a different conversation than its parent. + review_agent._cached_conversation_root = agent._conversation_root_id() _inherit_parent_tool_surface(review_agent, agent) _detach_fork_compression(review_agent) # Compaction bounds a single request; this bounds the WHOLE review (checked in diff --git a/agent/prompt_cache_scope.py b/agent/prompt_cache_scope.py index 0aebf35409..2efec6971e 100644 --- a/agent/prompt_cache_scope.py +++ b/agent/prompt_cache_scope.py @@ -87,23 +87,21 @@ def declared_conversation_scope(agent: Any) -> Optional[str]: """Host-declared logical conversation scope (``gwk_``), or None. Hashes ``(source, gateway_session_key, generation)``. None (fall back to the physical id) - when no key is declared, for an explicit fork child, and on any DB error (fail closed - rather than merge a fork onto its parent's key). + when no key is declared, for a background-review fork (``_persist_disabled``), for an + explicit fork child, and on any DB error (fail closed rather than merge a fork onto its + parent's key). - A same-model background-review fork (``_persist_disabled=True``, ``_session_db=None``) - resolves the parent's *already-resolved* scope via ``_inherited_cache_scope`` BEFORE the - ``_persist_disabled`` fail-closed branch (#109964): the fork's entire purpose is prefix - parity with the parent, but the exclusion below plus the missing DB made both resolvers - diverge — the header path (affinity/sticky session) and the body path - (``prompt_cache_key``) keyed the fork into a different bucket, costing one cold - ~full-context request per review. The inherited value is stamped once at fork time by - ``build_cache_parity_fork`` (no DB access from the fork), so persistence stays fully - detached. Nothing sets the attribute for routed (different-model) forks, ``/branch`` - children, delegate/tool children, or fresh sessions — the fail-closed default stands. + The one sanctioned exception is a same-model cache-parity fork (#109964): its whole purpose + is prefix parity with the parent, yet ``_persist_disabled`` + ``_session_db=None`` made both + resolvers key it into a different bucket (one cold ~full-context request per review). + ``build_cache_parity_fork`` stamps the parent's ALREADY-RESOLVED scope as + ``_inherited_cache_scope`` (no DB access from the fork). Only a ``gwk_`` value is a declared + scope; a physical lineage root stays out of the affinity header so the fork publishes + exactly what its parent publishes (None → consumers fall back to the conversation root). """ inherited = getattr(agent, "_inherited_cache_scope", None) - if inherited: - return str(inherited) + if isinstance(inherited, str) and inherited.startswith(_DECLARED_SCOPE_PREFIX): + return inherited key = str(getattr(agent, "_gateway_session_key", "") or "").strip() if not key or getattr(agent, "_persist_disabled", False): return None @@ -143,12 +141,12 @@ def declared_conversation_scope(agent: Any) -> Optional[str]: def resolve_prompt_cache_scope(agent: Any) -> str: - """Rotation-stable cache-scope id: declared scope, else the compression-lineage root of - ``agent.session_id`` (the physical id without ancestry/DB). Memoized on the agent. - - A same-model review fork's inherited scope (``_inherited_cache_scope``) resolves through - :func:`declared_conversation_scope` above, which honors it first — fixing header and body - paths together (#109964).""" + """Rotation-stable cache-scope id: the inherited parent scope of a same-model cache-parity + fork, else the declared scope, else the compression-lineage root of ``agent.session_id`` + (the physical id without ancestry/DB). Memoized on the agent.""" + inherited = getattr(agent, "_inherited_cache_scope", None) + if isinstance(inherited, str) and inherited: + return inherited sid = str(getattr(agent, "session_id", None) or "") if not sid: return "" diff --git a/run_agent.py b/run_agent.py index ce76b3d6c0..e6601e440c 100644 --- a/run_agent.py +++ b/run_agent.py @@ -1333,6 +1333,9 @@ class AIAgent( def _conversation_root_id(self) -> Optional[str]: """Session-lineage ROOT id for Portal usage attribution, so one conversation keeps a single ``conversation=`` tag across compression rotation; subagents resolve via ``_parent_session_id``.""" + cached = getattr(self, "_cached_conversation_root", None) + if cached: + return str(cached) sid = getattr(self, "session_id", None) if not sid: return None diff --git a/tests/agent/test_background_review_cache_parity.py b/tests/agent/test_background_review_cache_parity.py index 7dfa56cfc8..aca42a175e 100644 --- a/tests/agent/test_background_review_cache_parity.py +++ b/tests/agent/test_background_review_cache_parity.py @@ -520,10 +520,14 @@ def test_same_model_fork_inherits_parent_cache_scope_gateway_key(tmp_path): def test_same_model_fork_inherits_parent_cache_scope_rotated_lineage(tmp_path): """#109964 invariant 1 (rotated-lineage case): a CLI parent whose lineage root != current physical id must also pass its scope to the fork. Pre-fix - the parent resolved 'root-sid' while the fork fell to the physical id.""" + the parent resolved 'root-sid' while the fork fell to the physical id. + + Every identity the fork publishes must equal the parent's: body cache key, the + affinity header (None for both — a physical root is not a declared ``gwk_`` scope) + and the Portal ``conversation=`` root (fork has no DB to walk the lineage).""" import run_agent from agent.background_review import build_cache_parity_fork - from agent.prompt_cache_scope import resolve_prompt_cache_scope + from agent.prompt_cache_scope import declared_conversation_scope, resolve_prompt_cache_scope from hermes_state import SessionDB db = SessionDB(db_path=tmp_path / "state.db") @@ -543,6 +547,9 @@ def test_same_model_fork_inherits_parent_cache_scope_rotated_lineage(tmp_path): assert resolve_prompt_cache_scope(agent) == "root-sid" assert getattr(fork, "_inherited_cache_scope", None) == "root-sid" assert resolve_prompt_cache_scope(fork) == "root-sid" + assert declared_conversation_scope(fork) is declared_conversation_scope(agent) is None + fork_root = run_agent.AIAgent._conversation_root_id(fork) + assert fork_root == agent._conversation_root_id() == "root-sid" finally: db.close()