Skip to content

Fix: keep sealed stage authority private during crash observation (#146) - #158

Merged
flyingrobots merged 4 commits into
mainfrom
fix/146-sealed-stage-observer
Oct 3, 2026
Merged

flyingrobots merged 4 commits into
mainfrom
fix/146-sealed-stage-observer

Conversation

@flyingrobots

@flyingrobots flyingrobots commented Oct 2, 2026 •

Copy link
Copy Markdown
Owner

Problem and invariant

With repository-tasks enabled, SealedSegment::map_stage handed 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. ObservedSegmentStage owns private stage and observer fields, executes actual writes/flushes/synchronization, and exposes only lengths and durability boundary events to observers. Its specialized without_observer conversion 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-write Interrupted observer 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 11be73d adds an all-feature compile-fail law that attempts the old writable callback. It failed on main 6051abb because 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_stage is 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/main at 6051abb and does not depend on #156 or #157. Original roadmap checkboxes are unchanged.

Closes #146. Refs #131, #132.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

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
  • Configuration used: Organization UI
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 5a9d30fc-7cd6-4f8e-92ce-ce2901a48039
📥 Commits

Reviewing files that changed from the base of the PR and between d0cff10 and e781c0b.

📒 Files selected for processing (15)
  • CHANGELOG.md
  • docs/formats/segment-store-v1/rationale.md
  • docs/formats/segment-store-v1/requirements.md
  • docs/testing-evidence/sealed-stage-observation.md
  • src/adapters/exports.rs
  • src/adapters/mod.rs
  • src/adapters/observed_segment_stage.rs
  • src/adapters/sealed_segment.rs
  • src/adapters/segment_stage_observer.rs
  • src/lib.rs
  • tests/observed_segment_stage.rs
  • tests/observed_segment_stage/byte_equivalence.rs
  • tests/observed_segment_stage/durability_failure.rs
  • xtask/src/durability_crash_matrix/production_protocol/publication.rs
  • xtask/src/durability_crash_matrix/production_protocol/segment_stage.rs

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)
  • GitHub Check: Runtime fuzz smoke
  • GitHub Check: Rust quality gates
  • GitHub Check: Documentation and workflow integrity
  • GitHub Check: Dependency policy
🧰 Additional context used
🪛 LanguageTool
docs/testing-evidence/sealed-stage-observation.md

[style] ~16-~16: The words ‘observation’ and ‘observed’ are quite similar. Consider replacing ‘observed’ with a different word.
Context: ...a fixed repeating byte pattern, compare observed seven-byte-prefix writes against plain ...

(VERB_NOUN_SENT_LEVEL_REP)

🔇 Additional comments (15)
src/adapters/segment_stage_observer.rs (1)

1-41: LGTM!

src/adapters/sealed_segment.rs (1)

10-22: LGTM!

src/adapters/observed_segment_stage.rs (1)

1-85: LGTM!

src/adapters/mod.rs (1)

154-155: LGTM!

Also applies to: 218-219

src/adapters/exports.rs (1)

67-68: LGTM!

Also applies to: 97-98

src/lib.rs (1)

167-172: LGTM!

xtask/src/durability_crash_matrix/production_protocol/publication.rs (1)

8-9: LGTM!

Also applies to: 16-16, 35-35, 44-44

xtask/src/durability_crash_matrix/production_protocol/segment_stage.rs (1)

3-5: LGTM!

Also applies to: 17-17, 22-23, 68-69, 83-88, 98-124

tests/observed_segment_stage.rs (1)

1-147: LGTM!

tests/observed_segment_stage/byte_equivalence.rs (1)

1-49: LGTM!

tests/observed_segment_stage/durability_failure.rs (1)

1-62: LGTM!

docs/formats/segment-store-v1/rationale.md (1)

179-182: LGTM!

docs/formats/segment-store-v1/requirements.md (1)

50-50: LGTM!

docs/testing-evidence/sealed-stage-observation.md (1)

1-30: LGTM!

CHANGELOG.md (1)

