fix: refuse held state.db publish; replay diverted transcripts - #110179
ngpestelos wants to merge 8 commits into
Conversation
A missing pathname can still have deleted WAL/SHM holders. Import raises OSError instead of atomically publishing a new generation. Turn explainers name the diverted JSONL path and `hermes sessions import --from diverted` replays it into a healthy store.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok executable reviewer (change-pipeline Phase 7)
SHA reviewed: 39df0c969a0172f6f968d4444e12d097fa947e3e
Verdict: ZERO_BLOCKERS
One-pass executable review of the supplied SHA. No IN_SCOPE_BLOCKER. Own-PR: COMMENT only (not APPROVE, not REQUEST_CHANGES). Do not merge from this pass.
Hunt (in-scope)
_import_db_membermissing-target holder refuse — holds. Missingtargetcalls_foreign_db_holder_pids; a non-empty PID list raisesOSErrorbefore_extract_member_atomically. Empty list /None(off-Linux or/procfail) still publishes. Caller records theOSErroras a skipped member, not a silent replace (hermes_cli/backup.py847–855, 944–962).- Diverted path interpolation — holds.
replaced/deleted_waltemplates use{diverted_path};_format_turn_completion_explanationreplaces it;turn_finalizerpasses_last_diverted_transcript_pathfrom divert (agent/turn_explainers.py113–137, 346–350;agent/turn_finalizer.py377–380;agent/session_persistence.py256–262).DeletedWalGenerationErroris aStateDbReplacedError, so divert runs for both causes. sessions import --from divertedreplay — holds. Appends JSONL viaSessionDB.append_message; does not replacestate.db.--inspect-onlyprints path + line count and returns before any DB open/write (hermes_cli/foreign_sessions.py329–348, 385–399). Apply keeps pre-existing messages (already-in-storestill present in the test).- Tests fail if the change is reverted — holds for the three production hooks they bind:
- holder refuse →
test_import_db_member_refuses_missing_target_with_deleted_holders - interpolation →
test_deleted_wal_explanation_names_diverted_path_and_import - diverted inspect-only / apply / idempotency →
test_import_diverted_appends_jsonl_into_session
- holder refuse →
Findings
IN_SCOPE_BLOCKER: none
AUTHORIZED_IMPROVEMENT: none required this pass
REJECTED_WITH_EVIDENCE: none
DEFERRED_FOLLOWUP (not blockers for this SHA):
- Holder-refuse test monkeypatches
_foreign_db_holder_pids; nolinux_onlyintegration that a real deleted WAL holder on a missing pathname refuses import. - Explainer unit test does not go through
turn_finalizerwiring. Wiring is present; reverting only that call would not fail the new test. --inspect-onlyis on the shared import parser and is ignored for--from claude|codex.- Replay stores
role/contentonly (dropstool_calls/ sidecars). Acceptable for unwritten-batch recovery; not a full transcript restore.
Tests (ran from work_dir)
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py
Result: 119 passed, 0 failed, 12 skipped (linux_only on darwin). Exit 0.
Confirmed git rev-parse HEAD == 39df0c969a0172f6f968d4444e12d097fa947e3e before review and before this comment.
alt-glitch
added
type/bug
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok 4.5 executable reviewer (pr-review-fix-loop, review-only pass)
Reviewed SHA: 39df0c969a0172f6f968d4444e12d097fa947e3e
HEAD confirmed equal to bound SHA before review and after tests.
Verdict: ZERO_BLOCKERS
PR is draft and statusCheckRollup was empty at review time — review-clean only; not merge-ready claim.
Findings and dispositions
| # | Finding | Disposition | Evidence |
|---|---|---|---|
| 1 | Missing-target holder refuse (if holders:) |
REJECTED_WITH_EVIDENCE (no defect) | _foreign_db_holder_pids returns None off-Linux /proc fail, [] no holders, [pid…] held. if holders: → None/[] publish, non-empty OSError. _canonical strips (deleted) so deleted fds for missing pathname still match watched db/wal/shm. Test covers refuse [99999] + publish []; Darwin real-None path still publishes via existing missing-target import coverage. |
| 2 | Diverted path interpolation wiring | REJECTED_WITH_EVIDENCE (wired correctly) | session_persistence._db_flush_failed sets agent._last_diverted_transcript_path from divert_session_transcript_jsonl; turn_finalizer._explain_abnormal_exit passes it into _format_turn_completion_explanation(..., diverted_path=...); replaced/deleted_wal templates interpolate {diverted_path}. DeletedWalGenerationError subclasses StateDbReplacedError, so divert runs for deleted_wal. |
| 3 | --from diverted / --inspect-only write contract |
REJECTED_WITH_EVIDENCE (honored) | import_diverted_transcript only create_session/append_message; no state.db replace. inspect_only prints path+line count and returns before opening SessionDB. Covered by tests/hermes_cli/test_sessions_import_diverted.py. |
| 4 | Replay drops tool_calls / empty-content tool rows |
DEFERRED_FOLLOWUP | Divert writes full message dicts; _diverted_jsonl_records keeps only non-empty role/content and append_message is called without tool_calls/tool_name/tool_call_id. Acceptable for this PR’s text-recovery scope; full tool-graph replay is follow-up, not an in-scope blocker. |
| 5 | New wrapper classes / full CAS lock expansion | REJECTED_WITH_EVIDENCE (not present) | Diff adds divert import helpers + holder refuse gate only; no new wrapper classes; no full CAS lock. |
IN_SCOPE_BLOCKER items: none
AUTHORIZED_IMPROVEMENT items: none
Executable verification
scripts/run_tests.sh \
tests/hermes_cli/test_backup.py \
tests/agent/test_turn_completion_explainer.py \
tests/hermes_cli/test_sessions_import_diverted.py \
tests/hermes_state/test_deleted_wal_generation_guard.pyResult: exit 0 — 119 passed, 0 failed, 12 skipped (linux_only skips on darwin host; note in runner output).
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex executable reviewer (/root/pr_reviewer), independent review-only pass.
Reviewed SHA: 39df0c9
Verdict: BLOCKERS_FOUND — two reproduced P2 findings. No source edits, commits, pushes, or merge.
-
P2: repeated recovery duplicates messages after the session continues.
hermes_cli/foreign_sessions.py:363–370compares only the database suffix with the diverted JSONL prefix. Executed with real SessionDB and temporary HERMES_HOME: import user A / assistant B, append user C / assistant D, then reimport unchanged JSONL. Both imports report success; stored contents become[A,B,C,D,A,B]. The producer retains and appends to this JSONL, so a subsequent recovery can replay earlier imported rows. Track imported records independently of the current conversation suffix. Disposition: IN_SCOPE_BLOCKER. -
P2: native recovery silently loses tool history.
hermes_cli/foreign_sessions.py:309–315,370drops null-content assistant tool-call rows and strips tool metadata from results. Executed realdivert_session_transcript_jsonl→import_diverted_transcript→SessionDB.get_messages_as_conversation: assistant content null with tool_calls id call_1 disappears; tool result 42 survives without tool_call_id. Executingagent.agent_runtime_helpers.sanitize_api_messageson that resumed message shape drops the orphan result. Recovery therefore loses both tool execution and its output in resumed context. Preserve native replay fields and null-content tool-call rows. Disposition: IN_SCOPE_BLOCKER; the previously deferred tool-fidelity concern is concrete data loss in this recovery path, not an API-400 claim.
Executable verification:
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py— 118 passed, 1 failed, 12 skipped, exit 1. Failure: existingTestSafeCopyDb::test_locked_source_fails_fast_not_hangcleanup attests/hermes_cli/test_backup.py:1321;proc.kill()was blocked by the live-system guard attests/conftest.py:1402. Safe-copy assertions passed. This is a verification limitation, not a failure attributed to this diff. Linux-only coverage skipped on Darwin..venv/bin/pythonheredoc with temporary HERMES_HOME and real producer/importer/database: replay-after-resume output[('user','A'),('assistant','B'),('user','C'),('assistant','D'),('user','A'),('assistant','B')]..venv/bin/pythonreal tool-message round trip: resumed roles[user,tool,assistant]with no tool-call metadata; subsequentsanitize_api_messagesoutput[{'role':'user','content':'calculate'},{'role':'assistant','content':'answer 42'}].- Inspected scoped diff and runtime integration through diverted row production, persistence, recovery import, conversation loading, unconditional pre-call sanitization, turn explanation, and backup holder guard.
Parent reconfirmed OPEN and unchanged reviewed SHA before this authorized publication. No merge-ready claim.
Replay tracks consumed JSONL objects in a sidecar so a later import cannot duplicate after the live session continues. Null-content assistant tool_calls and tool_call_id results are appended instead of dropped.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok 4.5 executable reviewer (pr-review-fix-loop, implement-pass re-review)
Reviewed SHA: 06c0fe5bcb6b977d0c8ee3d8b4723837bf316e0b
Confirmed git rev-parse HEAD matches before review and before this post.
Verdict: ZERO_BLOCKERS
Prior Codex findings from review 5192785736 (SHA 39df0c969a…) — dispositions:
-
IN_SCOPE_BLOCKER — reimport after session continues duplicates (suffix/prefix) → RESOLVED
- Coverage:
test_reimport_after_session_continues_does_not_duplicate - Implementation: sidecar watermark (
<jsonl>.imported) inimport_diverted_transcript; reimport of unchanged JSONL after append C,D leaves store[A,B,C,D] - Independent probe (temp
HERMES_HOME, real SessionDB +run_sessions_import):FINDING1_PAIRS=[(user,A),(assistant,B),(user,C),(assistant,D)]
- Coverage:
-
IN_SCOPE_BLOCKER — null-content assistant tool_calls dropped; tool results lose tool_call_id → RESOLVED
- Coverage:
test_import_preserves_null_content_tool_call_rows - Implementation:
_diverted_jsonl_recordkeeps null-content rows with tool graph;_append_diverted_recordpassestool_calls/tool_call_id/tool_name - Independent probe via
divert_session_transcript_jsonl→ import →get_messages_as_conversation: roles[user,assistant,tool]; assistant retainstool_calls[0].id=call_1; tool keepstool_call_id=call_1;sanitize_api_messagesretains assistant+tool (no orphan drop)
- Coverage:
No new wrapper species invented this pass. No out-of-scope nits escalated.
test_cmd
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py
121 passed, 0 failed, 12 skipped, exit 0
(linux_only skips on Darwin; not attributed to this diff)
Notes
Review-clean for the two prior blockers. PR is draft and CI rollup empty at post time — not claiming merge-ready. No source edits/commits/pushes by this reviewer.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex executable reviewer (/root/pr_reviewer), independent review-only re-review.
Reviewed SHA: 06c0fe5
Verdict: BLOCKERS_FOUND — two P2 blockers in the new watermark behavior.
Prior findings:
- RESOLVED: unchanged diverted file replay after normal continuation no longer duplicates A/B. Real database result remains
[A,B,C,D]. - RESOLVED: native assistant null-content tool calls and tool-result IDs survive producer → importer → conversation loading → actual
sanitize_api_messages. The resumed roles remain[user,assistant,tool,assistant], with call_1 paired correctly.
Remaining findings:
-
P2 — bind recovery progress to the destination, not only the source file (
hermes_cli/foreign_sessions.py:431–439). The bare.importedcount is trusted even when the destination session/database has no recovered messages. Parent executable evidence used real SessionDB instances: import A/B into first.db/session s gives[A,B]; importing that same file into another session gives[]; importing into a fresh recovered.db/session s also gives[]. All calls report success. Restoring or rebuilding the database is central to this command's recovery purpose; the retained source sidecar must not prevent restoring the missing transcript. Bind progress to the target and verify it against durable destination state. Disposition: IN_SCOPE_BLOCKER. This finding uses the parent's real executable probe evidence, independently supported by the inspected destination-blind branch. -
P2 — retries duplicate committed rows after a partial apply (
hermes_cli/foreign_sessions.py:440–442). Each append commits independently, but the watermark is updated only after all appends. Executed real database probe: import A/B (watermark 2), append C/D to source, inject an OSError on the second append. State becomes[A,B,C]with watermark still 2. Retry succeeds with[A,B,C,C,D]. The existing-watermark branch bypasses destination overlap checks. Make progress and transcript writes recover consistently across partial failures; merely updating a separate file after the loop does not provide retry idempotency. Disposition: IN_SCOPE_BLOCKER.
Verification:
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py→ 120 passed, 1 failed, 12 skipped, exit 1. Log:/tmp/pr110179-rereview-tests.log. ExistingTestSafeCopyDb::test_locked_source_fails_fast_not_hangfails during cleanupproc.kill()at test_backup.py:1321 because conftest.py:1402 blocks the signal; safe-copy assertions passed. This host limitation is not attributed to the changed code. Linux-specific tests skipped on Darwin.PYTHONPATH=. .venv/bin/python /tmp/pr110179-rereview-probes.py→ exit 0, with assertions for both original fixes and reproduced partial-retry duplication. Output:/tmp/pr110179-rereview-probes.log. Temporary HERMES_HOME and real producer/importer/SessionDB were used; only the failure boundary was injected.- Reviewed delta from 39df0c9 and native replay integration. No source edits, commits, pushes, or merge.
- Fresh remote check immediately before publication: OPEN, exact reviewed SHA, statusCheckRollup empty. No merge-ready claim.
Drop the source-file .imported sidecar. Skip only a prefix already present in the target session so a rebuilt database or second session can restore, and a retry after a partial apply writes only the tail.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok 4.5 executable reviewer (pr-review-fix-loop, implement-pass re-review)
Reviewed SHA: 599eae00f5b56ebfad4402a43aa5fa7911c7f4b6
Verdict: ZERO_BLOCKERS
Finding dispositions (prior Codex review on 06c0fe5)
-
IN_SCOPE_BLOCKER — source
.importedwatermark skips restore into another session / rebuilt database → RESOLVED- Sidecar helpers (
_imported_watermark_path,_read_imported_object_count,_write_imported_object_count) and all.importedwrites/reads are gone. - Skip is now destination-bound only:
_longest_prefix_subsequence(existing_ids, incoming_ids)thenincoming[skip:]. - Empty dest / other session / new db → skip=0 → full
[A,B]restore. - Covered by
test_import_into_empty_destination_ignores_source_sidecar(second session + rebuiltSessionDB(db_path=…)).
- Sidecar helpers (
-
IN_SCOPE_BLOCKER — watermark + per-append commit duplicates on retry after partial apply → RESOLVED
- Without the object-count watermark, retry uses the contiguous destination prefix: dest
[A,B,C]+ JSONL[A,B,C,D]→ skip=3 → append onlyD. - Covered by
test_retry_after_partial_apply_does_not_duplicate.
- Without the object-count watermark, retry uses the contiguous destination prefix: dest
No new in-scope blockers invented this pass. Draft PR; CI check rollup empty at review time — review-clean only, not merge-ready.
test_cmd
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py
Result: 4 files, 123 passed, 0 failed, 12 skipped (linux_only on darwin). Includes both coverage tests plus prior continue + tool-call diverted import tests (tests/hermes_cli/test_sessions_import_diverted.py: 5✓).
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex executable reviewer (/root/pr_reviewer), review-only re-review.
Reviewed SHA: 599eae0
Verdict: BLOCKERS_FOUND — one remaining P2 in the original replay-idempotency scope.
P2 — repeated recoveries separated by normal conversation still duplicate the recovered tail (hermes_cli/foreign_sessions.py:341–351,414–416). The destination check requires the source prefix to occur contiguously. Real producer/importer evidence from the parent reviewer: divert/import A/B, append ordinary C/D to SessionDB, append diverted E/F through divert_session_transcript_jsonl to the same retained source, then import. Destination is correctly [A,B,C,D,E,F]. Immediately importing the unchanged file again produces [A,B,C,D,E,F,E,F]. Source is [A,B,E,F], which cannot occur contiguously across the normal C/D messages, so the matched prefix remains A/B and every retry appends E/F again. This is a residual of the original recovery idempotency finding, not unrelated scope. Recovery matching must recognize previously recovered runs separated by ordinary conversation. Disposition: IN_SCOPE_BLOCKER. Parent executed this against real SessionDB/producer/importer with temporary HERMES_HOME; I traced the reported result to the unchanged contiguous-prefix comparison.
Prior dispositions, independently executed on this SHA:
- RESOLVED: a single recovered A/B followed by normal C/D is not duplicated by reimporting A/B.
- RESOLVED: native tool calls/results survive producer → importer → conversation loading →
sanitize_api_messages, including a null-content assistant tool-call row and call_1 pairing. - RESOLVED: the exact destination watermark bug is removed; another session and a fresh destination database both recover all A/B/C/D rows.
- RESOLVED: the exact partial-failure probe without intervening normal messages now retries
[A,B,C]to[A,B,C,D]without duplicating C.
Executable verification:
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py→ 122 passed, 1 failed, 12 skipped, exit 1. Log/tmp/pr110179-pass3-tests.log. The existingTestSafeCopyDb::test_locked_source_fails_fast_not_hangcleanup again fails attest_backup.py:1321:proc.kill()blocked bytests/conftest.py:1402live-system guard. Safe-copy assertions passed; this host verification limitation is not attributed to the diff. Linux-only tests skipped on Darwin.PYTHONPATH=. .venv/bin/python /tmp/pr110179-pass3-probes.py→ exit 0. Assertions verify all four exact prior cases above using real SessionDB and diverted producer/importer, temporary HERMES_HOME, actual API sanitizer, and an injected second-append failure for the retry case. Output/tmp/pr110179-pass3-probes.log.- Reviewed delta from 06c0fe5. No source edits, commits, pushes, or merge.
- Fresh remote check before publication: OPEN, exact reviewed SHA, statusCheckRollup empty. No merge-ready claim.
Match recovered JSONL rows in destination order, not as a contiguous block, so a later divert after ordinary conversation is not replayed again on retry.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok 4.5 executable reviewer (pr-review-fix-loop, implement-pass re-review)
Reviewed SHA: be83369b629ae38a4d2ca2a9506288377dcbef44
HEAD check: matches bound SHA before review and before post
Verdict: ZERO_BLOCKERS
Finding dispositions
| Prior finding | Disposition | Notes |
|---|---|---|
IN_SCOPE_BLOCKER (Codex review 5192840653 on 599eae0): recovered A/B, ordinary C/D, diverted E/F imported, then reimport of source [A,B,E,F] duplicated E/F because skip required a contiguous prefix |
RESOLVED | _longest_prefix_subsequence now longest-prefix-matches via _is_ordered_subsequence (order-preserving, gaps allowed). For dest [A,B,C,D,E,F] + source [A,B,E,F], skip=4 so no re-append. Covered by test_reimport_after_intervening_turns_does_not_duplicate_later_divert. |
Prior resolved cases still hold in tests/hermes_cli/test_sessions_import_diverted.py:
- continue after live growth (
test_reimport_after_session_continues_does_not_duplicate) - null-content tool graph (
test_import_preserves_null_content_tool_call_rows) - empty destination / other session / rebuilt db (
test_import_into_empty_destination_ignores_source_sidecar) - partial apply then source growth (
test_retry_after_partial_apply_does_not_duplicate)
No new wrapper species invented. No out-of-scope nits elevated.
test_cmd results
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py
PASS: 4 files, 124 tests passed, 0 failed, 12 skipped (linux_only on darwin).
Publication notes
- PR is draft; CI
statusCheckRollupempty at review time. - Do not treat as merge-ready from this comment.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex executable reviewer (/root/pr_reviewer), review-only re-review.
Reviewed SHA: be83369
Verdict: BLOCKERS_FOUND — one P2 data-loss regression in the broadened matching rule.
P2 — ordered content matching can discard a new, never-persisted turn (hermes_cli/foreign_sessions.py:341–352, called by _longest_prefix_subsequence and the import skip calculation). The new matcher treats equal role/content values anywhere in historical order as evidence of prior recovery. Parent's executed real SessionDB/producer/importer probe: destination contains user status at timestamp 100, assistant working at 101, user finish at 102, assistant done at 103. The diverted file contains a NEW user status at 200 and assistant done at 201. Import reports success but leaves only the original four rows; both new messages are skipped. The producer supplied timestamps, but _diverted_jsonl_record / _diverted_record_identity omit them. The previous contiguous matcher would append this noncontiguous pair. This is a concrete new false-positive introduced by the broadened match, within the original recovery idempotency/data-integrity scope. Use durable message identity, not historical content similarity, to establish that a diverted record was already persisted. Disposition: IN_SCOPE_BLOCKER. Parent supplied executable evidence using temporary HERMES_HOME and real producer/importer/database; I inspected the matching and identity code that causes it.
Prior cases independently verified RESOLVED on this SHA:
- Single recovered A/B then ordinary C/D: reimport does not duplicate A/B.
- Tool-call graph survives actual producer/importer/conversation loading/API sanitizer, including null-content assistant and paired call_1 result.
- Another target session and fresh target database both recover all rows.
- Injected second-append failure retries
[A,B,C]to[A,B,C,D]without duplication. - Last named residual: recover A/B, ordinary C/D, recover E/F, immediate retry stays
[A,B,C,D,E,F].
Executable verification:
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py→ 123 passed, 1 failed, 12 skipped, exit 1. Log/tmp/pr110179-pass4-tests.log. ExistingTestSafeCopyDb::test_locked_source_fails_fast_not_hangfails during cleanupproc.kill()at test_backup.py:1321, blocked by conftest.py:1402 live-system guard. Safe-copy assertions passed; this host limitation is not attributed to the diff. Linux-only tests skipped on Darwin.PYTHONPATH=. .venv/bin/python /tmp/pr110179-pass4-probes.py→ exit 0, with assertions for every prior case above. Output/tmp/pr110179-pass4-probes.log. Uses temporary HERMES_HOME, real producer/importer/SessionDB/API sanitizer, with only the partial-write failure injected.- Reviewed source and regression-test delta from 599eae0. Clean worktree; no source edits, commits, pushes, or merge.
- Fresh remote check before publication: OPEN, exact reviewed SHA, statusCheckRollup empty. No merge-ready claim.
Do not treat historical role/content as prior recovery. When the source row has a timestamp it must match the destination row, so a new turn that reuses wording still persists.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Grok 4.5 executable reviewer (pr-review-fix-loop, implement-pass re-review)
Reviewed SHA: b11d724c7a68637c240e53969645f8ab5692eb1c
HEAD check: git rev-parse HEAD matched bound SHA before review and before this post.
Verdict: ZERO_BLOCKERS
PR is draft; statusCheckRollup empty at review time — review-clean only; not merge-ready.
Finding dispositions
| Prior finding | Disposition | Evidence |
|---|---|---|
IN_SCOPE_BLOCKER (Codex review 5192896309 on be83369b): ordered content matching discarded a new never-persisted turn — dest status@100/working@101/finish@102/done@103, diverted NEW status@200/done@201 both skipped because identity omitted producer timestamps |
RESOLVED | _diverted_jsonl_record keeps timestamp when present (hermes_cli/foreign_sessions.py 326–327). _same_diverted_row requires dest timestamp equality when incoming has one (354–361); content-only match remains only if incoming timestamp is absent (359–360). _append_diverted_record persists timestamp= (391). Skip path uses _longest_already_persisted over full message dicts (364–380, 442–444). Coverage: test_import_keeps_new_turn_that_reuses_historical_content asserts exact scenario → final rows include both historical and new (status,200) / (done,201). |
Prior resolved cases still hold
Untimestamped content-match path unchanged (incoming_ts is None → True). Suite includes:
test_import_into_empty_destination_ignores_source_sidecartest_retry_after_partial_apply_does_not_duplicatetest_reimport_after_intervening_turns_does_not_duplicate_later_diverttest_import_keeps_new_turn_that_reuses_historical_content
All passed in this pass’s test_cmd.
test_cmd
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.py
Result: 4 files, 125 passed, 0 failed, 12 skipped (linux_only skipped on darwin). Exit 0.
Own-PR: COMMENT only (not APPROVE, not REQUEST_CHANGES).
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex executable reviewer (/root/pr_reviewer), executable re-review.
Reviewed SHA: b11d724
Verdict: ZERO_BLOCKERS — review-clean for the scoped recovery findings. No merge-ready claim; PR remains a draft.
RESOLVED, independently verified with real SessionDB and diverted producer/importer:
- Latest false-skip: historical status@100 / working@101 / finish@102 / done@103 no longer suppresses new diverted status@200 / done@201. All six rows and their timestamps persist; immediate reimport still has six rows.
- Single recovery followed by ordinary conversation does not duplicate on retry.
- Multiple recovery batches separated by ordinary rows remain A/B/C/D/E/F after immediate reimport.
- A partial write failure retries A/B/C to A/B/C/D without duplicate C.
- Another destination session and a fresh database restore all source rows.
- Native null-content assistant tool calls and paired tool results survive import, conversation loading, and actual sanitize_api_messages.
DEFERRED_FOLLOWUP / limitation: source rows without a usable timestamp still use content/tool matching as a compatibility heuristic. This pass does not claim universal durable record IDs or collision-free identity for untimestamped legacy data. No additional in-scope blocker was found in the named timestamp-bearing regression and prior concrete cases.
Executable verification:
scripts/run_tests.sh tests/hermes_cli/test_backup.py tests/agent/test_turn_completion_explainer.py tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_state/test_deleted_wal_generation_guard.pyrun outside the sandbox with the test guard unchanged: 125 passed, 0 failed, 12 skipped, exit 0. Log/tmp/pr110179-pass5-tests-unsandboxed.log. Linux-only cases skipped on Darwin.- Initial sandbox run: 124 passed, 1 cleanup failure, 12 skipped. The existing subprocess cleanup guard could not validate proc.kill() there. The unchanged canonical rerun outside the sandbox passed; no guard bypass or source modification was used.
PYTHONPATH=. .venv/bin/python /tmp/pr110179-pass5-identity.py→ exit 0; latest false-skip and repeat assertions pass, with original/new timestamps preserved. Log/tmp/pr110179-pass5-identity.log.PYTHONPATH=. .venv/bin/python /tmp/pr110179-pass4-probes.py→ exit 0 on this SHA; all prior concrete cases pass. Current output/tmp/pr110179-pass5-probes.log. Probes use temporary HERMES_HOME and real producer/importer/database/API sanitizer; only the partial-write failure boundary is injected.- Reviewed source and regression-test delta from be83369. No source edits, commits, pushes, or merge by this reviewer.
- Fresh remote check before publication: OPEN, exact reviewed SHA, statusCheckRollup empty; bound worktree clean. Review-clean only.
| skip = 0 | ||
| for rec in incoming: | ||
| found = None | ||
| for i, dest in enumerate(existing): |
There was a problem hiding this comment.
[Bug] (blocking) This search starts at destination index zero for every source row. The used bitmap prevents reuse, but it does not preserve destination order. With destination [assistant done@201, user status@200] and source [user status@200, assistant done@201], this function returns 2 and the import appends nothing, although that source sequence was never persisted in order. I reproduced this with a real SessionDB at this head. The prefix match must move forward through the destination after each match.
| if _diverted_content_identity(dest) != _diverted_content_identity(incoming): | ||
| return False | ||
| incoming_ts = _diverted_timestamp(incoming) | ||
| if incoming_ts is None: |
There was a problem hiding this comment.
[Bug] (blocking) A source row without a usable timestamp is treated as already persisted when any historical row has the same role, content, and tool fields. This is reachable in current code. The gateway writes the MCP reload transcript notice without a timestamp in gateway/run_turn.py, and the replacement fallback diverts that row unchanged. If an earlier identical reload notice exists, import reports success but silently drops the new event. Existing legacy diverted files can have the same shape. An identity-less row must not be skipped solely because its content occurred before.
| of a silent success. | ||
| """ | ||
| if not target.exists(): | ||
| holders = _foreign_db_holder_pids(target) |
There was a problem hiding this comment.
[Bug] (blocking) The missing-target guard remains Linux-only, so macOS can publish a second database generation while a live process still owns the deleted first generation. _foreign_db_holder_pids() returns None off Linux, and this branch treats that as proof of no holders. On this exact head on macOS, I opened state.db in a child, unlinked its DB, WAL, and SHM paths, then imported a backup. This branch published the replacement. The child still read old-generation while a fresh connection read replacement-generation. Current main already has macOS libproc discovery for deleted SQLite generations. Please use the cross-platform holder authority here and fail closed when the scan cannot prove safety.
Preserve diverted recovery guidance and upstream model-aware turn explanations.
Commit recovery identity with each destination message and match legacy rows only by finite timestamps in forward order. Reuse the shared macOS and Linux deleted-generation scanner for missing database imports, including main-file holders and explicit unknown-scan refusal. Keep upstream profile-aware recovery guidance and model-aware explainers.
ngpestelos
left a comment
ngpestelos
left a comment
There was a problem hiding this comment.
Codex independent executable reviewer (/root/pipeline_fix/review), post-fix review.
Reviewed SHA: db6ccf1d2b48b715ea1b43361166314881de5972
Verdict: ZERO_BLOCKERS — review-clean. No new in-scope blocking findings.
Reviewed the scoped PR diff against upstream 4405813281d64e1dbff897e3d88ee554e48a9672, current fix, all 14 prior reviews and three inline findings. Earlier zero-blocker conclusions do not supersede the subsequently demonstrated defects.
Finding dispositions:
- RESOLVED — 4044741232, backwards matching.
hermes_cli/foreign_sessions.py:370consumes a forward-only destination iterator. The real SessionDB reversed-order regression appends the missing final row and a retry adds nothing. - RESOLVED — 4044742890, identity-less collision.
hermes_cli/foreign_sessions.py:355no longer accepts untimestamped content alone. Source-path/prefix recovery identities are persisted atomically with each message (:381,:440), distinguishing repeated identical records while keeping retries stable as the file grows. Executed missing/invalid/nonfinite timestamp collisions and post-commit failure retry tests. The previous decision to defer untimestamped collisions was incorrect; this pass treats the live producer as in scope. - RESOLVED — 4048806698, macOS missing-target split generation.
hermes_cli/backup.py:857requests the shared cross-platform scanner withinclude_main=True, strict=True. The libproc scanner filters non-vnode descriptors before vnode inspection and propagates unknown same-user scan failures. Executed real macOS child holders in DELETE and WAL modes: child discovered, publication refused, original connection still reads old-generation. Process-list, descriptor-list and vnode failures also refuse publication. - Prior blockers remain resolved: repeated recovery after normal continuation; multiple recovered batches separated by ordinary conversation; another session/fresh database; partial committed apply followed by source growth; same historical content with a new timestamp; native null-content assistant tool calls and paired tool results. Current regression suite exercises these cases against SessionDB. Inspected existing atomic message metadata encoding/decoding and insertion-order reads.
Independent executable verification on the reviewed SHA:
scripts/run_tests.sh tests/hermes_cli/test_sessions_import_diverted.py tests/hermes_cli/test_backup.py tests/agent/test_flush_diverts_on_corrupt_state_db.py tests/agent/test_turn_completion_explainer.py tests/hermes_state/test_deleted_wal_generation_guard.py -q
143 passed, 1 failed, 13 skipped
scripts/run_tests.sh tests/hermes_cli/test_backup.py -q
86 passed, 0 failed, 2 skipped
The first run's only failure was existing TestSafeCopyDb::test_locked_source_fails_fast_not_hang cleanup: the sandbox prevented the unchanged live-system guard from validating its child before proc.kill(). The affected file passed outside the sandbox with the guard intact. Combined successful evidence covers 144 passed, 13 Linux-only skipped. No source edits, commits, pushes or merge by this reviewer.
Limits and deferred follow-ups:
- macOS child tests restrict PID inventory to the actual child; descriptor/path/inode discovery is real. They do not establish that an unrestricted full-host scan is clean. A protected same-user process can deny inspection; strict restore intentionally refuses in that case. Generic clean-publication tests inject known-empty authority. This is the requested fail-closed contract, with a usability cost for fresh restores on such hosts.
- Linux-only execution remains for Linux CI. Earlier nonblocking parser
--inspect-onlybehavior for foreign sources and additional finalizer integration coverage remain deferred; no scope expansion here. - Live PR is OPEN and non-draft with a mergeable head and empty check rollup. This verdict concerns review findings; no merge or branch-protection bypass is authorized by it.
| for record in incoming: | ||
| digest.update(b"\0") | ||
| digest.update(json.dumps(record, sort_keys=True, ensure_ascii=False).encode("utf-8")) | ||
| record["display_metadata"] = {"diverted_recovery_id": digest.hexdigest()} |
There was a problem hiding this comment.
[Bug] (blocking) Diverted replay discards durable message metadata. _db_flush_row() writes display_kind, display_metadata, platform_message_id, and api_content into the diverted JSONL, but _diverted_jsonl_record() removes them and this assignment replaces the metadata object with only diverted_recovery_id. On this exact head, I diverted and replayed a user row with display_kind=hidden, diagnostic and gateway-owner metadata, platform_message_id=platform-message-1, and an api_content sidecar. The stored row had no display kind, no platform ID, no API sidecar, and only the recovery ID. The CLI resume projection rendered it as an ordinary user entry, and has_platform_message_id(..., "platform-message-1") returned false. Recovery can therefore expose internal rows and let an already accepted gateway message be dispatched again. The replay path must preserve supported durable fields and add its recovery identity without erasing existing metadata.
Carry producer sidecars through recovery and merge recovery metadata. Keep legacy recovery identities stable to avoid duplicate imports.
|
Codex executable reviewer (durable_reviewer), scoped post-fix review. Reviewed SHA: Finding dispositions:
Verification:
Scope limits: already-recovered lossy rows are not backfilled; this fix preserves fields on newly replayed rows and retains legacy retry identity. Existing out-of-scope foreign-import inspect-only behavior and extra finalizer integration coverage remain deferred. This is a review-clean verdict, not a merge or CI-readiness assertion. No source edits, commits, pushes or merge by this reviewer. |
|
Codex pipeline caller (pipeline_fix), residual review closure. Reviewed and pushed HEAD:
Fresh complete paginated review, issue-comment and inline-comment snapshots contain no new external findings. Live PR head, local HEAD, verified SHA and reviewed SHA match. GitHub reports |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
Reviewed exact head 4696d9d48131d842199db16ae87bb101eaabeaa1. No new blocking findings.
The prior durable-metadata blocker is resolved. Diverted replay now preserves every durable sidecar emitted by _db_flush_row() and accepted by append_message(). Existing dictionary or JSON-object display metadata is retained and merged with diverted_recovery_id. The compatibility hash still excludes these newly preserved sidecars, so rows recovered before this change remain recognizable.
The earlier ordered identity, untimestamped collision, partial retry, and cross-platform missing-database restore fixes remain intact. The macOS restore tests exercise real child-held deleted DELETE and WAL generations plus fail-closed libproc errors.
Verification:
scripts/run_tests.sh tests/hermes_cli/test_sessions_import_diverted.py -q: 16 passed.- Current-main composition against
a51143fbbe6ddbc0c7f403d0579c4d75504c6793: clean merge tree775e663cb2b75225f442d1ddd7ff79247ea7b2e5. - On that composition, the five focused files passed: 149 passed, 0 failed, 12 Linux-only skipped on macOS.
git diff --checkpassed.- GitHub reports no hosted checks for this head. This review does not claim Linux CI verification or merge readiness.
`_import_db_member` guards the replace path with `_safe_restore_db`, but its "target is missing" branch treated absence as proof of no holders. A gateway or dashboard that had the database open when it was unlinked keeps writing the deleted inode, so an atomic publish there re-creates exactly the #90950 split brain the replace path exists to prevent. Consult `_foreign_db_holder_pids` before the publish and raise an OSError naming the PIDs, which the caller already reports as a skipped file. Credit: @ngpestelos (#110179)
Merge fallout (my resolution errors, all caught by CI): - hermes_cli/backup.py + gateway.py: `theirs` on those hunks re-imported clusters HEAD had already moved to backup_restore.py / kept in the facade. backup.py loses the 349-line duplicate (main's NousResearch#110179 fix is ported into backup_restore._import_db_member); the systemd service-unit cluster returns to gateway.py (PM's _prepare_service_launcher / _pm_managed_node_dirs / _systemd_command have no home in main's extraction) with main's utf-8-sig read. gateway_service_unit.py is dropped. - gateway/run.py: main's plugin-update chore is not profile-scoped (the housekeeping ordering test pins the scope/drain sequence). - pyproject + 30 test files: `import yaml` -> `import hermes_yaml as yaml` (pm-clean has no pyyaml); gateway/config._bundled_platform_manifest_name reads through hermes_yaml. - tests re-seamed onto pm-clean's shape: residency admission (installed_engine), supervisor child env (binary is a constructor argument), update import guard (update_cmd_deps is gone; our probe already scrubs PYTHONPATH — both NousResearch#115032 invariants pass), shallow-count git responses (stash path asks `status --porcelain -z`); dropped tests for retired code (_run_node_bootstrap/_ensure_tui_node, Windows resume demotion). - tests/tools/test_local_env_blocklist.py: restore the two helpers the suite-reduction commit dropped and the blocklist import. Real fixes: - pm: classify_uv_failure/ResolutionConflict move beside the uv runner (pm.environment, stdlib-only). pm.workspace imports tomllib at module level and cannot load on the 3.10 bootstrap python that streams uv output in the Docker arm64 image. - tools/browser_tool.warm_agent_browser_npx_cache: back as a permanent definition — it is on the frozen old-updater surface, and the revert-scheduled compat pointer does not count. - hermes_cli/memory_setup: the dashboard's pip row uses pm.environments. running_from_selected_environment for installed vs restart_required. - scripts/windows-build-deps.ps1: export DISTUTILS_USE_SDK/MSSdk so setuptools trusts the primed MSVC environment instead of asking vswhere (`env -i` test runner on win32-arm64 compiling ruamel-yaml-clib); run_tests.sh forwards them. - tests/pm/test_windows_build_deps.py: start the protocol test from a parent env without the toolchain variables the runner job already exports. - tests/conftest.py scrubs HERMES_BUNDLED_PLUGINS (Nix-wrapped hermes on the dev host); tests/home_io_guard.py treats sys.path site-packages under the real home as the interpreter's installation (PM-activated developer shell). - tests-js: four `curly` lint errors from main's new scripts.
|
Thanks @ngpestelos. The dashboard half of #110054 has since landed: #121428 (4cc4072) returns 503 for deleted-WAL and replaced |
…ipped `_import_members` catches the PermissionError/OSError of each member it cannot publish and records it in `errors`. That includes the state.db the live-safe restore refuses because a gateway or dashboard still holds it (#100960, #110179). `run_import` printed those under "Warnings (N files skipped)", then printed "Done. Your Hermes configuration has been restored." and returned None. `cmd_import` did not forward a return value, so the shell status was 0. A script chained on `hermes import --force` carried on over a partial restore, and the dashboard's import action showed the green "done" badge. For the refused state.db case, the user's sessions were never restored. `run_import` now returns 1 when `errors` is non-empty. It words both the summary header and the final line as "Import incomplete", and `cmd_import` forwards the return code, the same contract `hermes backup` has for an incomplete archive. Runtime files the import deliberately keeps (gateway.pid, SQLite sidecars) and the older-backup session warning stay warnings. The gateway revive still runs, so the files that did land come up as before. Measured on main with a fresh HERMES_HOME through `main()`: a 3-file backup with one read-only target directory, and a refused state.db (live DB left at 3 sessions / 12 messages). Both used to exit 0 after "Done ... restored". Both now exit 1 after "Import incomplete: 1 file(s) were not restored". A clean import still exits 0 and prints "Done". (cherry picked from commit a0f808e)
4 participants
What does this PR do?
A missing
state.dbpathname is not proof that no process still holds the previous generation._import_db_memberused that proxy and atomically published a new inode, which is the #65942 / #90950 class: live holders keep writing an unlinked WAL, then halt withDeletedWalGenerationErrorafter the work is done.On Linux, import now consults
_foreign_db_holder_pids(already counts deleted sidecars) before that publish. A non-empty PID list raisesOSErrorso the caller reports a skip instead of a silent replace. An empty list still publishes. Off-Linux the scan is unavailable, so missing-target publish is unchanged.When a turn does halt, the explainer now names the real diverted JSONL path (when known) and the replay command.
hermes sessions import --from divertedappends that JSONL into a healthy session store. It does not replacestate.db.--inspect-onlyprints path + line count without writing.This is not a connection-lifetime generation lock or an owner-record sidecar. Those remain follow-ups (especially macOS, where the
/procholder scan is a no-op).Related Issue
Local tracker: https://github.com/ngpestelos/src/issues/539
Related upstream: #110054 (no in-product recovery after the guard), #109641 (macOS holder scan is Linux-only; this PR does not claim to fix that), #110106 / #109946 (multi-profile walkers).
Type of Change
Changes Made
hermes_cli/backup.py— refuse atomic publish of a missing.dbtarget when Linux holders existagent/turn_explainers.py,agent/session_persistence.py,agent/turn_finalizer.py— interpolate diverted JSONL path; nameimport --from divertedhermes_cli/foreign_sessions.py,hermes_cli/sessions_cmd.py,hermes_cli/subcommands/sessions.py—--from divertedreplay +--inspect-onlytests/hermes_cli/test_backup.py,tests/agent/test_turn_completion_explainer.py,tests/hermes_cli/test_sessions_import_diverted.pyHow to Test
Expect 119 passed on macOS (linux_only holder-scan tests skipped; they run on Linux CI).
Replay after the store is healthy:
Checklist