fix(client): reap the bridge process tree on close

[superWorldSavior]

Problem

When acpx terminates an agent bridge, killAgentIfRunning (src/acp/client.ts) called child.kill(signal) — signalling only the bridge PID. The bridge (codex-acp etc.) spawns its own children: the model process and MCP servers (e.g. serena). Those descendants were never signalled, so they survived as orphans and accumulated until the agent app-server choked and new sessions hung at session/new.

Fix

On POSIX, spawn the bridge in its own process group (detached, pgid == pid) and signal the whole group on close (process.kill(-pid, signal)), gated on an agentGroupLeader flag so acpx never signals its own group. Windows and any non-detached path keep the previous single-PID child.kill.

A final best-effort group sweep also runs after the bridge PID itself has gone — e.g. a well-behaved bridge that exits during the stdin-close grace — so descendants that outlived the bridge are still reaped. Signal failures other than ESRCH are traced (verbose) rather than silently swallowed.

Scope & limitations (explicit)

• Process group, not full tree. Reaps descendants that stay in the bridge's group. A descendant that re-detaches into its own session (setsid) leaves the group and is out of scope.
• POSIX-only. On Windows agentGroupLeader is false and behaviour is unchanged (no group reaping). A Job-Object path can follow.
• Cooperative close only. This couples descendants to AcpClient.close(). It does not cover abrupt death of acpx itself (SIGKILL/OOM) — there is no PR_SET_PDEATHSIG here. Note that detaching also means the bridge no longer receives signals sent to acpx's process group; both are left to a follow-up that adds a real die-with-parent mechanism.
• Headless assumption. detached puts the bridge in a new session without a controlling terminal; tools that need /dev/tty directly would be affected. acpx bridges are headless ACP adapters, so this is expected.

Real behavior proof

Deterministic regression (--exit-on-stdin-end makes the mock bridge exit on stdin EOF, so only the post-exit sweep can reap the grandchild). Same test, with and without the fix:

# WITH the fix
✔ AcpClient close reaps grandchildren when the bridge exits during stdin-close
ℹ pass 1  ℹ fail 0

# WITHOUT the fix (git stash of src/acp/client.ts)
✖ AcpClient close reaps grandchildren when the bridge exits during stdin-close
  AssertionError: grandchild 57370 must be dead after close even when the bridge exited on its own
ℹ pass 0  ℹ fail 1

Full suite: 850 tests, 847 pass, 0 fail, 3 skipped. typecheck + lint clean.

Tests

test/client.test.ts (+ test/mock-agent.ts), real-process, Unix:

• grandchild reaped when the bridge is stubborn (--ignore-sigterm --stay-alive-after-stdin-end, forcing the group SIGKILL);
• grandchild reaped when the bridge exits cleanly on stdin-close (--exit-on-stdin-end, exercising the post-exit group sweep — the case ClawSweeper flagged).

---

Addresses the ClawSweeper review of the previous revision (the stdin-close descendant leak). Maintainer edits enabled (personal fork).

[clawsweeper]

Codex review: needs real behavior proof before merge. Reviewed July 4, 2026, 11:10 PM ET / 03:10 UTC.

Summary
This PR makes POSIX agent bridges detached process-group leaders, signals that group during AcpClient.close(), adds a final group sweep, and adds mock-agent regression tests for bridge grandchildren.

Reproducibility: yes. at source level: current main only calls child.kill(signal) on the bridge PID, so a bridge-spawned grandchild is not directly signaled. I did not run a live current-main repro in this read-only review.

Review metrics: 2 noteworthy metrics.

• Changed surface: 3 files changed, +232/-5. The diff changes runtime bridge spawning/teardown plus real-process test helpers.
• Check state: 1 failed, 1 in progress. Format was failing and Mutation had not completed when reviewed, so merge readiness is not settled by CI yet.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🦪 silver shellfish
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

• [P1] Add real behavior proof from a live adapter/MCP setup, with private details redacted.
• Address or get explicit maintainer approval for the detached POSIX parent-death/process-group signal tradeoff.
• [P1] Fix the failing Format check and wait for Mutation to complete.

Proof guidance:

• [P1] Needs real behavior proof before merge: The PR body includes mock-agent red/green output and test claims, but external runtime changes need live adapter proof such as a redacted terminal transcript or logs showing real bridge descendants are reaped. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Risk before merge

• [P1] Unconditional POSIX detached changes process-group signal inheritance; if acpx is killed or cannot run close(), bridges no longer receive process-group signals and may survive.
• [P1] Real behavior proof is mock-only; no live adapter run demonstrates descendant cleanup under the reported codex-acp/MCP failure mode.
• [P1] CI was not fully green at inspection time: Format failed and Mutation was still in progress.

Maintainer options:

1. Preserve parent-death signal safety (recommended)
   Add die-with-parent or equivalent process-group signal coverage, or avoid unconditional detached launch until bridges still die when acpx is killed before close completes.
2. Accept the detached POSIX contract
   A maintainer can explicitly approve the tradeoff that detached bridges may survive abrupt acpx death and require a tracked follow-up for parent-death handling.
3. Pause generic cleanup
   If maintainers do not want to change agent launch semantics now, keep the merged Kiro workaround and pause or close this broader process-group PR.

Next step before merge

• [P1] Needs maintainer decision on detached POSIX bridge compatibility plus contributor real-behavior proof; the right parent-death contract is not a safe automated repair choice.

Security
Cleared: No concrete security or supply-chain issue was found; the diff changes process spawning/teardown and tests without adding dependencies, workflows, secrets handling, or install-time execution.

