From 539c6b9feb149187221e9b9f59348d323cf7c6fa Mon Sep 17 00:00:00 2001 From: JoaoMarcos44 Date: Wed, 23 Sep 2026 07:13:05 -0300 Subject: [PATCH] fix(sessions): atomically verify display history before export deletion (cherry picked from commit f6db4ffd686728d7adc8391e5b1144786a388246) --- hermes_cli/sessions_cmd.py | 38 +++++--- hermes_state_messages.py | 58 ++++++++---- hermes_state_sessions.py | 10 ++- .../test_save_transcript_history.py | 11 +-- .../hermes_cli/test_sessions_export_md_cli.py | 88 ++++++++++++++----- website/docs/user-guide/sessions.md | 2 +- 6 files changed, 146 insertions(+), 61 deletions(-) diff --git a/hermes_cli/sessions_cmd.py b/hermes_cli/sessions_cmd.py index 4488d221cf..92c678ee3f 100644 --- a/hermes_cli/sessions_cmd.py +++ b/hermes_cli/sessions_cmd.py @@ -479,13 +479,17 @@ def _export_markdown(db, args, filters, redact): # The history the user sees, not only the live rows: in-place compaction archives earlier turns under # the same id, and --delete-after-verified removes every row of it. export = db.export_session_lineage if include_lineage else db.export_session - data = export(session_id, include_compacted=True) - if not data: - return None, None - data = redact(data) + raw_data = export(session_id, include_compacted=True) + if not raw_data: + return None, None, None + snapshots = { + segment["id"]: segment.get("messages") or [] + for segment in (raw_data.get("segments") or [raw_data]) if segment.get("id") + } + data = redact(raw_data) path = write_session_markdown(data, output_dir, fmt=args.format, force=args.force) append_manifest_entry(output_dir, data, path, fmt=args.format) - return data, path + return data, path, snapshots if args.delete_after_verified and not args.yes: print("--delete-after-verified requires --yes.") return @@ -505,7 +509,7 @@ def _export_markdown(db, args, filters, redact): exported = 0 for row in candidates: try: - data, exported_path = _export_one(row["id"], include_lineage=lineage_is_logical) + data, exported_path, _ = _export_one(row["id"], include_lineage=lineage_is_logical) except FileExistsError as e: print(f"Skipping existing export: {e}. Pass --force to overwrite.") continue @@ -527,7 +531,7 @@ def _export_markdown_single(db, args, export_one, output_dir, lineage_is_logical exported_items = [] for target_id in delete_target_ids: try: - data, exported_path = export_one( + data, exported_path, snapshots = export_one( target_id, include_lineage=(target_id == resolved_session_id and lineage_is_logical), ) except FileExistsError as e: @@ -536,14 +540,15 @@ def _export_markdown_single(db, args, export_one, output_dir, lineage_is_logical if not data or not exported_path: print(f"Session '{target_id}' disappeared during export; nothing was deleted.") return - exported_items.append((data, exported_path)) - message_count = sum(len(data.get("messages") or []) for data, _path in exported_items) + exported_items.append((data, exported_path, snapshots)) + message_count = sum(len(data.get("messages") or []) for data, _path, _ in exported_items) n = len(exported_items) print(f"Exported {n} session{'' if n == 1 else 's'} ({message_count} message{'' if message_count == 1 else 's'}) " f"to {exported_items[0][1] if n == 1 else output_dir}") if not args.delete_after_verified: return - for data, exported_path in exported_items: + expected_messages = {} + for data, exported_path, snapshots in exported_items: ok, reason = verify_export_file(exported_path, data) # The file only proves it matches the dict it was written from; the delete removes what the store holds # now, so re-count the store just before it (like the adoption retire loop, outside its transaction). @@ -556,10 +561,19 @@ def _export_markdown_single(db, args, export_one, output_dir, lineage_is_logical if not ok: print(f"Export verification failed; not deleting session '{data.get('id')}': {reason}") return + for covered_id, snapshot in snapshots.items(): + previous = expected_messages.get(covered_id) + if previous is not None and previous != snapshot: + print(f"Export verification failed; not deleting session '{data.get('id')}': " + f"session '{covered_id}' changed while the export set was being built") + return + expected_messages[covered_id] = snapshot if not db.delete_session( - resolved_session_id, sessions_dir=_sessions_dir(), expected_delete_ids=delete_target_ids + resolved_session_id, sessions_dir=_sessions_dir(), expected_delete_ids=delete_target_ids, + expected_display_messages=expected_messages, ): - print(f"Exported, but session '{resolved_session_id}' was not deleted because its delegate set changed.") + print(f"Exported, but session '{resolved_session_id}' was not deleted because its history or delegate set " + "changed after export.") return delegates = len(delete_target_ids) - 1 delegate_suffix = f" and {delegates} delegate session{'' if delegates == 1 else 's'}" if delegates else "" diff --git a/hermes_state_messages.py b/hermes_state_messages.py index b3de077cf3..ade9b0d8cd 100644 --- a/hermes_state_messages.py +++ b/hermes_state_messages.py @@ -1002,6 +1002,44 @@ class SessionMessagesMixin: msg.pop(key) return msg + @staticmethod + def _display_rows_from_conn(conn, session_id: str, *, limit: Optional[int] = None, + offset: int = 0, latest: bool = False): + """One display-history projection for normal reads and transactional verification.""" + direction = "DESC" if latest else "ASC" + return conn.execute( + f"""WITH page AS ( + SELECT display_order FROM messages + WHERE session_id = ? AND (active = 1 OR compacted = 1) + GROUP BY display_order ORDER BY display_order {direction} + LIMIT ? OFFSET ? + ) + SELECT chosen.* FROM page + JOIN messages AS chosen ON chosen.id = ( + SELECT candidate.id FROM messages AS candidate + WHERE candidate.session_id = ? + AND candidate.display_order = page.display_order + AND (candidate.active = 1 OR candidate.compacted = 1) + ORDER BY candidate.active DESC, candidate.id DESC LIMIT 1 + ) + ORDER BY page.display_order ASC""", + (session_id, -1 if limit is None else limit, offset, session_id), + ).fetchall() + + def _display_messages_from_conn(self, conn, session_id: str) -> Optional[List[Dict[str, Any]]]: + """Exact display snapshot on an already-held transaction; None means fail closed.""" + if conn.execute("SELECT 1 FROM sessions WHERE id = ? LIMIT 1", (session_id,)).fetchone() is None: + return None + if conn.execute( + "SELECT 1 FROM messages WHERE session_id = ? AND (active = 1 OR compacted = 1) " + "AND display_order IS NULL LIMIT 1", (session_id,), + ).fetchone(): + return None + return [ + self._row_to_message_dict(row, warn_context="verified delete", summary_flag=True) + for row in self._display_rows_from_conn(conn, session_id) + ] + @staticmethod def _active_clause(include_inactive: bool, include_compacted: bool) -> str: """Audit: every row; display: active plus compaction-archived (never Undo/Rewind rows); default: live.""" @@ -1019,23 +1057,9 @@ class SessionMessagesMixin: raise ValueError("after_id is incompatible with include_compacted (deduped display reads use offset paging)") active_clause = self._active_clause(include_inactive, include_compacted) if include_compacted and not include_inactive and self._ensure_display_order(session_id): - direction = "DESC" if latest else "ASC" - sql = f"""WITH page AS ( - SELECT display_order FROM messages - WHERE session_id = ? AND (active = 1 OR compacted = 1) - GROUP BY display_order ORDER BY display_order {direction} - LIMIT ? OFFSET ? - ) - SELECT chosen.* FROM page - JOIN messages AS chosen ON chosen.id = ( - SELECT candidate.id FROM messages AS candidate - WHERE candidate.session_id = ? - AND candidate.display_order = page.display_order - AND (candidate.active = 1 OR candidate.compacted = 1) - ORDER BY candidate.active DESC, candidate.id DESC LIMIT 1 - ) - ORDER BY page.display_order ASC""" - rows = self._read_all(sql, [session_id, -1 if limit is None else limit, offset, session_id]) + with self._read_ctx() as conn: + rows = self._display_rows_from_conn( + conn, session_id, limit=limit, offset=offset, latest=latest) elif include_compacted: # Read-only legacy stores cannot persist display identities; keep only fixed-width # identities and representative ids while scanning, then fetch the selected payloads. diff --git a/hermes_state_sessions.py b/hermes_state_sessions.py index 8373524e58..68a6ce85b7 100644 --- a/hermes_state_sessions.py +++ b/hermes_state_sessions.py @@ -1535,10 +1535,11 @@ class SessionSessionsMixin: def delete_session( self, session_id: str, sessions_dir: Optional[Path] = None, expected_delete_ids: Optional[List[str]] = None, + expected_display_messages: Optional[Dict[str, List[Dict[str, Any]]]] = None, ) -> bool: """Delete a session and its messages; delegate children cascade, branch/compression children - are orphaned. *expected_delete_ids*: proceed only if parent + delegate cascade still equals that - set (re-walked inside the transaction on purpose: export-before-delete fails closed).""" + are orphaned. Optional expected ids fence delegate drift; expected display snapshots fence + transcript drift. Both checks run inside the same write transaction as deletion.""" removed_ids: List[str] = [] expected_ids = set(expected_delete_ids) if expected_delete_ids is not None else None def _do(conn): @@ -1548,6 +1549,11 @@ class SessionSessionsMixin: session_id, *_collect_delegate_child_ids(conn, [session_id]) }: return False + if expected_display_messages is not None and any( + self._display_messages_from_conn(conn, covered_id) != expected + for covered_id, expected in expected_display_messages.items() + ): + return False removed_ids.extend(_delete_delegate_children(conn, [session_id])) conn.execute( # orphan remaining children (branches) so FK is satisfied "UPDATE sessions SET parent_session_id = NULL WHERE parent_session_id = ?", (session_id,), diff --git a/tests/hermes_cli/test_save_transcript_history.py b/tests/hermes_cli/test_save_transcript_history.py index a296b43ba4..1416fbcf10 100644 --- a/tests/hermes_cli/test_save_transcript_history.py +++ b/tests/hermes_cli/test_save_transcript_history.py @@ -1,5 +1,4 @@ -"""/save md|html is a transcript: after in-place compaction it still holds every turn the chat shows. -/save json stays the live rows import_sessions restores.""" +"""Human-readable /save mirrors display history; JSON remains import-safe live context.""" import asyncio from datetime import datetime from types import SimpleNamespace @@ -17,8 +16,10 @@ def _compacted_store(path): db.append_message("s1", "user", f"question {i}") db.append_message("s1", "assistant", f"answer {i}") tail = [{"role": "user", "content": "question 6"}, {"role": "assistant", "content": "answer 6"}] - db.archive_and_compact("s1", [{"role": "user", "content": "[CONTEXT COMPACTION] summary"}, *tail], - watermark=db.get_active_message_watermark("s1"), tail_count=len(tail)) + db.archive_and_compact( + "s1", [{"role": "user", "content": "[CONTEXT COMPACTION] summary"}, *tail], + watermark=db.get_active_message_watermark("s1"), tail_count=len(tail), + ) return db @@ -57,7 +58,7 @@ def _gateway_save(db, fmt, out): @pytest.mark.parametrize("save", [_cli_save, _gateway_save], ids=["cli", "gateway"]) @pytest.mark.parametrize("fmt, expected", [("md", 6), ("html", 6), ("json", 1)]) -def test_save_transcript_holds_every_turn_the_chat_shows(tmp_path, monkeypatch, save, fmt, expected): +def test_save_transcript_holds_display_history(tmp_path, monkeypatch, save, fmt, expected): monkeypatch.setenv("HERMES_HOME", str(tmp_path)) db = _compacted_store(tmp_path / "state.db") try: diff --git a/tests/hermes_cli/test_sessions_export_md_cli.py b/tests/hermes_cli/test_sessions_export_md_cli.py index 36c855fd2f..aa5739b389 100644 --- a/tests/hermes_cli/test_sessions_export_md_cli.py +++ b/tests/hermes_cli/test_sessions_export_md_cli.py @@ -106,18 +106,17 @@ def test_sessions_export_redact_scrubs_secrets(monkeypatch, tmp_path): def _real_store(monkeypatch, tmp_path): - """Point the CLI's SessionDB at one real file; returns an opener for the test's own handles.""" import hermes_state real_session_db = hermes_state.SessionDB db_path = tmp_path / "state.db" - class _StoreAtTmp(real_session_db): + class StoreAtTmp(real_session_db): def __init__(self, *args, **kwargs): super().__init__(db_path=db_path) - monkeypatch.setattr(hermes_state, "SessionDB", _StoreAtTmp) - return _StoreAtTmp + monkeypatch.setattr(hermes_state, "SessionDB", StoreAtTmp) + return StoreAtTmp def _seed_six_turns(open_db, session_id, *, compact): @@ -128,16 +127,18 @@ def _seed_six_turns(open_db, session_id, *, compact): db.append_message(session_id, "user", f"question {i}") db.append_message(session_id, "assistant", f"answer {i}") if compact: - # Default in-place compaction, production shape: watermark from compression start, last turn carried. watermark = db.get_active_message_watermark(session_id) - tail = [{"role": "user", "content": "question 6"}, {"role": "assistant", "content": "answer 6"}] - db.archive_and_compact(session_id, [{"role": "user", "content": "[CONTEXT COMPACTION] summary"}, *tail], - watermark=watermark, tail_count=len(tail)) + tail = [{"role": "user", "content": "question 6"}, + {"role": "assistant", "content": "answer 6"}] + db.archive_and_compact( + session_id, [{"role": "user", "content": "[CONTEXT COMPACTION] summary"}, *tail], + watermark=watermark, tail_count=len(tail), + ) finally: db.close() -def _export_and_delete(monkeypatch, out_dir, session_id, *extra): +def _export_delete(monkeypatch, out_dir, session_id, *extra): import hermes_cli.main as main_mod monkeypatch.setattr(sys, "argv", [ @@ -148,11 +149,11 @@ def _export_and_delete(monkeypatch, out_dir, session_id, *extra): @pytest.mark.parametrize("lineage", ["single", "logical"]) -def test_delete_after_verified_exports_the_turns_in_place_compaction_archived(monkeypatch, tmp_path, capsys, lineage): +def test_delete_after_verified_exports_compacted_display_history(monkeypatch, tmp_path, capsys, lineage): open_db = _real_store(monkeypatch, tmp_path) _seed_six_turns(open_db, "s1", compact=True) - _export_and_delete(monkeypatch, tmp_path / "out", "s1", "--lineage", lineage) + _export_delete(monkeypatch, tmp_path / "out", "s1", "--lineage", lineage) text = next((tmp_path / "out").glob("*.md")).read_text(encoding="utf-8") assert [f"answer {i}" in text for i in range(1, 7)] == [True] * 6 @@ -164,42 +165,81 @@ def test_delete_after_verified_exports_the_turns_in_place_compaction_archived(mo db.close() -def test_delete_after_verified_keeps_a_session_that_gained_a_message_after_the_export(monkeypatch, tmp_path, capsys): +def test_delete_after_verified_rejects_same_count_content_change(monkeypatch, tmp_path, capsys): + """A content rewrite is a real concurrent write that a count-only guard cannot see.""" import hermes_cli.session_export_md as session_export_md open_db = _real_store(monkeypatch, tmp_path) _seed_six_turns(open_db, "s1", compact=False) write_session_markdown = session_export_md.write_session_markdown - def write_then_a_turn_lands(*args, **kwargs): + def write_then_rewrite(*args, **kwargs): path = write_session_markdown(*args, **kwargs) writer = open_db() try: - writer.append_message("s1", "user", "sent after the export was read") + row = next(message for message in writer.get_messages("s1") if message.get("role") == "user") + assert writer.set_user_message_content("s1", row["id"], "changed after export") == 1 finally: writer.close() return path - monkeypatch.setattr(session_export_md, "write_session_markdown", write_then_a_turn_lands) - _export_and_delete(monkeypatch, tmp_path / "out", "s1") + monkeypatch.setattr(session_export_md, "write_session_markdown", write_then_rewrite) + _export_delete(monkeypatch, tmp_path / "out", "s1") - assert "Export verification failed; not deleting session 's1'" in capsys.readouterr().out + output = capsys.readouterr().out + assert "was not deleted because its history or delegate set changed after export" in output db = open_db() try: - assert db.get_messages("s1")[-1]["content"] == "sent after the export was read" + assert db.get_session("s1") is not None + assert db.get_messages("s1")[0]["content"] == "changed after export" + finally: + db.close() + + +def test_delete_after_verified_rechecks_history_at_the_delete_boundary(monkeypatch, tmp_path, capsys): + """A writer landing after any caller-side precheck must still block the destructive transaction.""" + open_db = _real_store(monkeypatch, tmp_path) + _seed_six_turns(open_db, "s1", compact=False) + original_delete = open_db.delete_session + injected = False + + def delete_after_late_append(self, *args, **kwargs): + nonlocal injected + if not injected: + writer = open_db() + try: + writer.append_message("s1", "user", "landed at delete boundary") + finally: + writer.close() + injected = True + return original_delete(self, *args, **kwargs) + + monkeypatch.setattr(open_db, "delete_session", delete_after_late_append) + _export_delete(monkeypatch, tmp_path / "out", "s1") + + text = next((tmp_path / "out").glob("*.md")).read_text(encoding="utf-8") + output = capsys.readouterr().out + assert "landed at delete boundary" not in text + assert "was not deleted because its history or delegate set changed after export" in output + db = open_db() + try: + assert db.get_session("s1") is not None + assert db.get_messages("s1")[-1]["content"] == "landed at delete boundary" finally: db.close() @pytest.mark.parametrize("argv, marker, expected", [ pytest.param(["--format", "html", "--session-id", "s1"], "answer", 6, id="html"), - pytest.param(["--format", "html"], "answer", 6, id="html-every-session"), - pytest.param(["--format", "md", "--only", "user-prompts", "--session-id", "s1"], "question", 6, id="only-prompts"), - # The importable payload keeps the live rows: import_sessions would replay archived turns as live context. - pytest.param(["--format", "jsonl", "--session-id", "s1"], "answer", 1, id="jsonl-live-only"), + pytest.param(["--format", "html"], "answer", 6, id="html-all"), + pytest.param(["--format", "md", "--only", "user-prompts", "--session-id", "s1"], "question", 6, id="only"), + pytest.param( + ["--format", "jsonl", "--only", "user-prompts", "--session-id", "s1"], + "question", 6, id="only-jsonl", + ), + pytest.param(["--format", "jsonl", "--session-id", "s1"], "answer", 1, id="jsonl-live"), ]) -def test_human_readable_exports_carry_the_turns_in_place_compaction_archived( - monkeypatch, tmp_path, argv, marker, expected): +def test_human_readable_exports_use_display_history(monkeypatch, tmp_path, argv, marker, expected): import hermes_cli.main as main_mod open_db = _real_store(monkeypatch, tmp_path) diff --git a/website/docs/user-guide/sessions.md b/website/docs/user-guide/sessions.md index 10531c2004..64ef5b6590 100644 --- a/website/docs/user-guide/sessions.md +++ b/website/docs/user-guide/sessions.md @@ -456,7 +456,7 @@ hermes sessions export --format md --model sonnet --min-messages 50 --redact hermes sessions export --format md --session-id 20250305_091523_a1b2c3d4 --delete-after-verified --yes ``` -Markdown/QMD export writes one `.md` or `.qmd` file per exported session plus a `manifest.jsonl` with the file path, message count, lineage ids, and SHA-256. Bulk export requires at least one filter; a bare bulk export is refused. `--delete-after-verified` is intentionally limited to `--session-id` and requires `--yes`. Because deleting a parent session also removes its delegate/subagent sessions, this mode exports and verifies each delegate in a separate file before deleting anything. If the delegate set changes during export, deletion is refused. Markdown/QMD files hold the full history you see in the session, including turns that in-place compaction summarized away, and deletion is also refused if a session's message count no longer matches its file. The same holds for `--format html`, `--only user-prompts` and `/save md` or `/save html`; JSON and JSONL exports carry only the live rows, because importing them would restore the summarized turns as live context. `--redact` scrubs secrets (API keys, tokens, credentials) from message content and tool output before writing — recommended for any export you plan to share. +Markdown/QMD export writes one `.md` or `.qmd` file per exported session plus a `manifest.jsonl` with the file path, message count, lineage ids, and SHA-256. Bulk export requires at least one filter; a bare bulk export is refused. `--delete-after-verified` is intentionally limited to `--session-id` and requires `--yes`. Because deleting a parent session also removes its delegate/subagent sessions, this mode exports and verifies each delegate in a separate file before deleting anything. Markdown/QMD files hold the full history shown by the session, including turns archived by in-place compaction. Deletion compares that exact display transcript and the delegate set again inside the same database transaction that performs the delete; any intervening append, rewrite, rewind, compaction, or delegate change refuses deletion. The same display-history rule applies to `--format html`, `--only user-prompts` (with either Markdown or JSONL output), and `/save md|html`. Full-session JSON/JSONL exports and `/save json` remain live-only because importing archived turns would restore them as live model context. `--redact` scrubs secrets (API keys, tokens, credentials) from message content and tool output before writing — recommended for any export you plan to share. ### Delete a Session