ouyangbo/hermes-agent
1
0
Fork 0
Code Issues Pull Requests Actions 9 Packages Projects Releases Wiki Activity

refactor(agent): clone the review snapshot once at the spawn chokepoint

Browse Source 
Move the structural clone from the four call sites (auto review, codex
runtime, CLI /refine, gateway /refine) into AIAgent._spawn_background_review,
which every review path — immediate, idle-queue deferred, requeued — passes
through. Callers can no longer forget it, and the private helper is no longer
imported across hermes_cli/ and gateway/ package boundaries.

Tests now bind the real chokepoint (capturing at _spawn_background_review_now)
so they still fail if the clone is removed.
This commit is contained in:
kshitijk4poor
2026-09-02 13:22:13 +05:30
committed by kshitij
parent 26f0de23cf
commit 2adb1a4ea6
6 changed files with 43 additions and 43 deletions
Download Patch File Download Diff File Expand all files Collapse all files
agent/codex_runtime.py
+1 -8
View File
@@ -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,
)
agent/turn_finalizer.py
+3 -7
View File
@@ -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,
)
gateway/slash_commands.py
+1 -6
View File
@@ -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."
hermes_cli/cli_commands_mixin.py
+1 -6
View File
@@ -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
run_agent.py
+7
View File
@@ -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,
tests/agent/test_refine_snapshot_isolation.py
+30 -16
View File
@@ -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)