test(maintenance): bind rebuild ownership fixtures to receipts - #3865
Conversation
Problem: successful external-ground-truth rehashes refreshed the validator result but not the in-pass token, and offline operation-id admission could resume a transaction whose generation was already active after interrupted promotion attestation. What changed: retain the refreshed inventory token in RebuildProvenanceContext, extract the existing active-generation reconciliation into the maintenance rebuild engine, and use it from both offline and daemon transaction resolution. Add real-route coverage for inventory scan reuse and terminal offline recovery. Compatibility/migration: receipt schemas, typed reason codes, operation identifiers, and production write behavior are unchanged.
Problem: the mandatory schema-inference receipt gate made six existing ownership tests stop before exercising their intended preflight or lock behavior.\n\nWhat changed: file-backed fixtures now create identity-matching blobs, initialize the tiers required by the raw-frontier gate, and pass a valid receipt into the rebuild route.\n\nVerification: devtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py tests/unit/maintenance/test_rebuild_index_ownership.py\n\nRef polylogue-q4qpl.
📝 WalkthroughWalkthroughThe rebuild flow now refreshes external inventory evidence and persists it across rebuild states. It reconciles resumed transactions whose generation is already active. Bulk rebuilds reuse shared terminal-status and reconciliation logic. Tests add receipt-backed fixtures and regression coverage. ChangesRebuild reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RebuildOperation
participant RebuildProvenanceContext
participant ExternalInventory
participant TransactionStore
participant ReceiptStore
RebuildOperation->>RebuildProvenanceContext: validate rebuild evidence
RebuildProvenanceContext->>ExternalInventory: refresh inventory token
ExternalInventory-->>RebuildProvenanceContext: return refreshed evidence
RebuildProvenanceContext->>TransactionStore: persist consumed evidence
RebuildProvenanceContext->>ReceiptStore: persist receipt evidence
ReceiptStore-->>RebuildOperation: return updated rebuild receipt
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 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/rebuild_index.py`:
- Around line 226-245: Update the terminal recovery flow around the generation
and active-pointer checks to also require generation.owner_id to equal
transaction.generation_owner_id before calling store.checkpoint_transaction.
Preserve the existing early return for mismatched generation state or active
pointer, and return the transaction unchanged when ownership does not match.
🪄 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: eca4b198-1b08-4d00-b888-162ce89796c5
📒 Files selected for processing (4)
polylogue/daemon/bulk_rebuild.pypolylogue/maintenance/rebuild_index.pytests/unit/maintenance/test_rebuild_index_ownership.pytests/unit/maintenance/test_rebuild_index_provenance_gate.py
Problem: the raw-failure fixture now satisfies schema-inference and raw-frontier prerequisites before asserting lifecycle classification, but that ordering was implicit.\n\nWhat changed: document the fixture contract at the test boundary.\n\nVerification: devtools test tests/unit/maintenance/test_rebuild_index_ownership.py::test_rebuild_source_preflight_rejects_unexplained_raw_failure\n\nRef polylogue-q4qpl.
![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: ff058ca368
ℹ️ 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".
| monkeypatch.undo() | ||
| with pytest.raises(RuntimeError, match="promoted-attestation-failed; start a new operation"): | ||
| rebuild_index_from_source_sync( | ||
| RebuildIndexRequest( |
There was a problem hiding this comment.
This added offline resume already invokes the shared reconciler and persists promoted-attestation-failed, so the subsequent daemon resolver only takes its terminal fast path and never executes the daemon's new _reconcile_active_generation_transaction call. Deleting or bypassing that daemon call would therefore leave this test green even though a daemon restart would resume an already-active generation indefinitely. Keep separate offline and daemon recovery scenarios, or restore the transaction to ready before invoking the daemon resolver.
Useful? React with 👍 / 👎.
Problem: an interrupted transaction could checkpoint an active generation whose owner no longer matched the transaction record.\n\nWhat changed: require the generation owner and transaction owner to match before recording promoted-attestation-failed, with a real-route regression test.\n\nVerification: .venv/bin/python -m devtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py tests/unit/maintenance/test_rebuild_index_ownership.py (50 passed).\n\nRef polylogue-q4qpl.
Document that interrupted-promotion reconciliation requires the transaction and active generation owners to match.\n\nRef polylogue-q4qpl.
Problem: resumed rebuild validation could retain a refreshed external inventory token only in an ephemeral context, and an invalid receipt could stale a transaction whose owned generation was already active. The daemon recovery test also passed through the offline terminal path instead of exercising resolver recovery.\n\nWhat changed: propagate refreshed inventory evidence through the pass context and active validator, reconcile owned active generations before stale classification, and reset the fixture to the interrupted ready state before invoking daemon recovery.\n\nVerification: 50 focused maintenance tests and devtools verify --quick.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1d664b948
ℹ️ 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".
| recovery_store = IndexGenerationStore(owned.location) | ||
| existing = recovery_store.load_transaction(request.operation_id) | ||
| existing = _reconcile_active_generation_transaction(recovery_store, existing) |
There was a problem hiding this comment.
When a receipt is invalid or expired and the supplied operation_id is missing or its transaction JSON is unreadable, this unguarded load raises FileNotFoundError, JSONDecodeError, or TypeError instead of the original RebuildProvenanceError. The existing _mark_rebuild_transaction_stale_after_provenance_failure path deliberately catches these load failures, attaches them as notes, and preserves the gate error, but this new pre-reconciliation load bypasses that handling; CLI or HTTP callers therefore receive an unrelated low-level failure (typically a 500 on HTTP) precisely when receipt admission should report the controlled provenance rejection. Catch transaction-load failures here and fall through to the existing guarded stale-marking path.
Useful? React with 👍 / 👎.
Problem: automated review found recovery and inventory-refresh paths that could lose ownership or refreshed evidence when provenance admission failed.\n\nWhat changed: carry refreshed inventory tokens into non-resumable rebuild validation, reconcile active generations without losing the original provenance error when transaction state is unreadable, and test both paths.\n\nVerification: focused provenance tests passed 4; devtools verify --quick passed all 24 steps.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8bedb08538
ℹ️ 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".
| existing = _reconcile_active_generation_transaction(recovery_store, existing) | ||
| if existing.status in _REBUILD_TERMINAL_NOT_RESUMABLE: | ||
| raise initial_provenance_error |
There was a problem hiding this comment.
When the first receipt validation succeeds but the external corpus changes before the lease-bound validation at lines 1401–1408, this reconciliation branch is skipped and the catch instead marks the transaction stale. If a previous process already flipped the pointer but left the transaction ready, that misclassifies an active generation and prevents later reconciliation. Fresh evidence beyond the earlier initial-admission report is this final ownership-bound revalidation path; reconcile the transaction there before writing stale as well.
Useful? React with 👍 / 👎.
| generation = store.load(transaction.generation_id) | ||
| active_path = store.active_pointer.resolve(strict=True) | ||
| generation_path = Path(generation.index_path).resolve(strict=True) | ||
| except (FileNotFoundError, OSError, ValueError): |
There was a problem hiding this comment.
When receipt admission fails and the transaction JSON is readable but its referenced generation metadata has a missing field, an unexpected field, or a non-path index_path, store.load() or Path(...) raises TypeError, which this new recovery helper does not catch. That exception escapes from the pre-stale reconciliation call and replaces the intended RebuildProvenanceError, bypassing the guarded stale-marker path. Fresh evidence beyond the existing unreadable-transaction cases is malformed generation metadata; treat these decoding/type failures as an unreconcilable record and preserve the original provenance rejection.
Useful? React with 👍 / 👎.
Problem: rebuild recovery could retire an active owned generation after a receipt failure, while checkpoint and pass-receipt validation depended on ambient inventory-token state. What changed: make the pass provenance context the explicit validation carrier for transaction creation, checkpoints, and persisted receipts. Reconcile active owned generations before either stale-retirement path. Separate daemon recovery from offline recovery in real-route tests. Verification: devtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py (43 passed); devtools verify --quick (passed). Co-Authored-By: Codex <noreply@openai.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0968823a4
ℹ️ 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 Exception: | ||
| return False | ||
| transaction = _reconcile_active_generation_transaction(store, transaction) | ||
| return transaction.status in _REBUILD_TERMINAL_NOT_RESUMABLE |
There was a problem hiding this comment.
When an offline resumable transaction is already failed and its receipt expires or drifts before a retry, this returns true solely because failed is in the daemon-oriented terminal set, so both admission-failure branches skip _mark_rebuild_transaction_stale_after_provenance_failure. Offline rebuilds intentionally allow failed transactions to resume (the owned body excludes failed from its terminal check, and rebuild_status recommends retrying them), while the stale marker intentionally converts every status except promoted/stale states. The rejected operation therefore remains advertised as retryable despite losing valid provenance; only suppress stale retirement when reconciliation established an active promotion, not merely because the status is failed.
Useful? React with 👍 / 👎.
Carry the latest receipt-bound inventory evidence through transaction checkpoints and pass receipts, including non-resumable rebuilds. Preserve provenance failures when recovery metadata is malformed and reconcile active generations before stale retirement.
Refresh the commit-bound CI receipt after correcting the ready PR scope carrier. The empty trigger commit carries no product diff and will disappear in the squash merge.
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 `@tests/unit/maintenance/test_rebuild_index_provenance_gate.py`:
- Around line 1104-1137: Make
test_active_generation_reconciliation_requires_transaction_owner_match establish
valid generation and active-pointer paths before calling
_reconcile_active_generation_transaction: ensure root/index.db exists and
configure store.active_pointer to resolve to the same generation path. Keep the
transaction and generation owners mismatched so the owner comparison is the only
condition causing the unchanged transaction result.
🪄 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: 8834e251-78dc-4df4-a42b-2b7e8a7a58bc
📒 Files selected for processing (4)
polylogue/maintenance/rebuild_index.pypolylogue/storage/index_generation.pytests/unit/maintenance/test_rebuild_index_ownership.pytests/unit/maintenance/test_rebuild_index_provenance_gate.py
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.
Trigger CI after the ready PR carrier was regenerated for the branch head. This metadata-only trigger has no product diff and disappears in the squash merge.
Problem: the owner-mismatch regression test also passed when unrelated path or state guards returned early.\n\nWhat changed: assert the active generation state and pointer target before exercising reconciliation, so the owner comparison is the only early-return condition under test.\n\nVerification: focused provenance test passed.
1 participant
Summary
Bind the existing offline rebuild ownership tests to the mandatory schema-inference receipt contract and keep each test focused on its intended preflight or ownership invariant.
Problem
The q4qpl provenance hardening made receipt validation mandatory, which caused six inherited ownership tests to fail before reaching their intended assertions. The failures were fixture omissions, not evidence that the receipt gate should be weakened.
Solution
Ref polylogue-q4qpl. The file-backed fixtures now create identity-matching blob bytes, initialize the index and ops tiers required by raw-frontier readiness, commit fixture rows before receipt creation, and pass the receipt into the rebuild route. Recovery reconciliation also verifies that the active generation owner matches the transaction owner before recording a terminal attestation failure. No production behavior or receipt validation was relaxed.
Verification
.venv/bin/python -m devtools test tests/unit/maintenance/test_rebuild_index_provenance_gate.py tests/unit/maintenance/test_rebuild_index_ownership.py50 passeddevtools verify --quick: exit0devtools workspace pr-scope check-ci --expected-head-sha 143deb9ad9f9a9c460064ad5b29ced6abc81329b: exit0Scope disposition
Implementation-complete for the fixture residual and ownership-mismatch recovery guard. Production migration, candidate construction, promotion, and live proof remain outside this PR.
Summary by CodeRabbit
Bug Fixes
Reliability