[Bugfix #1241] Auto-restart only on unnatural exits — a deliberate quit ends cleanly

[waleedkadous]

Summary

Auto-restart now only fires on unnatural exits. A deliberate quit — double Ctrl+C, /quit, any clean shutdown — ends the agent instead of respawning it two seconds later.

Fixes #1241

Root Cause

Two surfaces respawned the agent, and both were exit-code blind:

1. Builder launch scripts — spawn-worktree.ts generates while true; do <agent>; sleep 2; done in five places (resume / role / no-role in startBuilderSession, role / no-role in buildWorktreeLaunchScript). The loop never looked at $?.
2. Shellper auto-restart — session-manager.ts setupAutoRestart incremented restartCount and re-SPAWNed on any exit. Builder sessions don't set restartOnExit, so this is the layer that governs architect terminals, which Tower launches directly with no bash loop.

So quitting forced the user to race a second Ctrl+C into the 2s window — and a mistimed one lands in the fresh agent. It also feeds the #1224 class: a respawn within ~2s can collide with the dying predecessor's session lock.

A third surface, not named in the issue, had to move with them: pty-session.ts prints [Process exited — restarting...] and arms a 10s wait-for-the-respawn timer whenever restartOnExit is set. Once layer 2 stops restarting clean exits, that notice is a lie and the timer tears the session down 10s later anyway.

The trap: code === 0 is not sufficient

node-pty reports a signal death as exit code 0 with the signal attached — probed directly here:

normal exit  -> {"exitCode":0,"signal":0}
SIGKILL      -> {"exitCode":0,"signal":9}
exit 3       -> {"exitCode":3,"signal":0}

ShellperProcess then stringifies that field. A naive code === 0 check would have stopped restarting killed agents — the exact opposite of what the issue asks for. Hence one shared predicate, isDeliberateExit(): code 0 and no signal (null, '' or '0').

Fix

File	Change
shellper-protocol.ts	isDeliberateExit() — the single predicate both layers use
session-manager.ts	Deliberate exit → log, emit session-clean-exit, drop the session. Not counted, not respawned. The shellper husk is left alive (no SIGTERM).
pty-session.ts	Deliberate exit → print [Agent exited at your request — not restarting.] and end cleanly; no false "restarting" notice, no 10s timer
spawn-worktree.ts	All five loops share one LAUNCH_LOOP_TAIL: exit 0 clears the screen, prints the hint, and gates the relaunch on Enter; EOF on stdin exits rather than spins. Nonzero/signal keeps the 2s auto-restart.

96 lines of source; the rest is tests.

One deviation from the prescribed design

The issue says the shellper layer should "leave the PTY open". For the architect that would wedge it: Tower gates architect creation on !entry.architects.has('main'), so a registered-but-dead terminal is unrelaunchable without a full workspace stop/start (which takes every builder with it). Ending cleanly clears the architect row, so afx workspace start brings the architect back. The shellper process itself is not killed. Builders are unaffected — their bash loop never exits on a clean quit, so they get the literal keypress-gated behavior the issue describes.

Not found: the Kimi variants

The issue mentions "the Kimi provider-owned variants" of the launch script. There is no Kimi provider in this repo (grep -ri kimi is empty) and all launch-loop generation lives in the five sites above — all covered.

Test Plan

• launch-loop-exit-code.test.ts (new) — executes the generated script under bash rather than pattern-matching it; a shell control-flow regression is only provable by running it. Exit 0 → agent launched exactly once, script exits 0, no restart message. One Enter → exactly two launches. Exit 7 → still loops, still prints Restarting in 2 seconds.
• shellper-protocol.test.ts — isDeliberateExit across code/signal combinations, including the code-0-with-signal case.
• session-manager.test.ts — clean exit does not spawn, does not increment restartCount, emits session-clean-exit, drops the session; signal death (code 0 + signal 9) and crash both still respawn. The real-shellper crashLoopFallback test now asserts the session ends rather than exhausting maxRestarts.
• tower-shellper-integration.test.ts — clean exit emits exit immediately with the at-your-request notice and nothing left armed; signal death still takes the restart path. Two pre-existing Dashboard: Auto-restart architect terminal when it exits #418 tests simulated exit 0 to mean "crash"; they now simulate a crash.
• spawn-worktree.test.ts — every generated variant (resume / role / no-role) carries the exit-0 gate, so a new launch-script code path can't be added without it.

Full suite: 3655 passed, 0 failures; build green.

Not exercised: a live architect quit against a running Tower — that needs a Tower restart, which would kill the active builder sessions. The shellper layer is covered by the real-shellper integration test (/bin/sh -c 'exit 0' under an actual shellper process), and the builder layer end-to-end by the bash-execution tests.

[waleedkadous]

CMAP 3-way review — unanimous APPROVE

Model	Verdict	Confidence	Key issues
Gemini	APPROVE	HIGH	None
Codex	APPROVE	HIGH	None
Claude	APPROVE	HIGH	None

All three independently confirmed the isDeliberateExit predicate handles the node-pty signal-death trap (SIGKILL → {exitCode: 0, signal: 9}), that all five launch-loop generation sites are covered, and that the architect deviation from "leave the PTY open" is sound.

Gemini — "Excellent, highly targeted bugfix with precise exit classification and execution-based test coverage across all restart layers."

Codex — "Correctly stops auto-restart on deliberate exits, preserves restart behavior for crashes/signals, and is protected by strong regression coverage." Verified the shared loop tail is applied in every script-generation path, not just the runtime start path.

Claude — "The bash-execution test is a standout — testing shell control flow by pattern-matching is fragile; executing it proves the behavior." Also confirmed the deliberate-exit branch is guarded by _restartOnExit, so non-restart sessions are unaffected, and that the pre-#1241 tests which used exit 0 to mean "crash" were correctly migrated to exit 1.

No changes requested by any reviewer; nothing to address.