[Bugfix #1264] A clean harness exit reruns fresh instead of killing the session

codev/projects/bugfix-1264-regression-3-2-4-double-ctrl-c/status.yaml

@@ -0,0 +1,17 @@
+id: bugfix-1264
+title: regression-3-2-4-double-ctrl-c
+protocol: bugfix
+phase: pr
+plan_phases: []
+current_plan_phase: null
+gates:
+  pr:
+    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-27T07:20:57.862Z'
+pr_ready_for_human: false

codev/state/bugfix-1264_thread.md

@@ -0,0 +1,135 @@
+# 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.
+
+### 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
+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.
+
+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`:
+
+| 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.

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 }),
       });
 

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

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<void> {
           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

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<string, string>;
+  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.