fix: bind rebuild receipts to replay evidence - #3862
Conversation
Problem: resumable index rebuilds could validate a stale receipt against a narrow source snapshot, rehash external ground truth at every checkpoint, and leave a ready transaction after post-promotion evidence failed. What changed: bind raw authority and parser semantics into the shared source snapshot, cache a pass-scoped external inventory token with metadata change detection, verify referenced blob bytes before candidate readiness, and record terminal post-promotion attestation state through IndexGenerationStore. Resolve relative CLI receipt paths before daemon serialization and add production-route anti-vacuity coverage. Compatibility/migration: existing receipt paths remain valid; older receipts without the optional blob sub-snapshot continue through ordinary validation, while candidate readiness requires live BlobStore verification.
Problem: the quick verification gate found optional inventory-token and generation-path typing errors. What changed: initialize the token binding before receipt-shape branching and narrow the test generation path cast.
|
Warning Review limit reached
Next review available in: 18 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe rebuild flow validates immutable source evidence, reuses external inventory tokens, verifies referenced blobs, and records post-promotion attestation. CLI receipt paths become absolute. Daemon rebuilds preserve active generations after terminal attestation failures. ChangesRebuild safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant RebuildIndex
participant SchemaInferenceGate
participant IndexGenerationStore
participant Daemon
CLI->>SchemaInferenceGate: resolve and validate receipt path
CLI->>RebuildIndex: submit rebuild request
RebuildIndex->>SchemaInferenceGate: validate inventory and blob evidence
SchemaInferenceGate-->>RebuildIndex: return inventory token
RebuildIndex->>IndexGenerationStore: checkpoint candidate and attestation
RebuildIndex->>Daemon: report promoted-attestation-failed
Daemon->>IndexGenerationStore: preserve active generation and skip retry
Possibly related issues
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 |
![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: bc67e666bb
ℹ️ 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".
| transaction = generation_store.checkpoint_transaction( | ||
| transaction, | ||
| status="promoted-attestation-failed", | ||
| error=str(attestation_error), |
There was a problem hiding this comment.
When the initial post-promotion checkpoint fails but this recovery checkpoint succeeds, the daemon receives a transaction with status promoted-attestation-failed. However, polylogue/daemon/bulk_rebuild.py neither includes that status in _TERMINAL_NOT_RESUMABLE nor handles it alongside promoted, so the next daemon tick treats the already-active generation as resumable and passes its operation ID back to the rebuild engine, which rejects it as terminal. This leaves automatic bulk convergence failing on every subsequent tick after the exact checkpoint fault this recovery path is intended to handle; update the daemon's terminal-status handling while ensuring it does not attempt to discard the active generation.
Useful? React with 👍 / 👎.
| if ( | ||
| inventory_token is not None | ||
| and token_origin.get("declared_roots") == [str(root) for root in resolved_roots] | ||
| and token_origin.get("inventory_change_detector") == detector | ||
| ): |
There was a problem hiding this comment.
During a multi-pass rebuild, an in-place rewrite that preserves file length and restores the original mtime (for example, a synchronizer or restore tool preserving metadata) also preserves the inode, so this branch reuses the receipt's old content hashes without opening the changed file. The external corpus can therefore change while validate_schema_inference_receipt continues returning success, defeating the new replay-evidence binding; the cache needs a content-backed or otherwise immutable change signal rather than treating size/inode/mtime equality as proof that bytes are unchanged.
Useful? React with 👍 / 👎.
| try: | ||
| consumed_evidence = _validate_rebuild_provenance_receipt(root, request.schema_inference_receipt_path) | ||
| except BaseException as exc: | ||
| _mark_rebuild_transaction_stale_after_provenance_failure(root, request.operation_id, exc) |
There was a problem hiding this comment.
When receipt validation fails for a supplied operation ID, this call writes the transaction record before offline_maintenance_block_reason, OwnedArchiveLocation.acquire, and RebuildLease run. Consequently, an offline CLI invocation made while the daemon or another rebuild owns the archive can mutate that live writer's transaction to stale before the invocation is refused, racing subsequent checkpoints and potentially terminating an otherwise valid rebuild. Move this lifecycle mutation under the same ownership and rebuild lease used by the rest of the operation.
AGENTS.md reference: AGENTS.md:L163-L166
Useful? React with 👍 / 👎.
| except BaseException as exc: | ||
| _mark_rebuild_transaction_stale_after_provenance_failure(root, request.operation_id, exc) |
There was a problem hiding this comment.
If an operator interrupts the potentially long receipt or corpus validation with Ctrl-C, KeyboardInterrupt is caught here because it derives from BaseException, and an existing resumable transaction is permanently changed to stale even though no provenance check actually failed. The same applies to task cancellation and other non-validation control-flow exceptions, causing expensive valid candidates to become unresumable merely because validation was aborted; only actual validation exceptions should trigger the stale transition.
Useful? React with 👍 / 👎.
| FROM raw_sessions | ||
| ORDER BY raw_id | ||
| """ | ||
| ) | ||
| ).fetchall() |
There was a problem hiding this comment.
This fetchall() materializes every selected column from every raw_sessions row merely so the function can later derive the distinct origin set. Because the source snapshot is recomputed repeatedly at admission, checkpoints, and readiness, a large archive now allocates an archive-sized Python object graph on each validation and can exhaust memory before replay begins; the previous implementation streamed this cursor. Accumulate origins while iterating the cursor, or query distinct origins separately, without retaining all rows.
Useful? React with 👍 / 👎.
| "missing_references": _sample(missing_references, DEFAULT_SAMPLE_LIMIT), | ||
| "referenced_blob_integrity_snapshot": _referenced_blob_integrity_snapshot( | ||
| archive_root, referenced_hashes=referenced_hashes | ||
| ), |
There was a problem hiding this comment.
At this point _full_blob_hash_evidence has already run BlobStore.verify_all, which reads and SHA-256-hashes every canonical blob, but _referenced_blob_integrity_snapshot immediately opens and hashes every source-referenced blob again. Since referenced raw payloads normally comprise most of a large blob store, receipt generation now adds another full multi-GiB read—and rebuild_source_revision_snapshot performs another referenced-blob verification shortly afterward. Derive the referenced evidence during the existing verify_all traversal instead of repeating the complete I/O pass.
Useful? React with 👍 / 👎.
| if not current_blob_snapshot.get("passed"): | ||
| errors.append("referenced source blob integrity verification failed before candidate readiness") | ||
| if recorded_blob_snapshot and recorded_blob_snapshot != current_blob_snapshot: | ||
| errors.append("receipt referenced source blob integrity snapshot changed") |
There was a problem hiding this comment.
Candidate readiness compares the complete recorded and current snapshots, including each blob's inode and mtime_ns, even after BlobStore.verify has proved that its bytes still match the content-addressed hash. Replacing a blob atomically with identical bytes or restoring/touching its metadata therefore rejects an otherwise valid rebuild with “snapshot changed,” despite no replay input changing. Compare the verified hash/size content evidence rather than volatile inode and timestamp fields.
Useful? React with 👍 / 👎.
Problem: rebuild receipt failures could mutate resumable operations before ownership, route active generations back through daemon rebuild, and repeat expensive evidence scans. Metadata-only corpus checks also failed to detect some in-place rewrites, while raw-session snapshot collection materialized the selected rows. What changed: classify promoted-attestation-failed as terminal without discarding its active generation; defer stale mutation until archive and rebuild ownership are held and catch only validation failures; include ctime in the corpus change signal; stream raw-session evidence rows; and derive referenced-blob snapshots from completed BlobStore.verify_all content evidence. Compatibility/migration: receipt evidence algorithms advance to v2 for the changed snapshot shapes. No live archive or production reindex was run. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the rebuild receipt review findings lacked regression coverage for terminal daemon routing, ownership ordering, control-flow interruption, corpus rewrites, streamed raw evidence, and blob verification reuse. What changed: add real-route tests for all reviewed failure modes, including an active archive owner guard, resumability assertions, preserved corpus metadata rewrites, a fetchall guard, and a BlobStore.verify counter. Verification: the provenance-gate test file passes all 31 tests; the affected maintenance and CLI selection was run separately and its two existing CLI receipt-fixture failures remain recorded in the handoff. Co-Authored-By: Claude <noreply@anthropic.com>
…-receipt-integrity
…-receipt-integrity
Problem: The rebuild receipt implementation had no durable edge into the live-operation receipt aggregate, so the PR could claim implementation without making the later live evidence requirement explicit.\n\nWhat changed: Bind polylogue-q4qpl to polylogue-live-operation-receipts and update the PR scope carrier target.\n\nCompatibility/migration: No production or archive state changes. Live migration, candidate build, and promotion remain out of scope.
Problem: Review identified that volatile inode and mtime metadata could be mistaken for replay-affecting blob changes even after content verification.\n\nWhat changed: Add an anti-vacuity regression proving a metadata-only touch leaves the referenced blob evidence unchanged.\n\nCompatibility/migration: No production or archive state changes.
|
Codex review triage for PR #3862:
Verification: worker evidence includes 31 provenance tests, 109 affected maintenance and CLI tests with two existing CLI fixture failures classified, and |
|
run-ci |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b305fe3d4
ℹ️ 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 recorded_blob_snapshot and recorded_blob_snapshot != current_blob_snapshot: | ||
| errors.append("receipt referenced source blob integrity snapshot changed") |
There was a problem hiding this comment.
For every newly generated receipt, the recorded snapshot identifies its verifier as BlobStore.verify_all, while this readiness-time snapshot identifies it as BlobStore.verify; comparing the complete dictionaries therefore reports a changed snapshot even when every blob and size is unchanged. Because rebuild readiness now requests verify_blob_integrity=True, a normal rebuild using a fresh receipt is rejected before promotion. Compare the content-bearing fields (such as digest, entries, count, and pass state) rather than the implementation label.
Useful? React with 👍 / 👎.
|
circleci run |
Problem: The content-backed blob regression should explicitly guard the receipt schema against inode and mtime fields returning through a future refactor.\n\nWhat changed: Assert the referenced blob evidence contains only content identity and verification fields while preserving the metadata-touch test.\n\nCompatibility/migration: No production or archive state changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polylogue/storage/index_generation.py (1)
1179-1213: 🗄️ Data Integrity & Integration | 🔵 TrivialDocument that this snapshot change stales in-flight rebuilds.
rebuild_source_evidence_snapshotis persisted asIndexRebuildTransaction.source_snapshotand compared on start. This change adds raw-session provenance columns plus lowering/parser fingerprints, so existing in-flight rebuilds fail that check and are checkpointedstale. Operators should drain or restart rebuilds before upgrading, and any unrelated change to the lowering fingerprints will start staling rebuilds mid-operation.🤖 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 `@polylogue/storage/index_generation.py` around lines 1179 - 1213, Document the operational impact of the updated rebuild_source_evidence_snapshot used by IndexRebuildTransaction.source_snapshot: existing in-flight rebuilds must be drained or restarted before upgrading because they will be checkpointed stale, and changes to lowering_fingerprint() or parser_fingerprint_for_origin() can stale rebuilds mid-operation.
🤖 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/daemon/bulk_rebuild.py`:
- Around line 229-234: Update resolve_or_start_daemon_bulk_rebuild_transaction
so a persisted promoted-attestation-failed transaction is retained or returned
instead of being discarded and replaced with a running transaction. In
polylogue/daemon/bulk_rebuild.py lines 229-234, exclude
promoted-attestation-failed from terminal-record cleanup; at lines 346-346,
perform the terminal-state check before replacement or use a resolver result
that preserves this status, ensuring no rebuild starts after attestation
failure.
In `@polylogue/maintenance/rebuild_index.py`:
- Around line 209-227: Guard IndexGenerationStore.for_archive_root(root) and
transaction loading within the existing recovery handling so
ArchiveLocationError/OSError and related lookup failures return without
replacing the original provenance error. In the checkpoint_transaction cleanup
around the stale status update, catch only ordinary Exception when adding a
note; allow KeyboardInterrupt, SystemExit, and other control-flow BaseException
subclasses to propagate unchanged.
- Around line 2043-2061: Update the attestation failure handler around
generation_store.checkpoint_transaction so control-flow exceptions such as
KeyboardInterrupt, SystemExit, and asyncio.CancelledError propagate instead of
being recorded as promoted-attestation-failed. When the recovery checkpoint also
fails, re-raise or otherwise surface a hard failure after adding the recovery
context; do not fall through to pointer flipping or receipt construction.
Preserve the existing recovery checkpoint and attestation error details for
ordinary exceptions.
- Around line 983-1011: Hoist lowering_fingerprint() out of the raw_evidence
comprehension in rebuild_selection_evidence and compute it once per rebuild
operation; cache parser_fingerprint_for_origin by origin so shared origins reuse
the same value while preserving each row’s fingerprint fields. Also review the
raw_id IN expansion in the same function and chunk queries when expanded exceeds
SQLite’s variable limit, combining all fetched rows before building evidence.
- Around line 1900-1904: Update the rebuild flow around
_validate_before_derived_state and its two call sites to retain the completed
referenced-blob verification result from the acceptance gate and pass or reuse
it at the readiness gate. Ensure the later validation still enforces the
readiness boundary while avoiding a second BlobStore.verify re-hash of the same
referenced blobs.
In `@polylogue/maintenance/schema_inference_gate.py`:
- Around line 1856-1888: Update the inventory-token binding guard in the receipt
validation flow to run only when inventory_token is truthy, so an empty dict is
treated as absent and does not trigger the mismatched-token error. Preserve the
existing full-validation fallback and receipt_token handling for absent or empty
tokens.
- Line 1303: The canonical external ground-truth digest currently omits
inventory_change_detector, allowing stale receipt inventory and mappings to be
reused when the detector changes. Update _canonical_external_ground_truth_digest
to include the detector metadata, or alternatively make the per-origin detector
in the token binding match the receipt during validate_schema_inference_receipt;
preserve receipt reuse only when the detector is validated.
In `@polylogue/storage/index_generation.py`:
- Around line 189-193: Update save_transaction to serialize the transaction with
the same default=str fallback already used by save_pass_receipt, ensuring
post_promotion_attestation values such as Paths, datetimes, or exceptions cannot
make the durable write raise after promotion.
In `@tests/unit/maintenance/test_rebuild_index_phase_timing.py`:
- Around line 99-100: Update _seed_distinct_codex_sessions in
tests/unit/maintenance/test_rebuild_index_phase_timing.py (lines 99-100) to
accept pytest.MonkeyPatch and configure the schema receipt with
monkeypatch.setenv. Apply the same change to _seed in
tests/unit/maintenance/test_rebuild_index_resume_correctness.py (lines 93-94),
updating their callers to pass the fixture so environment changes are
automatically restored.
In `@tests/unit/maintenance/test_rebuild_index_provenance_gate.py`:
- Around line 839-862: Remove the stubs for
resolve_or_start_daemon_bulk_rebuild_transaction and
_validate_rebuild_provenance_receipt in this test so
run_daemon_bulk_rebuild_pass exercises the real terminal-transaction resolution
logic. Preserve the unexpected_rebuild guard and assertions, allowing the
resolver to read the terminal state established by the earlier production
rebuild path and verify no rebuild is routed.
- Around line 704-745: Update
test_revision_authority_binding_stales_a_resumed_real_rebuild to pin the
rebuild_source_evidence_snapshot mechanism: capture its digest before mutating
revision_authority_evidence, then assert the digest changes afterward. Also
distinguish the evidence-drift failure from the receipt source-snapshot
validation by matching the evidence-path error text, so the test cannot pass
through the unrelated rebuild_source_revision_snapshot or shared stale-status
paths.
In `@tests/unit/maintenance/test_schema_inference_gate.py`:
- Around line 505-536: Add coverage alongside
test_external_inventory_token_reuses_metadata_detector_and_rejects_changed_bytes
for rejecting a token with mismatched binding fields such as receipt_nonce,
asserting SchemaInferenceGateError with “inventory token is not bound to this
pass”; also assert that inventory_token={} falls back to normal full validation
rather than being treated as a mismatched token.
---
Outside diff comments:
In `@polylogue/storage/index_generation.py`:
- Around line 1179-1213: Document the operational impact of the updated
rebuild_source_evidence_snapshot used by
IndexRebuildTransaction.source_snapshot: existing in-flight rebuilds must be
drained or restarted before upgrading because they will be checkpointed stale,
and changes to lowering_fingerprint() or parser_fingerprint_for_origin() can
stale rebuilds mid-operation.
🪄 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: 3938929a-a55b-464c-a8bd-5b19944baa93
📒 Files selected for processing (11)
.beads/issues.jsonlpolylogue/cli/commands/maintenance/_rebuild_index.pypolylogue/daemon/bulk_rebuild.pypolylogue/maintenance/rebuild_index.pypolylogue/maintenance/schema_inference_gate.pypolylogue/storage/index_generation.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/maintenance/test_rebuild_index_phase_timing.pytests/unit/maintenance/test_rebuild_index_provenance_gate.pytests/unit/maintenance/test_rebuild_index_resume_correctness.pytests/unit/maintenance/test_schema_inference_gate.py
Problem: automated review found that rebuild attestation failures could be overwritten, replay evidence could exceed SQLite bind limits, blob evidence was rehashed at two readiness boundaries, and inventory detector or token state could be omitted from admission proofs. What changed: preserve terminal attestation failures, propagate control-flow and recovery errors, cache verified blob evidence per rebuild context, chunk closure queries, cache parser fingerprints, bind inventory detectors and empty-token behavior correctly, serialize transaction metadata safely, and make regression fixtures exercise the real daemon resolver. Verification: devtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py (34 passed); devtools test tests/unit/maintenance/test_schema_inference_gate.py (23 passed); devtools test tests/unit/maintenance/test_rebuild_index_phase_timing.py tests/unit/maintenance/test_rebuild_index_resume_correctness.py (9 passed); devtools test tests/unit/storage/test_index_generation.py (37 passed); devtools verify --quick (exit 0). Ref polylogue-q4qpl
|
Automated review triage for head
Verification on this head: 34 provenance tests, 23 schema-gate tests, 9 phase/resume tests, 37 index-generation tests, and |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 432f5cb96a
ℹ️ 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 transaction.status == "promoted-attestation-failed": | ||
| # The generation is already active. Preserve the terminal | ||
| # attestation failure so the caller can stop without starting | ||
| # another rebuild under the same operation id. | ||
| return transaction |
There was a problem hiding this comment.
When a promotion is recorded as promoted-attestation-failed, every later resolver call returns this same transaction, and run_daemon_bulk_rebuild_pass immediately returns None. The well-known transaction is therefore never retired, so even a later source change or rebuild backlog cannot start another daemon rebuild. Fresh evidence beyond the prior comment is this new special-case return itself; retire only the transaction record while preserving the already-active generation.
Useful? React with 👍 / 👎.
| self.receipt_path, | ||
| inventory_token=self.external_inventory_token, | ||
| verify_blob_integrity=verify_blob_integrity, | ||
| verified_blob_integrity_snapshot=self.verified_blob_integrity_snapshot, |
There was a problem hiding this comment.
On the second verify_blob_integrity=True validation, this passes the snapshot cached by the earlier post-replay check, causing validate_schema_inference_receipt to reuse it instead of hashing current blobs. If a referenced blob is replaced or corrupted during the intervening acceptance and insight-materialization work, the purported final readiness check accepts the stale result and can promote a candidate without verifying the bytes that exist at promotion time.
Useful? React with 👍 / 👎.
| { | ||
| "origin": origin, | ||
| "roots": sorted(str(Path(root).expanduser().resolve()) for root in roots), | ||
| "inventory_change_detector": evidence.get("inventory_change_detector"), |
There was a problem hiding this comment.
Including inventory_change_detector in the canonical digest makes inode, mtime, and ctime part of external corpus identity, even though the detector is documented as only a signal to trigger authoritative rehashing. A metadata-only operation such as chmod, or atomically replacing a file with identical bytes, changes ctime/inode; the validator then rehashes identical content but still rejects the receipt because this metadata digest changed. Use the detector to decide when to rehash, but compare the resulting content inventory and mapping for identity.
Useful? React with 👍 / 👎.
Problem: A promoted-attestation-failed daemon transaction remained permanently blocking even after a later source change, and the final readiness check reused an earlier blob verification snapshot. The inventory metadata detector also contaminated content identity.\n\nWhat changed: Retire only the terminal transaction after source drift, refresh referenced blob bytes at the final readiness boundary, and keep metadata as a rehash trigger rather than corpus identity. Add production-route regressions for each boundary.\n\nCompatibility/migration: No schema or production archive changes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polylogue/maintenance/rebuild_index.py (1)
2174-2226: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not record control-flow exceptions as attestation failures.
Lines 2176 and 2206 catch
BaseException. AKeyboardInterrupt,SystemExit, orasyncio.CancelledErrorduring receipt persistence is converted intopromoted-attestation-failedbefore it propagates. The operation state then changes because of process control flow, not an attestation failure.Catch
Exceptionin both attestation handlers and their recovery handlers. Let control-flow exceptions propagate without transaction mutation.🤖 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 `@polylogue/maintenance/rebuild_index.py` around lines 2174 - 2226, Update both attestation persistence handlers around _persist_candidate_receipt and save_pass_receipt to catch Exception instead of BaseException, and likewise narrow their nested recovery handlers to Exception. Preserve the existing checkpointing for ordinary attestation and recovery errors while allowing KeyboardInterrupt, SystemExit, and cancellation exceptions to propagate without mutating transaction state.
🤖 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/schema_inference_gate.py`:
- Around line 1883-1894: Update the comparison in the candidate-readiness flow
around verified_snapshot and recorded_blob_snapshot to ignore the verifier
field, since fallback and receipt evidence use different verifier methods.
Compare only the semantic blob-evidence fields, including entries, digest, and
verification status, while preserving the existing integrity-failure error
handling.
In `@tests/unit/maintenance/test_schema_inference_gate.py`:
- Around line 540-555: The test
test_inventory_change_detector_triggers_rehash_without_changing_content_identity
should wrap gate._external_inventory after creating the inventory token, touch
session.jsonl, and validate using the token. Assert the wrapped inventory
function is invoked exactly once, while preserving the existing successful
validation and canonical digest assertions.
---
Outside diff comments:
In `@polylogue/maintenance/rebuild_index.py`:
- Around line 2174-2226: Update both attestation persistence handlers around
_persist_candidate_receipt and save_pass_receipt to catch Exception instead of
BaseException, and likewise narrow their nested recovery handlers to Exception.
Preserve the existing checkpointing for ordinary attestation and recovery errors
while allowing KeyboardInterrupt, SystemExit, and cancellation exceptions to
propagate without mutating transaction state.
🪄 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: e93da70f-1bfa-4cf1-82b0-0fb6058089ee
📒 Files selected for processing (9)
polylogue/daemon/bulk_rebuild.pypolylogue/maintenance/rebuild_index.pypolylogue/maintenance/schema_inference_gate.pypolylogue/storage/index_generation.pytests/infra/rebuild_receipt.pytests/unit/maintenance/test_rebuild_index_phase_timing.pytests/unit/maintenance/test_rebuild_index_provenance_gate.pytests/unit/maintenance/test_rebuild_index_resume_correctness.pytests/unit/maintenance/test_schema_inference_gate.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30c5ff814a
ℹ️ 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".
| "active generation post-promotion attestation checkpoint failed: " | ||
| f"{type(recovery_error).__name__}: {recovery_error}" | ||
| ) | ||
| raise |
There was a problem hiding this comment.
If the initial promoted checkpoint fails before replacing the transaction file and this recovery block also fails (for example, both writes hit a transient full disk), the pointer has already been flipped but the transaction remains ready. The next daemon tick treats that record as resumable, fails because its generation is active, then can become permanently stuck trying to discard an active candidate. Preserve enough state to reconcile the active generation on the next resolver pass rather than leaving this double-failure path with a resumable transaction.
Useful? React with 👍 / 👎.
Problem: a double failure while persisting the post-promotion attestation could leave an already-active generation represented by a resumable transaction. Candidate readiness also compared verifier provenance as if it were blob content identity, and the metadata-detector regression did not prove that rehashing occurred. What changed: reconcile active generations during daemon transaction resolution, compare semantic referenced-blob evidence independently of verifier method, and add restart, verifier-equivalence, and inventory-rehash regression coverage. Compatibility/migration: no schema or operator workflow changes.
|
circleci run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719a187e30
ℹ️ 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 not inventory_token and receipt_token: | ||
| active_token = receipt_token |
There was a problem hiding this comment.
When external-file metadata changes but the bytes remain identical, the authoritative rehash succeeds, yet this restores the detector captured when the receipt was created rather than the detector just observed. The next provenance.validate() therefore sees the same metadata mismatch and hashes the entire external corpus again; because rebuilds validate at many checkpoints, a single touch or content-identical replacement can cause repeated multi-GiB scans for the rest of the pass. Return an updated token containing the current detectors after a successful full rehash.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 719a187e30
ℹ️ 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 not inventory_token and receipt_token: | ||
| active_token = receipt_token |
There was a problem hiding this comment.
When external-file metadata changes but the bytes remain identical, the authoritative rehash succeeds, yet this restores the detector captured when the receipt was created rather than the detector just observed. The next provenance.validate() therefore sees the same metadata mismatch and hashes the entire external corpus again; because rebuilds validate at many checkpoints, a single touch or content-identical replacement can cause repeated multi-GiB scans for the rest of the pass. Return an updated token containing the current detectors after a successful full rehash.
Useful? React with 👍 / 👎.
Problem: a metadata-only detector change forced a successful full inventory rehash but returned the stale detector token, causing every later receipt checkpoint to rescan the same corpus.\n\nWhat changed: return current per-origin detector bindings from the external digest calculation and refresh the pass token after a successful validation. Add a regression proving the next validation reuses the refreshed token.\n\nCompatibility/migration: token schema and receipt identity remain unchanged; only the in-memory token returned by validation is refreshed.
|
circleci run |
|
Codex findings 3731416371 and 3731416449 are addressed in 13a52ab. A successful metadata-triggered rehash now returns a refreshed per-origin detector binding, and the regression validates that the next receipt validation performs no second inventory scan. Focused schema-inference tests: 24 passed. Quick verification: all 24 steps passed. No live or candidate archive operation was performed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13a52ab4c2
ℹ️ 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".
|
|
||
| def validate(self, *, verify_blob_integrity: bool = False, refresh_blob_integrity: bool = False) -> None: | ||
| cached_snapshot = None if refresh_blob_integrity else self.verified_blob_integrity_snapshot | ||
| validated = _validate_rebuild_provenance_receipt( |
There was a problem hiding this comment.
When external-file metadata changes during a rebuild but the authoritative rehash finds identical content, validation returns an updated external_ground_truth_inventory_token, but this method only extracts the blob snapshot and leaves self.external_inventory_token unchanged. Every later checkpoint therefore sees the same stale detector and rehashes the entire external corpus again, potentially repeating multi-GiB scans throughout the pass. Fresh evidence after the earlier token-refresh fix is that the refreshed token is now returned by the validator but is discarded at this call site.
Useful? React with 👍 / 👎.
| ) | ||
| transaction_created_here = True | ||
| if transaction.status in {"promoted", "stale"}: | ||
| if transaction.status in {"promoted", "promoted-attestation-failed", "stale"}: |
There was a problem hiding this comment.
If an offline resumable rebuild flips the active pointer and both the normal and recovery transaction checkpoints fail, the persisted transaction remains ready; a later invocation with its operation ID passes this status check and then rejects the now-active generation as a lost inactive candidate, leaving the operation permanently unresumable. Fresh evidence beyond the earlier daemon-focused report is that _reconcile_active_generation_transaction is only called by the daemon's well-known resolver, so operator-created transaction IDs still have no equivalent recovery path.
Useful? React with 👍 / 👎.
Problem: the rehash regression proved scan reuse but did not assert that the returned detector binding changed.\n\nWhat changed: assert the refreshed token carries the new detector before proving the next validation avoids another inventory scan.\n\nVerification: the targeted inventory-detector test passed.
1 participant
Summary
Bind resumable index rebuilds to the raw replay evidence, external corpus inventory, current blob bytes, and terminal promotion attestation. Resolve relative schema receipt references before daemon requests.
Problem
The prior receipt snapshot omitted replay-affecting authority and parser inputs. Repeated checkpoint validation could rehash the full external corpus, source.db hashes did not prove current blob bytes, and a post-pointer failure could leave lifecycle state looking resumable.
Solution
rebuild_source_evidence_snapshotand the replay closure now include capture mode, revision and predecessor authority fields, revision authority evidence, and parser/lowering fingerprints. A receipt-identity-bound inventory token reuses the first full inventory while checking metadata cheaply, then recalculates on detector change. Referenced source blobs are verified throughBlobStorebefore candidate readiness. Promotion completes all fallible checks before the pointer flip and records promoted or promoted-attestation-failed state throughIndexGenerationStorewithout post-flip admission validation. The CLI resolves relative receipt paths with archive-root containment enforcement.Production-route anti-vacuity tests cover review findings 3724479717, 3724479723, 3724479730, 3724479731, and 3724479738.
Verification
devtools test tests/unit/maintenance/test_schema_inference_gate.pydevtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py tests/unit/maintenance/test_rebuild_index_resume_correctness.py tests/unit/maintenance/test_rebuild_index_phase_timing.pydevtools test tests/unit/cli/test_archive_maintenance_cli.py -k 'schema_inference or receipt'\n-devtools verify --quick\nAll commands passed.
Carrier disposition\n\nImplementation-complete for this packet. Successor
polylogue-live-operation-receiptsremains applicable for later live-operation receipt work and is intentionally not closed. No live reindex was run.\n\nRef polylogue-q4qpl.Summary by CodeRabbit
New Features
Bug Fixes