test(reindex): harden Codex 804 recovery proof - #3874
Conversation
📝 WalkthroughWalkthroughThe PR strengthens Codex 804 live-proof and rebuild-resume tests. It adds source remediation, fixed timestamp checks, durable interruption coverage, exact suffix replay validation, receipt propagation, and final authority, ledger, membership, and census conservation assertions. ChangesCodex 804 recovery validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 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 |
|
@codex review |
|
|
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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: The Codex 804 proof did not preserve baseline message timestamps explicitly, its interruption wait observed only post-checkpoint progress, and recovery accepted permissive authority and membership outcomes. The focused resume regression also lacked the current schema-inference receipt gate. What changed: Preserve and compare baseline timestamps across four revisions. Add a pre-checkpoint subprocess boundary with transaction and receipt assertions, retain the committed-page boundary, and trace production restart selections to prove exact suffix replay. Require byte-authority conservation for all 804 raws, complete application coverage, and one terminal head. Supply valid receipts to the maintenance parity fixture. Verification: direnv exec . devtools test tests/unit/scenarios/test_codex_804_live_proof.py (1 passed in 276.97s); direnv exec . devtools test tests/unit/maintenance/test_rebuild_index_resume_correctness.py (1 passed in 7.41s); direnv exec . devtools verify --quick (exit 0). Disposition: implementation-complete; live-proof-pending. Successor: polylogue-codex-804-live-proof.
df07f6b to
9d81e77
Compare
![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: 9d81e774fb
ℹ️ 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".
| resumed_raw_pages = tuple( | ||
| tuple(json.loads(line)) for line in selection_trace_path.read_text(encoding="utf-8").splitlines() | ||
| ) | ||
| assert resumed_raw_pages, "restart observed no production replay selections for the suffix" | ||
| resumed_raw_ids = {raw_id for page in resumed_raw_pages for raw_id in page} |
There was a problem hiding this comment.
If restart mistakenly schedules any uncommitted raw more than once, converting every traced page into resumed_raw_ids silently removes the duplicate, so both suffix assertions still pass. This trace also wraps rebuild_selection_evidence, which is called before replay and again during terminal readiness for the final page, rather than the actual replay call; consequently it cannot distinguish legitimate evidence recomputation from duplicate replay. Record the production replay/page-selection calls with multiplicity so the claimed exact-once suffix recovery is actually checked.
Useful? React with 👍 / 👎.
| precheckpoint_process = subprocess.run( | ||
| [sys.executable, "-c", precheckpoint_script, str(root), operation_id, str(schema_inference_receipt_path)], | ||
| cwd=Path.cwd(), | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| timeout=120, |
There was a problem hiding this comment.
On runners only modestly slower than the verification machine, this additional full production replay can make the test exceed its unchanged 300-second pytest timeout: the recorded verification already took 280.84 seconds, leaving less than 20 seconds of headroom, and this new subprocess has its own 120-second deadline. That makes the incident-scale test prone to environment-dependent timeout failures even when recovery is correct; raise/rebalance the enclosing timeout or avoid performing the page twice.
Useful? React with 👍 / 👎.
| for revision, terminal in ((0, False), (1, False), (800, False), (803, True)): | ||
| assert _baseline_message_timestamps(_codex_payload(revision, terminal=terminal)) == ( | ||
| _BASELINE_MESSAGE_TIMESTAMPS | ||
| ) |
There was a problem hiding this comment.
If the Codex parser or revision-application path rewrites an inherited message's timestamp while materializing a later revision, this assertion still passes because _baseline_message_timestamps examines only the synthetic raw input produced by _codex_payload. The final candidate checks do not compare the baseline messages' occurred_at_ms values against these constants, so the production timestamp-drift regression this change claims to cover remains undetected; assert the persisted candidate timestamps for the baseline message IDs after recovery.
Useful? React with 👍 / 👎.
| assert len(application_rows) == REVISION_COUNT | ||
| assert {str(row[0]) for row in application_rows} == raw_ids | ||
| assert {str(row[1]) for row in application_rows} <= {"selected_baseline", "applied_append", "superseded"} | ||
| assert all(row[2] is not None for row in application_rows) |
There was a problem hiding this comment.
If recovery writes an orphan or otherwise incorrect accepted_raw_id into the application ledger while independently maintaining the correct raw_revision_heads row and materialized session, all of these new assertions still pass because they require only a non-null value; raw_revision_applications.accepted_raw_id has no foreign key enforcing the relationship. Downstream retention authority relies on these application references, so the proof should require every accepted ID to belong to raw_ids and validate the expected chain/head relationships, not just row coverage and decision vocabulary.
Useful? React with 👍 / 👎.
Reissue the exact carrier head after rebasing onto current master.
Problem: the Codex 804 proof observed a helper-level selection calculation instead of the production replay path, and its fixture let source remediation occur after the candidate receipt was frozen. What changed: run the production source remediation route in an isolated archive before freezing the candidate receipt, instrument the actual replay seam across resumed pages, assert terminal raw-head ownership, and verify persisted baseline timestamps in the candidate index. Compatibility/migration: this changes only the incident-scale proof fixture; no production archive or migration behavior is changed. Co-Authored-By: Claude <noreply@anthropic.com>
Problem: the expanded Codex 804 proof exceeded the recorded timeout exception after adding the real source-remediation and replay paths. What changed: update the timeout manifest to the measured 420-second bound and record the source-remediation plus interrupted/resumed replay rationale. Compatibility/migration: test policy metadata only; production behavior is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
Refresh the non-draft PR trigger after the verified Codex 804 proof and exact-head carrier update. No product or test behavior changes.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/scenarios/test_codex_804_live_proof.py`:
- Around line 748-760: Update the timestamp-sensitive test around the persisted
baseline timestamp assertion to accept the frozen_clock fixture and use it when
generating receipt timestamps. Ensure all timestamps written during the scenario
derive from frozen_clock so the persisted values remain deterministic.
- Around line 610-621: Update the assertions around resumed_raw_pages to read
source raw IDs in the existing `(blob_hash, raw_id)` order, verify
committed_page_raw_ids is the prefix of that ordered sequence, and compare the
flattened replay trace directly with the remaining ordered IDs. Remove the
set-based comparison so duplicate or reordered replay IDs cannot pass.
🪄 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: 3642a60f-2221-47f9-aee8-59a90dd667c0
📒 Files selected for processing (3)
devtools/pytest_timeout_overrides.tomltests/unit/maintenance/test_rebuild_index_resume_correctness.pytests/unit/scenarios/test_codex_804_live_proof.py
| expected_baseline_timestamps = tuple( | ||
| int(datetime.fromisoformat(timestamp.replace("Z", "+00:00")).timestamp() * 1000) | ||
| for timestamp in _BASELINE_MESSAGE_TIMESTAMPS | ||
| ) | ||
| with sqlite3.connect(candidate.index_path) as conn: | ||
| persisted_baseline_timestamps = tuple( | ||
| int(row[0]) | ||
| for row in conn.execute( | ||
| "SELECT occurred_at_ms FROM messages WHERE message_id LIKE ? ORDER BY message_id", | ||
| (f"%{SESSION_NATIVE_ID}-message-%",), | ||
| ) | ||
| ) | ||
| assert persisted_baseline_timestamps == expected_baseline_timestamps |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use frozen_clock for this timestamp-sensitive test.
The test now validates persisted message timestamps. Add the frozen_clock fixture and use it for generated receipt timestamps where needed. This prevents clock-dependent behavior.
As per coding guidelines, “Timestamp-sensitive tests must use frozen_clock.”
🤖 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/scenarios/test_codex_804_live_proof.py` around lines 748 - 760,
Update the timestamp-sensitive test around the persisted baseline timestamp
assertion to accept the frozen_clock fixture and use it when generating receipt
timestamps. Ensure all timestamps written during the scenario derive from
frozen_clock so the persisted values remain deterministic.
Source: Coding guidelines
Problem: the Codex recovery proof collapsed resumed replay selections into a set, allowing a duplicated suffix raw to pass if every expected raw eventually appeared. What changed: retain the production replay page order, reject duplicate raw IDs, and compare the resumed sequence with the exact source-order suffix after the committed page. Compatibility/migration: proof-only assertion strengthening; production replay behavior is unchanged. Ref polylogue-27522 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: 8f87d63287
ℹ️ 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".
| precheckpoint_process = subprocess.run( | ||
| [sys.executable, "-c", precheckpoint_script, str(root), operation_id, str(schema_inference_receipt_path)], | ||
| cwd=Path.cwd(), | ||
| capture_output=True, | ||
| text=True, | ||
| check=False, | ||
| timeout=120, |
There was a problem hiding this comment.
When this workload receipt is used as incident-scale resource evidence, the newly added remediation and precheckpoint_process run after the census observation is finalized but before replay_before is sampled, so their wall time, CPU, and storage costs are omitted from every phase even though the receipt reports the full crash/recovery workload as succeeded. Start a phase sample before these operations or fold them into an explicitly measured phase so the receipt does not under-report the recovery route.
Useful? React with 👍 / 👎.
| persisted_before_page = store.load_transaction(operation_id) | ||
| # Mutation that advances the cursor before checkpoint_transaction returns | ||
| # fails these pre-checkpoint invariants and the receipt census below. | ||
| assert persisted_before_page.status == "running" | ||
| assert persisted_before_page.processed_raw_count == 0 | ||
| assert persisted_before_page.last_raw_id is None | ||
| assert persisted_before_page.last_blob_hash_hex is None | ||
| receipt_directory = store.transactions_root / f"{operation_id}.receipts" | ||
| assert not tuple(receipt_directory.glob("pass-*.json")), "a pre-checkpoint kill emitted a false paused receipt" |
There was a problem hiding this comment.
If the replay becomes a no-op while still reaching checkpoint_transaction, or the checkpoint is moved ahead of the candidate commit, this child still exits with 97 and these assertions only confirm that transaction metadata and receipts were not persisted. The following subprocess can then perform the real replay and make every final-state assertion pass, so the test does not actually prove recovery from committed candidate work whose cursor was not checkpointed. Inspect the inactive candidate immediately after this kill and require the first page's materialized state before starting the second replay.
Useful? React with 👍 / 👎.
Problem: The recovery receipt started measuring after source remediation and the pre-checkpoint crash boundary, while the test did not prove that the killed process had already materialized candidate work.\n\nWhat changed: Extend the existing replay phase over source remediation and the crash boundary, and assert the inactive generation contains the first page immediately after the pre-checkpoint kill.\n\nCompatibility/migration: The workload phase contract remains unchanged.\n\nCo-Authored-By: Claude <noreply@anthropic.com>
Problem: The structured PR scope carrier was refreshed after the last proof commit, but the CI status still belongs to the preceding head. What changed: Create a new verification boundary so Circle and automated review evaluate the exact carrier-bound proof head. Compatibility/migration: No production or archive mutation.
## Summary Add a deterministic, private-data-free whale fixture pack for Codex and bind it into the reindex campaign manifest and incident-scale recovery proof. ## Problem The scale-hardening suite lacked one reusable fixture covering the observed 804-revision chain, 90,822,451-byte terminal snapshot, two-million-event stream, and giant inline base64 attachment. The existing 804 proof built recursive payloads in memory, had a stale 420-second timeout for a measured 501-second production route, and ordinary Codex message images were silently dropped even though function-call outputs retained bounded evidence. ## Solution The generator writes revisions and base64 content in bounded chunks, emits a content-bound manifest, and exposes a command-line generation module. The campaign manifest carries dimensions bound to the production ordinary and whale limits. Codex stream parsing now spools distinct decoded records to a private disk replay so lookahead remains bounded in memory. Ordinary inline images persist as typed image blocks with MIME, approximate byte count, and encoded-payload hash without storing data URLs or inflating authored word counts. The 804 proof uses production acquisition and whale admission, source remediation, crash/checkpoint handling, suffix-only resume, inactive candidate construction, promotion, and public/canonical assertions while preserving the timestamp and authority hardening from #3874. ## Verification - devtools test tests/unit/scenarios/test_codex_804_live_proof.py: 1 passed in 566.03s after the full automated-review repair set. - Distinct two-million-event fixture, manifest, streaming parser, and inline-image selection: 6 passed in 45.83s. - Retained-record guard and typed-image parser selection: 2 passed in 1.38s. - Production whale selection and daemon routing: 4 passed in 1.48s. - Codex image and context parser class: 11 passed in 1.17s. - devtools verify --quick: all 24 steps passed at 6b5de09. - devtools lab policy bead-graph: no dependency cycles; exit 1 remains from the repository-wide 220 missing AC and 218 missing structured-contract residual owned by polylogue-n2dmn. ## Residual state The frozen current-master full-suite failure ledger remains under polylogue-93xe; this PR does not claim a green full suite or any live production reindex receipt. Ref polylogue-3hdz2. <!-- polylogue-pr-scope:v1 { "assigned_beads": [ "polylogue-3hdz2" ], "beads_digest": "82c26c7a5c281f90e7d4531be9761a04ad72f16c1fc10b627e16d3a7ec857d56", "dispositions": [ { "bead_id": "polylogue-3hdz2", "disposition": "satisfied", "evidence": [ { "kind": "commit", "ref": "6b5de097f36da2f61e56754ea1609a5b48972c2f" }, { "kind": "test", "ref": "tests/unit/scenarios/test_codex_804_live_proof.py: 1 passed in 566.03s" }, { "kind": "test", "ref": "distinct two-million-event fixture and bounded parser checks: 6 passed in 45.83s" }, { "kind": "test", "ref": "production whale selection and daemon routing: 4 passed in 1.48s" }, { "kind": "test", "ref": "Codex image and context parser class: 11 passed in 1.17s" }, { "kind": "command", "ref": "devtools verify --quick: all 24 steps passed at 6b5de09" } ], "successors": [] } ], "head_sha": "6b5de097f36da2f61e56754ea1609a5b48972c2f", "scope_digest": "b105bdbf7d4a0dc590d87d5682bad3d04cd7c94ff6d80d24aa27be1bafa2717d", "version": 1 } --> --------- Co-authored-by: Codex <noreply@openai.com>
1 participant
Summary
Harden the Codex 804 revision recovery proof so it verifies stable baseline timestamps, interruption before the durable checkpoint, exact suffix replay, and complete raw-authority conservation.
Problem
The existing incident-scale fixture could accept timestamp drift in recursively extended messages, observe only a post-checkpoint interruption, and treat permissive authority or application coverage as sufficient. Those gaps could make a recovery receipt claim stronger continuity than the production route had proved.
Solution
The fixture now preserves and compares baseline message timestamps across all revisions, injects a subprocess exit before the paused checkpoint, verifies that no paused receipt or cursor progress exists at that boundary, traces restart selections to prove committed-page exclusion, and requires all 804 raw revisions to have byte-proven authority and complete application coverage. The existing resumable rebuild route and public/canonical assertions remain in use. This is implementation and test scope only. The live receipt remains a separate acceptance obligation.
Verification
direnv exec . devtools test tests/unit/scenarios/test_codex_804_live_proof.py-> 1 passed in 280.84sdirenv exec . devtools test tests/unit/maintenance/test_rebuild_index_resume_correctness.py-> 1 passed in 7.63sdirenv exec . devtools verify --quick-> all 24 steps exit 0Follow-ups
The production live-proof receipt remains open under
polylogue-codex-804-live-proof. Ref polylogue-27522.Summary by CodeRabbit
Tests
Chores