diff --git a/agent/codex_runtime.py b/agent/codex_runtime.py index 87e6bc5d7c..30b86c6171 100644 --- a/agent/codex_runtime.py +++ b/agent/codex_runtime.py @@ -933,15 +933,8 @@ def run_codex_app_server_turn( and (should_review_memory or should_review_skills) ): try: - # Keep the review fork's in-place transcript normalization from - # mutating the live foreground messages after persistence. A - # shallow list copy still aliases nested tool-call/content data, - # which can make the next rebuilt request diverge from the cached - # prefix. - from agent.turn_finalizer import _clone_background_review_messages - agent._spawn_background_review( - messages_snapshot=_clone_background_review_messages(messages), + messages_snapshot=list(messages), review_memory=should_review_memory, review_skills=should_review_skills, ) diff --git a/agent/turn_finalizer.py b/agent/turn_finalizer.py index dbe073495f..93506f680c 100644 --- a/agent/turn_finalizer.py +++ b/agent/turn_finalizer.py @@ -819,14 +819,10 @@ def finalize_turn( and (_should_review_memory or _should_review_skills) ): try: - # The review fork sanitizes and repairs its private transcript in - # place. A shallow list copy would leave the message dicts (and - # nested tool-call/content containers) shared with the live - # foreground transcript, allowing the review to mutate the - # representation that was just persisted and break prefix-cache - # parity on the next turn. + # _spawn_background_review clones the snapshot structurally so + # the fork's in-place sanitizers can't reach the live transcript. agent._spawn_background_review( - messages_snapshot=_clone_background_review_messages(messages), + messages_snapshot=list(messages), review_memory=_should_review_memory, review_skills=_should_review_skills, ) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 092feb696d..5493e849dc 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -3019,12 +3019,7 @@ class GatewaySlashCommandsMixin: if agent is None: return "Nothing to refine yet — send a message first." - # Structural clone; see _clone_background_review_messages (#100795). - from agent.turn_finalizer import _clone_background_review_messages - - snapshot = _clone_background_review_messages( - getattr(agent, "_session_messages", None) or [] - ) + snapshot = list(getattr(agent, "_session_messages", None) or []) if not snapshot: return "Nothing to refine yet — the conversation is empty." diff --git a/hermes_cli/cli_commands_mixin.py b/hermes_cli/cli_commands_mixin.py index df82c2100c..7fe5737319 100644 --- a/hermes_cli/cli_commands_mixin.py +++ b/hermes_cli/cli_commands_mixin.py @@ -3007,12 +3007,7 @@ class CLICommandsMixin: _cprint(f" {_DIM}Nothing to refine yet — send a message first.{_RST}") return - # Structural clone; see _clone_background_review_messages (#100795). - from agent.turn_finalizer import _clone_background_review_messages - - snapshot = _clone_background_review_messages( - getattr(self, "conversation_history", None) or [] - ) + snapshot = list(getattr(self, "conversation_history", None) or []) if not snapshot: _cprint(f" {_DIM}Nothing to refine yet — the conversation is empty.{_RST}") return diff --git a/run_agent.py b/run_agent.py index 6ec67b2a4e..86b62db372 100644 --- a/run_agent.py +++ b/run_agent.py @@ -2021,6 +2021,13 @@ class AIAgent: if not enabled: return + # Structural clone at the single chokepoint every review path + # (automatic, /refine, idle-queue deferral) goes through. The fork + # sanitizes its transcript in place; a shallow copy would alias the + # nested tool_calls/content containers of the live history (#100795). + from agent.turn_finalizer import _clone_background_review_messages + messages_snapshot = _clone_background_review_messages(messages_snapshot) + kwargs = dict( messages_snapshot=messages_snapshot, review_memory=review_memory, diff --git a/tests/agent/test_refine_snapshot_isolation.py b/tests/agent/test_refine_snapshot_isolation.py index c1abfa9d26..234beb1993 100644 --- a/tests/agent/test_refine_snapshot_isolation.py +++ b/tests/agent/test_refine_snapshot_isolation.py @@ -1,11 +1,13 @@ -"""/refine hands the review fork a snapshot that cannot alias the live transcript. +"""Every review path hands the fork a snapshot that cannot alias the live transcript. -The automatic post-turn review already clones structurally -(``_clone_background_review_messages``); the two explicit ``/refine`` entry -points (CLI mixin + gateway slash command) build their own snapshot and must -use the same clone — a shallow ``list()`` shares the nested ``tool_calls`` / -``content`` containers with the persisted history, so the fork's in-place -transcript sanitization would rewrite the parent's messages (#100795). +``AIAgent._spawn_background_review`` is the single chokepoint the automatic +post-turn review, the idle-queue deferral and both explicit ``/refine`` entry +points (CLI mixin + gateway slash command) go through; it clones the snapshot +structurally there. A shallow ``list()`` would share the nested +``tool_calls`` / ``content`` containers with the persisted history, so the +fork's in-place transcript sanitization would rewrite the parent's messages +(#100795). These tests drive the real /refine handlers into the real +chokepoint and capture what reaches the spawn. """ import threading @@ -14,6 +16,21 @@ from unittest.mock import MagicMock import pytest +def _agent_with_real_chokepoint(): + """MagicMock agent whose _spawn_background_review is the REAL method. + + Everything below the chokepoint (thread spawn) is captured at + ``_spawn_background_review_now`` so no fork actually runs. + """ + from run_agent import AIAgent + + agent = MagicMock() + agent.valid_tool_names = {"memory"} + agent._delegate_depth = 0 + agent._spawn_background_review = AIAgent._spawn_background_review.__get__(agent) + return agent + + def _nested_history(): return [ {"role": "user", "content": [{"type": "text", "text": "ask"}]}, @@ -30,7 +47,6 @@ def _nested_history(): def _assert_isolated(live, snapshot): assert snapshot == live # same shape/bytes … - assert snapshot is not live for live_msg, snap_msg in zip(live, snapshot): assert snap_msg is not live_msg # … but no shared containers for key in ("content", "tool_calls"): @@ -47,16 +63,15 @@ def test_cli_refine_snapshot_does_not_alias_live_history(monkeypatch): from hermes_cli.cli_commands_mixin import CLICommandsMixin monkeypatch.setattr("cli._cprint", lambda *a, **k: None, raising=False) - agent = MagicMock() - agent.valid_tool_names = {"memory"} + agent = _agent_with_real_chokepoint() cli = object.__new__(CLICommandsMixin) cli.agent = agent cli.conversation_history = _nested_history() cli._handle_refine_command("/refine") - agent._spawn_background_review.assert_called_once() - snapshot = agent._spawn_background_review.call_args.kwargs["messages_snapshot"] + agent._spawn_background_review_now.assert_called_once() + snapshot = agent._spawn_background_review_now.call_args.kwargs["messages_snapshot"] _assert_isolated(cli.conversation_history, snapshot) @@ -65,8 +80,7 @@ async def test_gateway_refine_snapshot_does_not_alias_live_history(): from gateway.run import GatewayRunner key = "agent:main:test:dm:1" - agent = MagicMock() - agent.valid_tool_names = {"memory"} + agent = _agent_with_real_chokepoint() agent._session_messages = _nested_history() runner = object.__new__(GatewayRunner) @@ -82,6 +96,6 @@ async def test_gateway_refine_snapshot_does_not_alias_live_history(): out = await runner._handle_refine_command(event) assert out.startswith("⚗") - agent._spawn_background_review.assert_called_once() - snapshot = agent._spawn_background_review.call_args.kwargs["messages_snapshot"] + agent._spawn_background_review_now.assert_called_once() + snapshot = agent._spawn_background_review_now.call_args.kwargs["messages_snapshot"] _assert_isolated(agent._session_messages, snapshot)