fix(minimald): keep a timed-out hook's captured output by gominimal-aw-bot[bot] · Pull Request #2318 · gominimal/minimal

gominimal-aw-bot · 2026-10-18T03:30:11Z

Routing-Key: inbox-route/I_kwDOSUhdos8AAAABX3va8Q

A hook run with captured output waited on wait_with_output() under tokio::time::timeout; at the deadline the dropped future discarded every byte it had already read, so a TimedOut outcome reported empty output — losing exactly the output that said what the hook was doing when it hung.

The capturing branch in crates/minimald/src/hooks.rs now drains stdout/stderr on their own tail-bounded reader tasks (last OUTPUT_TAIL_BYTES) while the deadline waits on child.wait() alone. On timeout the child is killed and reaped, and the reader buffers are joined under a 1s bound, so a backgrounded grandchild holding a pipe open cannot stretch the report: whatever arrived is the tail.

Verification

cargo fmt --all --check --manifest-path target/Cargo.toml — pass
cargo clippy --workspace --all-targets --locked --manifest-path target/Cargo.toml -- -D warnings — pass
cargo test --locked --manifest-path target/Cargo.toml -p minimald — hooks: 27 passed, box_actor: 133 passed; 12 failures in net::switch/connection/rpc reproduce identically on the untouched tree (verified by stashing the change and rerunning)
New test hook_that_times_out_keeps_what_it_printed — a hook that echoes a line then sleeps past its 1s timeout reports TimedOut with the printed line in its captured output (previously empty)
New test hook_timing_out_behind_a_held_pipe_still_reports — a grandchild holding stdout open past the kill cannot stretch the report past the join bound; the pre-hang line is still reported

Summary by CodeRabbit

Bug Fixes
- Hook output is now preserved when scripts succeed, fail, time out, or encounter input errors.
- Scripts that produce substantial output while running are less likely to stall.
- Timed-out hooks now finish reporting within a bounded period, even if background processes keep output streams open.
- Captured output remains readable when long output is shortened.

A hook run with captured output waited on wait_with_output() under tokio::time::timeout. On timeout the future is dropped, and with it every byte it had already read, so a TimedOut outcome carried an empty output -- losing exactly the output that said what the hook was doing when it hung. The capturing branch now drains stdout/stderr on their own tasks (tail-bounded to OUTPUT_TAIL_BYTES) while the deadline waits on child.wait() alone. On timeout the child is killed and reaped, and the reader buffers are joined under a 1s bound so a backgrounded grandchild holding a pipe open cannot stretch the report: whatever arrived is the tail. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

coderabbitai · 2026-10-18T03:30:43Z

📝 Walkthrough

Captured hooks now drain stdout and stderr concurrently into bounded tails. The runner preserves output across exit paths and bounds collection after timeouts. Tests cover timeout reporting, held pipes, reader cancellation, UTF-8 truncation, and large scripts.

Changes: Hook output capture

Layer / File(s)	Summary
Concurrent capture and bounded reporting `crates/minimald/src/hooks.rs`	The runner drains captured stdout and stderr while writing the script. It retains up to 4096 bytes per stream, preserves output on success, failure, and timeout, and limits reader joining after timeout. Tests cover timeout output, held pipes, cancellation, UTF-8 truncation, and large scripts.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: evanspearman

Merge Risk: 🔵 Low · up to 6661e

Hook output capture is mergeable with a bounded test gap: the UTF-8 test should exercise the split it is intended to protect.

Pre-merge checks: 5

Check name	Status	Explanation
Title check		The title follows the required Conventional Commit format and clearly describes the main change: retaining captured output from timed-out hooks.
Description check		The description explains the problem, implementation, and verification results. It includes testing evidence, but it omits the template's Checklist section and does not explicitly state whether docume…
Docstring Coverage		Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 1 files.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Description check

Explanation

The description explains the problem, implementation, and verification results. It includes testing evidence, but it omits the template's Checklist section and does not explicitly state whether documentation or a breaking change applies.

✨ Finishing Touches

📝 Generate docstrings

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{A rabbit watched the pipe tails flow,
While stdout and stderr stole the show.
The clock rang out; the readers stayed,
Their bounded little baskets made.
Then off hopped scripts, with prints in tow.}

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

Dropping a JoinHandle detaches its task, so a reader blocked on a pipe that a backgrounded grandchild holds open kept running (and kept its span open) after the run reported. TailReader now aborts its task on drop, which covers the bounded join, the failed wait and early returns. The timeout tests get a 3 s deadline so the pre-hang echo cannot miss it on a loaded runner.

Spawn the pipe readers before the stdin write, so a script larger than the stdin pipe whose early commands fill stdout is not stalled until the deadline, and the stdin-failure path keeps its output. Drop the continuation bytes a raw tail cut splits, so the kept tail does not open on U+FFFD. Note in run_hooks that output collection can add TAIL_JOIN_BUDGET past a budgeted run's deadline.

norrietaylor · 2026-10-18T07:03:06Z

/review

minimal-foundry · 2026-10-18T07:03:12Z

Approved: no findings at 6661e5aa1d07.

Nothing to do.

Clobber: none: git log -- crates/minimald/src/hooks.rs shows the three fix commits on top of the box-rename refactors, no unrelated work reverted.

minimal-foundry