[lilyshen0722]

When the seat runner's supervisor kills a stalled turn, the kill now reaches everything the agent CLI started, not only the CLI's own process. Before this change, the kill reached only the direct child, so the CLI's MCP servers, tool commands and helper processes kept running on the host. An agent CLI that the supervisor kills is now started as the leader of its own process group, and it is killed as a group, together with any descendant that moved to a group of its own.

Stacked on #2701's branch (feat/seats-record-redact), which carries the supervisor.

What changes

• packages/seats/src/process-group.js (new).
  • spawnInOwnGroup starts a child detached and marks it. Nothing infers that a child leads a group; only a marked, live, unreaped child is ever signalled by negative pid.
  • killProcessTree reads the process table once (ps -A -o pid=,ppid=,lstart=) before any signal. It kills the child's descendants deepest first, then signals the group, or falls back to child.kill.
  • Before signalling each descendant, it re-reads that pid and kills it only if the ppid and the start time still match the snapshot. A pid that exited and was reused by another process on the host is left alone.
• packages/seats/src/supervisor.js. Sets ctx.ownProcessGroup = true for the adapter, tracks any group-leading child it reports through onSpawn (including a late one), and kills through killProcessTree.
• cli/src/lib/adapters/{claude,codex}.js.
  • Spawn through spawnInOwnGroup only when the supervisor asks for it.
  • Without that flag, the options reach spawn unchanged, so seats that run without the supervisor (every live seat today) spawn exactly as before.
  • The adapters' own timeout still signals the direct child only.
• Wrapper exit. While a tracked group is live, the wrapper kills it on exit, and on SIGTERM, SIGINT or SIGHUP when nothing else handles the signal. In that last case it then re-raises the signal so the wrapper still dies by it.
  • A host that handles the signal itself owns the timing. agent run does this: it finishes the in-flight turn on SIGTERM.
• cli/src/lib/seat-stop.js (new) and agent run.
  • On SIGTERM, agent run keeps the turn and arms a deadline, and its exit path calls killSupervisedGroups().
  • The deadline is the seat's launchd ExitTimeOut minus min(5 s, a quarter). It is read from ~/Library/LaunchAgents/me.commonly.seat-<name>.plist and never written. Unset, 0 or unreadable means 20 s, launchd's default, so the kill lands at 15 s.
  • With no supervised group live, both calls do nothing.

Choices to check

• The walk runs before the group signal, not after. Once the group dies, every descendant outside the group is reparented to 1, and a ppid check after that would skip all of them. Doing it the other way round would mean loosening the ppid match, so the order is: snapshot, verified walk with leaves first, then the group kill.
• What the walk does not catch:
  • a descendant reparented to 1 mid-walk (skipped, never signalled on a guess);
  • a process started after the snapshot, such as a parent respawning a child it just lost;
  • a SIGKILL sent to the wrapper itself, which no handler can catch.
• A seat the daemon spawns has no plist, so it gets the 20 s default. A daemon handoff therefore cuts a supervised turn that is still running at 15 s. Unsupervised turns are unaffected.

Why codex needed the walk

ps on live seats showed codex starting its MCP server, its helper processes and every tool command (jest, tsc, vite) in a group of its own. A group kill of codex alone reaches only the launcher and the binary. Claude's MCP servers share its group, and only its Bash-tool shells move out.

Tests

All runs were in a container, under the shared test lock.

Suite	Result
New seats tests (supervisor-group-kill, supervisor-tree-and-deadline)	15/15
packages/seats	167/168
cli adapter, seats-package and seat-stop tests	508 passed, 1 failed, 9 skipped
eslint on the changed src files	clean

Both failures also fail on the base sha a1e3d9ec4:

• valet-env's positive control needs ~/.claude on the host.
• claude.environment "bwrap off darwin" is a Linux-only check failing on macOS.

The new tests cover:

• a grandchild in a new group is gone after a stall kill;
• a reused pid is never signalled;
• SIGTERM mid-turn: the turn completes, then the groups die;
• a hit deadline kills the groups before ExitTimeOut;
• a child that doesn't lead a group gets child.kill and never a negative pid;
• an unsupervised spawn carries no detached.

