Fix: validate completed migration namespaces and restart evidence (#111) - #161
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 (24)
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⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used🪛 LanguageToolCHANGELOG.md[grammar] ~11-~11: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) docs/testing-evidence/migration-restart-matrix.md[grammar] ~22-~22: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) [style] ~30-~30: ‘new record’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_NEW_RECORD) [style] ~30-~30: ‘new record’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_NEW_RECORD) [grammar] ~54-~54: Ensure spelling is correct (QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1) 🔇 Additional comments (9)
Summary by CodeRabbit
WalkthroughCompleted migration recovery now verifies version-two namespace admission before reporting success. The storage interface requires a read-only ChangesMigration recovery and restart verification
Priority: ⚪ Not assessed Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue or failing Rust check is established for this head. Normal merge checks can proceed. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change strengthens completed-migration validation without expanding filesystem authority. Invalid reserved entries now prevent success, while lawful retention state remains permitted. Remaining uncertainty concerns downstream storage implementations and behavior outside the documented writer-coordination and restart guarantees. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
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 64.10% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 39 functions across 14 files. (18 skipped: 18 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. The stages line up, or recovery says no Comment |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Lawyer: complete-plan namespace acceptance remains unproved
This is a compiled runtime probe in an isolated source/build copy, with unchanged production code. It changes only the new namespace law's selected directory to The complete fast path returns before adoption/preflight. Nested residue observation checks expected directory kinds/presence, not unknown membership. The documented pre-mutation namespace guarantee for resumed recovery remains supported; this finding does not allege mutation during Complete. It concerns the broader unknown-evidence completion claim and #111 acceptance scope. Do not indiscriminately require empty retention pools on completed stores or reopen #99's separate retention-stage disposition decision. PR #161 remains unmerged pending this obligation and the independent review's finite calibration assessment. @codex — independent confirmation welcome. |
|
To use Codex here, create an environment for this repo. |
Independent ULTRA STRICT review — PR #161Repository: This is the user-authorized independent Codex fallback, applying the mandatory agy-review protocol; it is not represented as an agy execution. Review was read-only in FindingsP2 — KEEP-MIGRATION-005 is marked implemented despite a reproduced completed-restart unknown-entry gapChanged locations: The new unknown-entry law inserts residue after This is reproduced, not a hypothetical hardening request. The parent supplied an isolated copied-source probe that changes only the unknown-entry test's parent list to This production omission predates the topic; the introducing defect in this diff is declaring its broader ambiguity requirement complete without covering this state. The probe establishes successful admission of unknown evidence, not resumed mutation, returned wrong content, or physical durability damage. The existing normative wording that nested membership is checked before mutating recovery ( Required resolution: keep this closure unaccepted until completed-restart admission enforces the applicable canonical namespace and the regression checks the exact refusal and complete preserved witness. A narrow implementation is a read-only P2 — Recorded falsification covers common preservation and four guards, but not the distinct new refusal/diagnostic outcomesChanged location: Five useful calibration families are recorded: root identity, exact-record inode identity, filesystem preservation, nested preflight, and inventory/store-identifier binding. All were verified against the retained mutant source differences and raw runtime failure output. However, the HEAD-deletion mutation makes record checksum and overlong tests fail at the shared preservation assertion ( One complete finite calibration batch is required for these existing distinct promises; no per-byte, per-coordinate, per-matrix-row, or mutation-percentage campaign is requested:
A bounded common-boundary negative control that preserves storage and substitutes an incorrect refusal after the real restart attempt can exercise these diagnostic assertions in one batch, provided each named assertion actually executes and fails for that intended reason. Label such an experiment oracle calibration, not evidence of a production bug or mutation of every production guard. Reuse existing successful root/inode/inventory/preflight/preservation receipts; do not redo them merely for additional counts. Record immutable source/tree, variant or patch, exact commands, runtime RED output, and restored GREEN. Add any regression from the completed-state gap above with its real baseline RED; do not fabricate RED for the original missing-verification-only change. Verification ChecklistEntire topic diff and evidence classification
Every changed runtime contract pathPaths below use
Every merge audited against both parentsBoth-parent changed-file sets and combined diffs inspected. All six mainline PR integration merges below are exact topic-parent trees (zero delta to second parent). Earlier normal merges preserve both change streams; combined resolution changes are documentation only. No migration production file differs between original base and current main, or between original topic head and current head. Hence imported production changes cannot reroute this migration implementation; shared admission/authority invariants still needed explicit checking below.
Imported production invariants checked at current head: partial-seal framing validation at #99 binding decisions inspected in Constants, numeric claims, and documentary dispositions
Review feedback reconciliation
Checks executed, inspected, and unavailableExecuted by this reviewer: read-only Git head/tree/status/log/diff/diff-check; complete13file source inspection; both-parent/combined merge examination; deterministic parent CHANGELOG-entry preservation comparison; queue and pagination inspection; read-only Docker discovery/differences of retained mutation sources and copied completion probe. Inspected execution receipts: historical issue111 focused/full debug/release validations, initial compile/setup failures (excluded as behavioralRED), preserved five calibration families and inventory-only survivor; fresh exact tracked-tree copied Docker validation log and manifest; parent completion probe with runtime failure. Fresh validation log shows commands advancing through full debug/release tests, docs, MSRV and fuzz formatting/check/Clippy, but terminal session status and final hosted checks remain the parent's gate. After inspection, the parent reported terminal session10260 exit0 for the full exact-head chain. This report distinguishes that parent-attested terminal result from its own receipt inspection; it does not infer completion from intermediate output. Dependency/workflow/runtime-fuzz hosted results are not independently attested by this report. Coverage limitations remain explicit: private/repository-only admission isolates migration semantics; in-process drop/reopen is deterministic restart evidence, not process death; process-death campaigns are not physical power-loss evidence; no arbitrary external raw-writer isolation; witness equality alone cannot rule out a transient restored write; ordinary per-test resource ceilings/suite budgets are not established. No parser or production transformer changed, so this topic does not need a fabricated new fuzz or benchmark claim. Imported historical receipts are preserved as their pinned evidence and not relabeled measurements of this exact head. Exact-head verdict
REQUEST CHANGES |
flyingrobots
changed the title
Independent bounded delta review — PR #161Reviewed exact head Both prior finite P2 findings are CLOSED for the reviewed changes and evidence. No new verified defect in this bounded delta was found. Required hosted CI is currently failed and still blocks merging; this review is not a waiver of that gate. Disposition of both prior findingsCompleted restart namespace gap — CLOSED. The actual bug fix adds The permanent regression The lawful-retention law publishes a real retention root, drops publication authority, obtains fresh migration authority, requires Complete with no forward phases, then reads the exact retained bytes through the public snapshot ( Distinct refusal/diagnostic falsification gap — CLOSED. The finite batch implements the previously requested boundary negative controls without changing expectations. Two corrected controls close the earlier masked comparisons. The initial pool control's unused-import compilation failure and initial nested control's earlier-boundary failure were inspected and are excluded from admitted evidence, as the ledger states. Original failure attempts remain in scratch. Four committed RED receipts were independently compared to their raw scratch logs after the documented path normalization/trailing-empty-line handling: all match exactly. The restored GREEN receipt likewise exactly matches its raw log, pins fixed source tree Mandatory Verification Checklist
Exact-head verdict and limits
Merging remains blocked by the failed required hosted reader-fence test and final repository gates. Approval is an independent review disposition, not a clean-CI assertion, flake diagnosis, merge authorization, or risk waiver. Existing private platform-admission, fixed-schedule, resource-ceiling, raw-writer and physical-power-loss limitations remain exactly as scoped in the prior review and ledger. APPROVE |
Code Lawyer activity — source findings closed, CI blocked
MERGE GATE: LOCKED. The two source-review findings are resolved; failed required CI remains the specific blocker. No automatic retention-stage disposition, broader diagnostic redesign, platform isolation or power-loss guarantee is added. CodeRabbit's rate limit is not an approval; the authorized independent review is the source-review gate. Requirements remain candidate-only until integration. |
Independent integration delta review — PR #161Reviewed head No verified integration defect was found. Both earlier finite P2 findings remain CLOSED. The approved migration completion implementation and calibration evidence are unchanged; the merge imports the independently reviewed reader-fixture correction without changing production locking. This review approves this exact head, not a later successor or a waiver of repository gates. Mandatory Verification ChecklistScope and both-parent integration
Every changed path and semantic interaction
Prior findings, regression and calibration evidence
Constants, figures, documentation and review feedback
Execution and final gate boundary
Existing admitted limits remain explicit: untraced original hosted inheritance schedule, arbitrary fork/exec interleavings, physical power loss, hostile noncooperating writers, full GC and fully enforced resource ceilings are not newly established. They neither reopen resolved findings nor justify speculative hardening in this bounded integration review. Reviewed SHA: APPROVE |
Code Lawyer closure — integrated candidate 2f22d0d
Delivered: complete migration namespace refusal before recovery effects, preserved lawful retention state, exact diagnostic and full filesystem restart witnesses. Compatibility: public recovery-port implementors must supply the new required read-only Final integration review separately records exact-head approval and the mandatory both-parent merge/path/evidence checklist. The target is MERGE GATE: OPEN under the maintainer's existing authorization. |
1 participant
Problem and outcome
Migration restart lacked runtime evidence for several ambiguity classes. The expanded matrix also exposed a production defect: a completed migration returned success with an unexpected entry in a reserved namespace. This PR adds exact refusal and preserved-evidence laws, then requires effect-free namespace admission before returning
Complete.Change kind: missing runtime verification plus a demonstrated bug fix. The permanent regression was observed RED on unfixed production in
6351c7c;a3ea6c3fixes it. The weaker entry-count-only corrupt-intent test is replaced by filesystem observations of names, identities, bytes, directories and symbolic-link targets. No test-count assertion is introduced.Invariant and approach
A completed migration must still satisfy its reserved namespace contract.
MigrationRecoveryStorage::verify_completeis a required read-only capability; the filesystem adapter uses existing version-two namespace admission and preserves the typed Observation → NamespacePreflight cause. Planning refusals retain precedence. Lawful published retention state remains admissible and has a real filesystem success law.Using the incomplete-migration empty-retention-pool policy for completion was rejected because it would refuse lawful later retention state. Blind acceptance and automatic cleanup were rejected because they cannot establish the invariant. The #99 incomplete-retention-stage disposition deferral remains unchanged.
Evidence and current gate
The consolidated evidence maps the record, identity, ordering, namespace, immutable-pool and root-authority laws, historical controls, new regression and finite diagnostic calibration batch. Controls exercise the actual restart and unchanged preservation witness before perturbing observed diagnostics. Compilation failures and controls that failed at an earlier assertion are explicitly excluded from their intended calibration claims.
The full required copied-Docker validation chain and hosted checks passed at code candidate
a3ea6c3e2ac58ca82a5c26f43d902ba1ed883be7. Receipt-only successore20629852f402e129d889e398014781d20d6ff0breceived independent APPROVE, closing both original review findings, but its hosted Rust check failed at the existing reader-fence setup withBusy. The first failure is preserved. Independently reviewed PR #175 repaired that fixture handoff and merged as1c2b9d788fd4029d2469d2651faf0f8db2e0869eafter all final checks passed. Current candidate2f22d0d9097820503d2a81085bb748273de9f56dnormally merges that mainline repair, preserving both changelog entries. Its full copied-Docker validation passes at exact tracked tree658e4fc920a4584ec1153c4c30f45af1b295427d, and all four required jobs in final run 37154563204 pass on that exact candidate. Independent integration review verifies both-parent merge semantics and is recorded in the final review comment. Earlier approvals/checks are not substituted for these final integration gates.Compatibility, failure modes and limits
The new required recovery-port method is a source compatibility change for external trait implementors. On-disk bytes, formats, dependencies and existing partial-migration recovery policy are unchanged. Completion now refuses invalid reserved namespace membership without initiating recovery mutations. Namespace admission is not certification of every retained artifact's content; existing retention recovery owns that evidence.
Tests use repository-only platform admission, real filesystem restart boundaries and deterministic fault inputs. They do not establish production platform eligibility, physical power-loss behavior or isolation from arbitrary concurrent raw namespace changes. Before/after equality does not exclude transient changes restored before observation. Existing process-death and forward-success campaigns remain separate evidence owners. Ordinary-test resource enforcement limitations are recorded.
No benchmark improvement is claimed. Completion adds bounded namespace admission I/O; no content identity or security boundary is weakened. Broader diagnostic redesign under #110 remains separate. Original roadmap checkboxes are unchanged; mainline delivery requires integration.
Closes #111. Refs #131, #132.