Repository navigation
test: keep overflow fixtures idle after broken pipes - #305
Conversation
|
@sfl review |
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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". |
There was a problem hiding this comment.
✅ Verdict: PASS • Round 1
Review done by claude-opus-5-5 (adversarial to codex)
The PR changes only test fixtures. The held and overflow fixtures now treat BrokenPipe as the end of their output and stay idle until killed, any other write error aborts, and new closed-pipe regressions run in both the plain and the instrumented profile tests. Approve: the comments match the runner code at the exact SHA, the new checks fail on the old unwrap path, and no Critical, High or Medium defect was found in the full diff.
🔨 Full-Spectrum Review Summary
Effort: high
Review time: 2m 15s
Token count: Unavailable
| Pass | 🔴 Critical | 🟠 High | 🟡 Medium | ⚪ Low |
|---|---|---|---|---|
| 🔒 Security | 0 | 0 | 0 | 0 |
| ✅ Accuracy | 0 | 0 | 0 | 0 |
| 🧰 Quality & Maintainability | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 |
Key Findings
No material defects found.
Coverage
- Files reviewed:
crates/codexbar-providers/tests/command_runner.rscrates/codexbar-providers/tests/command_runner/profile.rs
- Context read:
crates/codexbar-providers/src/command.rscrates/codexbar-providers/src/command/pipe.rscrates/codexbar-providers/Cargo.tomlAGENTS.md
- Verified:
- Compare
base_commit.shaisd213f24be8c939bdbe345fce371cd7efd0e88ef3and the only commit is682a3f34e907538f3a3de868540f62abe0fcb62a(ahead 1, behind 0, 2 files changed) - Comment says the runner drops both pipes before the kill: confirmed.
collectreturnsErr(Failed)fromCapture::drain, which drops bothCapturereaders beforeRunning::stopcallspipe::stop_group outputjoins both writer threads insidethread::scopebeforeheldcallssent, so asentmarker means both writes returned- The old
emit(...).unwrap()path would panic onBrokenPipeand exit beforesent, soclose_readerswould fail with its 'exited on closed pipes' panic: the regression catches the old bug emit(finite output) still unwraps, so the finite-stream and cap-boundary assertions stay strict- The
closedprofile helper callsholdbeforedivert, soreleasedfindsclosed.readyand the lock file.OwnedChild::dropkills and reaps it first - The
closedprofile assertion requires exactlydiverted,sentwith no.profraw.positionandboundarytrace every runtime seek and write, so any started profile write would show - The new
evidencehelper keeps the old profile-gathering logic, apart from the borrowed-name tuple type scoped-parentstill expectsErr(Failed)fromheld stdout 4MiB+1, and theoverflowmode now usesspill; the other mode expectations are unchangedharness = falsein Cargo.toml confirmsmainis the test entry point, so the newclose_readerscall runs on every platform- Check runs on the head SHA: ubuntu, macOS, build and rust-mutations succeeded; Windows, rust-metrics and security-scan were still in progress
- Compare
- Not verified:
- Whether Rust maps the Windows closed-pipe write error (
ERROR_NO_DATA) toErrorKind::BrokenPipe. This lives in the standard library, not this repository; if it does not, the Windows fixture would abort andclose_readerswould fail. The Windows CI job had not finished - Hosted coverage (
cargo-llvm-cov) timing and therust-metricsresult, which was still in progress - PR-body claims about the mutant runs, Mini evidence and probe results; these are outside the repository
- Linux-only
containedWindows job/drain paths were not re-read; the diff doesn't touch them
- Whether Rust maps the Windows closed-pipe write error (
- Tests run: None. Reviewer execution is disabled.
HemSoft full-spectrum review • 3 evidence-based passes • Reviewed head 682a3f34e907538f3a3de868540f62abe0fcb62a • SFL v0
Summary
Overflow can close a fixture's output pipes before the runner kills it. A
BrokenPipepanic then exits through LLVM's profile writer, and the kill can truncate that write. The recurrence and diagnosis explain the failed unchanged-base collection in PR #303.Keep finite output strict. Held/overflow fixtures now tolerate
BrokenPipeand stay idle until killed. Other write errors abort without entering the profile writer. Add a closed-pipes regression with owned-child cleanup and an instrumented control that requires exactlydiverted, sentand no profile.Acceptance criteria
Definition of done
d213f24be8c939bdbe345fce371cd7efd0e88ef3; only two fixture files changed.Validation
cargo fmt --all --checkandgit diff --check: passed.1a2c4797e, LLVM 23.1.4: candidate plain 5/5 and instrumented 3/3 passed; every coverage set strictly merged with the pinnedllvm-profdata. Closed control traced onlydiverted, sent, without a raw profile.cargo clippy --workspace --all-targets --locked -- -D warningsandcargo test --workspace --lockedpassed in hosted CI on Windows, macOS and Ubuntu./tmp/chief310-275fix-096c0d89c644, manifest SHA-256d1fcc4338be8437465d0a0a33b2e61e2b33693b70cdc6c5536f91b2ab7cdb76e.Risks and deferred work
This PR's metrics base still contains the old fixture and remains exposed to that race. Candidate code cannot fix its base. No retries, corrupt-profile suppression or baseline substitution are used. After this lands, update PR #303 onto fixed main and verify its actual
METRICS_BASE, then advance stacked PR #304 and qualify all required CI and triggered Package jobs. Neither waiting PR is ready to merge yet.Closes #275
Merge evidence
Checked head
682a3f34e907538f3a3de868540f62abe0fcb62a; all seven GitHub Actions checks passed with integration ID 15368, authenticated SFL/Codex clean, zero unresolved threads and zero formal approvals required. Guarded squash merge landed as9df7787f0a68e8f23ee5235b89d1ad01014f6a97. Mini and Home main both fast-forwarded to that commit. The original audit report is preserved unchanged.