Fix: refuse corrupt partial segment seals during recovery (#171) - #172
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (36)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-07-29T05:54:58.524ZApplied to files:
🪛 LanguageTooldocs/testing-evidence/partial-seal-corruption.md[grammar] ~15-~15: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🔇 Additional comments (9)
Summary by CodeRabbit
WalkthroughThe recovery classifier now validates available fixed fields in incomplete version-one segment seals. It returns typed seal errors for observed framing contradictions instead of classifying those inputs as truncation. Tests and fuzzing cover the refusal and retain a version-2 counterexample. ChangesPartial-seal recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change appears ready to merge after normal checks. It refuses observed corruption in incomplete seals without changing canonical truncation behavior. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change prevents damaged data from being treated as safely discardable. The demonstrated behavior is narrowly scoped and strengthens protection against deletion, but full failure and concurrent-operation behavior has not been established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 19.05% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 9 files. (27 skipped: 27 unsupported.)
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. A short seal shows its fields in view Comment |
Verified acceptance blockers at f94e418
These are static/evidence blockers, not newly demonstrated product defects. @codex |
|
To use Codex here, create an environment for this repo. |
Independent adversarial review — PR #172Reviewed in an isolated checkout at exact pushed head FindingsP2 — Exact diagnostics/source preservation and canonical-truncation assertions lack direct falsification evidence. P2 — Trailing whitespace in fuzz receipts fails required documentation CI. No additional concrete production-correctness violation was found within the approved scope. Verification Checklist
Execution and remaining gatesExecuted only read-only source/Git/GitHub inspection, raw-log comparisons, fixture-byte verification and the whitespace check. No host Rust/Python, product tests, mutation replay, configuration changes, external comments, source edits or delegation. The report is the only written artifact. The active copied final source/target tree was not modified or used for tests. Final-head dependency policy passed; documentation/workflow integrity failed as above; Rust quality and runtime fuzz smoke remained in progress at inspection. The local REQUEST CHANGES — |
Independent adversarial delta review — PR #172Reviewed in an isolated checkout at exact pushed head FindingsBoth prior P2 findings are closed. The new mutation receipts directly reach the exact diagnostic/source and canonical-truncation assertions; receipt whitespace now passes the exact required check. The additional seed-materialization law verifies the emitted counterexample through Keep rather than asserting corpus cardinalities, and its named-input, selector and diagnostic outcomes have direct runtime calibration. No verified remaining source or scoped acceptance blocker was found. Verification Checklist
Execution and remaining gatesExecuted read-only Git/source/receipt inspection, patch applicability, exact whitespace check and live GitHub queries. No host tests, mutation execution, Python, source changes, configuration changes, external comments, delegation or merge. Only this report was written. The running final copied source/build tree was not touched. Inspected restored debug/release and Clippy receipts for both the original public laws and the replacement materialization law. Structure/format execution remains author-supplied evidence rather than a reviewer rerun. Final This source/evidence approval does not establish universal future-prefix feasibility, all recovery fault schedules, actual deletion preservation, physical power loss, or complete ambient isolation. Required final checks, later review feedback and human merge authorization remain separate gates. APPROVE — |
Activity Summary — ready candidate 07e6cc4
Independent Codex review using the agy-review protocol approves this exact head with a complete Verification Checklist: #172 (comment). All four binding hosted jobs pass on this head: https://github.com/flyingrobots/keep/actions/runs/37097418652. The copied-Docker full local chain also passes: formatting, structure, conformance/worldline, both feature Clippy profiles, debug/release workspace tests, doctests/docs, and debug/optimized production process-death campaigns. Earlier failures remain recorded; they are not retried into evidence of unchanged correctness. The final review queue has no inline threads, active changes-requested reviews or unresolved verified findings. CodeRabbit skipped the draft and is not counted as approval; marking ready may trigger additional review, which will be reconciled. Human merge authorization remains separate. No merge performed. Separate audit issue #173 addresses an existing duplicate-identity invariant bypass across incomplete tails. It is outside this fixed-framing correction and is not presented as solved by this PR. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Independent Code Lawyer landing review — PR #172Repository: This is a fresh, independent, read-only Code Lawyer review using the complete agy-review protocol, performed by the user's authorized Codex substitute. It reviews the entire 36-file base-to-head change. Previous approvals were treated as claims, not transferred to this verdict. FindingsNo verified remaining P0–P5 defect or PR-scoped acceptance blocker was found. Both previous P2 findings are closed by independently inspected evidence: exact error/source and canonical-truncation assertions now have direct runtime calibration, and the required repository whitespace check passes. The existing duplicate-identity/incomplete-tail defect tracked by #173 remains outside this correction. This review does not represent it as fixed or certify the entire recovery subsystem. The changed validator establishes the promised available-fixed-framing refusal; it does not establish canonical future completion for variable coordinates, digests, or checksums. Verification ChecklistExact source, history, and merge audit
Every runtime path and boundary
Regression, generated family, fuzz, and tool evidence
Raw receipts, calibration, and every changed numerical claimAll 14 checked-in
Repository rules, complete feedback, and landing gates
Executed versus inspected scopeExecuted by this reviewer: read-only source/Git/GitHub inspection, raw-to-checked-in receipt comparisons, fixture-byte verification, patch applicability checks, base-to-head and empty-tree-to-HEAD whitespace checks, merge-parent comparisons, and exact-head/CI/feedback queries. Only this scratch report was written. No source change, commit, push, configuration change, external comment, merge, or delegation occurred. Inspected rather than independently rerun: supplied copied-Docker RED/GREEN/calibration/fuzz logs and No host Rust tests or benchmarks were run. This reviewer did not rerun mutations or launch another container campaign. Static inspection and inspected author/landing execution receipts are distinguished throughout. Unrun physical power-loss, actual corrupt-stage unlink, allocation benchmarks, universal future-prefix feasibility, all recovery schedules, and a complete subsystem audit are neither claimed nor required to substantiate this bounded read-only framing correction. The reviewed source/evidence gate is satisfied for the exact head above. Merge authorization and immediately pre-merge head/protection verification remain the owning agent's responsibility; this report does not authorize bypassing any protection or attest a merge that has not occurred. APPROVE |
Code Lawyer landing pass — candidate 07e6cc4Target reviewed: main
Fresh landing inspection covers the complete diff, surrounding full seal decoder/admission, partial-field comparator, classifier, assessment, resume and discard planning paths, typed sources, fixtures, fuzz route and seed materialization. No merge commits occur in the four-commit PR history. No write protocol, persistent format, variable-field completion proof or #99 v2 disposition change is introduced. All global conversation comments, review bodies and inline threads were fetched with pagination exhausted. Existing independent findings are accounted for above. Current CodeRabbit review is APPROVED on this exact head; there are no changes-requested reviews or inline threads. Hosted Codex's usage-limit message is not review evidence. The fresh copied-Docker tree is clean and has tree The prior complete copied-Docker validation and all four successful current-head hosted jobs remain valid for this unchanged source: CI run 37097418652. The fresh focused run supplements them without claiming another broad crash/fuzz campaign. Current required signature and branch-history rules were inspected; normal signed GitHub merge is the authorized route. Independent fresh review is posted separately and must approve this exact head before merge. |
Mainline integration verifiedMerged normally as The actual mainline CI run 37146130023 completed successfully: Rust quality gates, Documentation and workflow integrity, Runtime fuzz smoke, and Dependency policy all pass. This is post-merge evidence for the integration SHA, not a transfer of the earlier candidate's green status. Issue #171's scoped correction is delivered. Separate #173 and the remaining completed-roadmap audit remain open. |
Problem and result
Closes #171 under audit #131. Version-one recovery previously treated a recognized but incomplete seal as discardable truncation even when its available version bytes contradicted the format. The classifier now checks available fixed framing and preserves precise typed seal refusals through fingerprint-bound assessment, before any discard request can be constructed.
Change kind: bug fix, with a seed-test oracle correction. Current candidate:
07e6cc4875c05592b71bb1f8b9c80631a31bcda5. Independent branch from origin/main6051abb25a9fd33ae7ee0de5614514b709a4d82a; no unmerged feature dependency.Invariant and approach
KEEP-RECOVERY-010 requires demonstrated corruption to remain a refusal. A bounded, allocation-free seal-prefix validator checks version, flags, seal length, reserved fields and algorithms using existing format constants.
RecoverySegmentStageError::Sealpreserves the original seal cause. Complete-seal decoding and the separate v2 incomplete-stage disposition contract remain unchanged.Missing bytes are used only in temporary fixed-field comparisons, never admitted as evidence. This does not claim future completion feasibility of every variable coordinate. Rejected alternatives: unconditional discard after magic recognition, relabeling every partial stage corruption, and expanding this repair into a new storage format or disposition scheme.
Evidence and remaining acceptance
The two public regressions are committed RED separately at
1b6da984ece52e94777fe1b52dcf1bb6b82c3960, and pass with the fix. The fixed-framing family sweep checks precise refusals across short boundaries and retains typed truncation when the mutated byte is absent. New and adjacent classification/assessment/discard laws pass debug/release in copied Docker trees. Formatting, source structure, all-feature Clippy and Markdown checks pass.Checked-in evidence and actual receipts distinguish runtime classification from static immutable-borrow preservation; no actual unlink or power-loss experiment is claimed.
The parser fuzz/corpus extension is delivered in
f94e4188a715a5962d55343538efa648acefaadf: a retained reduced counterexample is replayed by ordinary tests and the registered segment target; the independent semantic oracle fails on main and passes with the fix, followed by a seeded bounded ASan campaign. Full local validation, all four hosted CI jobs and independent review pass on07e6cc4. The PR is ready; human merge authorization remains separate. T-13.2's remaining audit obligations are not declared complete.Compatibility and operational implications
No persistent format, identity, dependency, namespace or write protocol changes. The public error enum gains a typed incomplete-seal variant; exhaustive consumers may need to handle it. Previously admitted contradictory stages now block discard, preserving corrupt evidence. Canonical incomplete stages retain their previous truncation behavior. No performance improvement is claimed; the new checks examine a fixed bounded frame and perform no allocation or I/O. Security impact is refusal of malformed retained input before discard authorization.
Review remediation
The independent review of f94e418 found two acceptance blockers, both addressed in 07e6cc4: line-end whitespace in raw fuzz receipts and missing direct error/coordinate assertion calibration. Checked-in mutation patches and runtime RED/GREEN receipts now calibrate diagnostic values, source preservation and canonical-truncation coordinates. The full suite also exposed frozen seed-count assertions; these are replaced with actual emitted-counterexample replay through Keep and exact typed refusal, while deterministic materialization remains checked. Its emitted-input assertions have their own bounded calibrations. The full local chain passed on 07e6cc4, including debug/release workspace tests, both feature Clippy profiles, conformance/worldline checks, doctests/docs and both debug/optimized process-death campaigns. Independent delta review approves this exact head with its complete checklist. All four hosted jobs pass on exact-head run 37097418652. No earlier CI is transferred.