fix(review): isolate background-review fork from the canonical session (salvage #50296)

[hashbender]

Salvage of NousResearch#50296 (@arminanton), rebased onto current main.

Issue

The forked skill/memory review agent shares the parent's session_id for prompt-cache warmth. Without isolation it wrote its harness turn ("Review the conversation above and update the skill library…") plus its curator-mode reply into the user's real session in state.db. The next live turn re-read that injected user message as a standing instruction → the agent "became" the curator and refused the actual task. Silent.

Fix (defense-in-depth)

• Root: a _persist_disabled flag on the fork that hard-stops every DB write / lazy-open path (_flush_messages_to_session_db, _ensure_db_session, _get_session_db_for_recall). The review writes only to the skill/memory stores via its tools.
• Heal: _strip_background_review_harness drops any stray harness message (and the assistant reply that followed) at load time in get_messages_as_conversation, so an already-polluted session resumes clean.

Verification

Contributor shipped 13 unit tests of the pure matcher/stripper. Phase 2c mutation-check found those covered zero of the two integration wirings (removing the _persist_disabled flush guard or the load-time _strip call left all 13 green). Added 2 integration guards (my follow-up commit): a _persist_disabled agent's flush writes nothing to a live DB; a polluted session resumes clean end-to-end through get_messages_as_conversation. Mutation-checked: each fails when its wiring is reverted. 15 tests pass (canonical runner).

Closes NousResearch#50296.

Co-authored-by: arminanton 29869547+arminanton@users.noreply.github.com

---

Mirror-of: NousResearch#56308
NousResearch#56308

[tenki-reviewer]

Review Complete

Files Reviewed: 5
Findings: 2

By Severity:

• 🟢 Low: 2

---

PR introduces _persist_disabled flag to prevent background review forks from polluting session DBs. Three low-severity defense-in-depth gaps remain: the harness strip filter misses combined-review prompts, leaves orphaned tool results in polluted sessions, and the JSON snapshot path doesn't honor _persist_disabled.

Files Reviewed (5 files)

agent/agent_init.py
agent/background_review.py
hermes_state.py
run_agent.py
tests/test_background_review_session_isolation.py

[tenki-reviewer]

Risk: 🟡 Medium (35/100) — 2 low findings · 290 LOC across 5 files

---

Background Review Session Isolation

This PR adds _persist_disabled to AIAgent to prevent background review subagents from writing harness messages into the parent session's state.db. The primary mechanism (flag gating on _flush_messages_to_session_db and _ensure_db_session) is correctly wired for the core DB path.

Gaps Found

Harness Strip Filter Incomplete

The defense-in-depth _strip_background_review_harness in hermes_state.py misses the combined review prompt prefix ("Review the conversation above and update two things:") used when both review_memory=True and review_skills=True. Only the individual skill-review and memory-review prefixes are in the detection tuple.

Tool-Call Cycles Not Handled

The strip function assumes harness → assistant(single reply) sequence, but background review agents commonly use tool calls (memory_add, skill_manage). This leaves orphaned tool results and the curator summary in the conversation history, breaking message-role adjacency.

JSON Snapshot Path Unprotected

_save_session_log in run_agent.py gates on _session_json_enabled but not _persist_disabled, contradicting the documented contract. The sole consumer sets both flags, but future callers relying only on _persist_disabled would leak to JSON snapshots.

Assessment

All three findings are low severity — no active bugs exist since the primary prevention flag works correctly. These are defense-in-depth and contract-completeness gaps that would manifest only with already-polluted sessions or future callers using the flag incompletely.

[tenki-reviewer]

🟢 Combined background-review harness prefix not covered by defense-in-depth strip filter (bug)

The _REVIEW_HARNESS_PREFIXES tuple in hermes_state.py (lines 210-213) contains:

• "Review the conversation above and update the skill library"
• "Review the conversation above and consider saving to memory"

But the _COMBINED_REVIEW_PROMPT in agent/background_review.py (line 276) starts with:

• "Review the conversation above and update two things:"

This prompt is selected when both review_memory=True and review_skills=True (background_review.py:887-888). The defense-in-depth filter _strip_background_review_harness (hermes_state.py:235-259) relies on _REVIEW_HARNESS_PREFIXES for detection, so combined-review harness messages would not be detected or stripped from polluted sessions. The primary fix (_persist_disabled) prevents new pollution; this gap only affects already-polluted sessions from older builds.

💡 Suggestion: Add the _COMBINED_REVIEW_PROMPT opening to _REVIEW_HARNESS_PREFIXES: add '"Review the conversation above and update two things"' as a third entry in the tuple.

📋 Prompt for AI Agents

In hermes_state.py, lines 210-213, add the combined review prompt opening 'Review the conversation above and update two things' as a third element in the _REVIEW_HARNESS_PREFIXES tuple. The combined prompt is defined at line 276 of agent/background_review.py and is selected when both review_memory=True and review_skills=True (line 887-888). Without this entry, the defense-in-depth harness strip filter will not detect combined-review harness messages.

[tenki-reviewer]

🟢 Defense-in-depth harness filter leaves orphaned tool results and curator reply when tool calls intervene (bug)

The _strip_background_review_harness function in hermes_state.py (lines 235-259) walks the message list and when it finds a harness message: (1) drops the harness message, (2) sets skip_next_assistant=True, (3) on the next iteration, if the next message has role=='assistant', drops it too and resets the flag. However, in practice the background review agent typically calls memory/skill tools, producing a sequence like:

user (harness: "Review the conversation above and update the skill library...")
assistant (tool_calls: memory_add, skill_manage, ...)
tool (results)
tool (more results)
assistant (no tool_calls, curator summary: "Nothing to save.")

The function strips the harness + first assistant(tool_calls), but the orphaned tool results and the final assistant(curator_reply) survive. These surviving messages break the expected message-role adjacency contract (tool results without preceding tool_calls) and the curator reply remains in the conversation history. The comment at line 240-241 describes stripping 'the curator-mode assistant reply that immediately followed each one' but does not account for intervening tool-call cycles.

💡 Suggestion: Extend the skip logic to consume all messages belonging to the curator turn: after detecting a harness, skip all subsequent assistant and tool messages until the next user or system message that is not itself a harness. This ensures the entire curator turn (tool calls, tool results, and final reply) is removed.

📋 Prompt for AI Agents

In hermes_state.py, rewrite _strip_background_review_harness (lines 235-259) to handle the common case where the review agent used tools. Change the skip logic from 'skip harness + next assistant' to 'skip harness + all subsequent assistant and tool messages until the next non-harness user/system message or end of list'. This ensures all review artifacts (harness prompt, tool calls, tool results, and curator summary) are removed from polluted sessions.