mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-17 14:42:06 +00:00
fix(acp): stop _persist from deleting compression-archived history
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 #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
This commit is contained in:
parent
b4342a83bb
commit
897240462a
2 changed files with 95 additions and 4 deletions
|
|
@ -461,10 +461,31 @@ class SessionManager:
|
|||
except Exception:
|
||||
logger.debug("Failed to update ACP session metadata", exc_info=True)
|
||||
|
||||
# Replace stored messages with current history atomically so a
|
||||
# mid-rewrite failure rolls back and the previously persisted
|
||||
# conversation is preserved (salvaged from #13675).
|
||||
db.replace_messages(state.session_id, state.history)
|
||||
# When the agent owns persistence to this same SessionDB it has
|
||||
# already flushed the live transcript incrementally during
|
||||
# run_conversation (append_message), and it preserves pre-compaction
|
||||
# turns non-destructively via archive_and_compact() — keeping them on
|
||||
# disk as searchable active=0/compacted=1 rows. Calling
|
||||
# replace_messages() here would then be a redundant double-write that
|
||||
# DELETEs exactly those archived rows (and, after a compression-driven
|
||||
# id rotation where agent.session_id no longer equals
|
||||
# state.session_id, clobbers the ended parent transcript) — silent
|
||||
# data loss for any ACP conversation long enough to compress.
|
||||
#
|
||||
# Only fall back to the destructive atomic replace when the agent is
|
||||
# NOT persisting itself to this DB (e.g. a test agent factory, or a
|
||||
# fresh create/fork whose copied history the agent has not flushed
|
||||
# yet). That path still rolls back on a mid-rewrite failure so the
|
||||
# previously persisted conversation survives (salvaged from #13675).
|
||||
agent = state.agent
|
||||
agent_db = getattr(agent, "_session_db", None)
|
||||
agent_owns_persistence = (
|
||||
agent_db is not None
|
||||
and agent_db is db
|
||||
and bool(getattr(agent, "_session_db_created", False))
|
||||
)
|
||||
if not agent_owns_persistence:
|
||||
db.replace_messages(state.session_id, state.history)
|
||||
except Exception:
|
||||
logger.warning("Failed to persist ACP session %s", state.session_id, exc_info=True)
|
||||
|
||||
|
|
|
|||
|
|
@ -260,6 +260,76 @@ class TestListAndCleanup:
|
|||
assert messages[0]["content"] == "original"
|
||||
assert isinstance(messages[0].get("timestamp"), (int, float))
|
||||
|
||||
def test_save_session_preserves_agent_archived_history(self, tmp_path):
|
||||
"""Regression: ACP _persist must not destroy compression-archived rows.
|
||||
|
||||
When the agent owns persistence to the same SessionDB, it has already
|
||||
flushed the transcript itself and used archive_and_compact() to keep
|
||||
pre-compaction turns as searchable active=0/compacted=1 rows. A blind
|
||||
replace_messages() here used to DELETE those archived rows (and the FTS
|
||||
index entries with them) on every save — silent data loss for any ACP
|
||||
conversation long enough to compress.
|
||||
"""
|
||||
db = SessionDB(tmp_path / "state.db")
|
||||
|
||||
def factory():
|
||||
# Mimic a live ACP agent: it persists to *this* db and has already
|
||||
# created its session row / flushed at least one turn.
|
||||
return SimpleNamespace(
|
||||
model="test-model",
|
||||
_session_db=db,
|
||||
_session_db_created=True,
|
||||
)
|
||||
|
||||
manager = SessionManager(agent_factory=factory, db=db)
|
||||
state = manager.create_session(cwd="/work")
|
||||
|
||||
# Simulate the agent's own persistence: it flushed the live transcript,
|
||||
# then compression archived the pre-compaction turns and inserted a
|
||||
# compacted summary as the new active set.
|
||||
db.append_message(
|
||||
session_id=state.session_id, role="user", content="archived needle"
|
||||
)
|
||||
db.archive_and_compact(
|
||||
state.session_id, [{"role": "user", "content": "compacted summary"}]
|
||||
)
|
||||
|
||||
# ACP's in-memory history only tracks the post-compaction (active) set.
|
||||
state.history = [{"role": "user", "content": "compacted summary"}]
|
||||
manager.save_session(state.session_id)
|
||||
|
||||
# The archived pre-compaction turn must survive and stay discoverable.
|
||||
contents = [
|
||||
m["content"]
|
||||
for m in db.get_messages(state.session_id, include_inactive=True)
|
||||
]
|
||||
assert "archived needle" in contents
|
||||
assert "compacted summary" in contents
|
||||
hits = {r["session_id"] for r in db.search_messages("needle")}
|
||||
assert state.session_id in hits
|
||||
|
||||
def test_save_session_still_replaces_when_agent_not_self_persisting(self, manager):
|
||||
"""Agents that don't own DB persistence keep ACP as the source of truth.
|
||||
|
||||
The default fixture's MagicMock agent has a ``_session_db`` that is *not*
|
||||
the manager's db, so the destructive replace path stays active and ACP
|
||||
history overwrites cleanly (no orphaned rows from a prior save).
|
||||
"""
|
||||
state = manager.create_session()
|
||||
db = manager._get_db()
|
||||
|
||||
state.history = [{"role": "user", "content": "v1"}]
|
||||
manager.save_session(state.session_id)
|
||||
assert [
|
||||
m["content"] for m in db.get_messages_as_conversation(state.session_id)
|
||||
] == ["v1"]
|
||||
|
||||
state.history = [{"role": "user", "content": "v2 replaced"}]
|
||||
manager.save_session(state.session_id)
|
||||
assert [
|
||||
m["content"] for m in db.get_messages_as_conversation(state.session_id)
|
||||
] == ["v2 replaced"]
|
||||
|
||||
def test_cleanup_clears_all(self, manager):
|
||||
s1 = manager.create_session()
|
||||
s2 = manager.create_session()
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue