fix(storage): attest source mutations across train continuity - #3868
Conversation
📝 WalkthroughWalkthroughThe change adds source continuity evidence to released durable change trains. Authorized blob-reference reconciliation captures pre-mutation evidence, persists a pending intent, refreshes continuity after deletion, and reports the result. Startup recovery and validation tests cover committed, failed, malformed, and unauthorized mutation states. ChangesSource continuity refresh
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant MaintenanceReconciliation
participant MutationReceipt
participant refresh_released_source_train_continuity
participant DurableChangeTrainManifest
participant StartupReconciliation
MaintenanceReconciliation->>MutationReceipt: write committed deletion receipt
MaintenanceReconciliation->>refresh_released_source_train_continuity: pass receipt, backup, and pre-mutation evidence
refresh_released_source_train_continuity->>DurableChangeTrainManifest: validate and persist refreshed continuity evidence
DurableChangeTrainManifest-->>MaintenanceReconciliation: return refresh receipt
StartupReconciliation->>DurableChangeTrainManifest: process pending continuity intent
DurableChangeTrainManifest-->>StartupReconciliation: clear intent after processing
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/maintenance/blob_ref_liveness_reconciliation.py`:
- Line 767: Confirm that the additional full-database hashing performed by
capture_durable_database_evidence in the apply path is acceptable for the
largest expected source.db, accounting for the repeated hashes in
refresh_released_source_train_continuity and startup verification. Document the
expected duration and cost in the maintenance runbook, without changing the
evidence placement or offline apply behavior.
- Around line 911-922: Update reconcile_blob_ref_liveness to catch
DurableChangeTrainError from refresh_released_source_train_continuity after the
deletion is committed, store its message in a new continuity_refresh_error field
on BlobRefLivenessReconciliationReport, and still return the report. Expose the
field through to_dict and _render_blob_reference_liveness_plain, pass it through
the report construction near the existing return path, and remove the
train_root.is_dir() guard so refresh_released_source_train_continuity owns
train-layout decisions.
In `@polylogue/storage/sqlite/durable_change_train.py`:
- Around line 322-345: Update the receipt-loading logic around mutation_receipt
to read its bytes once, compute mutation_digest from that buffer, and decode the
same buffer for JSONL parsing and validation. Remove the separate
read_text/read_bytes calls while preserving the existing error handling and
operation-binding checks.
- Around line 854-861: The RELEASED-train path in execute_durable_change_train
must use continuity-aware verification instead of
_verify_persisted_live_tier_continuity. Replace that fallback with
_verify_released_train_live_tier, or update the shared verifier to prefer
source_continuity_evidence for RELEASED source trains while preserving existing
verification for other train types.
- Around line 853-861: Update the actual.user_version == train.target_version
branch to pass the already-captured actual evidence into
_assert_durable_database_continuity instead of calling
capture_durable_database_evidence again. Keep the existing
source_continuity_evidence guard and _verify_persisted_live_tier_continuity
fallback unchanged.
- Around line 405-415: Update the refresh receipt handling in the durable
change-train flow to parse existing JSON inside a JSONDecodeError handler and
raise DurableChangeTrainError for truncated or invalid receipts. Replace
write_text with a uniquely named temporary-file write, flush and fsync the file,
atomically replace refresh_path, and fsync its parent directory using the
existing _fsync_manifest_directory helper and uuid conventions from
write_durable_change_train_manifest, ensuring this completes before the manifest
is written.
In `@polylogue/storage/sqlite/migration_runner.py`:
- Around line 3039-3056: Update the source continuity branch in the validator
around _validate_database_evidence to require a matching
proof:source-continuity-refresh:* reference in proof_refs before validating
train.source_continuity_evidence. Reuse the existing proof-reference validation
pattern used for release_evidence_ref and backup authorization, preserving
refresh_released_source_train_continuity’s generated reference format.
In `@tests/unit/storage/test_durable_change_train.py`:
- Around line 429-435: Extend the positive-path test around
refresh_released_source_train_continuity to assert refreshed.apply_evidence
preserves released.apply_evidence, refreshed.revision increments by one, and
refreshed.proof_refs contains a source-continuity-refresh proof. Add
negative-path tests covering an uncommitted footer phase, mismatched
candidate_digest and operation_id, receipts with fewer than two records, and no
released source train for the live schema; also verify the exported function’s
backup_manifest behavior matches its docstring’s “verified backup” contract.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 68114cb2-82db-40f9-9fc4-92ebfb287373
📒 Files selected for processing (6)
.beads/issues.jsonlpolylogue/cli/commands/maintenance/_blob_integrity.pypolylogue/maintenance/blob_ref_liveness_reconciliation.pypolylogue/storage/sqlite/durable_change_train.pypolylogue/storage/sqlite/migration_runner.pytests/unit/storage/test_durable_change_train.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0a26847c5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pre_mutation_evidence.archive_identity_digest != train.apply_evidence.post.archive_identity_digest: | ||
| raise DurableChangeTrainError("source continuity refresh pre-state has the wrong archive identity") |
There was a problem hiding this comment.
If source.db has any unreceipted content change after the train was released, a later valid liveness run captures that drift in pre_mutation_evidence, but this check validates only archive identity; schema version and quick_check likewise do not authenticate the content. Even a zero-candidate liveness receipt can therefore replace source_continuity_evidence with the already-drifted current state and make startup accept the unreceipted mutation. Require the pre-state to match the full current baseline (train.source_continuity_evidence when present, otherwise train.apply_evidence.post) before recording the successor evidence.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Disposition: informational. The full-database evidence hash remains intentional because continuity compares authenticated content, schema, identity, and quick-check evidence. The refreshed path reuses the captured post-mutation evidence for its live-tier verification, so no duplicate capture was added.
| or header.get("candidate_digest") != operation_id | ||
| or footer.get("kind") != "blob_ref_liveness_reconciliation" | ||
| or footer.get("phase") != "committed" | ||
| ): |
There was a problem hiding this comment.
If a committed receipt is truncated or any candidate record is changed before this helper reads it, these checks still accept it because only the header's claimed digest and the final phase are inspected; the candidate count and digest are never recomputed from the intervening records. The resulting continuity refresh therefore treats a malformed receipt as exact mutation evidence. Parse the receipt with the reconciliation receipt validator, recompute its candidate digest/count, and verify the committed footer's deletion and postcondition fields before updating the train.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 463f38f. Continuity refresh now validates the complete receipt stream, reconstructs every candidate, recomputes the canonical candidate count and digest, and requires a committed footer with matching deletion and post-orphan counts.
| mutation_digest = hashlib.sha256(mutation_receipt.read_bytes()).hexdigest() | ||
| backup_digest = hashlib.sha256(backup_manifest.read_bytes()).hexdigest() |
There was a problem hiding this comment.
If the backup manifest is replaced after the caller's pre-delete/live-fingerprint validation but before this refresh, hashing its current bytes merely records the replacement; the prepared mutation receipt binds only the manifest path, so nothing proves this digest belongs to the backup that covered pre_mutation_evidence. Startup will then accept the refreshed continuity using an unverified backup artifact. Persist the validated manifest digest in the prepared receipt and require the same immutable digest here rather than trusting whichever file occupies the path after the mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 463f38f. The prepared receipt records backup_manifest_sha256 after backup validation, and continuity refresh requires that digest to match the exact backup bytes it reads before updating the train.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5fd9e799d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except DurableChangeTrainError as exc: | ||
| # The source deletion and its committed receipt are already durable. | ||
| # Keep the report truthful while leaving the train fail-closed until a | ||
| # separate continuity refresh succeeds. | ||
| continuity_refresh_error = str(exc) |
There was a problem hiding this comment.
When continuity refresh fails with an exception other than DurableChangeTrainError, this handler still lets the command abort after the deletion and committed receipt are already durable. For example, backup_manifest.read_bytes() can raise OSError if the manifest disappears after its is_file() check, while evidence capture can raise sqlite3.Error; neither is normalized by the helper. In those cases callers never receive the intended truthful applied=True report and continuity residual. Normalize expected refresh exceptions to DurableChangeTrainError or catch them at this post-commit boundary.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 463f38fabc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| evidence_ref = str(raw["evidence_ref"]) | ||
| except (DurableChangeTrainError, KeyError, TypeError, ValueError) as exc: | ||
| raise DurableChangeTrainError(f"source continuity pending intent is malformed: {path}") from exc | ||
| refresh_released_source_train_continuity( |
There was a problem hiding this comment.
When the apply is rejected after the pending intent is written but before the first batch commits—for example, because the locked backup fingerprint or candidate-plan validation fails—the receipt remains only prepared and the database transaction rolls back. On the next daemon startup, this unconditional refresh requires a final committed footer, raises, and leaves the intent in place, so every subsequent startup remains blocked after an otherwise safe maintenance refusal. Recovery needs to classify the receipt/database state and clear rolled-back intents rather than treating every pending file as committed.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6c33c16 and covered by the focused pending-state tests. Startup now reconciles prepared or batch-committed receipts: rolled-back plans clear their intent, full commits continue to continuity refresh, and partial plans remain explicitly fail-closed.
| if not manifest_candidates: | ||
| raise DurableChangeTrainError("source continuity refresh found no released source train") |
There was a problem hiding this comment.
On a freshly initialized current-version archive, bootstrap creates source.db without creating any durable-change-train manifest. A successful blob-ref cleanup still writes a pending intent and commits its deletion, then this branch makes the refresh fail; the liveness command catches that failure and returns applied=True, but the next bootstrap processes the retained intent first and raises here again, preventing the daemon from starting indefinitely. Treat the absence of a released train as having no train baseline to refresh, and consume or avoid the pending intent in that case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6c33c16 and covered by the bootstrap cleanup test. A committed pending intent with no released source train is consumed without attempting a train refresh, so a valid current-version archive does not remain startup-blocked.
| or footer.get("kind") != "blob_ref_liveness_reconciliation" | ||
| or footer.get("phase") != "committed" | ||
| or footer.get("deleted_count") != header.get("candidate_count") | ||
| or footer.get("post_orphaned_count") != 0 |
There was a problem hiding this comment.
If the process exits after the last bounded SQLite batch commits but before the final committed footer is appended, all candidate deletions are durable and the pending intent survives, but startup rejects the receipt here because its last record is only batch_committed. Fresh evidence in the new recovery path is that even the existing receipt reconciler's exact recovered_committed outcome is rejected by this strict phase check, so restarting or reconciling the receipt cannot complete the continuity transition and daemon startup stays blocked. Accept a fully validated recovered-commit receipt or reconcile the candidate state before attempting the refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 6c33c16. Startup invokes the existing prepared-receipt reconciler when the pending receipt ends at prepared or batch_committed, accepts the fully validated recovered_committed outcome, and leaves partial or indeterminate states fail-closed.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/storage/test_durable_change_train.py (2)
408-431: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNo test exercises a receipt that contains candidate records.
Every receipt fixture in this file declares
candidate_count: 0andcandidate_digest: _EMPTY_LIVENESS_DIGEST. The candidate reconstruction loop in_validate_liveness_receipt_bytes(polylogue/storage/sqlite/durable_change_train.pylines 467-503) therefore never runs.That loop performs the core authentication of the receipt. It validates each
candidaterecord, rebuildsBlobRefLivenessCandidate, and recomputes the digest with the producer's exact framing. A framing divergence, a field rename inBlobRefLivenessCandidate, or a broken type check would pass this suite.Add at least one receipt with two candidate records and a correct digest, plus one with a tampered candidate field. Also add a case with an intermediate
batch_committedfooter, which the loop skips.I can generate these fixtures if you want them.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/storage/test_durable_change_train.py` around lines 408 - 431, Extend the durable change train receipt tests around _validate_liveness_receipt_bytes with fixtures containing two candidate records and a correctly framed digest, plus a receipt where a candidate field is tampered and rejected. Add coverage for an intermediate batch_committed footer while preserving the existing prepared/committed receipt flow, ensuring the candidate reconstruction and digest validation paths execute.
369-581: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftSplit the eight scenarios into independent tests.
This single test function covers the happy path, pending-intent recovery, a missing refresh receipt, an invalid footer, an incomplete receipt, malformed JSONL, a mismatched operation id, and a missing released train.
The scenarios share mutable state and run in a required order. Line 461 unlinks the refresh receipt for the case at line 462. Line 548 unlinks the manifest for the case at line 573. A failure in any earlier assertion prevents every later scenario from running, so one regression hides seven checks.
Extract the released-train setup into a fixture. Then parametrize the receipt-rejection cases over
(receipt_body, expected_match)and keep the happy path and the recovery path as separate tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/storage/test_durable_change_train.py` around lines 369 - 581, Split test_released_source_train_can_record_an_authorized_mutation_refresh into independent tests: extract the common released-train/database setup into a fixture, keep the happy path and pending-intent recovery as separate tests, and parametrize the receipt-rejection cases over receipt_body and expected_match. Isolate manifest deletion in the missing-released-train test and receipt deletion in the missing-refresh-receipt test so scenarios do not share mutable state or depend on execution order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/maintenance/blob_ref_liveness_reconciliation.py`:
- Around line 35-39: Rename _clear_source_continuity_pending_intent and
_write_source_continuity_pending_intent to public names in the durable change
train module, update all call sites including blob_ref_liveness_reconciliation,
and add both names to that module’s __all__ alongside
refresh_released_source_train_continuity.
- Around line 80-81: Update the decorator’s dry_run lookup before invoking the
wrapped function so its omitted-argument default matches
reconcile_blob_ref_liveness’s declared default of True, preventing lease
acquisition for the documented read-only call. Keep explicit dry_run values
unchanged.
In `@polylogue/storage/sqlite/durable_change_train.py`:
- Around line 516-544: Update the source-continuity refresh validation around
refresh_refs and the receipt-reading loop to support bounded retention: validate
the current retained receipt plus an archived-chain digest instead of requiring
every historical receipt forever. Add or reuse the established
retention/compaction representation, ensure the chain preserves integrity and
identifies the expected source state, and keep the exactly-one matching-refresh
invariant while avoiding startup reads that grow with archive history.
- Around line 415-417: Replace the message-text check in the surrounding durable
change train exception handler with typed control flow: define and raise a
dedicated DurableSourceTrainMissingError from
refresh_released_source_train_continuity when no released source train exists,
then catch that subclass here while re-raising other DurableChangeTrainError
instances.
- Around line 404-405: Update durable_change_train.py lines 404-405 in the
recovery logic to consume and clear terminal receipt phases such as
postcondition_failed, while preserving the fail-closed exception for
recovered_partial; update blob_ref_liveness_reconciliation.py lines 954-971 so
permanent refresh failures mark or clear the pending intent, retaining it only
for failures that a later run can resolve.
- Around line 464-503: Extract the candidate digest logic currently duplicated
in the receipt validator into a shared helper alongside the existing
_candidate_digest implementation. Update receipt writing, receipt staging, and
train validation to call that helper, preserving the canonical JSON ordering and
bracket/comma framing so candidate counts and digests remain consistent.
In `@tests/unit/storage/test_blob_ref_liveness.py`:
- Around line 1027-1028: Add a concise comment immediately before the
pending-intent assertion explaining that the fixture has no durable train
manifests, so startup recovery invokes the real refresh function and clears the
intent after its “no released source train” error; note that the monkeypatch
affects only blob_ref_liveness_reconciliation and that a future typed exception
would require revisiting this assertion.
---
Outside diff comments:
In `@tests/unit/storage/test_durable_change_train.py`:
- Around line 408-431: Extend the durable change train receipt tests around
_validate_liveness_receipt_bytes with fixtures containing two candidate records
and a correctly framed digest, plus a receipt where a candidate field is
tampered and rejected. Add coverage for an intermediate batch_committed footer
while preserving the existing prepared/committed receipt flow, ensuring the
candidate reconstruction and digest validation paths execute.
- Around line 369-581: Split
test_released_source_train_can_record_an_authorized_mutation_refresh into
independent tests: extract the common released-train/database setup into a
fixture, keep the happy path and pending-intent recovery as separate tests, and
parametrize the receipt-rejection cases over receipt_body and expected_match.
Isolate manifest deletion in the missing-released-train test and receipt
deletion in the missing-refresh-receipt test so scenarios do not share mutable
state or depend on execution order.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6c5eb75e-871c-4e7a-9000-3a2a3412aaaa
📒 Files selected for processing (6)
polylogue/cli/commands/maintenance/_blob_integrity.pypolylogue/maintenance/blob_ref_liveness_reconciliation.pypolylogue/storage/sqlite/durable_change_train.pypolylogue/storage/sqlite/migration_runner.pytests/unit/storage/test_blob_ref_liveness.pytests/unit/storage/test_durable_change_train.py
| assert reconcile_durable_change_train_startup(archive_root) == () | ||
| assert not list((archive_root / ".maintenance-state" / "source-continuity-pending").glob("*.json")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Record why the pending intent is cleared here.
Line 1028 passes only because the fixture archive has no durable train manifests. The real refresh_released_source_train_continuity therefore raises "source continuity refresh found no released source train" during startup recovery, and _recover_pending_source_continuity_intents clears the intent through its substring match on that message.
The monkeypatch at line 1005 replaces the symbol in blob_ref_liveness_reconciliation only, so the startup path uses the real function. Add a short comment stating both facts. If the substring match is replaced by a typed exception, this assertion must be revisited.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/storage/test_blob_ref_liveness.py` around lines 1027 - 1028, Add a
concise comment immediately before the pending-intent assertion explaining that
the fixture has no durable train manifests, so startup recovery invokes the real
refresh function and clears the intent after its “no released source train”
error; note that the monkeypatch affects only blob_ref_liveness_reconciliation
and that a future typed exception would require revisiting this assertion.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d970025eab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| candidates=candidates, | ||
| candidate_digest=candidate_digest, | ||
| ) | ||
| pending_intent = _write_source_continuity_pending_intent( |
There was a problem hiding this comment.
When a committed cleanup returns with continuity_refresh_error, its pending intent remains, but a second apply can reach this call and commit another receipt without first resolving the earlier one. Startup processes pending files in digest order rather than mutation order, and the live database already contains both operations, so the later pre-state can be checked before the earlier baseline or the earlier receipt can attest the combined post-state; recovery then leaves at least one intent permanently blocking startup. Refuse a new source mutation while any continuity intent is pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c779a1d5fe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except DurableChangeTrainError as exc: | ||
| # Semantic continuity rejection cannot become valid by retrying the | ||
| # same committed source mutation. Preserve it durably for startup to | ||
| # consume without hiding the fail-closed train mismatch. | ||
| _mark_source_continuity_pending_intent_terminal(pending_intent, error=exc) |
There was a problem hiding this comment.
When a released source train exists and the backup manifest is temporarily unavailable after the source transaction commits, refresh_released_source_train_continuity() raises DurableChangeTrainError, so this catch marks the pending intent terminal. Startup then deletes that intent without retrying the refresh and rejects source.db against the old train evidence, even if the exact backup is restored, leaving the archive startup-blocked with no automatic recovery path. Only immutable semantic mismatches should become terminal; artifact/readability failures must retain the pending intent for startup recovery.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef5243bc5b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except DurableChangeTrainError as exc: | ||
| # Semantic continuity rejection cannot become valid by retrying the | ||
| # same committed source mutation. Preserve it durably for startup to | ||
| # consume without hiding the fail-closed train mismatch. | ||
| _mark_source_continuity_pending_intent_terminal(pending_intent, error=exc) |
There was a problem hiding this comment.
When any earlier continuity-refresh receipt is temporarily unreadable or contains partial JSON, refresh_released_source_train_continuity() converts that prior receipt's OSError/JSONDecodeError into DurableChangeTrainError, so this branch marks the current intent terminal after its source deletion has committed. Startup then deletes the intent and rejects source.db against the old train baseline; restoring the earlier receipt can no longer complete the current refresh. Fresh evidence beyond the earlier backup-artifact case is the prior-receipt scan at lines 705-711, which classifies these recoverable artifact failures as DurableChangeTrainError; only immutable continuity mismatches should be made terminal.
Useful? React with 👍 / 👎.
| if pre_mutation_evidence.archive_identity_digest != train.apply_evidence.post.archive_identity_digest: | ||
| raise DurableChangeTrainError("source continuity refresh pre-state has the wrong archive identity") | ||
| if current.archive_identity_digest != train.apply_evidence.post.archive_identity_digest: | ||
| raise DurableChangeTrainError("source continuity refresh changed archive identity") | ||
| if pre_mutation_evidence.quick_check != ("ok",) or current.quick_check != ("ok",): | ||
| raise DurableChangeTrainError("source continuity refresh requires successful quick_check evidence") |
There was a problem hiding this comment.
If source.db receives any unrelated row change after the liveness deletion commits but before pending-intent recovery captures current—notably across a process crash—the candidate receipt and authenticated pre-state still validate, while these checks require only the original archive identity and a successful quick_check. The unrelated change is then embedded in source_continuity_evidence and becomes the accepted baseline. Fresh evidence beyond the earlier pre-state continuity report is that no comparison proves the post-state equals that authenticated pre-state with exactly the receipt's candidate keys removed; reconstruct or validate that exact delta before refreshing the train.
Useful? React with 👍 / 👎.
| unreleased = tuple( | ||
| candidate | ||
| for candidate in sorted(manifest_root.glob("source-*.json")) | ||
| if (train := load_durable_change_train_manifest(candidate)).target_version == current_version | ||
| and train.state is not DurableChangeTrainState.RELEASED | ||
| ) |
There was a problem hiding this comment.
When a released source train already has continuity evidence but its referenced refresh receipt has been deleted, this gate sees only the RELEASED state and permits another liveness mutation. The refresh scan considers only files that still exist, so it can create a new receipt, update the train, clear the pending intent, and report success; the next startup nevertheless follows every retained refresh proof reference and fails on the older missing file. Fresh evidence beyond the earlier unreleased-train case is that released-train proof artifacts are not validated here; run the released live-tier and receipt verification before staging or committing a new mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0ae0b3e579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d418b46de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac77c6d3c1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with sqlite3.connect(f"file:{archive_root / 'source.db'}?mode=ro", uri=True) as connection: | ||
| if not classify_blob_ref_liveness(connection).safe_to_apply: |
There was a problem hiding this comment.
When a receipt is postcondition_failed because orphan rows remain, this recovery accepts it whenever safe_to_apply is true, but BlobRefLivenessClassification.safe_to_apply only checks unknown/unavailable/rekeyable types and does not require orphaned_count == 0. It then labels the operation recovered-committed and refreshes continuity, attesting a database that still fails the original liveness postcondition. Fresh evidence beyond the earlier resolved thread is that the new recovery branch reruns only this weaker predicate; require both safe_to_apply and a zero orphan count before continuing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e27984e03b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Problem: authorized blob liveness cleanup mutates source.db after a released durable source train, leaving the continuity gate unable to distinguish a permitted maintenance mutation from an unproven drift.\n\nWhat changed: bind the committed liveness receipt and verified backup to a typed source-continuity refresh receipt, retain the original migration evidence, and make startup validate the refreshed live evidence. Add focused lifecycle coverage and expose the receipt in maintenance output.\n\nCompatibility/migration: existing released manifests remain readable but source mutations now require a continuity refresh before the next durable migration or startup reconciliation can proceed.\n\nRef polylogue-6k0na\n\nCo-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60fab47e1f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Bind the continuity-refresh implementation to its execution-grade Bead and preserve the explicit phase-2 production residual.\n\nRef polylogue-6k0na\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Make the phase-2 source remediation carrier consume the durable source continuity refresh before it can emit the frozen snapshot.\n\nRef polylogue-6k0na\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: a committed blob-reference deletion could lose its report when the follow-up source continuity refresh failed, and the refresh path had several integrity gaps around receipt snapshots, durable receipt publication, and released-train verification.\n\nWhat changed: retain the committed deletion report with a typed refresh residual, bind receipt validation to one byte snapshot, atomically fsync refresh receipts before manifest publication, reuse captured startup evidence, require the retained refresh proof reference, and add positive and negative file-backed coverage.\n\nCompatibility/migration: source trains remain fail-closed when refresh cannot be completed; the maintenance report now exposes that residual without pretending the deletion was rolled back.\n\nRef polylogue-6k0na\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: Source-tier cleanup could commit durable row changes before the released train had a durable continuity refresh, and the receipt did not authenticate its complete candidate stream or backup bytes.\n\nWhat changed: Hold archive ownership across liveness, bind and validate the full receipt, require pre-mutation content continuity, and persist a checksummed pending intent that startup can replay idempotently after a crash.\n\nCompatibility/migration: Existing released source trains gain a refresh-only recovery path. No schema or production archive mutation is performed by this change.
Problem: A released source train could trust a missing or modified refresh receipt, and an unexpected post-commit refresh error could abort after the source mutation was already durable.\n\nWhat changed: Validate the exact refresh receipt checksum, train identity, and source-after evidence during released-train startup verification. Normalize post-commit refresh exceptions into the truthful liveness residual report.\n\nCompatibility/migration: Existing manifests with continuity evidence require their retained refresh receipt at startup. No production archive mutation is performed by this change.
Problem: Pending continuity intents could block startup after a rolled-back apply, an archive without a released source train, or a crash before the final receipt footer. Read-only liveness census also acquired the writer lease.\n\nWhat changed: Reconcile prepared receipts into rollback, full-commit, or partial outcomes; consume safe rollback and no-train intents; accept validated recovered commits; and reserve archive ownership for mutating liveness runs only.\n\nCompatibility/migration: Partial source mutations remain fail-closed for manual recovery. No production archive mutation is performed by this change.
Problem: A committed liveness cleanup on a bootstrap archive has no released source train to refresh, so the pending marker must not block the next startup.\n\nWhat changed: Exercise startup consumption of the committed pending intent when continuity refresh reports that no released source train exists.\n\nCompatibility/migration: The test covers the no-train bootstrap path without changing production archive state.
Problem: semantic refresh rejections left committed source mutations pending indefinitely, while omitted dry runs acquired the archive writer lease.\n\nWhat changed: persist terminal continuity outcomes, use typed missing-train recovery, centralize candidate receipt digests, and keep default dry runs read-only.\n\nVerification: devtools test storage liveness and durable train tests; devtools verify --quick. Co-Authored-By: Codex <noreply@openai.com>
Problem: source liveness apply could persist relative recovery artifacts, stack a second mutation over a pending continuity refresh, or mutate a source tier while its current train was not released. Startup also left an already rolled-back intent pending. What changed: normalize liveness artifact paths before receipt and intent persistence, reject fresh applies until continuity recovery is clear and the current source train is released, and consume recovered rollback intents. Verification: focused source-continuity routes pass; devtools verify --quick passes format, lint, and mypy. The broader focused storage run reproduces an unrelated canonical trigger-literal inventory failure. Co-Authored-By: Codex <noreply@openai.com>
Authenticate retained refresh evidence against the released train, keep retryable failures pending, and preserve semantic mismatches as typed terminal outcomes. Keep source mutation ownership and publicize the cross-module pending-intent contract.
Problem: source liveness could begin while a released train's continuity receipt was missing, and recovery loaded large mutation receipts into several simultaneous in-memory structures. Committed mutations without a released source baseline or after a postcondition failure also needed explicit startup handling. What changed: validate the live released train before a new apply, bind backup bytes captured during validation, canonicalize continuity paths, stream receipt validation, ignore refresh artifacts from earlier trains, and reconcile no-train and postcondition-failed intents without losing the fail-closed behavior. Verification: devtools test tests/unit/storage/test_durable_change_train.py tests/unit/storage/test_blob_ref_liveness.py -k 'not canonical_inventory_preserves_trigger_literal_whitespace'; mypy polylogue/storage/sqlite/durable_change_train.py polylogue/maintenance/blob_ref_liveness_reconciliation.py
Problem: a source apply could race an active pre-apply train, cleanup errors could escape after a committed deletion, and continuity refreshes rewrote the original released proof bundle. What changed: block active reserved or backup-authorized source trains, retain cleanup failures in the post-commit report, and keep continuity references at train level while preserving the immutable release proof. Verification: devtools test tests/unit/storage/test_durable_change_train.py tests/unit/storage/test_blob_ref_liveness.py -k 'not canonical_inventory_preserves_trigger_literal_whitespace'; ruff check; ruff format --check; mypy on the two changed modules
Refresh the commit-bound CI receipt after synchronizing the ready PR scope carrier. The empty trigger commit carries no product diff and will disappear in the squash merge.
Trigger a fresh CI evaluation after recomputing the ready PR scope carrier for the final branch head. This metadata-only trigger has no product diff and disappears in the squash merge.
Problem: postcondition-failed recovery treated a safe classification as sufficient even when orphan rows remained.\n\nWhat changed: require both safe liveness classification and zero orphaned rows before refreshing source continuity. Added a regression test that keeps the pending intent when an orphan remains.\n\nVerification: focused durable change-train recovery tests passed.
Problem: committed source liveness mutations could hide terminalization failures, report a still-pending continuity refresh as successful, and reject a valid postcondition recovery footer. Schema inventory normalization also collapsed whitespace inside trigger string literals.\n\nWhat changed: preserve terminalization errors in the durable report, expose pending continuity state and return a nonzero CLI status while recovery remains pending, accept the validated postcondition_failed to recovered_committed receipt sequence, and protect SQL string literals during schema normalization.\n\nVerification: devtools test tests/unit/storage/test_blob_ref_liveness.py tests/unit/cli/test_blob_ref_liveness_cli.py tests/unit/storage/test_durable_change_train.py; devtools verify --quick\n\nCo-Authored-By: Claude <noreply@anthropic.com>
60fab47 to
b33ecc2
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Problem: the first source continuity pending-intent creation fsynced the new directory but not its parent directory entry, so a crash could lose the recovery directory name after the source mutation committed.\n\nWhat changed: fsync the maintenance-state parent when the pending-intent directory is first created and add a regression assertion for that parent-directory durability boundary.\n\nVerification: devtools test tests/unit/storage/test_durable_change_train.py\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: the new parent-directory durability regression used an untyped lambda that mypy rejected even though the behavior assertion passed.\n\nWhat changed: replace the recorder lambda with a typed helper that records and delegates the fsync call.\n\nVerification: devtools test tests/unit/storage/test_durable_change_train.py\n\nCo-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@polylogue/maintenance/blob_ref_liveness_reconciliation.py`:
- Around line 986-994: Wrap the success-path
clear_source_continuity_pending_intent call after
refresh_released_source_train_continuity in the same OSError handling used by
the existing error branches. Preserve the committed result and report it while
recording residual pending-intent state when cleanup fails, allowing
reconcile_blob_ref_liveness to return successfully after irreversible work is
committed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 280bbeaf-9a19-4cff-8066-7b746a10d92b
📒 Files selected for processing (8)
.beads/issues.jsonlpolylogue/cli/commands/maintenance/_blob_integrity.pypolylogue/maintenance/blob_ref_liveness_reconciliation.pypolylogue/storage/blob_ref_liveness.pypolylogue/storage/sqlite/durable_change_train.pypolylogue/storage/sqlite/migration_runner.pytests/unit/storage/test_blob_ref_liveness.pytests/unit/storage/test_durable_change_train.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b33ecc25ea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7c2d3138e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Problem: Source continuity recovery could publish a committed footer before its liveness postcondition was safe, and a durable refresh could lose successful state when pending-intent cleanup failed. Newly created receipt directories also lacked a parent-directory fsync. What changed: Run the recovery postcondition check before appending the recovered-committed footer, preserve a committed refresh report while surfacing cleanup residuals, and fsync newly created receipt-directory parents. Extend focused tests for the ordering and durability contracts. Verification: direnv exec . devtools verify --quick; direnv exec . devtools test tests/unit/storage/test_blob_ref_liveness.py tests/unit/cli/test_blob_ref_liveness_cli.py tests/unit/storage/test_durable_change_train.py Co-Authored-By: Claude <noreply@anthropic.com>
Problem: An inactive rolled-back source train could be mistaken for an irrelevant historical train, allowing blob-ref mutation against stale continuity evidence. A newly created receipt directory also lacked a durable parent-directory entry. What changed: Keep rolled-back failures at the live source version in the fail-closed train gate, and fsync the parent when creating the prepared-receipt directory. Add regression coverage for both recovery authority and receipt-directory durability. Verification: direnv exec . devtools test tests/unit/storage/test_blob_ref_liveness.py tests/unit/storage/test_durable_change_train.py; the pre-commit quick baseline is also required. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ceb4098399
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mutation_digest = hashlib.sha256(receipt_bytes).hexdigest() | ||
| backup_digest = hashlib.sha256(backup_manifest.read_bytes()).hexdigest() | ||
| receipt_backup_digest = header.get("backup_manifest_sha256") | ||
| if not isinstance(receipt_backup_digest, str) or receipt_backup_digest != backup_digest: | ||
| raise DurableChangeTrainError("source mutation receipt backup manifest digest mismatch") |
There was a problem hiding this comment.
When startup recovers a deletion committed before the continuity refresh, this path authenticates only the bytes of manifest.json; if a referenced backup database, blob, or verification receipt was deleted or tampered with after the apply-time check while the manifest itself remained unchanged, recovery still publishes the new train baseline and clears the pending intent even though the destructive source mutation no longer has a verified, restorable backup. Revalidate the manifest's attestation and complete artifact inventory against the recorded pre-mutation evidence before accepting recovery, rather than treating the manifest hash alone as backup proof.
AGENTS.md reference: AGENTS.md:L189-L192
Useful? React with 👍 / 👎.
## Summary Harden receipt-backed source-train continuity refresh for authorized source-tier mutations. ## Problem PR #3868 supplied the continuity refresh route, but its regression had drifted to a hard-coded source version and the guard lacked a direct typed rejection for wrong-tier pre-mutation evidence. ## Solution - Keep the real backup/receipt/archive/schema/operation/post-evidence continuity route fail-closed. - Make the liveness regression follow the current source train. - Reject non-source pre-mutation evidence explicitly. - Keep production source mutation under the phase-2 live receipt; this PR does not mutate production. ## Verification - Focused durable/liveness/CLI suite: 97 passed. - `devtools verify --quick`: 24 checks passed. - Red reproduction failed before the explicit wrong-tier guard. - Fresh adversarial review found no legitimate AC gaps. <!-- polylogue-pr-scope:v1 { "assigned_beads": ["polylogue-6k0na"], "beads_digest": "c8f1936452f647313deae3107d033d82846d111fa501e02fd6b3d9d970362db0", "dispositions": [{ "bead_id": "polylogue-6k0na", "disposition": "satisfied", "evidence": [ {"kind":"commit","ref":"ff6fe3e20f8984e713816308cf9c848ca9fdb5a7"}, {"kind":"commit","ref":"a81073cc708fb8bb4ff542125f9588cbfb208bd0"}, {"kind":"test","ref":"focused durable/liveness/CLI suite: 97 passed"}, {"kind":"command","ref":"devtools verify --quick: 24 checks passed"}, {"kind":"review","ref":"final adversarial review: no legitimate gaps"} ], "successors": [] }], "head_sha": "a81073cc708fb8bb4ff542125f9588cbfb208bd0", "scope_digest": "301853055202ba28989bffaece2eb0222c0e5fdfa0edc2490774f6610b777296", "version": 1 } -->
1 participant
Summary
Bind authorized source-tier maintenance mutations to a typed continuity refresh so the released durable source train remains auditable and startup can distinguish permitted liveness changes from unproven drift. Ref polylogue-6k0na.
Problem
The verified blob-reference liveness apply changed source.db after source train 028 was released. The next current-master migration correctly refused to proceed because the train still carried the pre-mutation content evidence. The archive therefore had a valid backup-gated mutation with no supported receipt path to refresh train continuity. The first implementation also needed to preserve the committed-deletion result when the follow-up refresh could not complete.
Solution
Add a receipt-backed source continuity refresh that requires the committed liveness JSONL receipt, the exact prevalidated backup manifest, the archive ownership lease, one released source train matching the live schema, unchanged schema and archive identity, successful quick checks, and post-mutation evidence. The original migration apply evidence remains immutable. Receipt validation now authenticates the complete candidate stream, terminal footer, and backup bytes. The liveness lease spans staging through continuity finalization, pre-mutation content must match the released baseline, and a checksummed pending intent lets startup recover a committed mutation if manifest refresh was interrupted. Refresh receipts are written and fsynced atomically before the manifest references them, and startup plus later migration execution use continuity-aware released-train verification. A continuity failure now remains fail-closed but is returned as a typed maintenance residual alongside the committed deletion report. The Beads phase-2 source remediation node consumes this blocker before it can emit a frozen source snapshot.
The implementation is intentionally partial against the full production Bead: it supplies the safe continuity seam and focused coverage, while the real phase-2 source remediation receipt remains open and is named as the successor in the structured carrier.
Verification
devtools test tests/unit/storage/test_blob_ref_liveness.py tests/unit/cli/test_blob_ref_liveness_cli.py tests/unit/storage/test_durable_change_train.py: 72 passed.devtools verify --quick: exit 0, receipt20260807T050152Z-quick-3750698-9d9f179e; Ruff, mypy, rendering, layering, policy checks, and schema promotion audit passed.Migration notes
The existing source train remains fail-closed until this receipt path is deployed and a fresh authorized maintenance receipt refreshes its continuity evidence. No production mutation is performed by this PR.
Summary by CodeRabbit
New Features
Bug Fixes