Review findings

• [P1] Preserve process-group shutdown semantics — src/acp/client.ts:720

Review details

Best possible solution:

Land generic process-group cleanup only after preserving or explicitly replacing parent-death/process-group signal behavior, with live adapter proof that descendants are reaped without introducing a headless/signal regression.

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

Yes, at source level: current main only calls child.kill(signal) on the bridge PID, so a bridge-spawned grandchild is not directly signaled. I did not run a live current-main repro in this read-only review.

Is this the best way to solve the issue?

No, not as submitted. Process-group killing is a plausible narrow cooperative-close fix, but enabling detached for every POSIX bridge before parent-death and process-group signal semantics are handled is not the safest merge path.

Full review comments:

• [P1] Preserve process-group shutdown semantics — src/acp/client.ts:720
  Unconditionally setting detached on POSIX moves the bridge out of acpx's process group. Today a supervisor or terminal signal sent to the group can reach both acpx and the bridge; with this change, if acpx is killed or cannot run close(), the bridge and its descendants survive. The PR body calls this out as a follow-up, but that is the same orphan-process failure class this patch is meant to fix, so please keep the old signal safety as the default or add die-with-parent/group-signal coverage before enabling detached for every bridge.
  Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 6a24a546d234.

Label changes

Label justifications:

• P1: The PR targets orphaned bridge descendants that can accumulate until sessions hang, which is a broken agent workflow affecting real users.
• merge-risk: 🚨 compatibility: Moving every POSIX bridge into a detached process group changes how existing setups receive terminal or supervisor process-group signals.
• merge-risk: 🚨 availability: If acpx dies before close runs, the new detached bridge group can survive and recreate the orphan-process availability failure this PR is trying to fix.
• rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
• status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body includes mock-agent red/green output and test claims, but external runtime changes need live adapter proof such as a redacted terminal transcript or logs showing real bridge descendants are reaped. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence reviewed

What I checked:

• Repository policy applied: AGENTS.md was read fully; this runtime/session behavior change falls under the repo guidance requiring full validation and careful scrutiny of process/session behavior. (AGENTS.md:261, 6a24a546d234)
• Current main direct-PID teardown: Current AcpClient.close() reaches killAgentIfRunning(), which calls child.kill(signal) against only the spawned bridge PID. (src/acp/client.ts:1411, 6a24a546d234)
• PR detaches POSIX bridges: The PR head sets detached: agentGroupLeader for every non-Windows bridge spawn, moving POSIX bridges into their own process group. (src/acp/client.ts:720, e13ace6216a0)
• PR adds group kill and final sweep: The patch routes close-time SIGTERM/SIGKILL through process.kill(-pid, signal) and adds a final SIGKILL sweep after bridge exit. (src/acp/client.ts:1397, e13ace6216a0)
• Normal interrupt paths close the client: Existing interrupt handling calls client.close() for normal SIGINT/SIGTERM flows, so the uncovered compatibility concern is parent/process-group death before close can complete. (src/async-control.ts:36, 6a24a546d234)
• Proof is mock-only: The PR body provides red/green output from mock-agent tests and full-suite claims, but no live codex-acp/MCP or other real adapter run showing the reported orphan cleanup. (e13ace6216a0)

Likely related people:

• steipete: Recent path history shows queue-owner shutdown and mock-agent lifecycle work adjacent to this bridge teardown behavior. (role: recent lifecycle contributor; confidence: high; commits: 87327c6e4ac2, 43013ead1ab8; files: src/cli/session/queue-owner-runtime.ts, test/mock-agent.ts, test/queue-owner-lifecycle.test.ts)
• vincentkoc: GitHub path history shows repeated recent changes to src/acp/client.ts, test/client.test.ts, and queue/process hardening. (role: recent area contributor; confidence: high; commits: 77715c8d7c18, de042d12cd63, f29a0e5ae133; files: src/acp/client.ts, test/client.test.ts, src/cli/queue/lease-store.ts)
• superWorldSavior: Beyond authoring this PR, the contributor is credited on the merged queue-owner shutdown fix in the same lifecycle area. (role: adjacent process-lifecycle contributor; confidence: medium; commits: 87327c6e4ac2; files: src/cli/session/queue-owner-runtime.ts, test/mock-agent.ts, test/queue-owner-lifecycle.test.ts)

What the crustacean ranks mean

• 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
• 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
• 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
• 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
• 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
• 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
• 🌊 off-meta tidepool: rating does not apply to this item.

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.

How this review workflow works

• 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.

[superWorldSavior]

Updated to address the ClawSweeper review:

• Fixed the stdin-close descendant leak — a final best-effort process-group sweep now runs even when the bridge PID exits on its own during the stdin-close grace, so descendants (model process, MCP servers) are still reaped. New deterministic regression (--exit-on-stdin-end) is red without the fix / green with it.
• Error observability — group/PID signal failures other than ESRCH are now traced instead of silently swallowed.
• Documented the scope explicitly in the PR body: process group (not full tree), POSIX-only, cooperative-close only (abrupt death + re-detached descendants left to a follow-up), headless assumption.
• Real behavior proof (red-without / green-with transcript) added to the PR body; typecheck + lint clean, full suite green.

@clawsweeper re-review

[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: the existing ClawSweeper review comment will be edited in place when the review finishes.

[superWorldSavior]

Superseded by #433, which now carries this commit as its base and gates POSIX detachment on a working lifeline helper — so the process-group reaping and the abrupt-death watchdog ship as one coherent, no-regression change. Continuing the review on #433.