Fix: keep sealed stage authority private during crash observation (#146) - #158
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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 (15)
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🪛 LanguageTooldocs/testing-evidence/sealed-stage-observation.md[style] ~16-~16: The words ‘observation’ and ‘observed’ are quite similar. Consider replacing ‘observed’ with a different word. (VERB_NOUN_SENT_LEVEL_REP) 🔇 Additional comments (15)
Summary by CodeRabbit
WalkthroughThe PR removes ChangesSealed-Stage Observation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The sealed-stage API change has no identified merge-blocking issue; proceed with normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change removes a writable escape while retaining integrity and ownership checks. No new security bypass was identified in the inspected paths. Interruption and cleanup behavior have limited verification, so the assessment remains low risk rather than a complete safety assurance. 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 32.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 28 functions across 11 files. (4 skipped: 4 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 sealed stage keeps its quiet byte-bound state Comment |
|
Validation receipt for final pushed head
All Rust execution used copied Docker source and isolated build directories. Filesystem laws used actual ext4 admission. Port-level errno injection, compiler capability rejection and process-death evidence are identified separately; none claims physical power-loss validation. Hosted run 37056000223 is running for the final SHA. Documentation and dependency jobs are green; Rust quality and fuzz smoke remain pending. CodeRabbit reports a rate limit and has not approved. No merge has been performed. |
PR #158 independent landing reviewReviewed FindingsNo verified P0–P5 defect in the complete PR diff or its semantic integration. No required review area was unavailable. The change removes the post-seal writable conversion and preserves actual stage operations, private publisher authority, canonical bytes, failure refusal and crash/restart behavior. The feature-gated removal intentionally changes source compatibility; it does not change a durable format or identity. Verification ChecklistLockdown and complete scope
Every production path and both-sided comparisons
All runtime error exits in the new wrapper and modified crash observer were traced: hook errors, excessive requested limits, underlying I/O failures, invalid write counts, post-effect interruption, unknown crash lengths, checked subtraction/addition failures, readiness errors and unexpected controller resumption. Refusal consumes staged writer state; no path silently resumes from ambiguous effects. No new parser, durable codec, unchecked arithmetic, ambient ordering, public boolean argument, unsafe block, dependency or persistent authority token is introduced. Every merge audited
Every changed constant, numeric claim and raw receipt
Execution versus inspection, and remaining limits
Final verdict for APPROVE |
Code Lawyer landing pass — e781c0bTarget: main
Fresh copied-Docker validation uses a clean source tree All four hosted jobs passed on the current head in run 37146207010. This includes the separate runtime fuzz smoke and documentation/dependency jobs. Earlier green checks were not transferred to the merge candidate. CodeRabbit now approves this exact head; all review bodies, global comments and inline connections are exhausted with no actionable findings or active changes-requested review. Its optional 32.14%/80% docstring heuristic and repeated-word suggestion do not establish a binding public-documentation defect; new public items are documented and documentation CI passes. The fresh independent review is posted separately before merge. No physical power-loss, arbitrary-input differential equivalence, universal resource isolation, or protection against caller-owned duplicate stage handles is claimed. The original exclusive-ownership port precondition remains binding. |
Problem and invariant
With
repository-tasksenabled,SealedSegment::map_stagehanded the owned writable stage to an arbitrary caller callback while retaining the old digest, length and completed-durability receipt. A caller could mutate the bytes after sealing. Publication-time revalidation did not make that sealed receipt truthful.Change kind: bug fix for KEEP-SEGMENT-005. Sealed receipts must not return writable authority to callers.
Approach
Remove the arbitrary sealed-stage mapping method.
ObservedSegmentStageowns private stage and observer fields, executes actual writes/flushes/synchronization, and exposes only lengths and durability boundary events to observers. Its specializedwithout_observerconversion keeps the same stage and metadata sealed together; no callback receives the stage. Filesystem publisher authority and byte checks remain intact.The sole production consumer now uses
CrashSegmentObserver, retaining the same header/record/seal prefix offsets and before/during/after scheduling. A post-writeInterruptedobserver cause is preserved inside a non-retryable error so ordinary write retry cannot repeat completed bytes. Rejected alternatives: a new arbitrary unwrap trait or callback, keeping the escape because later publication revalidates, or weakening crash injection to remove its segment-write coverage.RED → GREEN and validation
Regression commit
11be73dadds an all-feature compile-fail law that attempts the old writable callback. It failed on main6051abbbecause the escape compiled successfully. After removal it passes. This is capability evidence, explicitly separate from runtime verification.Public filesystem laws verify canonical golden bytes, retained publisher provenance, exact excessive-limit refusal before effects, and non-retryable post-write failure with preserved bytes. Generated payload lengths 1–64 compare observed short writes with ordinary filesystem writes; an independent golden complements the shared implementation oracle. Port-level fault laws verify that underlying flush/sync errors prevent sealing with their exact typed phase and errno.
Copied Docker validation passes full workspace debug/release, doctests, documentation build, focused debug/release, all-feature/minimal-feature Clippy, formatting/source structure, Markdown, and complete debug/optimized production crash campaigns. Separate mutation builds fail for skipped flush, skipped synchronization, clamped excessive limits, retryable post-effect interruption and corrupted writes; the corruption mutation also fails the generated differential law. All four required jobs in hosted run 37056000223 passed on final pushed head
15aa976a77d5b65a4b6e8f8a2f9e01d2ba3b0d58. CodeRabbit is rate limited and has supplied no approval.See capability evidence and the decision rationale for oracles, execution limits, traceability and replay commands.
Compatibility and limits
Removing
map_stageis an intentional source compatibility change for the repository-tasks feature. No on-disk format, identity, publication ordering, dependency or claimed performance improvement changes. Construction allocates nothing; after-write normalization allocates only while retaining an exceptional I/O cause. Observation does not manufacture exclusivity when a caller has already violated the stage ownership contract. Process-death and fault-port evidence are not physical power-loss proof.This branch starts directly from
origin/mainat6051abband does not depend on #156 or #157. Original roadmap checkboxes are unchanged.Closes #146. Refs #131, #132.