fix(review): isolate background-review fork from the canonical session (salvage #50296) - #56308
Merged
Merged
Conversation
…ssion
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
straight into the user's REAL session in state.db; the next live turn re-read
that injected user message as a standing instruction and the agent 'became' the
curator, refusing the actual task.
Root fix: a _persist_disabled flag on the fork that hard-stops every DB write
and 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. Defense-in-depth: _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.
Salvaged from NousResearch#50296.
Co-authored-by: arminanton <29869547+arminanton@users.noreply.github.com>
…eview) Phase 2c mutation-check found the salvaged tests covered only the pure helpers (_is_background_review_harness_message / _strip_background_review_harness) — the two integration WIRINGS had zero coverage: removing the _persist_disabled guard in _flush_messages_to_session_db, or the _strip call in get_messages_as_conversation, left all 13 tests green. Add: - TestPersistDisabledHardStop: a _persist_disabled agent's flush writes nothing to a live SessionDB (guards the run_agent hard-stop). - TestGetMessagesAsConversationStripsHarness: a session with stray harness rows resumes clean end-to-end through get_messages_as_conversation (guards the hermes_state load-time wiring). Mutation-checked: each new test fails when its wiring is reverted.
kshitijk4poor
enabled auto-merge (rebase)
July 1, 2026 10:46
alt-glitch
added
type/bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Salvage of #50296 (@arminanton), rebased onto current main.
Issue
The forked skill/memory review agent shares the parent's
session_idfor 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)
_persist_disabledflag 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._strip_background_review_harnessdrops any stray harness message (and the assistant reply that followed) at load time inget_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_disabledflush guard or the load-time_stripcall left all 13 green). Added 2 integration guards (my follow-up commit): a_persist_disabledagent's flush writes nothing to a live DB; a polluted session resumes clean end-to-end throughget_messages_as_conversation. Mutation-checked: each fails when its wiring is reverted. 15 tests pass (canonical runner).Closes #50296.
Co-authored-by: arminanton 29869547+arminanton@users.noreply.github.com