11-12: LGTM!


Summary by CodeRabbit

  • New Features
    • Added optional stage-write and durability observation for repository-task workflows, including controlled write limits and failure injection.
  • Changes
    • Sealed segment receipts no longer provide a method for transforming their stage into a writable handle.
    • Removing observation from a sealed segment preserves its staged data and publication metadata.
  • Documentation
    • Added guidance and regression evidence describing observation behavior, durability events, and failure handling.

Walkthrough

The PR removes SealedSegment::map_stage and adds a feature-gated observer wrapper for stage writes and durability operations. The crash harness now uses the wrapper. New tests and documentation cover the observer contract and its behavior.

Changes

Sealed-Stage Observation

Layer / File(s) Summary
Observer contract and sealed receipt
src/adapters/segment_stage_observer.rs, src/adapters/observed_segment_stage.rs, src/adapters/sealed_segment.rs, src/adapters/mod.rs, src/adapters/exports.rs, src/lib.rs
Adds observer hooks and ObservedSegmentStage. The wrapper controls permitted write lengths and observes flush and synchronization boundaries. Removes map_stage; without_observer() retains the original stage and sealed metadata. The observer API and modules are gated by repository-tasks.
Crash harness integration
xtask/src/durability_crash_matrix/production_protocol/*
Replaces CrashSegmentStage with CrashSegmentObserver. The crash protocol now supplies write limits and handles crash boundaries through observer callbacks.
Regression tests and documented evidence
tests/observed_segment_stage.rs, tests/observed_segment_stage/*, docs/formats/segment-store-v1/*, docs/testing-evidence/sealed-stage-observation.md, CHANGELOG.md
Adds tests for observer removal, write limits, interrupted post-write callbacks, byte equivalence, and flush or synchronization failures. Updates the requirement, rationale, evidence, and changelog text.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to e781c

The sealed-stage API change has no identified merge-blocking issue; proceed with normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e781c

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — In the inspected filesystem path, the observer can affect progress and write boundaries for the supplied stage, but the wrapper does not grant authority over additional stores or substitute publisher identity. The stage originates from a publisher, retains its authority, and must be selected by a matching publisher.

Trust Boundaries and Controls

  • observed — The boundary is capability encapsulation, not isolation of arbitrary callback code. SegmentStage already requires exclusive ownership of an empty staging object. The filesystem stage keeps its file private and creates the staging entry exclusively. The generic contract does not prevent a custom implementation from retaining pre-existing aliases, but the new wrapper does not expose or manufacture one.

Resilience and Maintainability Implications

  • observed — The inspected regression laws assert refusal of excessive observer limits before writes, preservation of the original post-write interruption cause without retry, canonical byte equivalence for bounded generated payloads, and propagation of underlying flush and synchronization failures. These are scoped capability and failure-containment checks, not comprehensive crash or panic-cleanup verification.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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:… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: keeping sealed-stage authority private during crash observation.
Description check ✅ Passed The description covers the problem, invariant, approach, change kind, alternatives, testing evidence, compatibility, and performance impact. It is mostly complete, but it does not explicitly address s…
Linked Issues check ✅ Passed #146 requires sealed receipts to keep writable stage authority private, retain metadata with the sealed stage, and preserve publication checks and crash coverage. SealedSegment no longer has `map_st…
Out of Scope Changes check ✅ Passed The added observer API, crash-harness adaptation, regression and filesystem tests, and capability documentation all support #146. The supplied change summary shows no unrelated catalog, migration, for…
Full details: Docstring Coverage

Explanation

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.)

  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

A sealed stage keeps its quiet byte-bound state
An observer marks each write and sync boundary
Limits meet the writer before bytes pass through
Crash points follow the operations they knew
Tests check the receipt, the bytes, the call
The stage stays private through it all

Comment @coderabbitai help to get the list of available commands.

@flyingrobots

Copy link
Copy Markdown
Owner Author

Validation receipt for final pushed head 15aa976a77d5b65a4b6e8f8a2f9e01d2ba3b0d58, branched from main 6051abb25a9fd33ae7ee0de5614514b709a4d82a:

  • Capability RED is preserved in 11be73d: the all-feature compile-fail example fails on the old implementation because the writable mapping compiles. It passes after removal. This is a static/API oracle, not claimed runtime crash evidence.
  • Full workspace debug/release, all-feature doctests and documentation build passed on implementation commit 038b283df9375424b44b1b68036def2e1ade927c. Existing golden, corruption, bounded-memory and publication laws remain green.
  • Complete debug and optimized production crash matrices passed with the new observer controlling the same write and durability boundaries. No crash coordinates or restart expectations were removed.
  • Focused public filesystem and fault-port laws, generated byte equivalence, source structure, formatting, both-feature Clippy and Markdown passed. Dedicated mutation sources demonstrated the named assertion failures recorded in the evidence document; those mutants are not present in the candidate.
  • The final commit adds only #[must_use] to the owned observation stage. After importing that exact head into the copied validation checkout, formatting, all-target/all-feature Clippy, the runtime observation target and the capability doctest passed again. No runtime implementation changed after the full campaign.

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.

@flyingrobots

Copy link
Copy Markdown
Owner Author

PR #158 independent landing review

Reviewed flyingrobots/keep, branch fix/146-sealed-stage-observer, exact head e781c0b276ec4d1f66a76668bd31893261a2e6dd, targeting main at d0cff10d7c911d33d615c3aa2246ae2b4497432a. Independent Codex fallback applies the complete agy adversarial protocol and Code Lawyer review requirements. This is a read-only code/evidence review, not a merge operation or a claim that agy itself executed.

Findings

No 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 Checklist

Lockdown and complete scope

  • Verified the isolated checkout for this branch was clean before and after review. Local HEAD and live GitHub head/base match the full SHAs above. Independently read the copied Docker checkout's clean status and tree; both candidate and copied tree are b9a452b4861436dc87118d7d6fc98113aea9f1d9.
  • Read root AGENTS.md (the only applicable AGENTS file), docs/Testing Standards.md, docs/testing/enforcement.md, PR template, CI and both requested skill files. The prior memory registry provided orientation only; current code and receipts establish this verdict.
  • Inspected all 15 changed paths: CHANGELOG.md; docs/formats/segment-store-v1/{rationale,requirements}.md; docs/testing-evidence/sealed-stage-observation.md; src/adapters/{exports,mod,observed_segment_stage,sealed_segment,segment_stage_observer}.rs; src/lib.rs; tests/observed_segment_stage.rs; tests/observed_segment_stage/{byte_equivalence,durability_failure}.rs; xtask/src/durability_crash_matrix/production_protocol/{publication,segment_stage}.rs. Read the entire diff and surrounding producers, consumers, error paths, tests and inherited merge changes.
  • Independently refreshed GraphQL connections at this head: initially exactly 3 global comments, 0 review bodies, 0 review threads, with hasNextPage=false for every connection. The final queue 158-queue-final.json contains the same 3 global comments, 1 exact-head CodeRabbit approval and 0 threads; independently verified that approval and read the entire updated CodeRabbit global body. No nested thread-comment pages exist. Usage-limit/rate-limit/in-progress messages themselves supply no approval. The old validation comment is pinned to 15aa976, not accepted as current-head execution proof.
  • Read the live PR body. Its issue, invariant, alternatives, failure modes, named static/runtime/differential oracles, compatibility, recovery/security implications and performance limitations agree with the implementation. Historical run 37056000223 is explicitly pinned to 15aa976; current acceptance is separately established below.

Every production path and both-sided comparisons

Path Trace and comparison Verified outcome
Feature exposure src/adapters/mod.rs:154,218 → src/adapters/exports.rs:67,97 → src/lib.rs:167 All three observation concepts use the same repository-tasks gate; ordinary production API has no new feature-dependent format semantics. Existing RepositoryInitializationStorage export remains gated.
Fresh observed writing filesystem_catalog_publisher.rs:119 → filesystem_segment_stage.rs:31,55 → observed_segment_stage.rs:18,25 → staged_segment.rs:43,92 → segment_stage_write.rs:7 Constructor performs no allocation and exposes neither private field. Allowed prefix is checked before the actual write; short successful writes pass their actual count to the observer and completion loop. Over-limit requests refuse without a write. Invalid returned counts refuse; zero progress cannot seal.
Write failures and effects observed_segment_stage.rs:27,34,41 ↔ segment_stage_write.rs:18,37,39; staged_segment.rs:15,49,94,119 Pre-effect and underlying I/O errors preserve sources. Only a post-effect observer Interrupted is wrapped in Other, retaining its original io::Error; the completion loop therefore cannot repeat already-written bytes. The writer's offset remains prior completed calls, explicitly not rollback proof. Every failed consuming operation drops the ambiguous stage state.
Prefix and sealed durability staged_segment.rs:114,209,215 → observed_segment_stage.rs:54,63 → filesystem_segment_stage.rs:60,65 For each durability phase: before hook → actual flush/sync → after hook. A before refusal prevents the operation; an actual failure prevents the after hook; an after refusal retains completed effects but cannot return a sealed receipt. Real filesystem synchronization still invokes sync_all. No Drop durability, async, buffering or hidden content materialization was added.
Sealed conversion and generic close Old main 6051abb:src/adapters/sealed_segment.rs:64 ↔ current sealed_segment.rs:45,69 and observed_segment_stage.rs:73 Arbitrary public map_stage(FnOnce(S)) is gone. Only the owned built-in wrapper can be dismantled internally; observer removal preserves the exact same hidden stage, count, length and digest. Nested wrappers remain private until similarly removed. Generic close drops storage and returns metadata without filesystem authority. No public extractor or arbitrary unwrap trait substitutes for the removed capability.
Publisher provenance and byte checks observed_segment_stage.rs:79 → filesystem_catalog_publisher.rs:136 → filesystem_segment_stage.rs:44 → filesystem_publisher_authority.rs:22; segment_publication.rs:46,54,102 → filesystem_catalog_segment.rs:10 Selection closes the original stage and checks the same private authority token plus admitted count/length/digest; publication additionally verifies actual named stage bytes. Metadata-only receipts cannot gain authority. Existing unrelated-publisher and external-stage laws (tests/catalog_filesystem_publication/authority_laws.rs:16,58) remain applicable.
Crash writes Old 6051abb:xtask/.../segment_stage.rs:78-106 ↔ current production_protocol/segment_stage.rs:69-98 plus observed_segment_stage.rs:26-51; consumer old production_protocol/publication.rs:34,43 ↔ current :35,44 Prefix selection, checked count advancement and before/during/after readiness coordinates are preserved. Stage I/O moves into the private wrapper; observer receives lengths only. Sole production map_stage consumer is replaced.
Crash durability Old 6051abb:xtask/.../segment_stage.rs:109-132 ↔ current production_protocol/segment_stage.rs:101-124 plus observed_segment_stage.rs:54-69 Prefix/sealed phase selection and DuringTiming::Before mapping match both old flush and synchronization paths. Successful underlying operations precede after readiness.
Process death and restart xtask/src/durability_crash_matrix.rs:21,118 → .../process.rs:22 → production_protocol/publication.rs:19 → .../control.rs:28,42,64; restart .../restart.rs:21 → restart/expectation/steps.rs:41,53,78 and restart/semantic.rs:25,50 Entire case enumeration is unchanged. Readiness precedes termination, then exact reopened artifact bytes, hard-link identity, semantic admission, writer release and published snapshot are checked. Retention/migration dispatch remains separate. Process death is not physical power-loss evidence.
Imported partial-seal refusal recovery/recovery_segment_classifier.rs:69 → recovery_segment_seal_framing.rs:9,32,55 → recovery_fixed_field_prefix.rs:3; assessment recovery_stage_assessor.rs:19,25 Available fixed seal bytes are compared to named v1 constants before a short seal becomes Truncated. Canonical fill is comparison-only, never admitted content. Exact seal cause survives classifier/assessment error chains. Complete seals still use AdmittedSegment::decode at classifier :81; other stage grammars remain separate.
Incoming fuzz/replay fuzz/fuzz_targets/segment_format.rs:15,70,98 ↔ xtask/src/fuzz_seed_corpus/segment_seeds.rs:44,57 → .../tests/materialization.rs:22,142; tests/recovery_partial_seal.rs:18,36,55 and .../framing_laws.rs:16 Selector 5 reaches the actual recovery classifier. Independent fixed-byte oracle checks returned truncations. Permanent input reaches both direct laws and actual emitted-input replay; retired cardinality assertions are not silently replaced by a count claim.
#99 retained contracts retention/recovery_stage_assessment.rs:8,52,91 → recovery_planner.rs:31-77; filesystem_retention_recovery.rs:33,53,119; recovery_execution.rs:43,80,98 Incomplete retention stages still require explicit disposition; corrupt stages remain precise refusals. Reopening preserves observed identity, execution stops on first error, and known/uncertain effects and original source remain available. Neither merge nor PR diff changes any retention implementation or tests. The observation wrapper does not reroute these APIs or release the retained writer authority.

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

  • Crash boundaries 64/32, 209/136, 337/273 at production_protocol/segment_stage.rs:10-15 are unchanged from old consumer. Header 64, record header 112, one payload byte and checksum 32 produce 209; seal 128 produces 337. Interrupted prefixes are strictly inside their phases. Values match restart expectation steps.rs:41-58, semantics semantic.rs:67-69, format specification docs/formats/segment-store-v1/segment.md:44,78,91,144 and immutable one-zero golden (337 bytes). New observation does not change these constants, timing policy, timeout or buffer budgets.
  • New law parameters: prefix 7 (tests/observed_segment_stage.rs:112), generated lengths 1..=64 (byte_equivalence.rs:15), excessive increment 1 with checked addition (:114), prior-call offset 0 (:57,79), exact one-header prefix 64 (:96), fault errno 5 (durability_failure.rs:28,36,47,59), restart cap 1_048_576 (observed_segment_stage.rs:141). These are explicit test inputs/expected outcomes, not throughput or latency claims. Fixtures fit that cap; borrowed maximum policies and durable sizes are unchanged. Six current laws are visible in both raw debug and release output; earlier receipts' 3/5-test counts are historical snapshots, not contradictory current counts.
  • docs/testing-evidence/sealed-stage-observation.md:7: raw keep-audit/146/capability-red.log:44-63 shows the named example compiling when marked compile-fail. Current all-feature doc receipt succeeds. :16-19 matches tests and raw focused/equivalence receipts; 1–64 and seven-byte prefixes are bounded differential evidence, errno 5 is simulated, and offset 0 is explicitly not rollback. No unsupported claim of arbitrary-input equivalence is made.
  • :20,26,28: historical full keep-audit/146/full-validation.log contains debug/release workspace, all-feature doctests and documentation, with complete matrix law passes at :1751,3727; current integrated acceptance is in keep-landing/158-validation.log. Rust 1.96.0 matches rust-toolchain.toml; edition 2024 matches Cargo manifests. clippy-final.log and markdown.log corroborate corrected final quality checks; the earlier final-focused.log Clippy doc-markdown failure is retained, corrected in current source and superseded by successful receipts, not hidden.
  • :22 calibration independently checked against the retained actual mutation script keep-landing/158-original-calibrate.sh and full 158-cal-{retry,flush,sync,clamp,bytes}.log. Each compiles and executes the relevant named law: retry reports repeated post-effect admission; flush fails the exact prefix/errno assertion; sync reports manufactured seal; clamp reports excessive-limit admission; zeroed writes fail independent golden/header equality. keep-audit/146/equivalence-calibration.log additionally fails differential equality at payload length 1. calibration.log alone was insufficient (counts only); the full logs close that gap. Mutations are absent from current source.
  • Incoming fixed-seal numeric table: version 1, flags/reserved 0, length 128, algorithm 1, offsets 16,18,20,22,28,56,57,58, sizes 2,4,6 match specification segment.md:148-164, complete decoder segment_seal_decoder.rs:35-49, constants segment_seal.rs:5-10, new validator, test table and independent fuzz table. XOR-one expected integers 257,0,256,1,384,129,16_777_216,65_536 and six-byte reserved arrays in framing_laws.rs:61-202 correctly follow big-endian positions. Sweep 16..128 means every recognized incomplete-seal length, not a claim of arbitrary-byte exhaustiveness.
  • Incoming tests/fixtures/recovery/README.md:3,5: retained input is 82 bytes (164 hex digits plus newline), exactly 64-byte header + 16-byte seal magic + unsupported version 2; selector 5 is outside that input. Empty golden is 192 bytes; one-zero golden is 337. Fixture/replay contents agree with classifier and emitted-input witness.
  • Incoming partial-seal-corruption.md:7,21,23,25-33,37-51: inspected historical RED/GREEN and calibration receipts, patches and permanent input. Main accepts the corrupt prefix; committed parent laws fail after compilation, exact error/source/coordinate mutants fail their named assertions, and emitted selector/missing/diagnostic mutants fail actual materialization law. Commands' runs=1, timeout 5, RSS 1024 MiB, input bound 1,048,576, campaign seed 17101 and requested 15 seconds match raw fuzz-green.txt:3,17,19; actual campaign completes 3,110,375 runs in 16 seconds at :708. That is a historical bounded campaign, not a hard wall-time or current performance guarantee. Nightly/cargo-fuzz versions are replay profile claims; this reviewer did not rerun them. Historical retired seed counts appear only in the audited deletion diff, not a replacement cardinality guarantee.
  • Changed CHANGELOG, requirement status and rationale paragraphs make no new timing/rate/throughput/budget figure. Issue/requirement IDs and SHAs are traceability coordinates, checked against history. README is unchanged by Prevent post-seal writable stage escape through receipt mapping #146. Receipt timings/counts are historical execution output, not evidence that green CI proves absence of all regressions. No missing measurement supports a claimed optimization because none is claimed.

Execution versus inspection, and remaining limits

  • Executed by this reviewer: read-only Git history/diffs/status/tree checks, git diff --check, live gh pr view, paginated-connection GraphQL verification, filesystem reads and Docker read-only Git identity/status checks. No host Rust tests, source changes, configuration changes, publication, commit, merge or subagents. Fetch was intentionally omitted under the explicit read-only mandate; exact commit objects and live remote PR coordinates were verified directly.
  • Inspected execution by the parent, not rerun by this reviewer: 158-validation-manifest.md records the actual copied Docker invocation and successful exit0 session 47084. It explicitly states its after-run recording limit. Independent clean copied-tree verification confirms the claimed tree. Full raw 158-validation.log contains six observer laws at :653-663 and :2651 onward, three integrated partial-seal laws at :780-787 and :2778 onward, emitted-input replay at :1770,3768, complete debug/release matrix laws at :1837-1843,3839, and successful doctests/docs/fuzz build/lint output. Quiet dedicated conformance/golden/source-structure/crash/format commands have no standalone success banners; their completion is evidenced by the manifest's fail-fast invocation and subsequent successful steps, complemented by actual matrix-law output.
  • Live hosted checks independently inspected: current-head run 37146207010 has successful Rust quality, documentation/workflow integrity, runtime fuzz smoke and dependency policy jobs. All four are completed SUCCESS, not pending or silently skipped. Hosted runtime fuzz success is separate from local fuzz compilation/lint. Historical Prevent post-seal writable stage escape through receipt mapping #146 receipts were checked as historical evidence and raw originals, not mislabeled as reruns on the merge head.
  • Unrun/limits: reviewer did not independently execute test campaigns or inspect every hosted job's raw transcript; no physical power-loss, arbitrary hostile-stage equivalence, exhaustive input/schedule exploration, new per-test sandbox/ceilings, latency SLO or allocation/performance benchmark is claimed. Existing repository enforcement gaps are disclosed, not transformed into green guarantees. No parser is added by Prevent post-seal writable stage escape through receipt mapping #146; imported parser fuzz and permanent replay are preserved. New tests remain below file/function hard limits, declare law/oracle subjects and sizes, use private scratch, deterministic inputs, no synchronization sleeps, and exact failure/byte checks. Feature-minimal and all-feature quality plus debug/release product evidence are covered.
  • Final provider reconciliation: CodeRabbit review PRR_kwDOTdinZc8AAAABQgDrJA, submitted 2026-10-03T19:05:37Z, APPROVES this exact head. Live reviewDecision=APPROVED and CodeRabbit status SUCCESS. Its full global body reports no actionable comments; 15 listed file observations are LGTM. The prose repetition suggestion is optional. Its 32.14%/80.00% docstring heuristic over 28 functions/11 files is provider output, not a repository requirement or independently endorsed coverage measurement; it identifies no missing public item. The new public constructor/conversion, type, enum/variants and observer methods have contract documentation, and documentation CI is green. Provider timeout omissions for CI do not weaken this review's separate live verification of all four successful jobs.
  • Landing boundary: this approval is the independent current-head review gate. Verify live branch protections/head/checks immediately before merging and reconcile any subsequent actionable feedback. Any head change requires a fresh review. The report does not itself establish unknown branch-protection requirements or execute the authorized merge.

Final verdict for e781c0b276ec4d1f66a76668bd31893261a2e6dd:

APPROVE

@flyingrobots

Copy link
Copy Markdown
Owner Author

Code Lawyer landing pass — e781c0b

Target: main d0cff10d7c911d33d615c3aa2246ae2b4497432a. Change kind: original capability bug fix plus integration of current main; no new product behavior was introduced during the landing pass. The maintainer explicitly authorizes merge after fresh clean independent review and green checks.

Obligation Source / severity Disposition
Sealed receipt exposes writable stage through public mapping #146 / P2 11be73d preserves compiler-enforced RED on the old API. Arbitrary mapping is removed; the built-in wrapper's private stage remains inside its sealed receipt when observation is removed.
Production crash instrumentation retains real storage behavior Original consumer / acceptance Traced private wrapper → real write/flush/synchronize → observer events, and both crash-control callers. Existing boundaries and restart outcomes are unchanged. Both complete process-death campaigns pass on this candidate.
Post-effect Interrupted must not repeat bytes Original correction / acceptance Non-retryable outer I/O kind retains the original cause. Public runtime law checks the failure and exact already-written header. Original retained retry mutation compiles and fails that named law.
Short writes, excessive limits, underlying durability and exact bytes Behavioral acceptance Public admitted filesystem laws, independent empty golden, finite 1–64 generated payload comparison, exact port-level errno failures. Original mutation sources and detailed logs for retry/flush/sync/clamp/zeroed bytes were recovered intact and inspected, rather than relying only on failure counts.
Integration of #172 partial-seal refusal Landing merge e781c0b Audited against both parents. The only conflict was CHANGELOG; both entries remain. #146 implementation/crash consumer files are byte-identical to the first parent; imported #172 classifier/error/fuzz/counterexample behavior is preserved. No #99 scope change.

Fresh copied-Docker validation uses a clean source tree b9a452b4861436dc87118d7d6fc98113aea9f1d9, exactly equal to the candidate, with dedicated build output and actual admitted ext4 scratch. The full shell chain returned zero: Worldline, debug and optimized production crash campaigns, conformance, source structure, formatting, both feature checks/Clippy profiles, workspace debug/release tests, all-feature doctests, documentation build, pinned MSRV, and fuzz-target format/build/Clippy. Historical calibration is not mislabeled as rerun on this integration head.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Prevent post-seal writable stage escape through receipt mapping

1 participant