fix(mcp): bound ShellTool process wait so hung commands cannot block forever

[SebTardif]

What Problem This Solves

ShellTool.execute() drains stdout/stderr concurrently (fixed in #292) but previously waited with no kill path for hung children and no opt-in deadline. Hung commands can block the MCP agent loop forever.

This PR adds an opt-in timeout that kills the shell process tree and bounds pipe draining. Omitting timeout preserves unlimited execution (legacy MCP contract).

A follow-up fix corrects process-tree enumeration: proc_listchildpids returns a PID count, not a byte length. Mis-dividing by pid_t stride skipped children, so only the root was signaled while descendants could keep running.

Call chain

ShellTool.execute()
  -> Process.run()
  -> non-blocking concurrent pipe drains
  -> wait (unlimited unless timeout set)
  -> on explicit timeout: walk children via proc_listchildpids (count), signal tree
  -> bound remaining pipe drain (close FDs if needed)

Evidence

Live terminal proof from the patched branch:

$ swift scripts/prove-shell-tool-timeout.swift
ShellTool process timeout proof (correct child count + descendant gone)
  case1: hung sleep with timeout=1
  timedOut=true status=15 elapsed=1.01s
  case2: fast echo with timeout=5
  timedOut=false status=0 elapsed=0.01s
  case3: descendant holds stdout; record child pid and verify gone after timeout=1
  timedOut=true status=15 elapsed=1.01s
  childPid=64982 childAliveAfterTimeout=false
  expected: timedOut=true elapsed<5s childAliveAfterTimeout=false
  case4: omitted timeout allows multi-second command
  timedOut=false status=0 elapsed=2.02s
PROOF_OK hang, success, process-tree kill (descendant gone), omitted-timeout unlimited

Key line: childAliveAfterTimeout=false with a recorded background sleep PID after timeout=1.

Summary

• Optional timeout: omit for unlimited (legacy); pass to opt into bounded execution.
• Explicit timeout walks the process tree with correct PID count, SIGTERM then SIGKILL.
• Bounded post-timeout pipe drain so a stuck writer cannot hang the agent forever.
• Tests: hang, process-tree response bound, recorded descendant PID gone, omitted multi-second success, resolveTimeout contract.

Real behavior proof

• Behavior or issue addressed: ShellTool could hang forever on non-exiting commands; opt-in timeout must kill descendants (not only bash root) and must not invent a default deadline when timeout is omitted.
• Real environment tested: macOS arm64, Swift toolchain, worktree /tmp/pr-peekaboo-shell-timeout on branch fix/shell-tool-process-timeout at head 8ec5568a.
• Exact steps or command run after this patch:
  1. swift scripts/prove-shell-tool-timeout.swift
  2. swift test --package-path Core/PeekabooCore --filter 'Shell tool'
• Evidence after fix: prove script output above: hang and process-tree cases return in ~1s; recorded descendant PID is gone after timeout; omitted timeout still allows sleep 2 success.
• Observed result after fix: Explicit timeout terminates the root and descendants; MCP response is bounded; legacy unlimited path preserved when timeout is omitted.
• What was not tested: Full MCP client session against a live gateway; capture-action CLI path (already covered by fix(capture): bound action process wait so kill failure cannot hang forever #215).

Related

• fix(mcp): read pipe data before waitUntilExit to prevent deadlock #292 — pipe deadlock; merged
• fix(capture): bound action process wait so kill failure cannot hang forever #215 — capture action process wait bound
• ClawSweeper: fixed P1 child-count walk; compatibility (omit timeout unlimited) retained

Test plan

• swift scripts/prove-shell-tool-timeout.swift → PROOF_OK (descendant gone)
• swift test --package-path Core/PeekabooCore --filter 'Shell tool' → 10 passed
• swiftlint clean on changed files

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed July 30, 2026, 4:14 PM ET / 20:14 UTC.

ClawSweeper review

What this changes

This PR adds an optional timeout to the MCP shell tool that terminates the shell process tree and bounds post-timeout output draining, while retaining unlimited execution when callers omit the timeout.

Merge readiness

⚠️ Ready for maintainer review - 3 items remain

Keep this PR open for maintainer review. The contributor supplied credible terminal proof that explicit timeouts return promptly, terminate a recorded descendant, and preserve the unlimited legacy path when timeout is omitted; however, the macOS process-tree implementation snapshots numeric PIDs and later signals them, leaving a narrow PID-reuse risk that can target an unrelated process.

Priority: P1
Reviewed head: 8ec5568a0a7f5a3031cd16bbece9d1fcc334a0d8
Owner decision: Required. See Decision needed.

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	Strong focused proof and regression coverage support the intended behavior, with one remaining maintainer-level process-identity risk.
Proof confidence	🦞 diamond lobster (5/6)	Sufficient (terminal): The PR body provides after-fix terminal output from a macOS run showing bounded timeout return, recorded descendant cleanup, normal completion, and preserved omitted-timeout behavior.
Patch quality	🐚 platinum hermit (4/6)	Security review found an item that needs attention.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (terminal): The PR body provides after-fix terminal output from a macOS run showing bounded timeout return, recorded descendant cleanup, normal completion, and preserved omitted-timeout behavior.
Evidence reviewed	4 items	Submitted real-behavior proof: The PR body reports a patched-branch terminal run where a one-second timeout returns in about one second, a recorded descendant PID is no longer alive after timeout, and an omitted timeout permits a two-second command to complete. Compatibility contract: The PR commits and tests explicitly changed the initial silent 30-second default to an opt-in timeout, preserving unlimited execution for omitted, zero, negative, or invalid values. Feature-history provenance: The related merged shell-pipe fix established the current ShellTool process/pipe surface, and this PR builds on that same path rather than duplicating a separate execution mechanism.
Findings	None	None.
Security	Needs attention	Avoid signaling recycled descendant PIDs: The timeout path walks numeric child PIDs and subsequently signals them. A child that exits before that signal can have its PID reused, allowing the recursive kill to target an unrelated process rather than the original command tree.

How this fits together

Peekaboo’s MCP shell tool runs commands on behalf of agent workflows and returns their stdout, stderr, and exit result to the MCP client. The proposed timeout path changes how that tool waits for child processes, drains inherited pipes, and recovers the agent loop from a hung command.

flowchart LR
  A[MCP shell request] --> B[Shell tool]
  B --> C[Shell process and output pipes]
  C --> D{Timeout supplied?}
  D -->|No| E[Wait for normal completion]
  D -->|Yes| F[Enumerate and stop descendants]
  F --> G[Bound remaining pipe drain]
  E --> H[MCP tool response]
  G --> H

Loading

Decision needed

Question	Recommendation
Should ShellTool’s opt-in timeout be allowed to recursively signal a snapshot of numeric descendant PIDs, or must it use an identity-safe process-group or equivalent termination boundary before merge?	Use identity-safe containment: Revise the timeout path to terminate a dedicated process group or equivalent identity-safe boundary, then prove descendant cleanup and legacy no-timeout behavior.

Why: The functional behavior is proven, but choosing whether a low-probability PID-reuse path may signal an unrelated local process is a safety boundary and contract decision rather than a mechanical test gap.

Before merge

• Resolve security concern: Avoid signaling recycled descendant PIDs - The timeout path walks numeric child PIDs and subsequently signals them. A child that exits before that signal can have its PID reused, allowing the recursive kill to target an unrelated process rather than the original command tree.
• Resolve merge risk (P2) - The timeout code enumerates descendants as numeric PIDs and later signals them recursively. If a short-lived child exits and macOS recycles its PID before the corresponding signal, the timeout path could signal an unrelated process, creating an availability and process-isolation risk not covered by the descendant-liveness proof.

Findings

• [medium] Avoid signaling recycled descendant PIDs

Agent review details

Security

Needs attention: The patch does not add dependencies or broaden credentials, but its PID-snapshot termination strategy can cross the intended local-process boundary if PID reuse occurs.

Review metrics

Metric	Value	Why it matters
Patch surface	3 files affected; 575 additions, 6 deletions	Most of the patch is process-lifecycle handling and targeted regression/proof coverage rather than broad MCP API churn.
Covered scenarios	4 live proof cases; 10 focused tests reported	The submitted evidence covers timeout, ordinary completion, descendant cleanup, and omitted-timeout compatibility.

Root-cause cluster

Relationship: canonical
Canonical: #298
Summary: This PR is the remaining implementation candidate for ShellTool hangs after the related merged pipe-deadlock repair.

Members:

• partial_overlap: fix(mcp): read pipe data before waitUntilExit to prevent deadlock #292 - That merged PR fixed pipe-buffer deadlock by draining output concurrently; this PR addresses a separate non-exiting-process hang after that path.
• adjacent_distinct: fix(capture): bound action process wait so kill failure cannot hang forever #215 - That merged PR bounds process waiting for the capture-action runner, while this PR changes the separate MCP ShellTool lifecycle.

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

1. Replace PID snapshots with process containment (recommended)
   Put timed commands in a dedicated process group or another identity-safe boundary, then terminate that boundary and retain the current explicit-timeout and pipe-drain tests.
2. Approve the local-process tradeoff
   If maintainers accept the rare PID-reuse possibility for an explicitly requested privileged shell timeout, document that decision and merge with the present proof.
3. Defer the opt-in timeout
   Pause this PR if an identity-safe process-control primitive is unavailable or unsuitable for the supported macOS versions.

Technical review

Best possible solution:

Use a process-group or otherwise identity-safe containment strategy for timeout termination, or obtain an explicit maintainer decision that the bounded PID-reuse risk is acceptable for this privileged local shell tool.

Do we have a high-confidence way to reproduce the issue?

Yes for the intended timeout behavior: the PR gives a concrete macOS terminal scenario using a hung command and a recorded descendant PID. The PID-reuse concern is source-reproducible as a race analysis, but not deterministically demonstrated by the submitted test.

Is this the best way to solve the issue?

Unclear. The opt-in contract, descendant cleanup, and bounded drain are well targeted, but recursive signaling of a numeric PID snapshot is not the safest process-identity boundary.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 8448599ab899.

Labels

Label justifications:

• P1: A hung command in the MCP shell tool can block an agent workflow indefinitely, while the proposed recovery path also controls local processes.
• merge-risk: 🚨 availability: Timeout termination can affect whether the agent regains control and whether child processes remain running.
• merge-risk: 🚨 security-boundary: A recycled descendant PID could cause the timeout path to signal a process outside the original command tree.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
• status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body provides after-fix terminal output from a macOS run showing bounded timeout return, recorded descendant cleanup, normal completion, and preserved omitted-timeout behavior.
• proof: sufficient: Contributor real behavior proof is sufficient. The PR body provides after-fix terminal output from a macOS run showing bounded timeout return, recorded descendant cleanup, normal completion, and preserved omitted-timeout behavior.

Evidence

Security concerns:

• [medium] Avoid signaling recycled descendant PIDs
  The timeout path walks numeric child PIDs and subsequently signals them. A child that exits before that signal can have its PID reused, allowing the recursive kill to target an unrelated process rather than the original command tree.
  Confidence: 0.78

What I checked:

• Submitted real-behavior proof: The PR body reports a patched-branch terminal run where a one-second timeout returns in about one second, a recorded descendant PID is no longer alive after timeout, and an omitted timeout permits a two-second command to complete. (scripts/prove-shell-tool-timeout.swift:1, 8ec5568a0a7f)
• Compatibility contract: The PR commits and tests explicitly changed the initial silent 30-second default to an opt-in timeout, preserving unlimited execution for omitted, zero, negative, or invalid values. (Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift:466, 8ec5568a0a7f)
• Feature-history provenance: The related merged shell-pipe fix established the current ShellTool process/pipe surface, and this PR builds on that same path rather than duplicating a separate execution mechanism. (Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift:1, ff2bffa5763a)
• Prior review continuity: Earlier cycles’ bounded-drain, omitted-timeout compatibility, and child-count defects were addressed in subsequent commits; the last completed review recorded no line-level finding but retained maintainer review for the remaining process-identity risk. (Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift:1, 8ec5568a0a7f)

Likely related people:

• SebTardif: Authored the previously merged ShellTool pipe-deadlock repair and all commits in this PR, providing established history on the affected command-execution path. (role: merged feature contributor; confidence: high; commits: ff2bffa5763a, 8ec5568a0a7f; files: Core/PeekabooCore/Sources/PeekabooAgentRuntime/MCP/Tools/ShellTool.swift, Core/PeekabooCore/Tests/PeekabooTests/MCP/MCPSpecificToolTests.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

• Choose an identity-safe timeout containment mechanism or explicitly approve the current PID-reuse tradeoff.

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (9 earlier review cycles; latest 8 shown)

• reviewed 2026-07-27T20:45:23.558Z sha 8f5b017 :: found issues before merge. :: [P1] Preserve no-timeout behavior or approve the new default
• reviewed 2026-07-27T22:51:37.502Z sha a838537 :: needs real behavior proof before merge. :: [P1] Use the returned child count when walking the process tree
• reviewed 2026-07-28T00:28:31.619Z sha 8ec5568 :: needs maintainer review before merge. :: none
• reviewed 2026-07-29T20:22:36.499Z sha 8ec5568 :: needs maintainer review before merge. :: none
• reviewed 2026-07-29T21:46:12.387Z sha 8ec5568 :: needs maintainer review before merge. :: none
• reviewed 2026-07-29T22:18:52.864Z sha 8ec5568 :: needs maintainer review before merge. :: none
• reviewed 2026-07-30T07:39:01.903Z sha 8ec5568 :: needs maintainer review before merge. :: none
• reviewed 2026-07-30T08:15:01.103Z sha 8ec5568 :: needs maintainer review before merge. :: none

[SebTardif]

@clawsweeper re-review

Addressed the P2 process-tree / pipe-drain finding:

1. Timeout now recursively signals the shell process tree via proc_listchildpids (SIGTERM then SIGKILL), not only the root bash pid.
2. Pipe drains use non-blocking reads; after timeout a short bound races the drains and closes FDs so a surviving writer cannot hang the MCP response.
3. New regression: sleep 120 & while true; do sleep 1; done with timeout=1 returns in ~1s.
4. Live proof refreshed in the PR body (scripts/prove-shell-tool-timeout.swift case3).

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

[SebTardif]

@clawsweeper re-review

Addressed the P1 omitted-timeout compatibility finding:

1. Omitting timeout (or invalid values) no longer invents a 30s default; wait is unlimited (legacy MCP contract).
2. Explicit timeout still applies process-tree SIGTERM/SIGKILL and bounded pipe drain.
3. Tests: resolveTimeout contract + sleep 2 without timeout succeeds (~2s).
4. Schema/docs updated to state timeout is optional/opt-in.

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

[SebTardif]

@clawsweeper re-review

Fixed the P1 proc_listchildpids child-count bug:

1. Return value is treated as a PID count (not bytes). buffersize remains bytes.
2. Regression: background sleep PID is written to a temp file; after timeout=1, kill(pid, 0) fails (process gone).
3. Prove script case3 prints childPid=… childAliveAfterTimeout=false.
4. Opt-in timeout / omitted unlimited contract unchanged.

[clawsweeper]

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

• State: Review in progress
• Detail: The exact-review queue leased this run; Codex is reviewing the item.
• Run: https://github.com/openclaw/clawsweeper/actions/runs/30525810314
• Updated: 2026-07-30T08:13:33.594Z