fix(acp): stop _persist from deleting compression-archived history (salvage #50405) - #56342
Merged
kshitijk4poor merged 2 commits intoJul 1, 2026
Conversation
ACP's SessionManager._persist() called db.replace_messages() on every save. That delete-then-reinsert is destructive by design. The agent backing each ACP session already persists to the same SessionDB itself: it flushes turns incrementally via append_message and, on context compression, preserves pre-compaction turns non-destructively through archive_and_compact() as searchable active=0/compacted=1 rows. So the per-save replace_messages() was a redundant double-write that deleted exactly those archived rows (and their FTS entries). Worse, after a compression-driven id rotation the agent's live head no longer equals the ACP session id, so the replace overwrote the ended parent transcript while new turns flowed to the new id — split-brain corruption of one conversation. Any ACP conversation (VS Code / Zed / JetBrains) long enough to compress lost history. Now _persist skips the destructive replace when the agent owns persistence to this DB (its _session_db is this db and its row exists), relying on the agent's own incremental + archival flush. It still falls back to the atomic replace when the agent is not self-persisting — test agent factories, and fresh create/fork sessions whose copied history the agent has not flushed yet — so the NousResearch#13675 rollback guarantee holds. ## What does this PR do? Fixes silent history loss in ACP editor sessions. ACP _persist no longer destroys the compression-archived transcript the agent already wrote. Long enough conversations compress; that compression archives old turns non-destructively; ACP then hard-deleted them on the next save. After an id rotation it also clobbered the ended parent and split the conversation across two ids. This change defers to the agent's own persistence when it owns the DB and only uses the destructive replace when nothing else is writing the transcript. ## Related Issue N/A ## Type of Change - [x] 🐛 Bug fix (non-breaking change that fixes an issue) - [ ] ✨ New feature (non-breaking change that adds functionality) - [ ] 🔒 Security fix - [ ] 📝 Documentation update - [ ] ✅ Tests (adding or improving test coverage) - [ ] ♻️ Refactor (no behavior change) - [ ] 🎯 New skill (bundled or hub) ## Changes Made - `acp_adapter/session.py`: in `SessionManager._persist`, guard the `db.replace_messages()` call. Skip it when the agent owns persistence to this DB (`agent._session_db is db` and `agent._session_db_created`); otherwise keep the destructive atomic replace as the fallback. - `tests/acp/test_session.py`: add a regression test proving archived (active=0/compacted=1) rows survive a save when the agent self-persists and stay FTS-searchable; add a test confirming the replace path still runs for agents that do not own DB persistence. ## How to Test 1. Run `pytest tests/acp/test_session.py -q` — 43 pass. 2. `test_save_session_preserves_agent_archived_history`: archive a turn via `archive_and_compact`, save, and confirm it survives and is found by `search_messages` (fails before this fix — replace_messages deleted it). 3. `test_save_session_still_replaces_when_agent_not_self_persisting`: confirm history still overwrites cleanly for non-self-persisting agents. ## Checklist ### Code - [x] I've read the Contributing Guide - [x] My commit messages follow Conventional Commits (`fix(scope):`, `feat(scope):`, etc.) - [x] I searched for existing PRs to make sure this isn't a duplicate - [x] My PR contains only changes related to this fix/feature (no unrelated commits) - [x] I've run `pytest tests/ -q` and all tests pass - [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features) - [x] I've tested on my platform: macOS 15 (Darwin 25.5) ### Documentation & Housekeeping - [x] I've updated relevant documentation (README, `docs/`, docstrings) — or N/A - [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A - [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A - [x] I've considered cross-platform impact (Windows, macOS) — or N/A - [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
Follow-up widening the archived-history fix to the sibling save paths the original PR did not cover. Model switches (_cmd_model, set_session_model) and _restore mint a fresh AIAgent with _session_db_created=False, so the agent-owns-persistence guard evaluates False and the blind full-history replace_messages() fired — DELETEing the durable active=0/compacted=1 rows on any compressed ACP session (same data-loss class the PR fixes, different trigger). - hermes_state.replace_messages: add active_only=True to delete/reinsert only the live (active=1) rows, leaving soft-archived rows untouched (idea adopted from the competing PR NousResearch#50306 by @mrparker0980, credited). - hermes_state.has_archived_messages: cheap existence probe for active=0 rows. - acp_adapter._persist: when the agent doesn't own persistence but the session already has archived rows on disk, replace active-only; otherwise the destructive full replace stays (fresh create/fork has nothing to lose). - Regression test: model-switch save on a compacted session keeps the archived turn discoverable via get_messages(include_inactive=True) + search_messages.
kshitijk4poor
force-pushed
the
salvage/50405-acp-archived-history
branch
from
July 1, 2026 11:34
a0a76c1 to
1e02478
Compare
alt-glitch
added
type/bug
This was referenced Jul 1, 2026
12 tasks
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.
Summary
ACP conversations long enough to compress no longer lose their pre-compaction transcript.
_persistused to call the explicitly-DESTRUCTIVEdb.replace_messages()on every save, which DELETEs all rows for the session — including theactive=0 / compacted=1turns thatarchive_and_compact()keeps on disk forsession_searchdurability — and drops their FTS index entries.Salvage of #50405 by @sasquatch9818, cherry-picked with authorship preserved, plus a maintainer follow-up widening the fix to the model-switch / restore save paths (with credit to @mrparker0980, whose competing PR #50306 supplied the
active_onlyprimitive).Changes
acp_adapter/session.py(contributor commit): when the agent owns persistence to this sameSessionDB(agent._session_db is dband_session_db_created), skipreplace_messages()entirely — the agent already flushed the live transcript and archived pre-compaction turns non-destructively. This also sidesteps the compression id-rotation clobber (state.session_idstaying on the ended parent).hermes_state.py(follow-up):replace_messages(active_only=True)deletes/reinserts only live (active=1) rows, leaving soft-archived rows intact;has_archived_messages()is a cheap existence probe.acp_adapter/session.py(follow-up, W1/W2): when the agent does not own persistence but the session already has archived rows on disk (model switch and_restoreboth mint a fresh agent with_session_db_created=False), replace active-only so the archived transcript survives; otherwise the destructive full replace stays (fresh create/fork has nothing to lose).Why this approach (vs #50306)
#50306 fixes the base bug with
active_only=Truebut always runs a delete+reinsert of the active set from the (possibly stale)state.history, and does not handle the compression id-rotation case. The owned-agent skip (this PR) handles the live path + id-rotation; the archived-rows guard handles the model-switch/restore paths #50306's unconditional replace also mishandles. #50306 will be closed as a partial duplicate with credit.Validation
tests/acp/test_session.py— 45 passed (incl. 3 regression tests: owned-agent archived-history, non-self-persist replace, and the new model-switch-on-compacted-session guard).tests/hermes_state/— 340 passed.SessionDB+SessionManager, temp dir): owned-agent save preserves archived row + FTS; model switch on a compressed session preserves archived rows + FTS (W1);has_archived_messagesprobe correct; fresh non-owning session still gets the destructive replace; stale in-memory history doesn't clobber on-disk truth.