From cf69b067a4d615e5a2cc74d4107f87f77d36a583 Mon Sep 17 00:00:00 2001 From: "panopticon-agent[bot]" <4205044+panopticon-agent[bot]@users.noreply.github.com> Date: Sun, 16 Aug 2026 08:13:03 -0400 Subject: [PATCH] fix(delivery): stop state-projected supervisor method from stalling review loops A codex app-server work agent was stamped supervisorEnabled=true at spawn even though its launcher never wraps in the PTY supervisor, and the state projection turned that into deliveryMethod: 'supervisor'. deliverAgentMessage treated the persisted value as a strict per-call opt-in (PAN-1769 semantics), so every state-routed delivery to the socketless agent threw socket-missing with no fallback. On 2026-08-16 this swallowed the inspect verdict for PAN-3743: the work agent parked waiting, the review loop stalled, and only a console.error recorded it until the PAN-2583 workspace-verdict sweep healed the loop ~30 minutes later. Three changes, root to symptom: - supervisor-channels: decideSupervisorForWorkAgent rejects codex on the app-server transport (the app-server branch of buildCodexCommand skips the wrap), so the supervisor stamp is never applied to a launch that cannot have a socket. Matches shouldUseSupervisorForConversation and the launcher's own config read. The codex.transport access is optional-chained like runtime-command.ts so a partial config can't throw mid-spawn. - delivery: deliverAgentMessage routes a state-derived 'supervisor' through resilientDeliveryMethod (PAN-1988), so a stale projection falls through to the live app-server/channels/tmux tiers. An explicit deliveryMethod caller argument keeps the strict throw-on-failure contract. - inspect-agent: a verdict that fails to deliver now also surfaces an operator-visible needs-you mark via surfaceIssueFeedbackNeedsYou (the PAN-2228 pattern), instead of console-only. Best-effort: the error log is preserved verbatim and a needs-you failure can't mask the original error. Refs PAN-3078, PAN-2848, PAN-3257, PAN-2580, PAN-3560. Co-Authored-By: Claude --- src/lib/agents/__tests__/delivery.test.ts | 55 ++++++++ .../supervisor-codex-transport.test.ts | 123 ++++++++++++++++++ src/lib/agents/delivery.ts | 12 +- src/lib/agents/supervisor-channels.ts | 15 ++- .../cloister/__tests__/inspect-agent.test.ts | 51 ++++++++ src/lib/cloister/inspect-agent.ts | 30 ++++- 6 files changed, 282 insertions(+), 4 deletions(-) create mode 100644 src/lib/agents/__tests__/supervisor-codex-transport.test.ts diff --git a/src/lib/agents/__tests__/delivery.test.ts b/src/lib/agents/__tests__/delivery.test.ts index b76cf9645f6..b0d43d61c5a 100644 --- a/src/lib/agents/__tests__/delivery.test.ts +++ b/src/lib/agents/__tests__/delivery.test.ts @@ -402,6 +402,61 @@ describe('PTY supervisor delivery', () => { }); }); +describe('state-projected supervisor method (PAN-3743 review-loop stall)', () => { + beforeEach(() => { + tmpHome = mkdtempSync(join(tmpdir(), 'pan-state-supervisor-delivery-')); + stateDir = join(tmpHome, 'agents'); + socketDir = join(tmpHome, 'sockets'); + mkdirSync(stateDir, { recursive: true }); + mkdirSync(socketDir, { recursive: true }); + process.env.OVERDECK_HOME = tmpHome; + vi.mocked(sendKeys).mockClear(); + }); + + afterEach(() => { + delete process.env.OVERDECK_HOME; + rmSync(tmpHome, { recursive: true, force: true }); + }); + + // Reproduces the 2026-08-16 PAN-3743 stall: a codex app-server work agent + // whose state.json carries deliveryMethod: 'supervisor' (projected from + // supervisorEnabled) but has no PTY supervisor socket. A state-routed + // delivery (the inspect verdict) previously threw socket-missing with no + // fallback; it must now ride the resilient cascade to the app-server tier. + it('routes a state-stamped supervisor method to the live app-server socket', async () => { + const agentId = 'agent-state-supervisor-appserver'; + writeAgentState(agentId, { deliveryMethod: 'supervisor', supervisorEnabled: true }); + writeAppServerToken(agentId); + const capture: { lastBody?: string } = {}; + const server = await startFakeBridge(join(socketDir, `appserver-${agentId}.sock`), { capture }); + try { + const result = await deliverAgentMessage(agentId, 'inspect verdict', 'inspect-verdict'); + expect(result).toEqual({ ok: true, path: 'app-server' }); + expect(JSON.parse(capture.lastBody!)).toMatchObject({ op: 'message', content: 'inspect verdict' }); + expect(vi.mocked(sendKeys)).not.toHaveBeenCalled(); + } finally { + await new Promise((resolve) => server.close(() => resolve())); + } + }); + + it('falls through to tmux when a state-stamped supervisor method has no sockets at all', async () => { + const agentId = 'agent-state-supervisor-tmux'; + writeAgentState(agentId, { deliveryMethod: 'supervisor', supervisorEnabled: true }); + const result = await deliverAgentMessage(agentId, 'inspect verdict', 'inspect-verdict'); + expect(result).toMatchObject({ ok: true, path: 'tmux' }); + expect(vi.mocked(sendKeys)).toHaveBeenCalledWith(agentId, 'inspect verdict'); + }); + + it('keeps the strict PAN-1769 contract for an explicit supervisor argument', async () => { + const agentId = 'agent-explicit-supervisor-strict'; + writeAgentState(agentId); + await expect( + deliverAgentMessage(agentId, 'resume continue', 'test-caller', 'supervisor'), + ).rejects.toThrow(/PTY supervisor delivery failed.*socket-missing/); + expect(vi.mocked(sendKeys)).not.toHaveBeenCalled(); + }); +}); + describe('keyedSupervisorFailureKind (PAN-1837)', () => { it('classifies connect-phase failures as definitive, never ambiguous', async () => { const { keyedSupervisorFailureKind } = await import('../delivery.js'); diff --git a/src/lib/agents/__tests__/supervisor-codex-transport.test.ts b/src/lib/agents/__tests__/supervisor-codex-transport.test.ts new file mode 100644 index 00000000000..d3a02194c35 --- /dev/null +++ b/src/lib/agents/__tests__/supervisor-codex-transport.test.ts @@ -0,0 +1,123 @@ +/** + * Codex work agents on the app-server transport never get a PTY supervisor + * wrap (the app-server branch of buildCodexCommand skips it), so the + * supervisor eligibility decision must reject them. Stamping + * supervisorEnabled anyway projected a strict 'supervisor' deliveryMethod + * with no socket behind it — every state-routed delivery died with + * socket-missing, which stalled the PAN-3743 review loop on 2026-08-16 when + * the inspect verdict could not reach the work agent. + */ +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockWritePtyToken, mockLoadConfigSync, getHome } = vi.hoisted(() => { + let home = ''; + return { + mockWritePtyToken: vi.fn(async () => undefined), + mockLoadConfigSync: vi.fn(), + getHome: (next?: string) => { + if (next !== undefined) home = next; + return home; + }, + }; +}); + +vi.mock('../../pty-token.js', () => ({ + writePtyToken: mockWritePtyToken, + readPtyToken: vi.fn(async () => null), +})); + +vi.mock('../../channels/pty-supervisor-locate.js', () => ({ + resolvePtySupervisorScriptPath: () => '/repo/dist/pty-supervisor.js', +})); + +vi.mock('../../paths.js', async (importOriginal) => ({ + ...(await importOriginal()), + getOverdeckHome: () => getHome(), +})); + +vi.mock('../../config-yaml.js', () => ({ + isClaudeCodeChannelsMcpEnabled: () => false, + loadConfigSync: mockLoadConfigSync, +})); + +import { decideSupervisorForWorkAgent, prepareSupervisorForFreshLaunch } from '../supervisor-channels.js'; +import type { AgentState } from '../agent-state.js'; + +function workState(overrides: Partial = {}): AgentState { + return { + id: 'agent-pan-3743', + issueId: 'PAN-3743', + workspace: '/tmp/workspaces/feature-pan-3743', + harness: 'codex', + role: 'work', + model: 'gpt-5.6-sol', + status: 'starting', + ...overrides, + } as AgentState; +} + +function configWithCodexTransport(transport: 'app-server' | 'tui') { + return { config: { codex: { transport } } }; +} + +let home: string; + +beforeEach(() => { + home = mkdtempSync(join(tmpdir(), 'overdeck-supervisor-codex-transport-')); + getHome(home); + mkdirSync(join(home, 'sockets'), { recursive: true }); + mockWritePtyToken.mockClear(); + mockLoadConfigSync.mockReset(); +}); + +afterEach(() => { + rmSync(home, { recursive: true, force: true }); +}); + +describe('decideSupervisorForWorkAgent codex transport gate', () => { + it('rejects codex on the default app-server transport', () => { + mockLoadConfigSync.mockReturnValue(configWithCodexTransport('app-server')); + const decision = decideSupervisorForWorkAgent('agent-pan-3743', { + issueId: 'PAN-3743', + workspace: '/tmp/workspaces/feature-pan-3743', + }, workState()); + expect(decision).toEqual({ eligible: false, reason: 'codex-app-server-transport' }); + }); + + it('keeps codex eligible on the work-tui transport, which the launcher does wrap', () => { + mockLoadConfigSync.mockReturnValue(configWithCodexTransport('tui')); + const decision = decideSupervisorForWorkAgent('agent-pan-3743', { + issueId: 'PAN-3743', + workspace: '/tmp/workspaces/feature-pan-3743', + }, workState()); + expect(decision).toEqual({ eligible: true }); + }); + + it('does not consult the codex transport for claude-code work agents', () => { + // No mock return configured: a claude-code decision must not touch the + // config load at all (short-circuit before the codex branch). + const decision = decideSupervisorForWorkAgent('agent-pan-3743', { + issueId: 'PAN-3743', + workspace: '/tmp/workspaces/feature-pan-3743', + }, workState({ harness: 'claude-code', model: 'claude-opus-5' })); + expect(decision).toEqual({ eligible: true }); + expect(mockLoadConfigSync).not.toHaveBeenCalled(); + }); +}); + +describe('prepareSupervisorForFreshLaunch codex app-server', () => { + it('leaves supervisorEnabled unset so no strict supervisor deliveryMethod is projected', async () => { + mockLoadConfigSync.mockReturnValue(configWithCodexTransport('app-server')); + const state = workState(); + const result = await prepareSupervisorForFreshLaunch('agent-pan-3743', { + issueId: 'PAN-3743', + workspace: '/tmp/workspaces/feature-pan-3743', + }, state); + expect(result).toEqual({ useSupervisor: false }); + expect(state.supervisorEnabled).toBeUndefined(); + expect(mockWritePtyToken).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/agents/delivery.ts b/src/lib/agents/delivery.ts index 37b7e335441..d2781926037 100644 --- a/src/lib/agents/delivery.ts +++ b/src/lib/agents/delivery.ts @@ -292,7 +292,17 @@ export async function deliverAgentMessage( try { state = await Effect.runPromise(getAgentState(normalizedId)); channelsEnabled = Boolean(state?.channelsEnabled); - resolvedMethod ??= state?.deliveryMethod ?? 'auto'; + // A persisted deliveryMethod is a launch-time hint, not a per-call + // transport opt-in: state can project 'supervisor' for an agent with no + // live PTY supervisor (codex app-server launches stamped + // supervisorEnabled=true while the launcher never wrapped; a crash-resume + // can lose the socket, PAN-3257). Route a state-derived 'supervisor' + // through the resilient cascade so delivery falls through to the + // app-server/channels/tmux tiers instead of throwing socket-missing with + // no fallback — the failure mode that stalled the PAN-3743 review loop + // when the inspect verdict could not reach the work agent. Only an + // explicit caller argument keeps the strict PAN-1769 supervisor contract. + resolvedMethod ??= resilientDeliveryMethod(state?.deliveryMethod) ?? 'auto'; } catch { resolvedMethod ??= 'auto'; } diff --git a/src/lib/agents/supervisor-channels.ts b/src/lib/agents/supervisor-channels.ts index bd0c446d83c..216ebd4e802 100644 --- a/src/lib/agents/supervisor-channels.ts +++ b/src/lib/agents/supervisor-channels.ts @@ -4,7 +4,7 @@ import { homedir } from 'os'; import { dirname, join } from 'path'; import { Effect } from 'effect'; import { emitActivityEntrySync } from '../activity-logger.js'; -import { isClaudeCodeChannelsMcpEnabled } from '../config-yaml.js'; +import { isClaudeCodeChannelsMcpEnabled, loadConfigSync } from '../config-yaml.js'; import type { ModelId } from '../settings.js'; import type { RuntimeName } from '../runtimes/types.js'; import { getHarnessBehavior } from '../runtimes/behavior.js'; @@ -156,6 +156,19 @@ export function decideSupervisorForWorkAgent( return { eligible: false, reason }; } + // Codex's app-server transport never wraps the launcher in the PTY + // supervisor (the app-server branch of buildCodexCommand skips the wrap), so + // a supervisor stamp here would project a strict 'supervisor' deliveryMethod + // with no socket behind it — every state-routed delivery then died with + // socket-missing (the PAN-3743 review-loop stall). Only the work-tui + // transport gets a real supervisor. Matches the launcher's own source of + // truth (getCodexLauncherFields reads the merged config the same way) and + // the conversation-side precedent in shouldUseSupervisorForConversation. + if (state.harness === 'codex' && loadConfigSync().config.codex?.transport !== 'tui') { + log(false, 'codex-app-server-transport'); + return { eligible: false, reason: 'codex-app-server-transport' }; + } + log(true); return { eligible: true }; } diff --git a/src/lib/cloister/__tests__/inspect-agent.test.ts b/src/lib/cloister/__tests__/inspect-agent.test.ts index 87b5ca64740..75b61112221 100644 --- a/src/lib/cloister/__tests__/inspect-agent.test.ts +++ b/src/lib/cloister/__tests__/inspect-agent.test.ts @@ -24,6 +24,7 @@ const mocks = vi.hoisted(() => ({ saveAgentState: vi.fn(), sessionExists: vi.fn(), spawnTierSupervisor: vi.fn(), + surfaceIssueFeedbackNeedsYou: vi.fn(), writeFileSync: vi.fn(), })); @@ -112,6 +113,10 @@ vi.mock('../../agents/delivery.js', () => ({ deliverAgentMessage: mocks.deliverAgentMessage, })); +vi.mock('../feedback-target.js', () => ({ + surfaceIssueFeedbackNeedsYou: mocks.surfaceIssueFeedbackNeedsYou, +})); + import { onInspectComplete, spawnInspectAgent } from '../inspect-agent.js'; describe('spawnInspectAgent', () => { @@ -507,6 +512,52 @@ describe('onInspectComplete verdict delivery (PAN-3078)', () => { expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('pane dead')); errorSpy.mockRestore(); }); + + // PAN-3743 stall hardening: a lost verdict deadlocks the work agent, so a + // delivery failure must reach the operator via the same needs-you surface + // the review-verdict path uses (PAN-2228) — not only the console. + it('surfaces needs-you when delivery reports ok=false', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + mocks.deliverAgentMessage.mockResolvedValue({ ok: false, path: 'tmux', failure: 'pane dead' }); + + await Effect.runPromise(onInspectComplete('overdeck', 'PAN-1613', 'workspace-b95lw', 'failed', '/workspace')); + + expect(mocks.surfaceIssueFeedbackNeedsYou).toHaveBeenCalledTimes(1); + const [issueId, message, opts] = mocks.surfaceIssueFeedbackNeedsYou.mock.calls[0]; + expect(issueId).toBe('PAN-1613'); + expect(message).toContain('workspace-b95lw'); + expect(message).toContain('agent-pan-1613'); + expect(message).toContain('pane dead'); + expect(opts).toEqual({ specialist: 'inspect-agent' }); + }); + + it('surfaces needs-you when delivery throws', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + mocks.deliverAgentMessage.mockRejectedValue( + new Error('PTY supervisor delivery failed (socket-missing)'), + ); + + await Effect.runPromise(onInspectComplete('overdeck', 'PAN-1613', 'workspace-b95lw', 'passed', '/workspace')); + + expect(mocks.surfaceIssueFeedbackNeedsYou).toHaveBeenCalledTimes(1); + const [issueId, message, opts] = mocks.surfaceIssueFeedbackNeedsYou.mock.calls[0]; + expect(issueId).toBe('PAN-1613'); + expect(message).toContain('socket-missing'); + expect(opts).toEqual({ specialist: 'inspect-agent' }); + }); + + it('still logs when the needs-you surface itself fails', async () => { + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + mocks.deliverAgentMessage.mockRejectedValue(new Error('socket-missing')); + mocks.surfaceIssueFeedbackNeedsYou.mockRejectedValue(new Error('db locked')); + + await expect(Effect.runPromise( + onInspectComplete('overdeck', 'PAN-1613', 'workspace-b95lw', 'passed', '/workspace'), + )).resolves.toBeUndefined(); + + expect(errorSpy).toHaveBeenCalledWith(expect.stringContaining('FAILED to deliver passed verdict')); + errorSpy.mockRestore(); + }); }); function planItem(id: string) { diff --git a/src/lib/cloister/inspect-agent.ts b/src/lib/cloister/inspect-agent.ts index 6e63694f0b6..8b0c1a1d35d 100644 --- a/src/lib/cloister/inspect-agent.ts +++ b/src/lib/cloister/inspect-agent.ts @@ -354,13 +354,39 @@ async function spawnInspectAgentPromise( const deliver = deps.deliver ?? deliverAgentMessage; const result = await deliver(workAgentId, message, 'inspect-verdict'); if (!result.ok) { - console.error(`[inspect] FAILED to deliver ${status} verdict for ${issueId} item ${itemId} to ${workAgentId}: ${result.failure ?? 'unknown'}`); + await reportInspectVerdictDeliveryFailure(issueId, itemId, workAgentId, status, result.failure ?? 'unknown'); } } catch (err) { - console.error(`[inspect] FAILED to deliver ${status} verdict for ${issueId} item ${itemId} to ${workAgentId}: ${err instanceof Error ? err.message : String(err)}`); + await reportInspectVerdictDeliveryFailure(issueId, itemId, workAgentId, status, err instanceof Error ? err.message : String(err)); } } +/** + * A lost inspect verdict deadlocks a waiting work agent by design (PAN-3078), + * so a delivery failure must be operator-visible, not only logged. Surface the + * same needs-you mark the review-verdict path uses (PAN-2228) — observed + * 2026-08-16 when a state-projected strict 'supervisor' method threw + * socket-missing for a codex app-server work agent and the PAN-3743 review + * loop stalled with only a console.error to show for it (PAN-2848 family). + */ +async function reportInspectVerdictDeliveryFailure( + issueId: string, + itemId: string, + workAgentId: string, + status: 'passed' | 'failed', + reason: string, +): Promise { + console.error(`[inspect] FAILED to deliver ${status} verdict for ${issueId} item ${itemId} to ${workAgentId}: ${reason}`); + try { + const { surfaceIssueFeedbackNeedsYou } = await import('./feedback-target.js'); + await surfaceIssueFeedbackNeedsYou( + issueId, + `Inspect verdict (${status}) for item ${itemId} could not be delivered to ${workAgentId}: ${reason}`, + { specialist: 'inspect-agent' }, + ); + } catch { /* best-effort — the error above still records the failure */ } +} + // ─── PAN-1249: additive Effect variants ─────────────────────────────────────── /**