From 360d58ca4f2fa1ed70bb6640e8bde3eef83b8634 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Sun, 12 Jul 2026 18:21:42 +0200 Subject: [PATCH] fix(review): breaker reset semantics, trip observability, push template (PR #147) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- src/session-cli-builder.ts | 9 ++- src/session.ts | 9 ++- src/types/session.ts | 5 ++ src/web/public/app.js | 45 +++++++++--- src/web/routes/respawn-routes.ts | 4 + src/web/routes/session-routes.ts | 28 ++++++- src/web/schemas.ts | 10 +++ src/web/server.ts | 11 +++ test/respawn-pty-breaker.test.ts | 113 ++++++++++++++++++++++++++++- test/routes/session-routes.test.ts | 50 +++++++++++++ 10 files changed, 265 insertions(+), 19 deletions(-) diff --git a/src/session-cli-builder.ts b/src/session-cli-builder.ts index 72ee3f96..95f9f0da 100644 --- a/src/session-cli-builder.ts +++ b/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 { - return { + const env: Record = { ...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 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); diff --git a/src/types/session.ts b/src/types/session.ts index 460f6e67..979fcbec 100644 --- a/src/types/session.ts +++ b/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; } /** diff --git a/src/web/public/app.js b/src/web/public/app.js index 3b0a8f58..7c7e0fba 100644 --- a/src/web/public/app.js +++ b/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 - // or detached for some other reason. Re-attach regardless of status. - 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); + if (session.respawnBlocked) { + // COD-118: the PTY-exit circuit breaker tripped for this session — the + // automatic re-attach must NOT silently clear it (that would re-arm the + // crash loop on every tab click / page load). Restart only on explicit + // user confirmation; the confirmed request carries clearBreaker:true. + const label = session.name || 'Session'; + if (window.confirm(`${label} was stopped after crashing repeatedly. Restart it?`)) { + try { + await fetch(`/api/sessions/${sessionId}/interactive`, { + method: 'POST', + 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); + } } } diff --git a/src/web/routes/respawn-routes.ts b/src/web/routes/respawn-routes.ts index 834ee3bf..2688d93d 100644 --- a/src/web/routes/respawn-routes.ts +++ b/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({ diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 7c81137e..3cd05d1d 100644 --- a/src/web/routes/session-routes.ts +++ b/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 - // circuit breaker so an intentional restart is never blocked by a prior crash-loop. - session.resetRespawnBreaker(); + // COD-118: ONLY an explicit user-initiated restart (body {clearBreaker:true}) + // clears a tripped PTY-exit circuit breaker. This endpoint is ALSO the frontend's + // 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', diff --git a/src/web/schemas.ts b/src/web/schemas.ts index 45aeddd4..b0da630b 100644 --- a/src/web/schemas.ts +++ b/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(), diff --git a/src/web/server.ts b/src/web/server.ts index 428630cc..286059e5 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -1264,6 +1264,13 @@ export class WebServer extends EventEmitter { } private async setupSessionListeners(session: Session): Promise { + // 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)}`; diff --git a/test/respawn-pty-breaker.test.ts b/test/respawn-pty-breaker.test.ts index 417ec5f2..86362afe 100644 --- a/test/respawn-pty-breaker.test.ts +++ b/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(); + 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[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>)[ + '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); + }); +}); diff --git a/test/routes/session-routes.test.ts b/test/routes/session-routes.test.ts index 7f0d8a39..9975a305 100644 --- a/test/routes/session-routes.test.ts +++ b/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 () => {