From cf430e1b99af205b093f975a5ab82bdc16ff6b43 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 17:49:40 -0700 Subject: [PATCH 1/7] chore(porch): bugfix-1264 init bugfix --- .../status.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml diff --git a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml new file mode 100644 index 000000000..3fc0b5e4b --- /dev/null +++ b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml @@ -0,0 +1,14 @@ +id: bugfix-1264 +title: regression-3-2-4-double-ctrl-c +protocol: bugfix +phase: investigate +plan_phases: [] +current_plan_phase: null +gates: + pr: + status: pending +iteration: 1 +build_complete: false +history: [] +started_at: '2026-07-27T00:49:40.123Z' +updated_at: '2026-07-27T00:49:40.124Z' From 1ade1cabc82d81bdd0d5a78018b0e9fdc8f6828f Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 17:56:31 -0700 Subject: [PATCH 2/7] chore(porch): bugfix-1264 fix phase-transition --- .../bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml index 3fc0b5e4b..3993c84d8 100644 --- a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml +++ b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml @@ -1,7 +1,7 @@ id: bugfix-1264 title: regression-3-2-4-double-ctrl-c protocol: bugfix -phase: investigate +phase: fix plan_phases: [] current_plan_phase: null gates: @@ -11,4 +11,4 @@ iteration: 1 build_complete: false history: [] started_at: '2026-07-27T00:49:40.123Z' -updated_at: '2026-07-27T00:49:40.124Z' +updated_at: '2026-07-27T00:56:30.983Z' From 37a1ff3cb687aa0d1974edbda20ce3e7ac844a41 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 22:36:38 -0700 Subject: [PATCH 3/7] [Bugfix #1264] Fix: a clean harness exit reruns fresh instead of killing the session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since v3.2.4 a double Ctrl-C left the agent dead, needing a manual respawn. #1241 had made any clean exit end the session, on the reading that a quit is the user's decision and must not be overridden. The corrected framing (architect, on the issue): the problem is not telling ^C^C apart from a typed quit — they are genuinely indistinguishable, both exiting {code: 0, signal: 0} on Claude CLI 2.1.220 — it is that a clean exit kills the shellper at all. So no discrimination is needed: - ANY clean exit: the shellper and session survive; the harness is rerun in the same PTY WITHOUT recovery — a fresh conversation, since the user just deliberately left the old one. - Unnatural exits (crash, signal, nonzero): unchanged, restart WITH recovery. - A session ends only via an explicit kill (afx / UI / DELETE). SessionManager cannot build recovery-free args itself — it holds only the argv it was launched with, which may already carry --resume. So callers supply a FreshLaunch factory. A factory, not a precomputed arg list: every clean exit is a genuinely new conversation and needs its own minted id. buildArchitectFreshLaunch mints it, re-injects the role (which the resume path deliberately skips), and PERSISTS it to the architect row — otherwise a later crash would resume the conversation the user just walked away from. All three architect wiring sites build it from baseArgs, never from the resolved args, so a --resume can never leak into the rerun. PtySession now takes the same restart-wait path for clean exits as for crashes — session kept alive, 'exit' suppressed so Tower keeps the architect row and its clients — sharing one extracted startRestartWait that differs only in the notice, since 'restarting' would misdescribe getting a NEW conversation back. Clean-exit reruns do not touch restartCount or the crash-loop history: a user quitting their agent is a gesture, not a failure, and a long-lived architect can out-quit any budget. Safety valve (beyond the prescribed design, flagged in the PR): unlimited reruns would remove a bound BOTH prior versions had (#1241 ended the session; pre-3.2.4 had maxRestarts), letting a harness misconfigured to exit 0 on startup respawn forever. A clean exit within 2s of launch is a broken command rather than a human gesture, so 5 consecutive such exits give up. A real quit never trips it; one fast exit followed by a healthy session resets the count. Builders are out of scope by architect decision — their bash launch loop is tracked separately in #1267. --- .../src/agent-farm/servers/tower-instances.ts | 20 ++++ .../src/agent-farm/servers/tower-terminals.ts | 13 ++- .../src/agent-farm/servers/tower-utils.ts | 53 ++++++++- packages/codev/src/terminal/pty-session.ts | 90 +++++++++------ .../codev/src/terminal/session-manager.ts | 109 ++++++++++++++++-- 5 files changed, 238 insertions(+), 47 deletions(-) diff --git a/packages/codev/src/agent-farm/servers/tower-instances.ts b/packages/codev/src/agent-farm/servers/tower-instances.ts index dd4157d9a..3d59b84fb 100644 --- a/packages/codev/src/agent-farm/servers/tower-instances.ts +++ b/packages/codev/src/agent-farm/servers/tower-instances.ts @@ -29,6 +29,7 @@ import { resolveArchitectLaunch, siblingRegistrationIsLive, buildArchitectCrashLoopFallback, + buildArchitectFreshLaunch, } from './tower-utils.js'; import { reconcileArchitectSessionHolder, @@ -594,6 +595,16 @@ export async function launchInstance(workspacePath: string): Promise<{ success: cwd: workspacePath, env: cleanEnv, crashLoopFallback, + // Issue #1264: a clean exit reruns the harness fresh (no --resume) + // instead of ending the session. Built from the ORIGINAL baseArgs, + // never from cmdArgs — those may already carry a `--resume`. + freshLaunch: buildArchitectFreshLaunch({ + workspacePath: resolvedPath, + architectName: DEFAULT_ARCHITECT_NAME, + baseArgs: cmdParts.slice(1), + baseEnv: cleanEnv, + log: _deps.log, + }), ...defaultSessionOptions({ restartOnExit: true, restartDelay: 2000, maxRestarts: 50 }), }); @@ -1089,6 +1100,15 @@ export async function addArchitect( cwd: workspacePath, env: cleanEnv, crashLoopFallback, + // Issue #1264: see the `main` launch path — same rule for siblings, + // built from baseArgs so a `--resume` can never leak into the rerun. + freshLaunch: buildArchitectFreshLaunch({ + workspacePath: resolvedPath, + architectName: name, + baseArgs: cmdParts.slice(1), + baseEnv: cleanEnv, + log: _deps.log, + }), ...defaultSessionOptions({ restartOnExit: true, restartDelay: 2000, maxRestarts: 50 }), }); diff --git a/packages/codev/src/agent-farm/servers/tower-terminals.ts b/packages/codev/src/agent-farm/servers/tower-terminals.ts index f781d5ea6..f366adedc 100644 --- a/packages/codev/src/agent-farm/servers/tower-terminals.ts +++ b/packages/codev/src/agent-farm/servers/tower-terminals.ts @@ -51,7 +51,7 @@ function extractShellperSessionId(socketPath: string | null): string | null { import type { SessionManager, ReconnectRestartOptions } from '../../terminal/session-manager.js'; import type { PtySession } from '../../terminal/pty-session.js'; import type { WorkspaceTerminals, TerminalEntry, DbTerminalSession } from './tower-types.js'; -import { normalizeWorkspacePath, resolveArchitectRestart, buildArchitectCrashLoopFallback } from './tower-utils.js'; +import { normalizeWorkspacePath, resolveArchitectRestart, buildArchitectCrashLoopFallback, buildArchitectFreshLaunch } from './tower-utils.js'; import { setArchitectByName } from '../state.js'; import { isIntentionallyStopping } from './tower-instances.js'; @@ -689,6 +689,17 @@ async function _reconcileTerminalSessionsInner(): Promise { env: { ...cleanEnv, ...harnessEnv }, restartDelay: 2000, maxRestarts: 50, + // Issue #1264: a clean exit inside this reconnected session reruns + // the harness fresh rather than ending the session. Built from + // cmdParts, not `architectArgs` — the latter has `--resume` baked in + // by the #832 restart resolution just above. + freshLaunch: buildArchitectFreshLaunch({ + workspacePath, + architectName, + baseArgs: cmdParts.slice(1), + baseEnv: cleanEnv, + log: _deps.log, + }), }; // Issue #1149: if the resumed session fast-fails at runtime (jsonl // vanished after the bake, or corrupt), degrade to a fresh launch diff --git a/packages/codev/src/agent-farm/servers/tower-utils.ts b/packages/codev/src/agent-farm/servers/tower-utils.ts index fdd415adc..828ef8b7f 100644 --- a/packages/codev/src/agent-farm/servers/tower-utils.ts +++ b/packages/codev/src/agent-farm/servers/tower-utils.ts @@ -17,7 +17,7 @@ import { loadRolePrompt, type RoleConfig } from '../utils/roles.js'; import { getArchitectHarness } from '../utils/config.js'; import type { HarnessProvider } from '../utils/harness.js'; import { getArchitectByName, setArchitectSessionId } from '../state.js'; -import type { CrashLoopFallback } from '../../terminal/session-manager.js'; +import type { CrashLoopFallback, FreshLaunch } from '../../terminal/session-manager.js'; import { cmdlineHoldsSession } from './architect-session-holder.js'; // ============================================================================ @@ -478,6 +478,57 @@ export function buildArchitectCrashLoopFallback(opts: { }; } +/** + * Issue #1264: the fresh-launch factory a shellper session uses to rerun the + * architect's harness after a *clean* exit (double Ctrl-C, `/quit`, `exit`). + * + * A clean exit means the user deliberately left that conversation, so the + * rerun must NOT carry `--resume`: it mints a brand-new conversation, with the + * role re-injected (which the resume path deliberately skips, since a resumed + * transcript already contains it). + * + * The new id is **persisted to the architect row**, so it becomes the identity + * a later crash recovers. Without that, an unnatural exit after a clean rerun + * would resume the conversation the user just walked away from. A persist + * failure is logged and tolerated rather than fatal — the launch itself is + * still correct, and the next cold start self-heals. + * + * Called once per clean exit (never memoized): each rerun is a genuinely new + * conversation and needs its own id. + */ +export function buildArchitectFreshLaunch(opts: { + workspacePath: string; + architectName: string; + baseArgs: string[]; + baseEnv: Record; + log: (level: 'INFO' | 'ERROR' | 'WARN', message: string) => void; +}): FreshLaunch { + const { workspacePath, architectName, baseArgs, baseEnv, log } = opts; + return { + next: () => { + const harness = getArchitectHarness(workspacePath); + // No resumable-session concept for this harness: there is no recovery to + // disable, so a plain rebuild of the launch args is already "fresh". + if (!harness.session) { + const built = buildArchitectArgs(baseArgs, workspacePath); + return { args: built.args, env: { ...baseEnv, ...built.env } }; + } + const sessionId = crypto.randomUUID(); + const built = buildArchitectArgs( + [...baseArgs, ...harness.session.newSessionArgs(sessionId)], + workspacePath, + ); + try { + setArchitectSessionId(workspacePath, architectName, sessionId); + } catch (err) { + log('WARN', `Failed to persist fresh session id for architect '${architectName}': ${err instanceof Error ? err.message : err}`); + } + log('INFO', `Architect '${architectName}' exited cleanly; rerunning with a fresh session ${sessionId.slice(0, 8)}… in ${workspacePath}`); + return { args: built.args, env: { ...baseEnv, ...built.env } }; + }, + }; +} + /** * Serve a static file from the React dashboard dist. * Returns true if the file was served, false otherwise. diff --git a/packages/codev/src/terminal/pty-session.ts b/packages/codev/src/terminal/pty-session.ts index e18369465..b01621012 100644 --- a/packages/codev/src/terminal/pty-session.ts +++ b/packages/codev/src/terminal/pty-session.ts @@ -177,48 +177,22 @@ 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). + // Issue #1264: a clean exit reruns the harness in this same PTY with a + // fresh conversation, so it takes the restart path below exactly like a + // crash does — same wait-for-the-respawn window, same suppressed 'exit' + // so WebSocket clients and the terminal's identity survive. Only the + // notice differs, because "restarting" would misdescribe what the user + // gets back: a new conversation, not the one they just left. + // (#1241 ended the session here; #1264 reversed that — a session now + // ends only on an explicit kill.) 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(); + this.startRestartWait(client, exitInfo, '\r\n\x1b[90m[Agent exited — starting a fresh session...]\x1b[0m\r\n'); return; } if (this._restartOnExit) { - // Clear any pending restart state from a previous exit (crash loop guard) - if (this._restartCleanupTimeout) { - clearTimeout(this._restartCleanupTimeout); - if (this._restartCancelFn) { - client.removeListener('data', this._restartCancelFn); - } - } // Process will auto-restart via SessionManager — keep WebSocket clients // connected and don't emit 'exit' so Tower doesn't clear references. - this.onPtyData('\r\n\x1b[90m[Process exited — restarting...]\x1b[0m\r\n'); - // Wait for the process to restart. If new data arrives (process restarted), - // cancel the cleanup timer. If no data within 10s (e.g. max restarts - // exceeded), fall through to normal exit cleanup. - this._restartCleanupTimeout = setTimeout(() => { - client.removeListener('data', cancelCleanup); - this._restartCleanupTimeout = null; - this._restartCancelFn = null; - this.emit('exit', exitInfo.code, exitInfo.signal); - this.cleanupShellper(); - }, 10_000); - const cancelCleanup = () => { - clearTimeout(this._restartCleanupTimeout!); - client.removeListener('data', cancelCleanup); - this._restartCleanupTimeout = null; - this._restartCancelFn = null; - // Process restarted — reset exitCode so write/resize work again - this.exitCode = undefined; - }; - this._restartCancelFn = cancelCleanup; - client.on('data', cancelCleanup); + this.startRestartWait(client, exitInfo, '\r\n\x1b[90m[Process exited — restarting...]\x1b[0m\r\n'); return; } this.emit('exit', exitInfo.code, exitInfo.signal); @@ -246,6 +220,50 @@ export class PtySession extends EventEmitter { }); } + /** + * Hold the session open while SessionManager respawns the child, printing + * `notice` in its place. + * + * Shared by both relaunch paths (#1264): an unnatural exit restarting with + * recovery, and a clean exit rerunning the harness fresh. They differ only in + * wording — structurally both keep WebSocket clients attached, suppress + * 'exit' so Tower doesn't clear its references, and arm a bounded wait. If + * new data arrives the child is back and the teardown is cancelled; if + * nothing arrives within the window (e.g. max restarts exhausted) the session + * falls through to normal exit cleanup. + */ + private startRestartWait( + client: IShellperClient, + exitInfo: { code: number; signal: string | null }, + notice: string, + ): void { + // Clear any pending restart state from a previous exit (crash loop guard) + if (this._restartCleanupTimeout) { + clearTimeout(this._restartCleanupTimeout); + if (this._restartCancelFn) { + client.removeListener('data', this._restartCancelFn); + } + } + this.onPtyData(notice); + this._restartCleanupTimeout = setTimeout(() => { + client.removeListener('data', cancelCleanup); + this._restartCleanupTimeout = null; + this._restartCancelFn = null; + this.emit('exit', exitInfo.code, exitInfo.signal); + this.cleanupShellper(); + }, 10_000); + const cancelCleanup = () => { + clearTimeout(this._restartCleanupTimeout!); + client.removeListener('data', cancelCleanup); + this._restartCleanupTimeout = null; + this._restartCancelFn = null; + // Process restarted — reset exitCode so write/resize work again + this.exitCode = undefined; + }; + this._restartCancelFn = cancelCleanup; + client.on('data', cancelCleanup); + } + /** Whether this session is backed by a shellper process. */ get shellperBacked(): boolean { return this._shellperBacked; diff --git a/packages/codev/src/terminal/session-manager.ts b/packages/codev/src/terminal/session-manager.ts index e51ad4b75..0153d4593 100644 --- a/packages/codev/src/terminal/session-manager.ts +++ b/packages/codev/src/terminal/session-manager.ts @@ -42,6 +42,29 @@ export interface CrashLoopFallback { onApply?: () => void; } +/** + * Issue #1264: how to relaunch the harness after a *clean* exit. + * + * A clean exit means the user ended the harness on purpose (double Ctrl-C, + * `/quit`, `exit`). The session and its shellper survive that — only the + * harness is rerun, and it is rerun **without recovery**: a fresh + * conversation, not a `--resume` of the one just abandoned. Restarting a + * crash is the opposite: that one *should* resume, which is what recovery + * exists for. + * + * A factory rather than a precomputed arg list, because every clean exit + * starts a genuinely new conversation and so needs its own freshly minted + * session id. This layer stays agnostic of what the args mean; the caller + * mints, persists, and returns them. + * + * Returning null (or omitting the option entirely) means "relaunch with the + * args as they stand" — correct for sessions that have no recovery concept, + * such as plain shells. + */ +export interface FreshLaunch { + next: () => { args: string[]; env?: Record } | null; +} + export interface CreateSessionOptions { sessionId: string; command: string; @@ -55,6 +78,7 @@ export interface CreateSessionOptions { maxRestarts?: number; restartResetAfter?: number; crashLoopFallback?: CrashLoopFallback; + freshLaunch?: FreshLaunch; } export interface ReconnectRestartOptions { @@ -66,8 +90,26 @@ export interface ReconnectRestartOptions { maxRestarts?: number; restartResetAfter?: number; crashLoopFallback?: CrashLoopFallback; + freshLaunch?: FreshLaunch; } +/** + * Issue #1264 safety valve. Clean-exit reruns are deliberately unlimited — + * quitting your agent is a gesture, not a failure, and a long-lived architect + * can out-quit any budget. But "unlimited" must not become an infinite spawn + * loop when the harness is broken rather than being quit: a misconfigured + * command that exits 0 immediately would otherwise respawn forever. + * + * A human cannot produce a deliberate quit within a couple of seconds of the + * harness starting, so a clean exit that fast is evidence of a broken command, + * not a gesture. Only those count, and only consecutively — a single fast exit + * followed by a healthy session resets the counter. This restores the bound + * that both #1241 (end the session) and pre-3.2.4 (maxRestarts) had, without + * charging real gestures for it. + */ +export const FAST_CLEAN_EXIT_MS = 2_000; +export const MAX_FAST_CLEAN_EXITS = 5; + /** Failing exits inside this window count toward crash-loop detection. */ export const CRASH_LOOP_WINDOW_MS = 30_000; /** Failing exits within the window needed to declare a crash loop. */ @@ -174,6 +216,10 @@ interface ManagedSession { recoveryRounds: number; /** Epoch of the last successful in-place reconnect, for the stability reset (#1198). */ lastRecoveryAt: number; + /** Epoch of the most recent (re)spawn, for #1264's fast-clean-exit guard. */ + lastSpawnAt: number; + /** Consecutive clean exits too fast to be a user gesture (#1264). */ + fastCleanExits: number; } function sleep(ms: number): Promise { @@ -291,6 +337,8 @@ export class SessionManager extends EventEmitter { stderrTailLogged: false, recoveryRounds: 0, lastRecoveryAt: 0, + lastSpawnAt: Date.now(), + fastCleanExits: 0, }; this.sessions.set(opts.sessionId, session); @@ -533,6 +581,8 @@ export class SessionManager extends EventEmitter { stderrTailLogged: false, recoveryRounds: 0, lastRecoveryAt: 0, + lastSpawnAt: Date.now(), + fastCleanExits: 0, }; this.sessions.set(sessionId, session); @@ -1078,16 +1128,56 @@ 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. + // Issue #1264: a clean exit ends the *harness*, not the session. The + // shellper and terminal survive and the harness is rerun in the same PTY + // with recovery disabled — a fresh conversation, because the user just + // deliberately walked away from the old one. (#1241 ended the session + // here instead, which made a double Ctrl-C unrecoverable without a manual + // respawn; only an explicit kill via afx/UI/DELETE ends a session now.) + // + // This is NOT counted as a restart: it consumes no part of the + // maxRestarts budget and never enters the crash-loop history. Those exist + // to stop a *failing* process from spinning forever, and a user quitting + // their agent is not a failure — a long-lived architect can easily do it + // more times than the restart budget allows. 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); + // Safety valve: a clean exit this soon after launch is a broken command, + // not a user gesture (see FAST_CLEAN_EXIT_MS). Bound those so a harness + // that exits 0 on startup can't respawn forever. + const uptime = Date.now() - session.lastSpawnAt; + session.fastCleanExits = uptime < FAST_CLEAN_EXIT_MS ? session.fastCleanExits + 1 : 0; + if (session.fastCleanExits >= MAX_FAST_CLEAN_EXITS) { + this.log( + `Session ${sessionId} exited cleanly ${session.fastCleanExits}x within ${FAST_CLEAN_EXIT_MS}ms of launch; the harness is exiting immediately rather than being quit — giving up`, + ); + this.emit( + 'session-error', + sessionId, + new Error(`Harness exited cleanly ${session.fastCleanExits} times immediately after launch`), + ); + this.removeDeadSession(sessionId); + return; + } + const fresh = session.options.freshLaunch?.next() ?? null; + if (fresh) { + session.options.args = fresh.args; + if (fresh.env) session.options.env = fresh.env; + } + this.log( + `Session ${sessionId} exited cleanly (code 0, no signal); rerunning harness without recovery${fresh ? '' : ' (no fresh-launch factory; reusing current args)'}`, + ); + this.emit('session-fresh-restart', sessionId, exit); + setTimeout(() => { + if (!this.sessions.has(sessionId)) return; + session.lastSpawnAt = Date.now(); + session.client.spawn({ + command: session.options.command, + args: session.options.args, + cwd: session.options.cwd, + env: session.options.env, + }); + this.startRestartResetTimer(session); + }, session.options.restartDelay ?? 2000); return; } @@ -1151,6 +1241,7 @@ export class SessionManager extends EventEmitter { // Re-check session still exists after delay if (!this.sessions.has(sessionId)) return; + session.lastSpawnAt = Date.now(); session.client.spawn({ command: session.options.command, args: session.options.args, From d6a12e5c9f7de5a6c52fa5e41690b84067476ff0 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 22:36:50 -0700 Subject: [PATCH 4/7] [Bugfix #1264] Test: clean exits rerun without recovery; crashes still resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the #1241 matrix, which asserted the behavior this issue reverses. session-manager: - a clean exit reruns the harness and the session STAYS (no session-clean-exit) - the relaunch argv carries no --resume and none of the old conversation id — the headline guarantee - every clean exit mints a NEW conversation, never reusing the first - restartCount and the crash-loop history stay untouched across repeated clean exits (the budget must not be consumed by gestures) - no fresh-launch factory (plain shells) still reruns, with args unchanged - signal death and crash restart WITH recovery: argv preserved and the factory never consulted The real-shellper 'exit 0' test now asserts the fast-clean-exit valve instead of the old end-the-session behavior: that command exits cleanly the instant it launches, which is a broken harness rather than a user gesture, so it is bounded and given up — and still never applies the crashLoopFallback (#1149). tower-shellper-integration: a clean exit suppresses 'exit' and awaits the rerun (emitting it would clear the architect row — the #1264 symptom), shows a notice that does not promise the old conversation back, and the bounded wait still tears down if no rerun ever arrives. --- .../__tests__/session-manager.test.ts | 172 ++++++++++++++---- .../tower-shellper-integration.test.ts | 38 +++- 2 files changed, 170 insertions(+), 40 deletions(-) diff --git a/packages/codev/src/terminal/__tests__/session-manager.test.ts b/packages/codev/src/terminal/__tests__/session-manager.test.ts index 520b2aa4b..cae0b5488 100644 --- a/packages/codev/src/terminal/__tests__/session-manager.test.ts +++ b/packages/codev/src/terminal/__tests__/session-manager.test.ts @@ -1316,12 +1316,18 @@ describe('SessionManager', () => { expect(manager.getSessionInfo('crashloop-test')).not.toBeNull(); }, 20000); - // Issue #1149: clean exits (code 0) never trigger the fallback — a user - // quitting a healthy session repeatedly must not lose a valid resumable - // conversation. Bugfix #1241 strengthened this: a clean exit is not - // restarted at all, so the session ends instead of exhausting maxRestarts. + // Issue #1149: clean exits (code 0) never trigger the crashLoopFallback — a + // user quitting a healthy session repeatedly must not lose a valid + // resumable conversation. + // + // Bugfix #1264: a clean exit now RERUNS the harness rather than ending the + // session. `exit 0` is the pathological case that makes this observable — + // it exits cleanly the instant it launches, which is not a user gesture but + // a broken command, so the fast-clean-exit valve bounds it and the session + // is given up rather than respawning forever. A real agent being quit by a + // human never trips this (see FAST_CLEAN_EXIT_MS). // Spawns real shellper processes — skip in CI. - it.skipIf(!!process.env.CI)('does not restart or apply crashLoopFallback on clean exits', async () => { + it.skipIf(!!process.env.CI)('bounds a harness that exits 0 immediately, without applying crashLoopFallback', async () => { const shellperScript = path.resolve( path.dirname(new URL(import.meta.url).pathname), '../../../dist/terminal/shellper-main.js', @@ -1367,19 +1373,23 @@ describe('SessionManager', () => { try { await manager.killSession('cleanexit-test'); } catch { /* noop */ } }); - // Bugfix #1241: the clean exit ends the session — no respawn, no - // give-up error, no fallback. - const deadline2 = Date.now() + 10000; + // Bugfix #1264: each clean exit reruns the harness; because this one + // exits instantly, the fast-clean-exit valve trips and the session is + // given up. Crucially it is NOT via the restart budget, and the + // crashLoopFallback is still never applied (#1149). + const deadline2 = Date.now() + 15000; 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'); + // Not a "clean exit ends the session" event any more — that concept is gone. + expect(cleanExits).toEqual([]); + expect(errors.some((m) => m.includes('immediately after launch'))).toBe(true); expect(errors.some((m) => m.includes('Max restarts'))).toBe(false); expect(onApply).not.toHaveBeenCalled(); expect(fs.existsSync(sentinel)).toBe(false); - }, 20000); + }, 30000); }); describe('isCrashLooping (Issue #1149)', () => { @@ -2318,12 +2328,17 @@ 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) { +describe('clean exits rerun the harness without recovery (Bugfix #1264)', () => { + // A clean exit (double Ctrl-C, `/quit`, `exit`) ends the *harness*, not the + // session: the shellper survives and the harness is rerun in the same PTY + // with recovery disabled — a fresh conversation, because the user just + // deliberately left the old one. Crashes and signal deaths keep restarting + // WITH recovery; node-pty reports a signal death as code 0 with a signal + // attached, so that stays the interesting negative case. + // + // (#1241 ended the session on a clean exit, which made double Ctrl-C + // unrecoverable without a manual respawn — the #1264 regression.) + function fakeSession(socketDir: string, freshLaunch?: { next: () => any }) { const client = new EventEmitter() as any; client.spawn = vi.fn(); return { @@ -2334,12 +2349,13 @@ describe('deliberate-quit exits are not restarted (Bugfix #1241)', () => { options: { sessionId: 'clean-1', command: 'claude', - args: [], + args: ['--resume', 'old-conversation-id'], cwd: '/tmp', - env: {}, + env: { KEEP: '1' }, restartOnExit: true, restartDelay: 1, maxRestarts: 50, + freshLaunch, }, restartCount: 0, restartResetTimer: null, @@ -2352,54 +2368,146 @@ describe('deliberate-quit exits are not restarted (Bugfix #1241)', () => { }; } - function driveExit(exit: { code: number | null; signal: string | null }) { + function driveExit( + exit: { code: number | null; signal: string | null }, + freshLaunch?: { next: () => any }, + ) { const socketDir = tmpDir(); const manager = new SessionManager({ socketDir, shellperScript: '/nonexistent/shellper.js', nodeExecutable: process.execPath, }); - const session = fakeSession(socketDir); + const session = fakeSession(socketDir, freshLaunch); (manager as any).sessions.set('clean-1', session); const cleanExits: string[] = []; manager.on('session-clean-exit', (id: string) => cleanExits.push(id)); + const freshRestarts: string[] = []; + manager.on('session-fresh-restart', (id: string) => freshRestarts.push(id)); (manager as any).setupAutoRestart(session, 'clean-1'); session.client.emit('exit', exit); - return { manager, session, cleanExits, socketDir }; + return { manager, session, cleanExits, freshRestarts, socketDir }; } - it('does not respawn or count a restart on a clean exit', async () => { - const { manager, session, cleanExits, socketDir } = driveExit({ code: 0, signal: null }); + /** A fresh-launch factory that mints a new id per call, like the real one. */ + function fakeFreshLaunch() { + let n = 0; + return { + calls: () => n, + factory: { + next: () => { + n++; + return { args: ['--session-id', `fresh-${n}`], env: { FRESH: String(n) } }; + }, + }, + }; + } + + it('reruns the harness and keeps the session alive on a clean exit', async () => { + const fresh = fakeFreshLaunch(); + const { manager, session, cleanExits, freshRestarts, socketDir } = driveExit( + { code: 0, signal: null }, + fresh.factory, + ); + try { + await new Promise((r) => setTimeout(r, 50)); + expect(session.client.spawn).toHaveBeenCalledTimes(1); + // The session must NOT be dropped — only an explicit kill ends it now. + expect((manager as any).sessions.has('clean-1')).toBe(true); + expect(cleanExits).toEqual([]); + expect(freshRestarts).toEqual(['clean-1']); + } finally { + rmrf(socketDir); + } + }); + + it('relaunches WITHOUT the recovery args — the headline guarantee', async () => { + const fresh = fakeFreshLaunch(); + const { session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); + try { + await new Promise((r) => setTimeout(r, 50)); + const argv = session.client.spawn.mock.calls[0][0].args; + expect(argv).not.toContain('--resume'); + expect(argv).not.toContain('old-conversation-id'); + expect(argv).toEqual(['--session-id', 'fresh-1']); + expect(session.client.spawn.mock.calls[0][0].env).toEqual({ FRESH: '1' }); + } finally { + rmrf(socketDir); + } + }); + + it('mints a NEW conversation for every clean exit, never reusing the first', async () => { + const fresh = fakeFreshLaunch(); + const { session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); try { - // Past the restartDelay: still no SPAWN frame. await new Promise((r) => setTimeout(r, 50)); - expect(session.client.spawn).not.toHaveBeenCalled(); + session.client.emit('exit', { code: 0, signal: null }); + await new Promise((r) => setTimeout(r, 50)); + expect(fresh.calls()).toBe(2); + expect(session.client.spawn.mock.calls[1][0].args).toEqual(['--session-id', 'fresh-2']); + } finally { + rmrf(socketDir); + } + }); + + it('leaves the restart budget and crash-loop history untouched across many clean exits', async () => { + // A user quitting their agent is a gesture, not a failure. A long-lived + // architect can easily out-quit the 50-restart budget, and burning it would + // eventually strand the session — the exact class of bug #1264 reported. + const fresh = fakeFreshLaunch(); + const { session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); + try { + for (let i = 0; i < 5; i++) { + await new Promise((r) => setTimeout(r, 20)); + session.client.emit('exit', { code: 0, signal: null }); + } + await new Promise((r) => setTimeout(r, 50)); 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); + expect(session.failingExitTimes).toEqual([]); + expect(session.client.spawn.mock.calls.length).toBeGreaterThanOrEqual(5); + } finally { + rmrf(socketDir); + } + }); + + it('reuses the current args when no fresh-launch factory is supplied', async () => { + // Sessions with no recovery concept (plain shells) still rerun. + const { session, socketDir } = driveExit({ code: 0, signal: null }); + try { + await new Promise((r) => setTimeout(r, 50)); + expect(session.client.spawn).toHaveBeenCalledTimes(1); + expect(session.client.spawn.mock.calls[0][0].args).toEqual(['--resume', 'old-conversation-id']); } finally { rmrf(socketDir); } }); - it('still respawns after a signal death (code 0 with a signal)', async () => { - const { session, socketDir } = driveExit({ code: 0, signal: '9' }); + it('still respawns after a signal death (code 0 with a signal), WITH recovery', async () => { + const fresh = fakeFreshLaunch(); + const { session, socketDir } = driveExit({ code: 0, signal: '9' }, fresh.factory); try { await new Promise((r) => setTimeout(r, 50)); expect(session.client.spawn).toHaveBeenCalledTimes(1); expect(session.restartCount).toBe(1); + // An unnatural exit must revive the SAME conversation — the fresh-launch + // factory is for clean exits only and must not have been consulted. + expect(fresh.calls()).toBe(0); + expect(session.client.spawn.mock.calls[0][0].args).toEqual(['--resume', 'old-conversation-id']); } finally { rmrf(socketDir); } }); - it('still respawns after a crash (nonzero exit)', async () => { - const { session, socketDir } = driveExit({ code: 1, signal: null }); + it('still respawns after a crash (nonzero exit), WITH recovery', async () => { + const fresh = fakeFreshLaunch(); + const { session, socketDir } = driveExit({ code: 1, signal: null }, fresh.factory); try { await new Promise((r) => setTimeout(r, 50)); expect(session.client.spawn).toHaveBeenCalledTimes(1); expect(session.restartCount).toBe(1); + expect(fresh.calls()).toBe(0); + expect(session.client.spawn.mock.calls[0][0].args).toEqual(['--resume', 'old-conversation-id']); + expect(session.failingExitTimes.length).toBe(1); } finally { rmrf(socketDir); } 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 a251c5007..29e9a682a 100644 --- a/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts +++ b/packages/codev/src/terminal/__tests__/tower-shellper-integration.test.ts @@ -350,7 +350,7 @@ describe('PtySession + ShellperClient integration', () => { vi.useRealTimers(); }); - it('ends cleanly on a deliberate quit even when restartOnExit is true (Bugfix #1241)', () => { + it('keeps the session alive on a clean exit and awaits the fresh rerun (Bugfix #1264)', () => { vi.useFakeTimers(); session.attachShellper(mockClient, Buffer.alloc(0), 9999); session.restartOnExit = true; @@ -360,16 +360,38 @@ describe('PtySession + ShellperClient integration', () => { 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); + // A rerun IS coming, so 'exit' must be suppressed — otherwise Tower + // clears the architect row and the terminal dies, which was #1264. + expect(exitSpy).not.toHaveBeenCalled(); const ringContent = session.ringBuffer.getAll().join(''); - expect(ringContent).toContain('Agent exited at your request'); - expect(ringContent).not.toContain('Process exited'); + // The notice must not promise the old conversation back. + expect(ringContent).toContain('starting a fresh session'); + expect(ringContent).not.toContain('at your request'); - // Nothing left armed that could re-fire exit later. + // The rerun lands: fresh output cancels the teardown and the session runs on. + vi.advanceTimersByTime(2000); + mockClient.simulateData('fresh agent\r\n'); + expect(session.status).toBe('running'); vi.advanceTimersByTime(20_000); - expect(exitSpy).toHaveBeenCalledTimes(1); + expect(exitSpy).not.toHaveBeenCalled(); + vi.useRealTimers(); + }); + + it('still tears down a clean exit if no rerun arrives (Bugfix #1264)', () => { + // The bounded wait is the backstop: if SessionManager gave up (e.g. the + // fast-clean-exit valve tripped), the session must not hang forever. + vi.useFakeTimers(); + session.attachShellper(mockClient, Buffer.alloc(0), 9999); + session.restartOnExit = true; + + const exitSpy = vi.fn(); + session.on('exit', exitSpy); + + mockClient.simulateExit(0); + expect(exitSpy).not.toHaveBeenCalled(); + + vi.advanceTimersByTime(10_000); + expect(exitSpy).toHaveBeenCalledWith(0, null); vi.useRealTimers(); }); From 37fe551f17d6fd8392515a09d258307e7ee5ad52 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 22:36:56 -0700 Subject: [PATCH 5/7] [Bugfix #1264] Builder thread + porch state for the reworked design --- .../status.yaml | 6 +- codev/state/bugfix-1264_thread.md | 98 +++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 codev/state/bugfix-1264_thread.md diff --git a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml index 3993c84d8..44cab98eb 100644 --- a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml +++ b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml @@ -1,14 +1,16 @@ id: bugfix-1264 title: regression-3-2-4-double-ctrl-c protocol: bugfix -phase: fix +phase: pr plan_phases: [] current_plan_phase: null gates: pr: status: pending + requested_at: '2026-07-27T01:14:29.680Z' iteration: 1 build_complete: false history: [] started_at: '2026-07-27T00:49:40.123Z' -updated_at: '2026-07-27T00:56:30.983Z' +updated_at: '2026-07-27T01:14:29.680Z' +pr_ready_for_human: true diff --git a/codev/state/bugfix-1264_thread.md b/codev/state/bugfix-1264_thread.md new file mode 100644 index 000000000..48769bf7d --- /dev/null +++ b/codev/state/bugfix-1264_thread.md @@ -0,0 +1,98 @@ +# bugfix-1264 — double Ctrl-C no longer restarts the agent (3.2.4 regression) + +> **Superseded design.** The first implementation discriminated `^C^C` from a typed quit by +> sniffing `0x03` in the shellper's input stream. The architect corrected the spec on 2026-07-27: +> the problem was never the discrimination, it's that a clean harness exit kills the shellper at +> all. The branch was force-pushed to carry only the corrected design, so the superseded commits +> (`ed10e2fc`…`3d525114`) are no longer on the branch — they remain reachable by SHA via the PR's +> force-push record. Their reasoning is kept below, because the empirical findings are what justify +> the corrected design. + +## Investigate + +**Empirical probes against the real Claude CLI (v2.1.220), node-pty, trusted cwd.** + +| Probe | Result | +|---|---| +| Double Ctrl-C in the REPL | `{exitCode: 0, signal: 0}` | +| Typed `/quit` | `{exitCode: 0, signal: 0}` | +| Generated builder launch loop under ^C^C | bash **survives**, prints "Press Enter to relaunch" | + +Exit status cannot discriminate the two gestures. (A first probe in a fresh temp cwd returned code 1 +— it hit the trust-folder prompt and never reached the REPL. Discarded.) + +**Why bash survives**: Claude puts the tty in raw mode (ISIG off), so `^C` is never turned into +SIGINT — it arrives as a literal `0x03` byte. That is also why the shellper *could* see it. + +### Blast radius + +`restartOnExit: true` is passed **only** for architect sessions (`tower-instances.ts:597`, `:1092`). +Builders and shells use `defaultSessionOptions()` and rely on the in-PTY bash loop. + +- **Architect terminal** — ^C^C → session dropped, row cleared, dead, needs `afx workspace start`. + The reported symptom. +- **Builder terminal** — bash loop Enter-gates the relaunch. Not dead. + +## Superseded attempt (kept for the record) + +Tracked `0x03` in `ShellperProcess.handleData`, flagged the EXIT frame, `isDeliberateExit` treated a +flagged exit as restartable. CMAP: gemini APPROVE, claude APPROVE, **codex REQUEST_CHANGES** — and +codex was right: my clear-the-stamp rule fired only on printable input, so **Ctrl-D after Ctrl-C** +would have restarted an EOF quit. Reproduced, then fixed by collapsing the rule to "the stamp +survives iff the most recent input byte is 0x03" (simpler and strictly more correct, net −11 lines). + +Lesson worth keeping: two APPROVEs did not outvote one specific, reproducible finding. + +## Corrected spec (architect, 2026-07-27) — what shipped + +1. **Any** clean exit of the harness (^C^C, `/quit`, `exit` — no discrimination): the shellper and + session **survive**, and the harness is rerun in the same PTY **without recovery** (no + `--resume`; fresh conversation). +2. Unnatural exits (crash, signal, nonzero) keep restart-**with**-recovery. +3. A session ends only via explicit kill (afx / UI / `DELETE /api/terminals/:id`). + +Architect decisions on my four questions: stay BUGFIX (design is human-prescribed on the issue); +**builders out of scope** (filed #1267, including that their Enter-relaunch bakes `--resume` and so +violates the corrected spec); **persist** the fresh conversation id; clean-exit reruns must not +touch the restart budget or crash-loop history. + +### Implementation + +All the `0x03` machinery deleted. What replaced it: + +| File | Change | +|---|---| +| `session-manager.ts` | Clean exit → rerun via a caller-supplied `FreshLaunch` factory instead of ending the session. Not counted as a restart. | +| `pty-session.ts` | Clean exit takes the restart-wait path (session survives); shared with the crash path via an extracted `startRestartWait`, differing only in the notice. | +| `tower-utils.ts` | `buildArchitectFreshLaunch` — mints a new conversation id, re-injects the role, **persists** the id to the architect row. | +| `tower-instances.ts` ×2, `tower-terminals.ts` ×1 | Wire the factory. Built from `baseArgs`/`cmdParts`, never from the resolved args — those may already carry `--resume`. | + +`FreshLaunch` is a **factory, not a precomputed arg list**: every clean exit is a genuinely new +conversation and needs its own id. Persisting it means a later crash resumes the post-rerun +conversation, not the one the user walked away from. + +### One addition beyond the four decisions — flagged, not smuggled + +Making clean-exit reruns unlimited removes a bound that **both** prior versions had (#1241 ended the +session; pre-3.2.4 had `maxRestarts`). A harness misconfigured to exit 0 on startup would then +respawn forever. So: a clean exit within `FAST_CLEAN_EXIT_MS` (2s) of launch is evidence of a broken +command rather than a human gesture — 5 *consecutive* such exits give up. A real quit never trips +it, and one fast exit followed by a healthy session resets the counter. Genuine gestures stay +unlimited, exactly as decided. + +### Verification + +End-to-end, real SessionManager → real shellper → real `claude`: + +| Gesture | Events | Relaunch argv | Verdict | +|---|---|---|---| +| `^C^C` | `fresh-restart`, session live, agent back | new `--session-id`, **no `--resume`**, factory called 1× | PASS | +| SIGKILL | `restart#1`, session live, agent back | **argv unchanged** (recovery preserved), factory called 0× | PASS | + +Unit coverage: session survival, the no-recovery guarantee, a new id per rerun, budget and +crash-loop history untouched across repeated clean exits, no-factory fallback, crash/signal keeping +recovery, PtySession's suppressed `exit` plus its bounded-wait backstop. + +> Trap for the next person: don't run `pnpm build` while the suite is running — `copy-skeleton` +> does `rm -rf skeleton` and pulls it out from under every test that resolves through it. Cost me a +> phantom "108 failures across 56 files" once. From 4b7db2d64995eabbfaae51da32753fcd8805cb4c Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Sun, 26 Jul 2026 22:47:03 -0700 Subject: [PATCH 6/7] [Bugfix #1264] Fix: carry freshLaunch across reconnect; make the give-up loud MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings plus the architect's ruling on the fast-exit valve. 1. CMAP (codex): reconnectSession accepted freshLaunch in its options type but never copied it onto the session. Worse than it sounds — a reconnected architect's args already carry --resume (baked by the #832 restart resolution), so after ANY Tower restart a clean exit would have relaunched straight back into the conversation the user just quit. The headline guarantee, silently broken on the most common surface, with every fresh-path test still green. Fixed, with a regression test that drives the real reconnectSession over a real shellper socket. (My first attempt at that test asserted on a hand-built object literal and never called reconnectSession — it would not have caught the bug it was written for. Rewritten.) 2. Architect ruled KEEP on the fast-clean-exit valve, requiring the give-up be loud. A new 'session-gave-up' event carries a plain-language reason that Tower writes into the terminal itself via PtySession.notice, not just the log: someone with a misconfigured command needs to see why their pane went quiet rather than meeting a mystery dead session. Deliberately NOT reusing 'session-error' — that also fires for transient client errors that do not end the session, so surfacing it as a teardown notice would be wrong. maxRestarts exhaustion has the same silent-death UX and is NOT covered here; noted in the PR as a follow-up rather than quietly widening scope. 3. Valve test coverage per the architect: gives up after 5 fast exits with a reason naming the behavior and saying respawning stopped; one healthy session resets the counter; real (slow) quits never trip it even at twice the threshold. --- codev/state/bugfix-1264_thread.md | 37 ++++ .../src/agent-farm/servers/tower-server.ts | 14 ++ .../__tests__/session-manager.test.ts | 161 +++++++++++++++++- packages/codev/src/terminal/pty-session.ts | 13 ++ .../codev/src/terminal/session-manager.ts | 20 ++- 5 files changed, 238 insertions(+), 7 deletions(-) diff --git a/codev/state/bugfix-1264_thread.md b/codev/state/bugfix-1264_thread.md index 48769bf7d..09e582940 100644 --- a/codev/state/bugfix-1264_thread.md +++ b/codev/state/bugfix-1264_thread.md @@ -71,6 +71,30 @@ All the `0x03` machinery deleted. What replaced it: conversation and needs its own id. Persisting it means a later crash resumes the post-rerun conversation, not the one the user walked away from. +### CMAP round 2 — codex found the bug that mattered most + +gemini APPROVE, claude APPROVE, **codex REQUEST_CHANGES** again, and again correctly: + +> `reconnectSession()` accepts `restartOptions.freshLaunch` in its type, but never copies it into +> `session.options`. + +Verified in the code — the reconnect path assembled `options` field by field and simply omitted the +new one. Impact is worse than it sounds: a reconnected architect's args already carry `--resume` +(baked by the #832 restart resolution), so **after any Tower restart** — which is every +`local-install` — a clean exit would have relaunched straight back into the conversation the user +just quit. The headline guarantee, silently broken on the most common surface, with the fresh-path +tests all still green. + +Fixed, plus the regression test codex noted was missing. My first attempt at that test was junk — it +asserted on a hand-built object literal and never called `reconnectSession`, so it would not have +caught the bug it was written for. Rewrote it against a real `ShellperProcess` over a real socket. +(Discovered while fixing it: several pre-existing reconnect tests guard with `if (client)` and pass +`Date.now()` as the process start time, which never matches, so those bodies silently never run. +Not mine to fix here — worth an issue.) + +That is two rounds where two APPROVEs sat alongside one specific, reproducible codex finding, and +both times the finding was real. + ### One addition beyond the four decisions — flagged, not smuggled Making clean-exit reruns unlimited removes a bound that **both** prior versions had (#1241 ended the @@ -80,6 +104,19 @@ command rather than a human gesture — 5 *consecutive* such exits give up. A re it, and one fast exit followed by a healthy session resets the counter. Genuine gestures stay unlimited, exactly as decided. +Architect ruled KEEP, with two requirements, both implemented: + +1. **The give-up must be loud.** A new `session-gave-up` event carries a plain-language reason that + Tower writes into the terminal itself (`PtySession.notice`) as well as the log — a user with a + misconfigured command sees *why* their pane went quiet instead of a mystery dead session. + Deliberately NOT reusing `session-error`: that also fires for transient client errors which do + not end the session, so surfacing it as a teardown notice would lie. + (`maxRestarts` exhaustion has the same silent-death UX and is *not* covered here — out of scope, + flagged in the PR as a follow-up candidate rather than quietly widened.) +2. **Test the valve**: gives up after 5 fast exits with a reason naming the behavior and saying + respawning stopped; one healthy session resets the counter; slow (real) quits never trip it even + at 2× the threshold. + ### Verification End-to-end, real SessionManager → real shellper → real `claude`: diff --git a/packages/codev/src/agent-farm/servers/tower-server.ts b/packages/codev/src/agent-farm/servers/tower-server.ts index e4c6680c3..0ab07a3ba 100644 --- a/packages/codev/src/agent-farm/servers/tower-server.ts +++ b/packages/codev/src/agent-farm/servers/tower-server.ts @@ -397,6 +397,20 @@ server.listen(port, bindHost, async () => { log('ERROR', `Shellper session ${sessionId}: ${err.message}`); }); + // #1264: SessionManager stopped respawning a session for a reason the user + // needs to see. Put it in the terminal itself, not just the Tower log — the + // person affected is watching a pane that just went quiet, and has no reason + // to go reading server logs to find out why. + shellperManager.on('session-gave-up', (sessionId: string, reason: string) => { + log('ERROR', `Shellper session ${sessionId} gave up: ${reason}`); + const ptySession = getTerminalManager().findByShellperSessionId(sessionId); + if (!ptySession) { + log('WARN', `Shellper session ${sessionId} gave up but no matching terminal session found to notify`); + return; + } + ptySession.notice(reason); + }); + // #1198: SessionManager re-established a session's connection in place // after an unexpected socket close. Re-attach the replacement client to the // PtySession so viewer I/O resumes (attachShellper is idempotent and diff --git a/packages/codev/src/terminal/__tests__/session-manager.test.ts b/packages/codev/src/terminal/__tests__/session-manager.test.ts index cae0b5488..b76bb0b48 100644 --- a/packages/codev/src/terminal/__tests__/session-manager.test.ts +++ b/packages/codev/src/terminal/__tests__/session-manager.test.ts @@ -4,7 +4,8 @@ import fs from 'node:fs'; import path from 'node:path'; import os from 'node:os'; import net from 'node:net'; -import { SessionManager, StderrBuffer, getProcessStartTime, isCrashLooping, CRASH_LOOP_WINDOW_MS, type CreateSessionOptions } from '../session-manager.js'; +import { SessionManager, StderrBuffer, getProcessStartTime, isCrashLooping, CRASH_LOOP_WINDOW_MS, + FAST_CLEAN_EXIT_MS, MAX_FAST_CLEAN_EXITS, type CreateSessionOptions } from '../session-manager.js'; import { ShellperProcess, type IShellperPty, type PtyOptions } from '../shellper-process.js'; import { ShellperClient } from '../shellper-client.js'; @@ -2365,6 +2366,10 @@ describe('clean exits rerun the harness without recovery (Bugfix #1264)', () => stderrTailLogged: false, recoveryRounds: 0, lastRecoveryAt: 0, + // Just launched: exits driven in these tests count as "fast" unless a + // test ages this deliberately (see the valve cases). + lastSpawnAt: Date.now(), + fastCleanExits: 0, }; } @@ -2457,19 +2462,167 @@ describe('clean exits rerun the harness without recovery (Bugfix #1264)', () => const fresh = fakeFreshLaunch(); const { session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); try { - for (let i = 0; i < 5; i++) { - await new Promise((r) => setTimeout(r, 20)); + for (let i = 0; i < 8; i++) { + // Each rerun stayed up long enough to be genuinely used, so every exit + // is a real quit rather than the broken-command case the valve catches. + session.lastSpawnAt = Date.now() - (FAST_CLEAN_EXIT_MS + 1000); session.client.emit('exit', { code: 0, signal: null }); + await new Promise((r) => setTimeout(r, 20)); } await new Promise((r) => setTimeout(r, 50)); expect(session.restartCount).toBe(0); expect(session.failingExitTimes).toEqual([]); - expect(session.client.spawn.mock.calls.length).toBeGreaterThanOrEqual(5); + // 9 = driveExit's own clean exit + the 8 driven above. Well past the + // valve's threshold, and past it repeatedly — none of them count. + expect(session.client.spawn.mock.calls.length).toBe(9); } finally { rmrf(socketDir); } }); + // --- The fast-clean-exit valve (#1264) --- + // + // Unlimited reruns are correct for gestures but must not become an infinite + // spawn loop for a harness that exits 0 on startup. `lastSpawnAt` is set to + // "just now" by fakeSession, so every exit driven here counts as fast unless + // the test ages it deliberately. + + it('gives up loudly after MAX_FAST_CLEAN_EXITS immediate clean exits', async () => { + const fresh = fakeFreshLaunch(); + const socketDir = tmpDir(); + const manager = new SessionManager({ + socketDir, + shellperScript: '/nonexistent/shellper.js', + nodeExecutable: process.execPath, + }); + const session = fakeSession(socketDir, fresh.factory); + (manager as any).sessions.set('clean-1', session); + const gaveUp: Array<[string, string]> = []; + manager.on('session-gave-up', (id: string, reason: string) => gaveUp.push([id, reason])); + const errors: string[] = []; + manager.on('session-error', (_id: string, e: Error) => errors.push(e.message)); + (manager as any).setupAutoRestart(session, 'clean-1'); + + try { + for (let i = 0; i < MAX_FAST_CLEAN_EXITS; i++) { + session.client.emit('exit', { code: 0, signal: null }); + await new Promise((r) => setTimeout(r, 20)); + } + // Stopped respawning, and the session is gone. + expect((manager as any).sessions.has('clean-1')).toBe(false); + // Loud: a user-facing reason, not just a log line. It must name the + // behavior and say respawning stopped, so a misconfigured command is + // diagnosable from the terminal alone. + expect(gaveUp.length).toBe(1); + expect(gaveUp[0][0]).toBe('clean-1'); + expect(gaveUp[0][1]).toMatch(/exited immediately/i); + expect(gaveUp[0][1]).toMatch(/respawning has stopped/i); + expect(errors.some((m) => /immediately after launch/.test(m))).toBe(true); + } finally { + rmrf(socketDir); + } + }); + + it('resets the valve after one healthy session, so gestures stay unlimited', async () => { + const fresh = fakeFreshLaunch(); + const { manager, session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); + try { + await new Promise((r) => setTimeout(r, 30)); + expect(session.fastCleanExits).toBe(1); + + // The harness then stayed up long enough to be genuinely used, so the + // next clean exit is a real quit and the counter must start over. + session.lastSpawnAt = Date.now() - (FAST_CLEAN_EXIT_MS + 1000); + session.client.emit('exit', { code: 0, signal: null }); + await new Promise((r) => setTimeout(r, 30)); + + expect(session.fastCleanExits).toBe(0); + expect((manager as any).sessions.has('clean-1')).toBe(true); + } finally { + rmrf(socketDir); + } + }); + + it('never trips the valve for exits that are slow enough to be real quits', async () => { + const fresh = fakeFreshLaunch(); + const { manager, session, socketDir } = driveExit({ code: 0, signal: null }, fresh.factory); + try { + for (let i = 0; i < MAX_FAST_CLEAN_EXITS * 2; i++) { + session.lastSpawnAt = Date.now() - (FAST_CLEAN_EXIT_MS + 1000); + session.client.emit('exit', { code: 0, signal: null }); + await new Promise((r) => setTimeout(r, 20)); + } + expect(session.fastCleanExits).toBe(0); + expect((manager as any).sessions.has('clean-1')).toBe(true); + } finally { + rmrf(socketDir); + } + }); + + // Caught in review: `reconnectSession` accepted `freshLaunch` in its options + // type but never copied it onto the session. Because a reconnected + // architect's args already carry `--resume` (baked by the #832 restart + // resolution), every clean exit AFTER a Tower restart would have relaunched + // straight back into the conversation the user had just quit — the spec + // violation this fix exists to prevent, on the most common surface there is. + it('carries freshLaunch across reconnect, so post-Tower-restart clean exits stay fresh', async () => { + const socketDir = tmpDir(); + let capturedPty: MockPty | null = null; + const socketPath = path.join(socketDir, 'reconnect-fresh.sock'); + const shellper = new ShellperProcess( + () => { + capturedPty = new MockPty(); + return capturedPty; + }, + socketPath, + 100, + ); + await shellper.start('/bin/bash', [], '/tmp', {}, 80, 24); + + const manager = new SessionManager({ + socketDir, + shellperScript: '/nonexistent/shellper.js', + nodeExecutable: process.execPath, + }); + + try { + const freshLaunch = { next: () => ({ args: ['--session-id', 'reconnect-fresh'] }) }; + // Reconnect exactly as Tower's reconciliation does, with the `--resume` + // args the #832 restart resolution bakes in for a surviving architect. + // The real start time — reconnectSession validates it against the process + // table to guard against PID reuse, and returns null on a mismatch. + const startTime = getProcessStartTime(process.pid)!; + const client = await manager.reconnectSession('reconnect-1', socketPath, process.pid, startTime, { + command: 'claude', + args: ['--resume', 'abandoned-conversation'], + cwd: '/tmp', + env: {}, + restartDelay: 1, + maxRestarts: 50, + freshLaunch, + }); + expect(client).not.toBeNull(); + + const spawnSpy = vi.fn(); + const realSpawn = client!.spawn.bind(client!); + client!.spawn = (msg: any) => { spawnSpy(msg); return realSpawn(msg); }; + + capturedPty!.simulateExit(0); + await new Promise((r) => setTimeout(r, 200)); + + expect(spawnSpy).toHaveBeenCalledTimes(1); + const argv = spawnSpy.mock.calls[0][0].args; + expect(argv).not.toContain('--resume'); + expect(argv).not.toContain('abandoned-conversation'); + expect(argv).toEqual(['--session-id', 'reconnect-fresh']); + // And the session survived, as on any other clean exit. + expect(manager.getSessionInfo('reconnect-1')).not.toBeNull(); + } finally { + shellper.shutdown(); + rmrf(socketDir); + } + }); + it('reuses the current args when no fresh-launch factory is supplied', async () => { // Sessions with no recovery concept (plain shells) still rerun. const { session, socketDir } = driveExit({ code: 0, signal: null }); diff --git a/packages/codev/src/terminal/pty-session.ts b/packages/codev/src/terminal/pty-session.ts index b01621012..9a091485c 100644 --- a/packages/codev/src/terminal/pty-session.ts +++ b/packages/codev/src/terminal/pty-session.ts @@ -264,6 +264,19 @@ export class PtySession extends EventEmitter { client.on('data', cancelCleanup); } + /** + * Write an out-of-band notice into the terminal, as if the process had + * printed it. Goes to the ring buffer and every attached client, so it + * survives reconnects and is visible to whoever is watching. + * + * For lifecycle news the process itself cannot report — #1264's give-up + * being the motivating case, where a broken harness would otherwise leave a + * silently dead terminal. + */ + notice(text: string): void { + this.onPtyData(`\r\n\x1b[33m${text}\x1b[0m\r\n`); + } + /** Whether this session is backed by a shellper process. */ get shellperBacked(): boolean { return this._shellperBacked; diff --git a/packages/codev/src/terminal/session-manager.ts b/packages/codev/src/terminal/session-manager.ts index 0153d4593..9c4ccbded 100644 --- a/packages/codev/src/terminal/session-manager.ts +++ b/packages/codev/src/terminal/session-manager.ts @@ -572,6 +572,12 @@ export class SessionManager extends EventEmitter { restartResetAfter: restartOptions?.restartResetAfter, }), crashLoopFallback: restartOptions?.crashLoopFallback, + // #1264: must be carried across reconnect. A reconnected architect's + // args already contain `--resume` (baked by the #832 restart + // resolution), so dropping the factory here would make every clean exit + // after a Tower restart relaunch INTO the conversation the user just + // quit — the exact thing the fix exists to prevent. + freshLaunch: restartOptions?.freshLaunch, }, restartCount: 0, restartResetTimer: null, @@ -1147,9 +1153,17 @@ export class SessionManager extends EventEmitter { const uptime = Date.now() - session.lastSpawnAt; session.fastCleanExits = uptime < FAST_CLEAN_EXIT_MS ? session.fastCleanExits + 1 : 0; if (session.fastCleanExits >= MAX_FAST_CLEAN_EXITS) { - this.log( - `Session ${sessionId} exited cleanly ${session.fastCleanExits}x within ${FAST_CLEAN_EXIT_MS}ms of launch; the harness is exiting immediately rather than being quit — giving up`, - ); + const reason = + `The harness exited immediately (cleanly, within ${FAST_CLEAN_EXIT_MS / 1000}s of launch) ` + + `${session.fastCleanExits} times in a row. This looks like a broken or misconfigured ` + + `command rather than you quitting it, so respawning has stopped.`; + this.log(`Session ${sessionId} gave up: ${reason}`); + // The give-up must be visible to the person watching the terminal — + // otherwise a misconfigured command reads as a mysteriously dead + // session. `session-error` is not a substitute: it also fires for + // transient client errors that do NOT end the session, so it cannot + // be surfaced to the user as a teardown notice. + this.emit('session-gave-up', sessionId, reason); this.emit( 'session-error', sessionId, From 151ff0766451cb1f053c389d3d8b15fc21ef27a5 Mon Sep 17 00:00:00 2001 From: M Waleed Kadous Date: Mon, 27 Jul 2026 00:20:57 -0700 Subject: [PATCH 7/7] chore(porch): bugfix-1264 pr gate-approved --- .../bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml index 44cab98eb..a1f5f86bd 100644 --- a/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml +++ b/codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml @@ -6,11 +6,12 @@ plan_phases: [] current_plan_phase: null gates: pr: - status: pending + status: approved requested_at: '2026-07-27T01:14:29.680Z' + approved_at: '2026-07-27T07:20:57.861Z' iteration: 1 build_complete: false history: [] started_at: '2026-07-27T00:49:40.123Z' -updated_at: '2026-07-27T01:14:29.680Z' -pr_ready_for_human: true +updated_at: '2026-07-27T07:20:57.862Z' +pr_ready_for_human: false