fix(review): breaker reset semantics, trip observability, push template (PR #147)

- Breaker reset is now explicit-only: POST /api/sessions/:id/interactive no
  longer unconditionally resets the PTY-exit breaker (that endpoint IS the
  frontend's automatic re-attach path, so the breaker could never trip on the
  COD-115 crash loop and any tab click silently re-armed it). The route accepts
  a schema-validated optional body flag {clearBreaker:true}
  (InteractiveStartSchema) and resets only when it is sent.
- Frontend restart control: app.js selectSession keeps the bare auto-attach
  (no body, never clears); when the selected session has respawnBlocked it asks
  for explicit user confirmation and only then re-POSTs with clearBreaker:true.
  respawnBlocked is surfaced via SessionState/toState() (runtime-only, not
  restored on boot so recovery can re-attach).
- Trip observability: WebServer.setupSessionListeners() is now idempotent
  (skips while refs are attached) and the re-attach routes (/interactive,
  /interactive-respawn, /shell) re-run it, restoring the wiring that the exit
  handler detaches on every PTY exit — without this the 5th-exit trip had
  guaranteed zero listeners (no SSE, no push, no persist, no run-summary).
- Push notification: added SessionRespawnBreakerTripped to PUSH_EVENT_MAP
  ('Session crash loop stopped', urgency critical) with an exit-count body
  branch; previously sendPushNotifications silently no-oped.
- Minor: buildMuxAttachEnv() truecolor param is now actually passed
  (codex/gemini, mirrors buildEnvExports); buildClaudeEnv() uses delete for
  COLORTERM/CLAUDECODE (same node-pty "KEY=undefined" quirk as COD-115).
- Tests: route tests assert auto-reattach does NOT reset, clearBreaker resets,
  invalid flag rejected, and listener re-wiring on /interactive + /shell;
  real-wiring lifecycle tests (createSessionListeners/attach/detach) prove the
  exit-detach gap and that re-setup keeps the 5th-exit trip observable;
  PUSH_EVENT_MAP regression guard.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

src/session-cli-builder.ts

@@ -102,14 +102,12 @@ export function buildPromptArgs(prompt: string, model?: string): string[] {
  * @returns Environment variables object for pty.spawn
  */
 export function buildClaudeEnv(sessionId: string): Record<string, string | undefined> {
-  return {
+  const env: Record<string, string | undefined> = {
     ...process.env,
     LANG: 'en_US.UTF-8',
     LC_ALL: 'en_US.UTF-8',
     PATH: getAugmentedPath(),
     TERM: 'xterm-256color',
-    COLORTERM: undefined,
-    CLAUDECODE: undefined,
     // Inform Claude it's running within Codeman (helps prevent self-termination)
     CODEMAN_MUX: '1',
     CODEMAN_SESSION_ID: sessionId,
@@ -117,6 +115,11 @@ export function buildClaudeEnv(sessionId: string): Record<string, string | undef
     // Path only (not the secret value) — hook curls cat it at execution time (COD-54)
     CODEMAN_HOOK_SECRET_FILE: dataPath('hook-secret'),
   };
+  // COD-115: `delete`, not `= undefined` — node-pty serializes a present-with-undefined
+  // key as the literal string "KEY=undefined" (see buildMuxAttachEnv below).
+  delete env.COLORTERM;
+  delete env.CLAUDECODE;
+  return env;
 }
 
 /**

src/session.ts

@@ -1029,6 +1029,11 @@ export class Session extends EventEmitter {
       geminiConfig: this._geminiConfig,
       resumeSessionId: this._resumeSessionId,
       effort: this._effort,
+      // COD-118: runtime-only — surfaced so the frontend can require explicit user
+      // intent before restarting a crash-looped session. Deliberately NOT restored
+      // by the constructor: a Codeman restart starts with a fresh breaker so boot
+      // recovery can re-attach.
+      respawnBlocked: this._respawnBlocked || undefined,
       attachmentHistory: this.attachmentHistory.length > 0 ? this.attachmentHistory : undefined,
       // envOverrides intentionally NOT on the public SessionState type — they must not
       // leak into SSE / GET /api/sessions broadcasts (schema allows OPENCODE_*, which
@@ -1180,7 +1185,9 @@ export class Session extends EventEmitter {
         cols: ptyCols,
         rows: ptyRows,
         cwd: this.workingDir,
-        env: buildMuxAttachEnv(),
+        // COD-75: codex/gemini get COLORTERM=truecolor — mirrors buildEnvExports()
+        // in tmux-manager.ts so the attach client and the tmux session agree.
+        env: buildMuxAttachEnv(this.mode === 'codex' || this.mode === 'gemini'),
       });
     } catch (spawnErr) {
       console.error(`[Session] Failed to spawn PTY for ${options.spawnErrLabel}:`, spawnErr);

src/types/session.ts

@@ -234,6 +234,11 @@ export interface SessionState {
   effort?: EffortLevel;
   /** Sanitized per-session attachment history. */
   attachmentHistory?: SessionAttachmentHistoryItem[];
+  /**
+   * PTY-exit circuit breaker tripped — respawn blocked until an explicit restart
+   * (COD-118). Runtime-only: never restored on boot (fresh server = fresh breaker).
+   */
+  respawnBlocked?: boolean;
 }
 
 /**

src/web/public/app.js

@@ -3575,17 +3575,40 @@ class CodemanApp {
     // Track working directory for path normalization in Project Insights
     this.currentSessionWorkingDir = session?.workingDir || null;
     if (session && session.pid === null) {
-      // Session has no PTY attached — either restored after server restart
+      if (session.respawnBlocked) {
-      // or detached for some other reason. Re-attach regardless of status.
+        // COD-118: the PTY-exit circuit breaker tripped for this session — the
-      try {
+        // automatic re-attach must NOT silently clear it (that would re-arm the
-        const endpoint = session.mode === 'shell'
+        // crash loop on every tab click / page load). Restart only on explicit
-          ? `/api/sessions/${sessionId}/shell`
+        // user confirmation; the confirmed request carries clearBreaker:true.
-          : `/api/sessions/${sessionId}/interactive`;
+        const label = session.name || 'Session';
-        await fetch(endpoint, { method: 'POST' });
+        if (window.confirm(`${label} was stopped after crashing repeatedly. Restart it?`)) {
-        // Update local session state
+          try {
-        session.status = 'busy';
+            await fetch(`/api/sessions/${sessionId}/interactive`, {
-      } catch (err) {
+              method: 'POST',
-        console.error('Failed to attach to restored session:', err);
+              headers: { 'Content-Type': 'application/json' },
+              body: JSON.stringify({ clearBreaker: true }),
+            });
+            session.respawnBlocked = false;
+            session.status = 'busy';
+          } catch (err) {
+            console.error('Failed to restart crash-looped session:', err);
+          }
+        }
+      } else {
+        // Session has no PTY attached — either restored after server restart
+        // or detached for some other reason. Re-attach regardless of status.
+        // Deliberately NO body: this automatic path must never clear a tripped
+        // PTY-exit breaker (COD-118).
+        try {
+          const endpoint = session.mode === 'shell'
+            ? `/api/sessions/${sessionId}/shell`
+            : `/api/sessions/${sessionId}/interactive`;
+          await fetch(endpoint, { method: 'POST' });
+          // Update local session state
+          session.status = 'busy';
+        } catch (err) {
+          console.error('Failed to attach to restored session:', err);
+        }
       }
     }
 

src/web/routes/respawn-routes.ts

@@ -246,6 +246,10 @@ export function registerRespawnRoutes(
         }
       }
 
+      // Re-attach listener wiring if a prior PTY exit detached it (the wiring exit
+      // handler removes ALL session listeners; idempotent — no-op while still attached).
+      await ctx.setupSessionListeners(session);
+
       // Start interactive session
       await session.startInteractive();
       getLifecycleLog().log({

src/web/routes/session-routes.ts

@@ -34,6 +34,7 @@ import {
   FlickerFilterSchema,
   QuickRunSchema,
   QuickStartSchema,
+  InteractiveStartSchema,
 } from '../schemas.js';
 import {
   autoConfigureRalph,
@@ -611,6 +612,14 @@ export function registerSessionRoutes(
 
   app.post('/api/sessions/:id/interactive', async (req) => {
     const { id } = req.params as { id: string };
+    // Body is optional (auto-reattach callers send none) — same idiom as /interactive-respawn.
+    const bodyResult = req.body
+      ? InteractiveStartSchema.safeParse(req.body)
+      : { success: true as const, data: {} as { clearBreaker?: boolean } };
+    if (!bodyResult.success) {
+      return createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Invalid request body');
+    }
+    const { clearBreaker } = bodyResult.data;
     const session = findSessionOrFail(ctx, id);
 
     if (session.isBusy()) {
@@ -633,9 +642,20 @@ export function registerSessionRoutes(
         }
       }
 
-      // COD-118: an explicit user-initiated start clears any tripped PTY-exit
+      // COD-118: ONLY an explicit user-initiated restart (body {clearBreaker:true})
-      // circuit breaker so an intentional restart is never blocked by a prior crash-loop.
+      // clears a tripped PTY-exit circuit breaker. This endpoint is ALSO the frontend's
-      session.resetRespawnBreaker();
+      // automatic re-attach path (selectSession auto-POSTs it for any pid===null
+      // session), so an unconditional reset here would re-arm the exact crash loop
+      // the breaker exists to stop — auto-reattach sends no body and must not clear.
+      if (clearBreaker) {
+        session.resetRespawnBreaker();
+      }
+      // Re-attach listener wiring if a prior PTY exit detached it: the wiring exit
+      // handler removes ALL session listeners (incl. respawnBreakerTripped), and only
+      // session-create/boot-recovery paths ran setupSessionListeners before this fix —
+      // without this, a re-attached session's SSE/terminal/trip events go unobserved.
+      // setupSessionListeners is idempotent (no-op while refs are still attached).
+      await ctx.setupSessionListeners(session);
       await session.startInteractive();
       getLifecycleLog().log({
         event: 'started',
@@ -663,6 +683,8 @@ export function registerSessionRoutes(
     }
 
     try {
+      // Re-attach listener wiring if a prior PTY exit detached it (see /interactive).
+      await ctx.setupSessionListeners(session);
       await session.startShell();
       getLifecycleLog().log({
         event: 'started',

src/web/schemas.ts

@@ -673,6 +673,16 @@ export const SubagentWindowStatesSchema = z
 /** PUT /api/subagent-parents */
 export const SubagentParentMapSchema = z.record(z.string(), z.string());
 
+/** POST /api/sessions/:id/interactive */
+export const InteractiveStartSchema = z.object({
+  /**
+   * COD-118: explicit user-initiated restart — clears a tripped PTY-exit circuit
+   * breaker before starting. Automatic reconnect/re-attach callers (e.g. the
+   * frontend's selectSession auto-attach) must NOT send this flag.
+   */
+  clearBreaker: z.boolean().optional(),
+});
+
 /** POST /api/sessions/:id/interactive-respawn */
 export const InteractiveRespawnSchema = z.object({
   respawnConfig: RespawnConfigSchema.optional(),

src/web/server.ts

@@ -1264,6 +1264,13 @@ export class WebServer extends EventEmitter {
   }
 
   private async setupSessionListeners(session: Session): Promise<void> {
+    // Idempotent: the wiring exit handler detaches ALL listeners on every PTY exit
+    // (removeSessionListenerRefs), so the re-attach routes (/interactive,
+    // /interactive-respawn, /shell) call this again to restore observability
+    // (terminal SSE, error/exit broadcasts, the COD-118 respawnBreakerTripped
+    // handler). Skip when the refs are still attached to avoid double-wiring.
+    if (this.sessionListenerRefs.has(session.id)) return;
+
     // Create run summary tracker for this session
     const summaryTracker = new RunSummaryTracker(session.id, session.name);
     this.runSummaryTrackers.set(session.id, summaryTracker);
@@ -1765,6 +1772,7 @@ export class WebServer extends EventEmitter {
     [SseEvent.HookStop]: { title: 'Response Complete', urgency: 'info' },
     [SseEvent.SessionError]: { title: 'Session Error', urgency: 'critical' },
     [SseEvent.RespawnBlocked]: { title: 'Respawn Blocked', urgency: 'critical' },
+    [SseEvent.SessionRespawnBreakerTripped]: { title: 'Session crash loop stopped', urgency: 'critical' },
     [SseEvent.SessionRalphCompletionDetected]: { title: 'Task Complete', urgency: 'warning' },
   };
 
@@ -1797,6 +1805,9 @@ export class WebServer extends EventEmitter {
     } else if (event === SseEvent.SessionRalphCompletionDetected && data.phrase) {
       body += body ? ' ' : '';
       body += String(data.phrase);
+    } else if (event === SseEvent.SessionRespawnBreakerTripped && data.count) {
+      body += body ? ' ' : '';
+      body += `Stopped after ${Number(data.count)} rapid crashes — restart the session to retry`;
     } else if (event === SseEvent.HookPermissionPrompt && data.tool_name) {
       body += body ? ' ' : '';
       body += `Tool: ${String(data.tool_name)}`;

test/respawn-pty-breaker.test.ts

@@ -5,14 +5,25 @@
  * INJECTED time (no real timers, fully deterministic), plus a Session-level
  * assertion via MockSession that repeated non-zero exits flip the session to
  * `error` + block respawn, and that an explicit reset re-enables spawning.
+ * Also covers the REAL listener wiring lifecycle (createSessionListeners /
+ * attach / detach): the wiring exit handler detaches everything on each PTY
+ * exit, so the re-attach routes must re-wire or the trip goes unobserved.
  */
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi } from 'vitest';
 import {
   InteractivePtyExitBreaker,
   DEFAULT_BREAKER_THRESHOLD,
   DEFAULT_BREAKER_WINDOW_MS,
 } from '../src/session-pty-exit-breaker.js';
 import { MockSession } from './mocks/index.js';
+import {
+  createSessionListeners,
+  attachSessionListeners,
+  detachSessionListeners,
+  type SessionListenerRefs,
+} from '../src/web/session-listener-wiring.js';
+import { SseEvent } from '../src/web/sse-events.js';
+import type { Session } from '../src/session.js';
 
 describe('InteractivePtyExitBreaker — pure logic', () => {
   it('exports sane default constants', () => {
@@ -188,3 +199,103 @@ describe('Session-level trip/reset (AC#4, via MockSession)', () => {
     expect(breaker.tripped).toBe(false);
   });
 });
+
+describe('trip observability through the REAL listener wiring (COD-118)', () => {
+  // Mirrors the server: refs map + removeSessionListenerRefs (called by the wiring
+  // exit handler on EVERY PTY exit) detaching all listeners, and an idempotent
+  // setup() like WebServer.setupSessionListeners that the re-attach routes
+  // (/interactive, /interactive-respawn, /shell) now re-run.
+  function makeHarness() {
+    const session = new MockSession('wiring-breaker-session');
+    const refsMap = new Map<string, SessionListenerRefs>();
+    const deps = {
+      broadcast: vi.fn(),
+      batchTerminalData: vi.fn(),
+      batchTaskUpdate: vi.fn(),
+      broadcastSessionStateDebounced: vi.fn(),
+      sendPushNotifications: vi.fn(),
+      persistSessionState: vi.fn(),
+      getSessionStateWithRespawn: vi.fn(() => ({ id: session.id })),
+      getRunSummaryTracker: vi.fn(() => undefined),
+      stopTranscriptWatcher: vi.fn(),
+      cleanupSessionBatches: vi.fn(),
+      cancelPersistDebounce: vi.fn(),
+      removeRunSummaryTracker: vi.fn(),
+      // Same as server.ts removeSessionListenerRefs: detach ALL wiring listeners.
+      removeSessionListenerRefs: (id: string) => {
+        const refs = refsMap.get(id);
+        if (refs) detachSessionListeners(session as unknown as Session, refs);
+        refsMap.delete(id);
+      },
+      cleanupRespawnOnExit: vi.fn(),
+      getStore: vi.fn(),
+      registerAttachment: vi.fn(async () => {}),
+    };
+    const setup = () => {
+      if (refsMap.has(session.id)) return; // idempotence guard, as in server.ts
+      const refs = createSessionListeners(
+        session as unknown as Session,
+        deps as unknown as Parameters<typeof createSessionListeners>[1]
+      );
+      refsMap.set(session.id, refs);
+      attachSessionListeners(session as unknown as Session, refs);
+    };
+    return { session, deps, setup };
+  }
+
+  it('every PTY exit detaches ALL wiring listeners (the gap the re-attach routes must close)', () => {
+    const { session, setup } = makeHarness();
+    setup(); // session-create wiring
+    expect(session.listenerCount('respawnBreakerTripped')).toBe(1);
+    session.emit('exit', 1);
+    // After exit #1 the trip listener is gone — a later trip would be unobserved.
+    expect(session.listenerCount('respawnBreakerTripped')).toBe(0);
+    expect(session.listenerCount('terminal')).toBe(0);
+  });
+
+  it('re-running setup() after each exit keeps the 5th-exit trip observable (SSE + push + persist)', () => {
+    const { session, deps, setup } = makeHarness();
+    setup(); // session-create wiring
+    // Exits 1–4: each detaches the wiring; the /interactive re-attach re-wires it.
+    for (let i = 1; i <= 4; i++) {
+      session.emit('exit', 1);
+      setup(); // what the fixed re-attach routes now do
+    }
+    // 5th rapid non-zero exit: the real Session emits respawnBreakerTripped
+    // (inside its onExit handler) BEFORE emitting 'exit'.
+    session.emit('respawnBreakerTripped', { count: 5 });
+    session.emit('exit', 1);
+
+    expect(deps.broadcast).toHaveBeenCalledWith(SseEvent.SessionRespawnBreakerTripped, {
+      sessionId: session.id,
+      count: 5,
+    });
+    expect(deps.sendPushNotifications).toHaveBeenCalledWith(
+      SseEvent.SessionRespawnBreakerTripped,
+      expect.objectContaining({ sessionId: session.id, count: 5 })
+    );
+    expect(deps.persistSessionState).toHaveBeenCalled();
+  });
+
+  it('setup() is idempotent — re-running while still wired must not double-attach', () => {
+    const { session, setup } = makeHarness();
+    setup();
+    setup(); // e.g. POST /interactive on a freshly created session
+    expect(session.listenerCount('respawnBreakerTripped')).toBe(1);
+    expect(session.listenerCount('exit')).toBe(1);
+  });
+});
+
+describe('push template registration (COD-118)', () => {
+  it('SessionRespawnBreakerTripped has a PUSH_EVENT_MAP entry (sendPushNotifications silently no-ops without one)', async () => {
+    const { WebServer } = await import('../src/web/server.js');
+    const map = (WebServer as unknown as Record<string, Record<string, { title: string; urgency: string }>>)[
+      'PUSH_EVENT_MAP'
+    ];
+    expect(map).toBeDefined();
+    const entry = map[SseEvent.SessionRespawnBreakerTripped];
+    expect(entry).toBeDefined();
+    expect(entry.urgency).toBe('critical');
+    expect(entry.title.length).toBeGreaterThan(0);
+  });
+});

test/routes/session-routes.test.ts

@@ -608,6 +608,54 @@ describe('session-routes', () => {
       const body = JSON.parse(res.body);
       expect(body.success).toBe(false);
     });
+
+    // COD-118: this endpoint is ALSO the frontend's automatic re-attach path, so it
+    // must never clear a tripped PTY-exit breaker unless the request explicitly asks.
+    it('does NOT clear the PTY-exit breaker on an automatic re-attach (no body)', async () => {
+      const res = await harness.app.inject({
+        method: 'POST',
+        url: `/api/sessions/${harness.ctx._sessionId}/interactive`,
+      });
+      expect(res.statusCode).toBe(200);
+      expect(harness.ctx._session.resetRespawnBreaker).not.toHaveBeenCalled();
+      expect(harness.ctx._session.startInteractive).toHaveBeenCalled();
+    });
+
+    it('clears the PTY-exit breaker when the explicit restart flag is sent (COD-118)', async () => {
+      const res = await harness.app.inject({
+        method: 'POST',
+        url: `/api/sessions/${harness.ctx._sessionId}/interactive`,
+        payload: { clearBreaker: true },
+      });
+      expect(res.statusCode).toBe(200);
+      expect(harness.ctx._session.resetRespawnBreaker).toHaveBeenCalledTimes(1);
+      expect(harness.ctx._session.startInteractive).toHaveBeenCalled();
+    });
+
+    it('rejects a non-boolean clearBreaker flag', async () => {
+      const res = await harness.app.inject({
+        method: 'POST',
+        url: `/api/sessions/${harness.ctx._sessionId}/interactive`,
+        payload: { clearBreaker: 'yes' },
+      });
+      expect(res.statusCode).toBe(400);
+      const body = JSON.parse(res.body);
+      expect(body.success).toBe(false);
+      expect(body.errorCode).toBe(ApiErrorCode.INVALID_INPUT);
+      expect(harness.ctx._session.resetRespawnBreaker).not.toHaveBeenCalled();
+      expect(harness.ctx._session.startInteractive).not.toHaveBeenCalled();
+    });
+
+    // COD-118: the wiring exit handler detaches ALL session listeners on PTY exit;
+    // re-attach must restore them or later trips/output go unobserved.
+    it('re-runs session listener wiring before starting', async () => {
+      const res = await harness.app.inject({
+        method: 'POST',
+        url: `/api/sessions/${harness.ctx._sessionId}/interactive`,
+      });
+      expect(res.statusCode).toBe(200);
+      expect(harness.ctx.setupSessionListeners).toHaveBeenCalledWith(harness.ctx._session);
+    });
   });
 
   // ========== POST /api/sessions/:id/shell ==========
@@ -622,6 +670,8 @@ describe('session-routes', () => {
       const body = JSON.parse(res.body);
       expect(body.success).toBe(true);
       expect(harness.ctx._session.startShell).toHaveBeenCalled();
+      // COD-118: re-attach restores listener wiring detached by a prior PTY exit.
+      expect(harness.ctx.setupSessionListeners).toHaveBeenCalledWith(harness.ctx._session);
     });
 
     it('returns error if session is busy', async () => {