fix(tools): stop unsandboxed shell hang after output cap

[adriannoes]

Summary

• Rewrite of the unsandboxed shell output-cap hang fix against current development (not a cherry-pick of the main-based Cursor draft).
• After the drain loop hits total >= max_len - 1, leftover children are reaped with WNOHANG + SIGKILL (same idea as sandbox_exec) instead of blocking waitpid(..., 0).
• The command runs in its own process group so sleep/grandchildren die with the shell, and the capture buffer is always NUL-terminated.

Supersedes #69.

Does not mix #79/#89 (allowlist / Landlock).

Test plan

• make test_shell / ./build/test_shell (test_shell_caps_output_without_hanging)
• CI=true make test
• make static

Made with Cursor

[cursor]

Summary

Focused rewrite of #69 against current development. Unsandboxed run_unsandboxed used to leave the drain loop once the result buffer filled, then block forever in waitpid(..., 0) if the child stayed alive without writing. That freezes agent_run (CLI, WebChat, inbound ASAP). The same fill path copied add + 1 bytes from a chunk whose NUL sat at buf[n], so a truncated last chunk wrote a data byte at result_buf[max_len-1] and left no terminator. agent.c then strdups a 4 KiB TOOL_RESULT_SIZE slot, so that was a real heap over-read, not only a test-buffer issue.

The new path matches sandbox_exec's WNOHANG + SIGKILL reap, adds a process group so sequential printf; sleep grandchildren die, always NUL-terminates, and retries poll on EINTR. I built test_shell with CI=true -Werror (linked -lm; this image has no libcurl) and reran it under GCC ASan+UBSan: 9/0. Sequential leftover sleep 9999 was gone. GitHub CI was still pending at review time.

Must Fix

None.

Should Fix

1. reap_running_child only calls kill_command_tree when the first waitpid(WNOHANG) is 0. If the shell already exited, or dies from SIGPIPE after close(pipefd[0]), the group is not signalled. I ran sleep 9998 & printf '%080d' 0 against this build: execute returned immediately and sleep 9998 was left with PPid 1. Sequential printf; sleep (the unit-test command) was reaped. Kill the process group first, then wait for the tracked pid. kill(-pid) still reaches leftover members after the shell is gone, as long as PGID equals the child pid.
2. test_shell_caps_output_without_hanging only checks return + a NUL + a 5s alarm. It does not scan /proc for leftover sleep, so a kill-the-shell-only implementation would pass. Same gap as test_timeout_kills_process on #89.
3. CHANGELOG Unreleased does not mention the hang or the missing NUL.

Nice to Have

• The final blocking waitpid(..., 0) after 2s of WNOHANG (copied from sandbox_exec) can hang again if the child is stuck in D-state.
• Output-cap kills set timed_out. The [Command timed out] suffix is skipped when the buffer is full (total == max_len - 1), so it does not fire on the cap path, but the flag name is misleading if that check is ever loosened.
• Unsandboxed child still inherits stdin and the pipe has no FD_CLOEXEC. sandbox_exec closes stdin and sets CLOEXEC on the read end.
• Assert truncated content (buf[0] == '0'), not only that some NUL exists.

Positive Highlights

• Right root cause: blocking waitpid after the cap, plus the add + 1 NUL skip. append_unsandboxed_output copies add bytes and writes the terminator itself.
• Process group via setpgid in both parent and child is the correct extra for the no-NEWPID path. kill(-pid) falls back to kill(pid) on ESRCH, so a failed setpgid does not SIGKILL the agent's own group.
• Tight scope: two files, no Landlock / allowlist mix. The new test hits the exact #69 trigger (printf then sleep, so SIGPIPE will not reap the child).

  

_{Sent by Cursor Automation: Adrianno’s personal code review}

[adriannoes]

Review follow-up in 76a763a:

Should fix

1. Process group is SIGKILL'd before waitpid (thread on reap_running_child).
2. Leftover sleep argv is counted after cap tests; background cmd & case added.
3. CHANGELOG Unreleased now records the hang + missing NUL (fix(tools): stop unsandboxed shell hang after output cap #69).

Nice to have (not in this PR)

• Final blocking waitpid(..., 0) after 2s of WNOHANG matches sandbox_exec. A D-state child is unkillable either way; not expanding timeout machinery here.
• Output-cap still sets timed_out, but [Command timed out] is skipped when total == max_len - 1, so it does not fire on the cap path. Renaming the flag is cosmetic.
• Closing stdin / FD_CLOEXEC on the unsandboxed pipe is sandbox_exec hygiene, not the hang/NUL leak. Out of scope vs fix(sandbox): keep agent workspace off gateway state files #79/fix(sandbox): Landlock FS bound plus workspace_only defense-in-depth #89.
• Truncated content is now asserted (buf[0] == '0').