fix(agent): add stale-state guard to background review writes (#9055)

[rodboev]

Summary

Background self-improvement review can write memory or skill updates after the parent agent has moved on to a new conversation turn. This adds a live-state guard to the background review tool dispatch path that classifies both memory payload forms and fails closed when the parent state is unavailable.

Write-capable memory calls now include top-level add, replace, and remove plus every nonempty operations batch — the batch form that memory_tool() routes to apply_batch(). All six skill_manage write actions (create, edit, patch, delete, write_file, remove_file) require a matching parent snapshot. skill_view and skills_list pass through unchanged.

A write proceeds only when the parent exposes a valid live message list whose canonical token matches the review snapshot. A missing, non-list, or non-dict-containing _session_messages blocks the write rather than falling back to the snapshot.

Changes

• agent/background_review.py: add payload classifier for both memory forms and all six skill_manage write actions, add fail-closed freshness predicate, install callback via set_thread_tool_whitelist, keep cleanup in the existing finally (~+80 lines net)
• hermes_cli/plugins.py: extend set_thread_tool_whitelist with optional block_callback, clear it in clear_thread_tool_whitelist, invoke it in _get_pre_tool_call_directive_details after the whitelist admit-check and convert a non-empty string result to a block directive (~+20 lines)
• tests/run_agent/test_background_review.py: add regression coverage for memory batch writes (all four operations mutation shapes), all four invalid parent-state variants, absent _session_messages attribute, matching-state single-action and batch writes, and read-only skill calls, all routed through _run_agent_tool_execution_middleware and the installed callback (~+180 lines)
• tests/run_agent/test_background_review_toolset_restriction.py: extend _capture_whitelist stub to accept and assert block_callback without changing whitelist membership assertions (~+3 lines)

Validation

Scenario	Before	After
Parent unchanged; background review writes memory (top-level action)	Write succeeds	Write succeeds
Parent changes; background review writes memory (top-level action)	Stale write persists	Blocked before persistence
Parent changes; background review writes memory (batch operations)	Stale write persists — guard missed batch form	Blocked before apply_batch()
Parent changes; background review patches a skill	Stale update persists	Blocked before persistence
_session_messages absent	Write permitted — fallback to snapshot	Blocked
_session_messages is None, dict, string, or list with non-dict item	Write permitted — fallback to snapshot	Blocked
Non-whitelisted tool	Denied by whitelist	Still denied (runs before callback)
skill_view or skills_list	Allowed	Still allowed

pytest tests/run_agent/test_background_review.py tests/run_agent/test_background_review_toolset_restriction.py -v --timeout=0 — 28 passed

Not in scope

This PR does not serialize cross-session, cron, or sibling-review writers. PR #55906 separately requires background review to read the exact skill target before writing; it covers a different cross-writer path and does not supersede this same-parent freshness guard.

Closes #9055.
Reported by @yexxx.

[liuhao1024]

Verified the stale-write guard design. The core concern is real: _run_review_in_thread runs on a background thread while the parent agent continues processing messages on the main thread. If the parent's _session_messages mutates between snapshot time and tool-call time, the review agent's memory/skill writes are based on stale context.

The snapshot-token approach is the right pattern — comparing a JSON-normalized hash of the snapshot messages against the live _session_messages at tool-call time. The normalization (_json_normalized with sorted keys, type coercion for non-serializable objects) handles the common mutation cases (new message appended, content changed).

Scope check: the block_callback added to set_thread_tool_whitelist is correctly scoped — it only fires for tools already in the whitelist, only blocks memory writes (add/replace/remove) and skill writes (create/edit/patch/delete/write_file/remove_file), and allows read-only tools (skill_view, memory with read actions) through unconditionally. The test at line 245 (assert observed["skill_read_block"] is None) confirms this.

One design note: _background_review_snapshot_token recomputes the normalized JSON on every tool call. For reviews with many tool calls on large conversation histories, this is O(n) per call. Not a blocking issue — background reviews are infrequent and bounded by tool whitelist — but worth noting if this pattern is reused elsewhere.

[maxgris]

Read through the guard — the snapshot-token approach is clean and the block_callback
plumbing into set_thread_tool_whitelist is nicely minimal (read-only tools passing through
unconditionally is the right call). One scope limitation worth deciding on explicitly,
because the issue's enumerated clobber sources are wider than what this catches:

The guard keys on this parent's conversation, but three of the four sources #9055 lists
mutate the shared store without touching it. _block_stale_parent_write compares
_background_review_snapshot_token(agent._session_messages) against the snapshot — so it
fires only when this parent agent's _session_messages changed. That fully covers the
"a later foreground turn" case (the common one). But #9055 also names:

• another concurrent session,
• cron / scheduled jobs,
• another background-review thread.

Each of those writes to the shared memory/skill store via a different agent whose activity
never changes this parent's _session_messages. So the snapshot token still matches, the
write isn't blocked, and the stale review can still clobber what the other writer just
committed. The guard is keyed on the wrong side of the race for those three — the data lives
in the store, but the freshness check is on the conversation.

The two stores aren't equally exposed to that residual, which affects how much it matters:

• Memory is largely backstopped already, independent of this PR. MemoryTool.replace/
  .remove call _reload_target() under a file lock before mutating, re-read live on-disk
  entries, substring-match against that fresh state, and fail closed on no-match
  ("No entry matched") or multiple-distinct-match ("Be more specific"); add dedups exact
  duplicates. So a cross-session/cron destructive memory write that no longer matches live
  state already fails closed at the tool layer. The residual there is thin (a stale add of a
  paraphrase that dodges dedup).

• Skills are the exposed side. tools/skill_manager_tool.py has no file lock anywhere and
  no reload-before-write: _write_file ("overwrite a supporting file") and the full-rewrite
  path call _atomic_write_text() unconditionally over existing content. _patch_skill reads
  fresh + fuzzy-matches + fails closed, so patch is fine — but a blind write_file/rewrite
  from a concurrent session or a cron-spawned review will still clobber a newer file, and this
  PR's parent-conversation guard won't catch it because that other writer didn't touch this
  parent's conversation.

So the net after this PR: the same-session race is closed for both stores (great — that's
likely the bulk of real incidents), but cross-writer skill overwrite (concurrent session /
cron / sibling review) remains. Two ways to handle it, either is reasonable:

1. Document the scope — note in the guard's docstring / PR that it covers same-session
   staleness, and that cross-session/cron coherence relies on the store layer (which holds for
   memory's destructive ops but not for skill overwrite). Cheap, honest, unblocks the merge.
2. Add a complementary store-level check for the skill-overwrite path — before
   write_file/full-rewrite in background-review context (the fork already tags itself via
   _memory_write_context == "background_review"), confirm the target file still matches what
   the review read, mirroring what MemoryTool._reload_target does for memory. That closes the
   cross-writer skill case the conversation-token can't see.

Not a blocker on the fix as-is — it's a real improvement over main. Just flagging so the
remaining surface is a conscious decision rather than an implied "fully closed." Happy to help
with (2) if that's the direction.

(Minor, separate: live_messages = getattr(agent, "_session_messages", messages_snapshot)
fails open if _session_messages is ever absent — token matches the snapshot, write
allowed. Fine given it's set on the parent in practice, but worth a comment since the whole
point is to fail closed on uncertainty.)

[rodboev]

Good analysis, thanks for tracing through the cross-writer paths.

You're right that the guard is keyed on the parent conversation, so it only covers the same-session race (which is the common one in practice, since the review fork is spawned from a specific parent turn). The three cross-writer sources you identified bypass the conversation token entirely.

Agreed that memory's store-layer reload-before-mutate already fails closed for the destructive cases there, so the residual is thin. The exposed surface is skill overwrite from a concurrent session or cron-spawned review, where _write_file/full-rewrite has no content check.

For now I'll go with option 1 and document the scope in the guard's docstring. A complementary store-level check for the skill overwrite path is a good follow-up but feels like a separate concern from this PR's same-session guard.

Also noted the fail-open edge on the getattr fallback, will add a comment since the intent is fail-closed.

Rebased onto current main to resolve the conflict (upstream added memory notification tests in the same file).

[teknium1]

Context: #55906 (merged) added a read-before-write invariant for background-review skill writes, closing the corruption path in #55647.

This PR addresses a different failure mode — a stale-state race where the review writes after the parent conversation has moved past the snapshot (#9055). Not superseded by #55906; leaving open for separate review.

[teknium1]

Thanks for isolating the same-session stale-review race. The premise still exists on current main: agent/turn_finalizer.py:500-508 starts a daemon review from list(messages), while the parent later refreshes _session_messages (run_agent.py:1693).

Problems

• agent/background_review.py:80-86 only treats top-level action as a memory mutation. tools/memory_tool.py:959-997 also accepts operations=[...] with no top-level action and persists it through apply_batch(), so a stale batched memory write bypasses this guard.
• agent/background_review.py:689 falls back to messages_snapshot if live state is absent. That makes the comparison succeed and permits the write, rather than failing closed.

Suggested changes

• Recognize mutating entries in memory.operations and add a stale-batch regression through the real pre-tool dispatch path.
• Block when _session_messages is unavailable or not a valid live message list.

Automated hermes-sweeper review.