Mutants, each of which fails its target test while the rest still pass:

Mutant	Fails
Walk removed	the new-group grandchild survives; the pid-reuse test's real descendant survives
ppid and start check removed	the reused, unrelated pid gets signalled
Start-time check only removed	the reused, unrelated pid gets signalled
Kill on signal even with a host handler	"SIGTERM mid-turn, the turn completes"; the deadline test
Deadline timer does nothing	the group outlives ExitTimeOut
seat-stop doesn't arm the deadline	2 seat-stop tests

No version lines changed.

🤖 Generated with Claude Code

https://claude.ai/code/session_013ENddpGzhE7S8qjDSYAMuu

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

• Source mode: Auto
• Repository: Team-Commonly/commonly
• Branch: feat/jellymoon-record
• File: .coderabbit.yaml
• Commit: 710b91b
• Layers: Repository YAML
• Review profile: QUIET
• Plan: Advanced
• Run ID: 906585e5-04d0-42a3-82e2-140aed20fca4

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

[lilyshen0722]

Reviewed at 034edeb (process-group.js, supervisor.js, seat-stop.js, agent.js, adapters).

The group and tree kill is right. Only a marked, unreaped child is signalled by negative pid. Each descendant is re-read for ppid and start time before it is signalled, leaves go first, and the host-owned SIGTERM path yields to the host through the listenerCount check.

One blocker: daemon-spawned seats get a deadline nothing enforces. createSeatStop arms groupKillDeadlineMs(readExitTimeOut(plist)) on every SIGTERM. When there is no plist it falls back to 20 s and kills the groups at 15 s. But a daemon seat is stopped by daemon-supervisor.js with seat.child.kill('SIGTERM') (stopSeat, and the ready === 'changed' restart), and the daemon never follows that with a SIGKILL. So no deadline is coming. Today the in-flight turn finishes. With this PR, its MCP servers and tool shells are SIGKILLed 15 s after any reassignment or config change, which cuts the turn this SIGTERM path exists to protect. Fix: arm the deadline only when readExitTimeOut found a plist, meaning launchd is the parent. Otherwise rely on onExit, which already kills the groups once the turn ends. One test each way.

On the other two questions:

• Ordering (walk first, then group kill) is acceptable. A pid reparented to 1 during the walk, or a child that calls setsid after the snapshot, is missed. Write that down as a known residual in the process-group.js header. Don't add a second pass.
• SIGINT and SIGHUP are covered. SIGINT goes to the host's requestStop with no deadline, and onExit kills the groups. SIGHUP has no host listener, so the module's own handler kills the groups and re-raises. No change.

GATE: HOLD

[lilyshen0722]

Re-gate at 68ced27. That's one commit on 034edeb, which I reviewed earlier.

The blocker is fixed. createSeatStop now arms the deadline only when readSeatPlist finds the seat's plist. A daemon seat has no plist, so it arms nothing and relies on onExit. A reassignment or config change no longer cuts its in-flight turn. If a plist exists but has no usable ExitTimeOut, it still falls back to launchd's 20 s, which is correct because launchd will SIGKILL that seat. The real-process test covers both branches, and the arm-regardless mutant fails exactly the two no-plist cases. The two residuals of the walk are now written in the process-group.js header.

Non-blocking, worth a row: a seat moved from launchd to the daemon that still has its old plist on disk would arm the deadline, even though nothing will SIGKILL it. Removing the plist at migration time fixes that. Checking that launchd actually has the job loaded (launchctl print gui/$UID/me.commonly.seat-<name>) would too. That's a stricter version of the same check, not a reason to hold this PR.

GATE: PASS

[lilyshen0722]

Re-gate at 21ccefa, after the rebase onto feat/jellymoon-record at 710b91b (#2701).

I checked the rebase with git range-diff against 68ced27, my previous PASS. Commits 2 through 5 are identical. Commit 1 differs only in its import line in supervisor.js: it now imports redactKnownCredentials from #2701 alongside the process-group import. Every other hunk applied unchanged, so the review at 68ced27 covers this code. There are 5 commits ahead of the base, all by Lily. The plist-existence fix and the residuals note are both still in commit 5.

GATE: PASS