Review summary: #4207

Change reviewed

This PR addresses flaky Windows lifecycle-hook tests where hook output could be truncated after the child process exited. It increases the non-PTY os/exec.Cmd.WaitDelay headroom from 1 second to 30 seconds. Normal commands whose output pipes close cleanly still complete immediately; the longer bound is observable only when output copying remains incomplete, such as a leaked pipe write end or an unusually slow post-exit drain.

To keep the existing leaked-pipe regression tests fast, the change also adds a positive process.Config.WaitDelay override. internal/shell carries that value into process.Config, preserves it through CloneWithStdin, and exposes a test-only WithProcessWaitDelay option from export_test.go. The process- and shell-level leaked-pipe tests set the override to one second rather than waiting for the new production default.

What I examined

• Read the complete five-file diff and the full changed files in internal/process and internal/shell.
• Checked Go 1.25.12's os/exec implementation and documentation for Cmd.WaitDelay: its timer bounds both post-cancellation process exit and post-exit I/O completion, and clean exits with pipes closed by the timer produce exec.ErrWaitDelay.
• Traced the affected paths through:
  • clicommand.agentLifecycleHook, which sends lifecycle-hook output through an io.Pipe and the non-PTY shell path;
  • internal/job.Executor, which constructs the shared shell used for job phases and hooks;
  • direct process.Config consumers in agent.JobRunner and Kubernetes bootstrap execution.
• Verified that the derived default remains max(SignalGracePeriod, 0) + waitDelayBuffer, so the agent's grace-period termination remains scheduled before os/exec's fallback kill. The explicit test override intentionally replaces that derived duration only in the two leaked-pipe tests.
• Checked the complete PR conversation and review threads. There were no inline comments, issue comments, outstanding earlier buildsworth concerns, or L2 approval grant. A prior human approval applies to an older revision.
• No Linear issue identifier was present in the branch name, PR title, or PR description, so there was no Linear issue to retrieve.

Checks run

Using the repository-pinned Go 1.25.12 toolchain:

• go test ./internal/process ./internal/shell — passed.
• go test ./internal/process ./internal/shell -count=5 — passed all five repetitions.
• go vet ./internal/process ./internal/shell — passed.
• go tool gofumpt -extra -d on all five changed files — produced no diff.

I also attempted the targeted lifecycle suite with:

go test ./clicommand -run 'TestAgentStartupHook|TestAgentShutdownHook|TestBuildCancel' -count=3

That package did not finish compiling in this environment: compilation of github.com/aws/aws-sdk-go-v2/service/ec2 was killed by the host, consistent with a resource limit. No targeted lifecycle tests ran in that command, and this was not a test assertion or code failure.

At submission time, the GitHub status for Buildkite agent build #13867 was still pending. Direct Buildkite build details were unavailable with the available API credentials, so I could not inspect its jobs or logs.

Findings and verdict

I found no code issues and posted no inline findings. The implementation is consistent with Cmd.WaitDelay semantics, keeps normal clean-exit latency unchanged, and gives the deliberately leaking tests a narrow override without exposing a production shell option.

The main trade-off is intentional and cross-cutting: a non-PTY process with output pipes that remain open can now delay completion for up to 30 additional seconds rather than one second. Because this changes the shared subprocess completion path used by hooks and jobs, I submitted a COMMENT review rather than an approval and noted that it warrants human review.

Submitted review: #4207 (review)

Trigger source: automatic.