security: kill the child process group when a command times out (#23)

[igmarin]

Why Timeout + capture3 failed to kill the child

RunCommand.execute previously wrapped Open3.capture3 in Timeout.timeout(max_time). When the deadline fired, Timeout::Error was raised in the Ruby thread, but Open3.capture3's own ensure calls wait_thr.value, which blocks waiting for the child to exit. The spawned process was therefore never signalled: a hung or runaway allowed command (e.g. an infinite test loop, a fork bomb via an allowed binary) kept running on the host while the caller got a "timed out" message. The command was also not placed in its own process group, so any children it forked were unreachable for cleanup.

The fix: process-group spawn + watchdog + TERM/KILL the group

execute now resolves the invocation and hands it to a new capture executor:

• Own process group: the command is spawned via Open3.popen3(*command, pgroup: true) (host adds chdir:), making the child its own process-group leader (pgid == pid).
• Watchdog deadline: wait_thr.join(max_time) waits up to the configured max_execution_time. If it returns nil the command overran.
• Kill the whole group: terminate_process_group sends SIGTERM to the negated pgid (Process.kill('TERM', -pgid)), waits a short grace period (TERM_GRACE_PERIOD = 2s), then escalates to SIGKILL if it has not exited. Signalling the negated group id reaps the command and any children it forked. signal_group ignores Errno::ESRCH (group already gone). popen3's block form performs the final reap on exit.

No-deadlock stdout/stderr reading

STDOUT and STDERR are drained on two separate threads (Thread.new { stdout.read }), so a chatty or hung child can never deadlock by filling a pipe buffer while the reader waits on the other stream. The reader threads are joined (readers.map(&:value)) before the block returns — after the child dies its write ends close, the reads hit EOF, and the threads finish cleanly.

Preserved behavior (fail-closed security + return shape)

This change only replaces the execution + timeout mechanism. All recently-merged guards are untouched:

• Empty-command guard, DANGEROUS_COMMANDS blocklist, allowed_commands allowlist, and the fail-closed HOST_EXECUTION_REFUSED (refuses host execution unless allow_host_execution is true or a container is active), plus the one-time host-execution warning.
• shellsplit + argv invocation (no shell interpretation).
• Identical success result shape (Exit Status: / STDOUT: / STDERR:) and the identical timeout string Error: Command execution timed out after N seconds. so callers/tests keying on it still work.
• Container path still builds the same docker exec -w /sandbox <id> ... invocation (now also timeout-protected and run in its own group).

Tests

• test_call_returns_timeout_result_without_waiting_for_full_runtime — a 1s timeout against sleep 30 returns the exact timeout string and the call comes back well before the full 30s sleep (proves the watchdog no longer blocks on wait_thr.value).
• test_call_kills_runaway_child_process_on_timeout — proof the child is actually terminated: after timeout, pgrep -P <pid> sleep finds no lingering child (the process group was killed and reaped). Verified meaningful: while alive the child is detected; after the group TERM + reap it is gone.
• Existing tests preserved/updated for the new mechanism: fast echo success, fail-closed refusal (now asserts Open3.popen3 is never invoked), host warning, and the container docker exec invocation.

Gates (all green from repo root)

• bundle exec rubocop — 272 files, no offenses
• bundle exec reek — clean
• bundle exec rake test — 762 runs, 0 failures, 0 errors (4 pre-existing skips)
• bundle exec rake yard:coverage — 0 undocumented public objects

Closes #23

[coderabbitai]

Warning

Review limit reached

@igmarin, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 16 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Repository UI (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: d9889344-8bbf-4d2e-a086-7d061359de6f

📥 Commits

Reviewing files that changed from the base of the PR and between 29c9a66 and 792fb24.

📒 Files selected for processing (2)

• lib/skill_bench/tools/run_command.rb
• test/evaluator/tools/run_command_test.rb

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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