diff --git a/codev/projects/bugfix-1241-shellper-launch-loops-auto-res/status.yaml b/codev/projects/bugfix-1241-shellper-launch-loops-auto-res/status.yaml new file mode 100644 index 000000000..d0f1cf667 --- /dev/null +++ b/codev/projects/bugfix-1241-shellper-launch-loops-auto-res/status.yaml @@ -0,0 +1,17 @@ +id: bugfix-1241 +title: shellper-launch-loops-auto-res +protocol: bugfix +phase: pr +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: approved + requested_at: '2026-07-25T09:59:32.448Z' + approved_at: '2026-07-25T12:32:57.528Z' +iteration: 1 +build_complete: false +history: [] +started_at: '2026-07-25T09:40:46.551Z' +updated_at: '2026-07-25T12:32:57.529Z' +pr_ready_for_human: false diff --git a/codev/state/bugfix-1241_thread.md b/codev/state/bugfix-1241_thread.md new file mode 100644 index 000000000..80d463cf2 --- /dev/null +++ b/codev/state/bugfix-1241_thread.md @@ -0,0 +1,65 @@ +# bugfix-1241 — auto-restart should only trigger on unnatural exits + +## Investigate + +**Repro / mechanism** (read from code, plus a node-pty probe): + +- Builder terminals: `.builder-start.sh` is a `while true; do ; echo "Agent exited. + Restarting in 2 seconds..."; sleep 2; done` loop, generated in 5 places in + `packages/codev/src/agent-farm/commands/spawn-worktree.ts` + (resume / role / no-role in `startBuilderSession`, role / no-role in + `buildWorktreeLaunchScript`). The loop is exit-code blind — a clean `/quit` + (exit 0) respawns exactly like a crash. +- Architect terminals: launched directly by Tower (`tower-instances.ts`, no bash + loop) with `restartOnExit: true`, so the respawn comes from + `session-manager.ts` `setupAutoRestart`, which is also exit-code blind: it + increments `restartCount` and re-SPAWNs on *any* exit. +- Builder sessions do NOT set `restartOnExit`, so the two layers are cleanly + split: layer 1 = builders, layer 2 = architects. + +**Key finding — the naive `code === 0` test is wrong.** node-pty reports signal +deaths as `exitCode 0` plus a signal (probed here: SIGKILL → +`{exitCode: 0, signal: 9}`; normal exit → `{exitCode: 0, signal: 0}`), and +`shellper-process.ts` stringifies that field. So "deliberate quit" must be +`code === 0 && signal in (null, '', '0')`, otherwise a SIGKILLed agent would +stop restarting — the opposite of what the issue asks. + +**Note on the issue text**: it mentions "the Kimi provider-owned variants" of +the launch script. There is no Kimi provider in this repo (`grep -ri kimi` is +empty); all launch-loop generation lives in the 5 sites above, and they are all +covered. + +**Third surface found (not named in the issue)**: `pty-session.ts` +`attachShellper`'s exit handler prints `[Process exited — restarting...]` and +arms a 10s "wait for the restart" timer whenever `restartOnExit` is set. With +the layer-2 fix in place that restart never comes, so it must also branch on a +deliberate exit — otherwise a clean architect quit shows a false "restarting" +notice and then tears down 10 seconds later. + +Scope: 3 source files + tests, well under 300 LOC. BUGFIX-appropriate. + +## Fix + +- `shellper-protocol.ts`: `isDeliberateExit()` — the one predicate both layers use. +- `session-manager.ts`: deliberate exit → log, emit `session-clean-exit`, drop + the dead session, do not count it, do not respawn. +- `pty-session.ts`: deliberate exit → print the clean-exit line and end cleanly + (no false "restarting" notice, no 10s timer). +- `spawn-worktree.ts`: all 5 loops share one `LAUNCH_LOOP_TAIL` — exit 0 clears + the screen and gates the relaunch on Enter; EOF on stdin exits instead of + spinning; nonzero/signal keeps the 2s auto-restart. + +Deviation from the issue's "leave the PTY open" for the architect: the session +is dropped from SessionManager and PtySession emits `exit`, because Tower's +`workspace start` is gated on `!entry.architects.has('main')` — keeping the +registered-but-dead terminal would make the architect unrelaunchable without a +full workspace stop/start. Ending cleanly clears the architect row, so +`afx workspace start` brings it back. The shellper husk itself is not killed. + +## PR + +PR #1244 opened. CMAP 3-way: gemini=APPROVE, codex=APPROVE, claude=APPROVE — all +HIGH confidence, zero key issues, nothing to address. Results posted as a PR +comment. Architect notified. Waiting at the `pr` gate. + +Full suite green throughout: 3655 passed / 0 failures; build green. diff --git a/packages/codev/src/agent-farm/__tests__/launch-loop-exit-code.test.ts b/packages/codev/src/agent-farm/__tests__/launch-loop-exit-code.test.ts new file mode 100644 index 000000000..01b099800 --- /dev/null +++ b/packages/codev/src/agent-farm/__tests__/launch-loop-exit-code.test.ts @@ -0,0 +1,95 @@ +/** + * Bugfix #1241 — the builder launch loop must only auto-restart on unnatural + * exits. A deliberate quit (exit 0: double Ctrl+C, `/quit`) ends the loop and + * waits for a keypress instead of respawning. + * + * These tests EXECUTE the generated script with bash rather than pattern-match + * it: the regression is a shell control-flow bug, and only running it proves + * the agent was launched once instead of in a loop. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { buildWorktreeLaunchScript } from '../commands/spawn-worktree.js'; + +let worktree: string; +let counter: string; + +/** Build the real launch script with a fake agent that exits with `code`. */ +function writeLaunchScript(code: number): string { + // The fake agent records each launch, so the test can count respawns. + const baseCmd = `sh -c "echo run >> '${counter}'; exit ${code}"`; + const script = buildWorktreeLaunchScript(worktree, baseCmd, null, worktree); + const scriptPath = path.join(worktree, 'launch.sh'); + fs.writeFileSync(scriptPath, script); + fs.chmodSync(scriptPath, 0o755); + return scriptPath; +} + +function runCount(): number { + if (!fs.existsSync(counter)) return 0; + return fs.readFileSync(counter, 'utf-8').trim().split('\n').filter(Boolean).length; +} + +beforeEach(() => { + worktree = fs.mkdtempSync(path.join(os.tmpdir(), 'codev-1241-')); + counter = path.join(worktree, 'runs.txt'); +}); + +afterEach(() => { + fs.rmSync(worktree, { recursive: true, force: true }); +}); + +describe('builder launch loop exit handling (Bugfix #1241)', () => { + it('does not respawn the agent after a deliberate exit (code 0)', () => { + const scriptPath = writeLaunchScript(0); + + // stdin is closed, so the relaunch prompt reads EOF and the script ends. + // Without the fix this loops forever and the timeout below kills it. + const result = spawnSync('/bin/bash', [scriptPath], { + stdio: ['ignore', 'pipe', 'pipe'], + encoding: 'utf-8', + timeout: 10_000, + }); + + expect(result.signal).toBeNull(); // exited on its own, was not timed out + expect(result.status).toBe(0); + expect(runCount()).toBe(1); + expect(result.stdout).toContain('Agent exited at your request'); + expect(result.stdout).not.toContain('Restarting in 2 seconds'); + }); + + it('relaunches once per keypress after a deliberate exit', () => { + const scriptPath = writeLaunchScript(0); + + // One Enter → one relaunch; then EOF ends the loop. + const result = spawnSync('/bin/bash', [scriptPath], { + input: '\n', + encoding: 'utf-8', + timeout: 10_000, + }); + + expect(result.status).toBe(0); + expect(runCount()).toBe(2); + }); + + it('still auto-restarts after a crash (nonzero exit)', () => { + const scriptPath = writeLaunchScript(7); + + // No natural end for a crash loop — kill it after two restart delays. + const result = spawnSync('/bin/bash', [scriptPath], { + stdio: ['ignore', 'pipe', 'pipe'], + encoding: 'utf-8', + timeout: 5_000, + killSignal: 'SIGKILL', + }); + + expect(runCount()).toBeGreaterThan(1); + expect(result.stdout).toContain('Restarting in 2 seconds'); + expect(result.stdout).toContain('code 7'); + expect(result.stdout).not.toContain('Agent exited at your request'); + }, 15_000); +}); diff --git a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts index 0d23c74f3..dfd5c1d18 100644 --- a/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts +++ b/packages/codev/src/agent-farm/__tests__/spawn-worktree.test.ts @@ -386,6 +386,29 @@ describe('spawn-worktree', () => { expect(script).not.toContain('--resume'); expect(script).toContain('--append-system-prompt'); }); + + // Bugfix #1241: every generated variant must gate the relaunch on exit + // code, not respawn blindly — this is the assertion that catches a new + // launch-script code path being added without the deliberate-quit branch. + it.each([ + ['resume', { sessionId: 'abc', scriptFragment: "--resume 'abc'" }, 'ROLE'], + ['role', undefined, 'ROLE'], + ['no role', undefined, null], + ] as const)('%s → script does not auto-restart on exit 0', async (name, resume, role) => { + await startBuilderSession( + { workspaceRoot: '/tmp/ws' } as any, + `b-${name}`, '/tmp/worktree', 'claude', + 'PROMPT', role, role ? 'codev' : null, resume, + ); + + const script = findScript()!; + expect(script).toContain('status=$?'); + expect(script).toContain('if [ "$status" -eq 0 ]; then'); + expect(script).toContain('Press Enter to relaunch'); + expect(script).toContain('read -r || exit 0'); + // The crash path is untouched. + expect(script).toContain('Restarting in 2 seconds'); + }); }); // ========================================================================= diff --git a/packages/codev/src/agent-farm/commands/spawn-worktree.ts b/packages/codev/src/agent-farm/commands/spawn-worktree.ts index 84432f3b7..37dc1b834 100644 --- a/packages/codev/src/agent-farm/commands/spawn-worktree.ts +++ b/packages/codev/src/agent-farm/commands/spawn-worktree.ts @@ -732,6 +732,34 @@ function installHarnessWorktreeFiles( } } +/** + * The tail shared by every builder launch loop, appended after the agent + * invocation inside `while true; do … done`. + * + * Issue #1241: exit code 0 is the user deliberately quitting (double Ctrl+C, + * `/quit`) — auto-respawning overrides that choice and forces them to race a + * second Ctrl+C into the sleep window, where a mistimed one lands in the fresh + * agent instead. It also feeds the #1224 class, where a respawn within ~2s + * collides with the dying predecessor's session lock. So a clean exit clears + * the screen and gates the relaunch on a keypress: recovery stays one keystroke + * away without anything happening on its own. Nonzero exits and signal deaths + * (bash reports those as 128+N) keep the historical auto-restart — that is what + * the loop is for. + * + * `read` failing means EOF on stdin, i.e. the terminal is gone; exit rather + * than spin the loop on an input that will never arrive. + */ +const LAUNCH_LOOP_TAIL = ` status=$? + if [ "$status" -eq 0 ]; then + clear + echo "Agent exited at your request. Press Enter to relaunch, or close this terminal." + read -r || exit 0 + continue + fi + echo "" + echo "Agent exited (code $status). Restarting in 2 seconds... (Ctrl+C to quit)" + sleep 2`; + /** * Start a terminal session for a builder. * @@ -767,9 +795,7 @@ export async function startBuilderSession( cd "${worktreePath}" while true; do ${baseCmd} ${resume.scriptFragment} - echo "" - echo "Agent exited. Restarting in 2 seconds... (Ctrl+C to quit)" - sleep 2 +${LAUNCH_LOOP_TAIL} done `; } else if (roleContent) { @@ -801,9 +827,7 @@ done cd "${worktreePath}" ${envBlock}while true; do ${baseCmd} ${fragment} "$(cat '${promptFile}')" - echo "" - echo "Agent exited. Restarting in 2 seconds... (Ctrl+C to quit)" - sleep 2 +${LAUNCH_LOOP_TAIL} done `; } else { @@ -819,9 +843,7 @@ done cd "${worktreePath}" while true; do ${baseCmd} "$(cat '${promptFile}')" - echo "" - echo "Agent exited. Restarting in 2 seconds... (Ctrl+C to quit)" - sleep 2 +${LAUNCH_LOOP_TAIL} done `; } @@ -894,9 +916,7 @@ export function buildWorktreeLaunchScript( cd "${worktreePath}" ${envBlock}while true; do ${baseCmd} ${fragment} - echo "" - echo "Agent exited. Restarting in 2 seconds... (Ctrl+C to quit)" - sleep 2 +${LAUNCH_LOOP_TAIL} done `; } @@ -907,9 +927,7 @@ done cd "${worktreePath}" while true; do ${baseCmd} - echo "" - echo "Agent exited. Restarting in 2 seconds... (Ctrl+C to quit)" - sleep 2 +${LAUNCH_LOOP_TAIL} done `; } diff --git a/packages/codev/src/terminal/__tests__/session-manager.test.ts b/packages/codev/src/terminal/__tests__/session-manager.test.ts index ef0b02b49..520b2aa4b 100644 --- a/packages/codev/src/terminal/__tests__/session-manager.test.ts +++ b/packages/codev/src/terminal/__tests__/session-manager.test.ts @@ -1318,8 +1318,10 @@ describe('SessionManager', () => { // Issue #1149: clean exits (code 0) never trigger the fallback — a user // quitting a healthy session repeatedly must not lose a valid resumable - // conversation. Spawns real shellper processes — skip in CI. - it.skipIf(!!process.env.CI)('does not apply crashLoopFallback on clean exits', async () => { + // conversation. Bugfix #1241 strengthened this: a clean exit is not + // restarted at all, so the session ends instead of exhausting maxRestarts. + // Spawns real shellper processes — skip in CI. + it.skipIf(!!process.env.CI)('does not restart or apply crashLoopFallback on clean exits', async () => { const shellperScript = path.resolve( path.dirname(new URL(import.meta.url).pathname), '../../../dist/terminal/shellper-main.js', @@ -1335,6 +1337,13 @@ describe('SessionManager', () => { const onApply = vi.fn(); const testEnv = { PATH: process.env.PATH || '/usr/bin:/bin' }; + // Subscribed before the session exists: `exit 0` can land during + // createSession's own await. + const cleanExits: string[] = []; + manager.on('session-clean-exit', (id: string) => cleanExits.push(id)); + const errors: string[] = []; + manager.on('session-error', (_id: string, err: Error) => errors.push(err.message)); + await manager.createSession({ sessionId: 'cleanexit-test', command: '/bin/sh', @@ -1358,20 +1367,16 @@ describe('SessionManager', () => { try { await manager.killSession('cleanexit-test'); } catch { /* noop */ } }); - // Clean exits still restart; the session exhausts maxRestarts without - // ever counting a failing exit. - const errorPromise = new Promise((resolve) => { - manager.on('session-error', (_id: string, err: Error) => { - if (err.message.includes('Max restarts')) { - resolve(err); - } - }); - }); - await Promise.race([ - errorPromise, - new Promise((_, reject) => setTimeout(() => reject(new Error('timeout waiting for max restarts')), 15000)), - ]); + // Bugfix #1241: the clean exit ends the session — no respawn, no + // give-up error, no fallback. + const deadline2 = Date.now() + 10000; + while (manager.getSessionInfo('cleanexit-test') && Date.now() < deadline2) { + await new Promise((resolve) => setTimeout(resolve, 100)); + } + expect(manager.getSessionInfo('cleanexit-test')).toBeNull(); + expect(cleanExits).toContain('cleanexit-test'); + expect(errors.some((m) => m.includes('Max restarts'))).toBe(false); expect(onApply).not.toHaveBeenCalled(); expect(fs.existsSync(sentinel)).toBe(false); }, 20000); @@ -2312,3 +2317,91 @@ describe('crash-loop give-up (Issue #1224)', () => { } }); }); + +describe('deliberate-quit exits are not restarted (Bugfix #1241)', () => { + // A double Ctrl+C / `/quit` is the user's decision — auto-restart exists for + // crashes, so a clean exit must not respawn. Signal deaths still must, and + // node-pty reports those as code 0 with a signal attached, so they are the + // interesting negative case. + function fakeSession(socketDir: string) { + const client = new EventEmitter() as any; + client.spawn = vi.fn(); + return { + client, + socketPath: path.join(socketDir, 'clean-exit.sock'), + pid: 424243, + startTime: 0, + options: { + sessionId: 'clean-1', + command: 'claude', + args: [], + cwd: '/tmp', + env: {}, + restartOnExit: true, + restartDelay: 1, + maxRestarts: 50, + }, + restartCount: 0, + restartResetTimer: null, + failingExitTimes: [] as number[], + stderrBuffer: null, + stderrStream: null, + stderrTailLogged: false, + recoveryRounds: 0, + lastRecoveryAt: 0, + }; + } + + function driveExit(exit: { code: number | null; signal: string | null }) { + const socketDir = tmpDir(); + const manager = new SessionManager({ + socketDir, + shellperScript: '/nonexistent/shellper.js', + nodeExecutable: process.execPath, + }); + const session = fakeSession(socketDir); + (manager as any).sessions.set('clean-1', session); + const cleanExits: string[] = []; + manager.on('session-clean-exit', (id: string) => cleanExits.push(id)); + (manager as any).setupAutoRestart(session, 'clean-1'); + session.client.emit('exit', exit); + return { manager, session, cleanExits, socketDir }; + } + + it('does not respawn or count a restart on a clean exit', async () => { + const { manager, session, cleanExits, socketDir } = driveExit({ code: 0, signal: null }); + try { + // Past the restartDelay: still no SPAWN frame. + await new Promise((r) => setTimeout(r, 50)); + expect(session.client.spawn).not.toHaveBeenCalled(); + expect(session.restartCount).toBe(0); + expect(cleanExits).toEqual(['clean-1']); + // The session is dropped, so SessionManager's view matches Tower's. + expect((manager as any).sessions.has('clean-1')).toBe(false); + } finally { + rmrf(socketDir); + } + }); + + it('still respawns after a signal death (code 0 with a signal)', async () => { + const { session, socketDir } = driveExit({ code: 0, signal: '9' }); + try { + await new Promise((r) => setTimeout(r, 50)); + expect(session.client.spawn).toHaveBeenCalledTimes(1); + expect(session.restartCount).toBe(1); + } finally { + rmrf(socketDir); + } + }); + + it('still respawns after a crash (nonzero exit)', async () => { + const { session, socketDir } = driveExit({ code: 1, signal: null }); + try { + await new Promise((r) => setTimeout(r, 50)); + expect(session.client.spawn).toHaveBeenCalledTimes(1); + expect(session.restartCount).toBe(1); + } finally { + rmrf(socketDir); + } + }); +}); diff --git a/packages/codev/src/terminal/__tests__/shellper-protocol.test.ts b/packages/codev/src/terminal/__tests__/shellper-protocol.test.ts index f3c384241..70d8c2681 100644 --- a/packages/codev/src/terminal/__tests__/shellper-protocol.test.ts +++ b/packages/codev/src/terminal/__tests__/shellper-protocol.test.ts @@ -19,6 +19,7 @@ import { encodeSpawn, createFrameParser, isKnownFrameType, + isDeliberateExit, parseJsonPayload, type ParsedFrame, type FrameTypeValue, @@ -404,6 +405,29 @@ describe('shellper-protocol', () => { }); }); + describe('isDeliberateExit (Bugfix #1241)', () => { + it('treats exit code 0 with no signal as deliberate', () => { + expect(isDeliberateExit({ code: 0, signal: null })).toBe(true); + expect(isDeliberateExit({ code: 0 })).toBe(true); + expect(isDeliberateExit({ code: 0, signal: '' })).toBe(true); + // node-pty reports "no signal" as 0, which ShellperProcess stringifies. + expect(isDeliberateExit({ code: 0, signal: '0' })).toBe(true); + }); + + it('treats a signal death as unnatural even though its code is 0', () => { + // Verified against node-pty: SIGKILL → { exitCode: 0, signal: 9 }. + expect(isDeliberateExit({ code: 0, signal: '9' })).toBe(false); + expect(isDeliberateExit({ code: 0, signal: '2' })).toBe(false); + expect(isDeliberateExit({ code: 0, signal: 'SIGTERM' })).toBe(false); + }); + + it('treats a nonzero exit as unnatural', () => { + expect(isDeliberateExit({ code: 1, signal: null })).toBe(false); + expect(isDeliberateExit({ code: 130, signal: null })).toBe(false); + expect(isDeliberateExit({ code: null, signal: null })).toBe(false); + }); + }); + describe('constants', () => { it('PROTOCOL_VERSION is 1', () => { expect(PROTOCOL_VERSION).toBe(1); diff --git a/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts b/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts index da4f36364..a251c5007 100644 --- a/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts +++ b/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts @@ -285,7 +285,9 @@ describe('PtySession + ShellperClient integration', () => { const exitSpy = vi.fn(); session.on('exit', exitSpy); - mockClient.simulateExit(0); + // Bugfix #1241: only an unnatural exit is awaiting a restart. A clean + // exit takes the deliberate-quit path below instead. + mockClient.simulateExit(1); // Exit event should NOT fire — auto-restart will handle it expect(exitSpy).not.toHaveBeenCalled(); @@ -304,8 +306,8 @@ describe('PtySession + ShellperClient integration', () => { const exitSpy = vi.fn(); session.on('exit', exitSpy); - // Process exits - mockClient.simulateExit(0); + // Process crashes (Bugfix #1241: only these await a restart) + mockClient.simulateExit(1); expect(exitSpy).not.toHaveBeenCalled(); expect(session.status).toBe('exited'); // exitCode is set initially @@ -348,6 +350,45 @@ describe('PtySession + ShellperClient integration', () => { vi.useRealTimers(); }); + it('ends cleanly on a deliberate quit even when restartOnExit is true (Bugfix #1241)', () => { + vi.useFakeTimers(); + session.attachShellper(mockClient, Buffer.alloc(0), 9999); + session.restartOnExit = true; + + const exitSpy = vi.fn(); + session.on('exit', exitSpy); + + mockClient.simulateExit(0); + + // No respawn is coming, so exit fires immediately rather than after the + // 10s wait-for-restart window, and the notice says what really happened. + expect(exitSpy).toHaveBeenCalledWith(0, null); + const ringContent = session.ringBuffer.getAll().join(''); + expect(ringContent).toContain('Agent exited at your request'); + expect(ringContent).not.toContain('Process exited'); + + // Nothing left armed that could re-fire exit later. + vi.advanceTimersByTime(20_000); + expect(exitSpy).toHaveBeenCalledTimes(1); + vi.useRealTimers(); + }); + + it('treats a signal death as unnatural despite exit code 0 (Bugfix #1241)', () => { + vi.useFakeTimers(); + session.attachShellper(mockClient, Buffer.alloc(0), 9999); + session.restartOnExit = true; + + const exitSpy = vi.fn(); + session.on('exit', exitSpy); + + // node-pty reports SIGKILL as exitCode 0 with signal 9. + mockClient.simulateExit(0, '9'); + + expect(exitSpy).not.toHaveBeenCalled(); + expect(session.ringBuffer.getAll().join('')).toContain('restarting'); + vi.useRealTimers(); + }); + it('emits exit normally when restartOnExit is false (default)', () => { session.attachShellper(mockClient, Buffer.alloc(0), 9999); // restartOnExit defaults to false diff --git a/packages/codev/src/terminal/pty-session.ts b/packages/codev/src/terminal/pty-session.ts index 9884e54d7..e18369465 100644 --- a/packages/codev/src/terminal/pty-session.ts +++ b/packages/codev/src/terminal/pty-session.ts @@ -9,6 +9,7 @@ import { EventEmitter } from 'node:events'; import type { IPty } from 'node-pty'; import { RingBuffer } from './ring-buffer.js'; import type { IShellperClient } from './shellper-client.js'; +import { isDeliberateExit } from './shellper-protocol.js'; export interface PtySessionConfig { id: string; @@ -176,6 +177,17 @@ export class PtySession extends EventEmitter { // Handle shellper exit (process inside shellper exited) client.on('exit', (exitInfo: { code: number; signal: string | null }) => { this.exitCode = exitInfo.code; + // Issue #1241: SessionManager does not restart a deliberate quit, so the + // "restarting" notice and its 10s wait-for-the-respawn timer would both + // be lying. Say what actually happened and end the session cleanly — + // which also clears the architect row, so `afx workspace start` can + // relaunch (Tower gates that on the terminal being gone). + if (this._restartOnExit && isDeliberateExit(exitInfo)) { + this.onPtyData('\r\n\x1b[90m[Agent exited at your request — not restarting.]\x1b[0m\r\n'); + this.emit('exit', exitInfo.code, exitInfo.signal); + this.cleanupShellper(); + return; + } if (this._restartOnExit) { // Clear any pending restart state from a previous exit (crash loop guard) if (this._restartCleanupTimeout) { diff --git a/packages/codev/src/terminal/session-manager.ts b/packages/codev/src/terminal/session-manager.ts index 425cebaad..e51ad4b75 100644 --- a/packages/codev/src/terminal/session-manager.ts +++ b/packages/codev/src/terminal/session-manager.ts @@ -21,7 +21,7 @@ import { execFile } from 'node:child_process'; import { defaultSessionOptions } from './index.js'; import type { Readable } from 'node:stream'; import { ShellperClient, type IShellperClient } from './shellper-client.js'; -import type { ExitMessage } from './shellper-protocol.js'; +import { isDeliberateExit, type ExitMessage } from './shellper-protocol.js'; export interface SessionManagerConfig { socketDir: string; @@ -1078,6 +1078,19 @@ export class SessionManager extends EventEmitter { session.restartResetTimer = null; } + // Issue #1241: a deliberate quit is the user's decision — never override + // it with a respawn. Auto-restart is for unnatural exits (crashes, signal + // deaths), which keep the behavior below. The shellper husk is left alive + // (not SIGTERMed) so its scrollback survives; dropping the session drops + // the socket, which is the same thing the non-restarting child-exit path + // does and keeps SessionManager's view in step with Tower's. + if (isDeliberateExit(exit)) { + this.log(`Session ${sessionId} exited cleanly (code 0, no signal); not restarting`); + this.emit('session-clean-exit', sessionId, exit); + this.removeDeadSession(sessionId); + return; + } + // Issue #1149: a fast-failing process (e.g. an unresumable `--resume` // replayed verbatim) never lives long enough for the reset timer to // clear the counter, so it would burn all restarts on identical args. diff --git a/packages/codev/src/terminal/shellper-protocol.ts b/packages/codev/src/terminal/shellper-protocol.ts index 0cfbd04c7..c782ff5a7 100644 --- a/packages/codev/src/terminal/shellper-protocol.ts +++ b/packages/codev/src/terminal/shellper-protocol.ts @@ -110,6 +110,27 @@ export interface SpawnMessage { env: Record; } +// --- Exit Classification --- + +/** + * Issue #1241: whether an exit was the user's own choice (double Ctrl+C, + * `/quit`, any clean shutdown) rather than a crash or a kill. Auto-restart + * exists for unnatural exits only — respawning after a deliberate quit + * overrides the user's decision, and a ~2s respawn can collide with the dying + * predecessor's session lock (the #1224 family). + * + * The code alone is NOT sufficient: node-pty reports a signal death as exit + * code 0 with the signal number attached (verified: SIGKILL → + * `{exitCode: 0, signal: 9}`), and `ShellperProcess` stringifies that field + * before it reaches the EXIT frame. So a deliberate exit is code 0 *and* no + * signal, where "no signal" is null/absent, '' or '0'. + */ +export function isDeliberateExit(exit: { code: number | null; signal?: string | null }): boolean { + if (exit.code !== 0) return false; + const signal = exit.signal; + return signal === null || signal === undefined || signal === '' || signal === '0'; +} + // --- Frame Encoding --- /**