Bound external digest execution - #63
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
WalkthroughA shared ChangesExternal digest boundary
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Checker
participant ExternalDigest
participant BoundedProcess
participant b3sum
Checker->>ExternalDigest: build digest-specific preimage
ExternalDigest->>BoundedProcess: execute b3sum with deadline and limits
BoundedProcess->>b3sum: stream stdin
b3sum-->>BoundedProcess: bounded stdout and stderr
BoundedProcess-->>ExternalDigest: process result or typed failure
ExternalDigest-->>Checker: exact digest or propagated error
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Self-audit activity summaryThe branch was audited against
No unresolved self-audit findings remain. Full local validation is green at @codex please confirm the findings and inspect for any missed correctness, determinism, cleanup, or documentation defects. |
![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: c11c7df193
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xtask/src/bounded_process/error.rs (1)
121-135: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPreserve the cleanup failure in the
Error::source()chain.
Cleanup::source()returns the primary so chain-walking skips theio::Errorthat represents the failed termination/kill/reap action. Return the cleanupsourceitself, or wrapCleanuparound theprimaryso the standard.source()chain keeps the full diagnostic cause instead of dropping the OS-level cleanup failure from error-reporting tooling.🤖 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 `@xtask/src/bounded_process/error.rs` around lines 121 - 135, Update ProcessError::source so the Cleanup variant exposes its cleanup failure source in the standard error chain instead of returning only primary. Keep Additional returning primary, and ensure Cleanup preserves both the primary error context and the termination/kill/reap io::Error for error-reporting consumers.Source: Coding guidelines
🤖 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 `@CHANGELOG.md`:
- Around line 102-105: Update finish in xtask/src/bounded_process/capture.rs so
wait_for_child timeout or failure invokes cleanup_process before propagating the
typed failure, matching the cleanup behavior of the receive paths. With that
root-cause fix, CHANGELOG.md lines 102-105,
docs/conformance/golden-file-worldline.md lines 165-167, and
docs/dependencies/blake3-1.8.5.md lines 26-29 require no direct changes because
their process-group kill/reap claims become enforced.
In `@docs/dependencies/blake3-1.8.5.md`:
- Around line 26-29: Trim the BLAKE3 dependency admission text to state only
that the external cross-check exists and mismatches are refused. Remove the
subprocess mechanics from the paragraph, including streaming, independent output
bounds, and process-group kill/reap details, and cross-reference the established
process-adapter documentation instead.
In `@xtask/src/golden_file_worldline/b3sum_oracle.rs`:
- Around line 15-21: The u64::try_from conversion in identity_digest is
guaranteed to succeed on all supported targets, so do not treat its
GoldenError::violation branch as meaningful or tested coverage. Prefer
simplifying the payload length conversion without the unreachable violation
handling, while preserving the existing big-endian digest encoding.
In `@xtask/src/golden_file_worldline/error.rs`:
- Around line 14-16: Change GoldenError::ExternalDigest to store the concrete
ExternalDigestError, matching the typed ConformanceError boundary. Update its
constructor and the sole b3sum_oracle.rs call site as needed, preserving direct
pattern matching for specific ExternalDigestError variants without boxed
trait-object allocation.
In `@xtask/src/protocol_conformance/external_digest.rs`:
- Around line 3-9: Remove the local DIGEST_BYTES declaration in digest and
import the existing crate::external_digest::DIGEST_BYTES instead. Ensure
external_digest exposes DIGEST_BYTES as pub(crate), while preserving the digest
function’s return type and existing behavior.
In `@xtask/tests/process_policy_contract.rs`:
- Around line 37-40: Strengthen the policy assertions in the relevant test by
also checking CONFORMANCE_B3SUM for the absence of “.wait()” and “.write_all(”,
matching the existing GOLDEN_B3SUM checks. Keep the current delegation-shape
assertions unchanged.
---
Outside diff comments:
In `@xtask/src/bounded_process/error.rs`:
- Around line 121-135: Update ProcessError::source so the Cleanup variant
exposes its cleanup failure source in the standard error chain instead of
returning only primary. Keep Additional returning primary, and ensure Cleanup
preserves both the primary error context and the termination/kill/reap io::Error
for error-reporting consumers.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f71b8bd0-766c-4490-bf46-9ffa76fd3965
📒 Files selected for processing (21)
CHANGELOG.mddocs/conformance/golden-file-worldline.mddocs/dependencies/blake3-1.8.5.mddocs/dependencies/cap-std-and-cap-fs-ext-4.0.2.mdxtask/Cargo.tomlxtask/src/bounded_process.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/error.rsxtask/src/bounded_process/input.rsxtask/src/bounded_process/reader.rsxtask/src/external_digest.rsxtask/src/external_digest/tests.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/golden_file_worldline/error.rsxtask/src/main.rsxtask/src/protocol_conformance/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/protocol_conformance/external_digest/tests.rsxtask/tests/process_policy_contract.rsxtask/tests/source_policy_contract.rs
💤 Files with no reviewable changes (2)
- xtask/src/protocol_conformance/external_digest/tests.rs
- xtask/tests/source_policy_contract.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Runtime fuzz smoke
- GitHub Check: Rust quality gates
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use stable, pinned Rust with edition 2024;
cargo fmt --checkand Clippy with-D warningsmust pass.
Files:
xtask/tests/process_policy_contract.rsxtask/src/main.rsxtask/Cargo.tomlxtask/src/bounded_process.rsxtask/src/external_digest/tests.rsxtask/src/external_digest.rsxtask/src/bounded_process/input.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/bounded_process/error.rsxtask/src/golden_file_worldline/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/reader.rsxtask/src/protocol_conformance/error.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not useunwrap,expect,panic!,todo!,unimplemented!,dbg!, stdout/stderr printing, unchecked indexing, lossy casts, or unsafe code. Unsafe requires a dedicated crate with documented invariants, tests, and measured necessity.
Use checked arithmetic for externally influenced values andTryFrom/try_frominstead of potentially lossyasconversions.
Public functions must not take boolean parameters; use enums instead. Prefer typed newtypes for IDs, lengths, offsets, generations, and namespaces.
Parse and validate untrusted bytes before admitting them as trusted types; make illegal states unrepresentable where practical.
Prefer concrete types, synchronous core APIs, and traits only at real substitution boundaries; do not introduce async without demonstrated consumer need.
Do not let HashMap iteration order affect identity, serialization, tests, or behavior.
Use hexagonal architecture: domain and port modules own invariants and semantic capabilities, dependencies point inward, and core/ports must not import adapters, filesystems, networks, CLIs, runtimes, or application policy.
Keep codecs at ingress and egress boundaries. Ports exchange semantic or validated types, not serializer-owned JSON, CBOR, or wire values.
Keep files near 200 lines, review files over 300, and do not exceed 500; keep functions near 20 logical lines, review over 40, do not exceed 60; limit nesting to 3 and parameters to 5.
Do not create genericutils.rs,helpers.rs,common.rs,misc.rs,shared.rs,manager.rs,service.rs,types.rs, ormodels.rs; name files after the concept they own.
Keep items private by default; usepub(crate)unless external consumers require public visibility. Public validated types must have private fields and checked constructors, and consequential staged work, plans, commits, and results should use#[must_use].
UseOptionfor normal absence, typed errors by boundary, preserved error sources, and expected/observed state in...
Files:
xtask/tests/process_policy_contract.rsxtask/src/main.rsxtask/src/bounded_process.rsxtask/src/external_digest/tests.rsxtask/src/external_digest.rsxtask/src/bounded_process/input.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/bounded_process/error.rsxtask/src/golden_file_worldline/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/reader.rsxtask/src/protocol_conformance/error.rs
**/*.{rs,json,cbor}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,json,cbor}: Identity, persistence, comparison, tests, and protocol behavior must be deterministic and independent of iteration order, host state, clocks, locale, and serializer defaults.
Boundary JSON and CBOR must use a named canonical profile with golden fixtures; reject duplicate fields and noncanonical identity-bearing encodings. Hash only typed, domain-separated canonical bytes.
Files:
xtask/tests/process_policy_contract.rsxtask/src/main.rsxtask/src/bounded_process.rsxtask/src/external_digest/tests.rsxtask/src/external_digest.rsxtask/src/bounded_process/input.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/bounded_process/error.rsxtask/src/golden_file_worldline/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/reader.rsxtask/src/protocol_conformance/error.rs
**/*.{rs,bin}
📄 CodeRabbit inference engine (AGENTS.md)
Durable formats must define magic bytes, versioning, canonical encoding, explicit endianness, bounds, checksums, and golden fixtures; reject trailing bytes, duplicate fields, noncanonical encodings, overflow, invalid ordering, excessive depth, and unknown mandatory flags. Do not use arbitrary Serde or Rust struct output as a durable format.
Files:
xtask/tests/process_policy_contract.rsxtask/src/main.rsxtask/src/bounded_process.rsxtask/src/external_digest/tests.rsxtask/src/external_digest.rsxtask/src/bounded_process/input.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/bounded_process/error.rsxtask/src/golden_file_worldline/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/reader.rsxtask/src/protocol_conformance/error.rs
🧠 Learnings (1)
📚 Learning: 2026-07-27T22:37:16.896Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 49
File: src/layout/record_length.rs:29-29
Timestamp: 2026-07-27T22:37:16.896Z
Learning: This repository targets Rust 1.96 (per `Cargo.toml` `rust-version` and `rust-toolchain.toml`). When writing or reviewing Rust code, only use APIs/language features stabilized in Rust 1.96 or earlier. Avoid using newer std/library APIs that wouldn’t be available on Rust 1.96 (e.g., you may rely on `u64::is_multiple_of` since it’s stabilized by 1.96).
Applied to files:
xtask/tests/process_policy_contract.rsxtask/src/main.rsxtask/src/bounded_process.rsxtask/src/external_digest/tests.rsxtask/src/external_digest.rsxtask/src/bounded_process/input.rsxtask/src/golden_file_worldline/b3sum_oracle.rsxtask/src/bounded_process/error.rsxtask/src/golden_file_worldline/error.rsxtask/src/protocol_conformance/external_digest.rsxtask/src/bounded_process/cleanup.rsxtask/src/bounded_process/capture.rsxtask/src/bounded_process/reader.rsxtask/src/protocol_conformance/error.rs
🔇 Additional comments (18)
xtask/src/main.rs (1)
22-26: LGTM!xtask/src/external_digest.rs (1)
1-164: LGTM!xtask/src/external_digest/tests.rs (1)
1-104: LGTM!xtask/src/golden_file_worldline/error.rs (1)
63-101: LGTM!xtask/src/protocol_conformance/error.rs (1)
11-17: LGTM!Also applies to: 39-41, 65-65, 99-104
docs/conformance/golden-file-worldline.md (1)
161-164: LGTM!Also applies to: 168-169
docs/dependencies/cap-std-and-cap-fs-ext-4.0.2.md (2)
28-33: LGTM!Also applies to: 109-111
66-66: 📐 Maintainability & Code QualityNo change needed. The manifest enables
rustix = { …, features = ["fs", "process", "std"] }underrepository-tasks, matching the documented dependency admission.xtask/tests/process_policy_contract.rs (2)
49-80: LGTM!
244-248: 🎯 Functional CorrectnessNo change needed for
require_docsindentation handling.
split_once(declaration)preserves the preceding token before the indented item, so the reverse scan skips attributes and lands on the declaration’s preceding///when present. Indented declarations are not systematically reported as missing rustdoc.> Likely an incorrect or invalid review comment.xtask/src/golden_file_worldline/b3sum_oracle.rs (1)
22-28: 🗄️ Data Integrity & IntegrationNo change needed. The post-content
logical_lengthordering matches ADR-0001 and the production hash preimage for non-encoded preimages.xtask/src/bounded_process.rs (1)
8-8: LGTM!Also applies to: 18-18
xtask/src/bounded_process/capture.rs (1)
8-9: LGTM!Also applies to: 22-24, 49-49, 62-82, 130-130, 155-156, 172-195
xtask/src/bounded_process/input.rs (1)
1-156: LGTM!xtask/Cargo.toml (1)
35-36: LGTM!xtask/src/bounded_process/cleanup.rs (1)
5-13: LGTM!Also applies to: 14-25, 27-44, 46-71, 73-87, 88-93, 95-115
xtask/src/bounded_process/error.rs (1)
8-15: LGTM!Also applies to: 48-48, 85-92, 113-116, 136-180
xtask/src/bounded_process/reader.rs (1)
6-6: LGTM!Also applies to: 16-16, 46-46, 59-59, 70-70, 82-117, 134-174
Additional self-review findingThe post-remediation full matrix found and resolved one issue that was not present in the original review threads.
The failing all-target build, targeted typed-error regression, all feature combinations, and full local matrix are now green at @codex please confirm this repair and inspect the updated head for any remaining target-boundary or type-erasure defects. |
Activity SummaryAll review and self-audit items are closed. Each valid defect was handled in an isolated RED → GREEN → VERIFY → COMMIT cycle; informational or false-positive items were resolved with code-path evidence rather than churn.
Final evidence at
No amend, rebase, force-push, draft PR, Python script, public API change, durable-format change, or Keep data mutation occurred. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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". |
1 participant
Problem
Golden-file and conformance checks owned divergent
b3sumprocess lifecycles. Input, output, and failed-process cleanup were not all bounded by an explicit contract.Invariant
Every external digest returns the exact bytes produced for its admitted preimage within declared resource bounds, or returns a typed refusal. No post-spawn input, output, or cleanup wait is unbounded.
Approach
Alternatives rejected
Failure modes
Spawn, environment, input, output, exit-status, encoding, deadline, interruption, and cleanup failures remain distinct typed errors.
Tests
Passed formatting; workspace checks, Clippy, debug, release, doctests, and Rust 1.96; Golden, conformance, source-structure, and full
xtask verify; fuzz formatting/check/Clippy; dependency deny/audit; workflow lint; Markdown and diff checks.Benchmark impact
No production data path changed. The bounded adapter is maintenance orchestration and introduces no benchmark obligation.
Format/API compatibility
No durable format, identity preimage, or public API changed. The affected surface is private to
xtask.Recovery implications
No Keep data is written. Recovery is limited to bounded subprocess teardown.
Security implications
The child environment is cleared except for the executable search path and deterministic locale; input, output, diagnostics, time, and cleanup are bounded.
Closes #59