From a607b8c8271b8d29a5a1dc67404d089be9483fbf Mon Sep 17 00:00:00 2001 From: Ethan Wang Date: Mon, 27 Apr 2026 12:18:22 +0000 Subject: [PATCH] fix: improve codex plan mode compatibility --- cli/src/codex/appServerTypes.ts | 8 ++ cli/src/codex/codexAppServerClient.ts | 8 ++ cli/src/codex/codexRemoteLauncher.test.ts | 99 +++++++++++++-- cli/src/codex/codexRemoteLauncher.ts | 115 ++++++++++++++++-- cli/src/codex/session.ts | 1 + cli/src/codex/utils/appServerConfig.test.ts | 13 ++ cli/src/codex/utils/appServerConfig.ts | 5 +- .../utils/appServerPermissionAdapter.test.ts | 27 ++++ .../codex/utils/appServerPermissionAdapter.ts | 23 ++++ 9 files changed, 283 insertions(+), 16 deletions(-) diff --git a/cli/src/codex/appServerTypes.ts b/cli/src/codex/appServerTypes.ts index e0996657c8..0cb272c23f 100644 --- a/cli/src/codex/appServerTypes.ts +++ b/cli/src/codex/appServerTypes.ts @@ -44,6 +44,14 @@ export interface ModelListResponse { [key: string]: unknown; } +export interface CollaborationModeListResponse { + data?: Array<{ mode?: string; name?: string; id?: string } | string>; + modes?: Array<{ mode?: string; name?: string; id?: string } | string>; + collaborationModes?: Array<{ mode?: string; name?: string; id?: string } | string>; + items?: Array<{ mode?: string; name?: string; id?: string } | string>; + [key: string]: unknown; +} + export interface ThreadStartParams { model?: string; modelProvider?: string; diff --git a/cli/src/codex/codexAppServerClient.ts b/cli/src/codex/codexAppServerClient.ts index d5a7b31779..0e73c38c61 100644 --- a/cli/src/codex/codexAppServerClient.ts +++ b/cli/src/codex/codexAppServerClient.ts @@ -4,6 +4,7 @@ import { killProcessByChildProcess } from '@/utils/process'; import type { InitializeParams, InitializeResponse, + CollaborationModeListResponse, ModelListParams, ModelListResponse, ThreadStartParams, @@ -142,6 +143,13 @@ export class CodexAppServerClient { return response as ModelListResponse; } + async listCollaborationModes(): Promise { + const response = await this.sendRequest('collaborationMode/list', {}, { + timeoutMs: 30_000 + }); + return response as CollaborationModeListResponse; + } + async startThread(params: ThreadStartParams, options?: { signal?: AbortSignal }): Promise { const response = await this.sendRequest('thread/start', params, { signal: options?.signal, diff --git a/cli/src/codex/codexRemoteLauncher.test.ts b/cli/src/codex/codexRemoteLauncher.test.ts index 22307e1dab..4f53b169a1 100644 --- a/cli/src/codex/codexRemoteLauncher.test.ts +++ b/cli/src/codex/codexRemoteLauncher.test.ts @@ -5,10 +5,14 @@ import type { EnhancedMode } from './loop'; const harness = vi.hoisted(() => ({ notifications: [] as Array<{ method: string; params: unknown }>, registerRequestCalls: [] as string[], + requestHandlers: new Map Promise | unknown>(), initializeCalls: [] as unknown[], + listCollaborationModeCalls: 0, startThreadIds: [] as string[], resumeThreadIds: [] as string[], startTurnThreadIds: [] as string[], + startTurnParams: [] as Array>, + startTurnErrors: [] as Error[], remainingThreadSystemErrors: 0 })); @@ -23,12 +27,18 @@ vi.mock('./codexAppServerClient', () => { return { protocolVersion: 1 }; } + async listCollaborationModes(): Promise<{ collaborationModes: Array<{ mode: string }> }> { + harness.listCollaborationModeCalls += 1; + return { collaborationModes: [{ mode: 'default' }, { mode: 'plan' }] }; + } + setNotificationHandler(handler: ((method: string, params: unknown) => void) | null): void { this.notificationHandler = handler; } - registerRequestHandler(method: string): void { + registerRequestHandler(method: string, handler: (params: unknown) => Promise | unknown): void { harness.registerRequestCalls.push(method); + harness.requestHandlers.set(method, handler); } async startThread(): Promise<{ thread: { id: string }; model: string }> { @@ -43,7 +53,12 @@ vi.mock('./codexAppServerClient', () => { return { thread: { id }, model: 'gpt-5.4' }; } - async startTurn(params?: { threadId?: string }): Promise<{ turn: { id?: string } }> { + async startTurn(params?: { threadId?: string; collaborationMode?: unknown }): Promise<{ turn: { id?: string } }> { + harness.startTurnParams.push((params ?? {}) as Record); + const nextError = harness.startTurnErrors.shift(); + if (nextError) { + throw nextError; + } const threadId = params?.threadId ?? 'thread-unknown'; harness.startTurnThreadIds.push(threadId); const turnId = `turn-${harness.startTurnThreadIds.length}`; @@ -98,17 +113,18 @@ type FakeAgentState = { function createMode(): EnhancedMode { return { permissionMode: 'default', - collaborationMode: 'default' + collaborationMode: 'default', + model: 'gpt-5.4' }; } -function createSessionStub(messages = ['hello from launcher test']) { +function createSessionStub(messages = ['hello from launcher test'], mode: EnhancedMode = createMode()) { const queue = new MessageQueue2((mode) => JSON.stringify(mode)); messages.forEach((message, index) => { if (index === 0 && messages.length > 1) { - queue.pushIsolateAndClear(message, createMode()); + queue.pushIsolateAndClear(message, mode); } else { - queue.push(message, createMode()); + queue.push(message, mode); } }); queue.close(); @@ -117,7 +133,9 @@ function createSessionStub(messages = ['hello from launcher test']) { const codexMessages: unknown[] = []; const thinkingChanges: boolean[] = []; const foundSessionIds: string[] = []; - let currentModel: string | null | undefined; + const collaborationModes: Array = []; + let currentModel: string | null | undefined = mode.model; + let currentCollaborationMode: EnhancedMode['collaborationMode'] | undefined = mode.collaborationMode; let agentState: FakeAgentState = { requests: {}, completedRequests: {} @@ -160,6 +178,13 @@ function createSessionStub(messages = ['hello from launcher test']) { getModel() { return currentModel; }, + getCollaborationMode() { + return currentCollaborationMode; + }, + setCollaborationMode(nextMode: EnhancedMode['collaborationMode']) { + currentCollaborationMode = nextMode; + collaborationModes.push(nextMode); + }, onThinkingChange(nextThinking: boolean) { session.thinking = nextThinking; thinkingChanges.push(nextThinking); @@ -187,6 +212,8 @@ function createSessionStub(messages = ['hello from launcher test']) { foundSessionIds, rpcHandlers, getModel: () => currentModel, + getCollaborationMode: () => currentCollaborationMode, + collaborationModes, getAgentState: () => agentState }; } @@ -195,10 +222,14 @@ describe('codexRemoteLauncher', () => { afterEach(() => { harness.notifications = []; harness.registerRequestCalls = []; + harness.requestHandlers = new Map(); harness.initializeCalls = []; + harness.listCollaborationModeCalls = 0; harness.startThreadIds = []; harness.resumeThreadIds = []; harness.startTurnThreadIds = []; + harness.startTurnParams = []; + harness.startTurnErrors = []; harness.remainingThreadSystemErrors = 0; }); @@ -260,4 +291,58 @@ describe('codexRemoteLauncher', () => { expect(session.sessionId).toBe('thread-2'); expect(session.thinking).toBe(false); }); + + it('retries plan turns without collaborationMode when the runtime rejects the field', async () => { + harness.startTurnErrors.push(new Error('unknown field collaborationMode; experimentalApi is required')); + const { session, sessionEvents } = createSessionStub(['plan this'], { + permissionMode: 'default', + collaborationMode: 'plan', + model: 'gpt-5.4' + }); + + const exitReason = await codexRemoteLauncher(session as never); + + expect(exitReason).toBe('exit'); + expect(harness.listCollaborationModeCalls).toBe(1); + expect(harness.startTurnParams).toHaveLength(2); + expect(harness.startTurnParams[0]?.collaborationMode).toMatchObject({ + mode: 'plan' + }); + expect(harness.startTurnParams[1]?.collaborationMode).toBeUndefined(); + expect(sessionEvents).toContainEqual({ + type: 'message', + message: 'Plan mode is not supported by this Codex runtime. Sent as a normal turn instead.' + }); + }); + + it('switches collaboration mode to default after approving exit_plan_mode', async () => { + const { session, rpcHandlers, collaborationModes, getCollaborationMode } = createSessionStub([], { + permissionMode: 'default', + collaborationMode: 'plan', + model: 'gpt-5.4' + }); + + const exitReasonPromise = codexRemoteLauncher(session as never); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const approvalHandler = harness.requestHandlers.get('item/tool/requestApproval'); + expect(approvalHandler).toBeTypeOf('function'); + const approvalPromise = approvalHandler?.({ + itemId: 'exit-1', + toolName: 'exit_plan_mode', + input: { plan: '1. Edit files' } + }); + await new Promise((resolve) => setTimeout(resolve, 0)); + + const permissionRpc = rpcHandlers.get('permission'); + expect(permissionRpc).toBeTypeOf('function'); + await permissionRpc?.({ id: 'exit-1', approved: true, decision: 'approved' }); + await expect(approvalPromise).resolves.toEqual({ decision: 'accept' }); + + const exitReason = await exitReasonPromise; + + expect(exitReason).toBe('exit'); + expect(collaborationModes).toContain('default'); + expect(getCollaborationMode()).toBe('default'); + }); }); diff --git a/cli/src/codex/codexRemoteLauncher.ts b/cli/src/codex/codexRemoteLauncher.ts index 8251a2a3a9..f278b9431a 100644 --- a/cli/src/codex/codexRemoteLauncher.ts +++ b/cli/src/codex/codexRemoteLauncher.ts @@ -25,6 +25,65 @@ import { type HappyServer = Awaited>['server']; type QueuedMessage = { message: string; mode: EnhancedMode; isolate: boolean; hash: string }; +function asString(value: unknown): string | null { + return typeof value === 'string' && value.length > 0 ? value : null; +} + +function isExitPlanModeTool(toolName: string): boolean { + return toolName === 'exit_plan_mode' || toolName === 'ExitPlanMode'; +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function shouldRetryWithoutCollaborationMode(error: unknown): boolean { + const message = errorMessage(error).toLowerCase(); + if (!message.includes('collaborationmode') && !message.includes('collaboration_mode')) { + return false; + } + return message.includes('experimentalapi') + || message.includes('unsupported') + || message.includes('unknown') + || message.includes('unexpected') + || message.includes('unrecognized') + || message.includes('invalid') + || message.includes('field') + || message.includes('mode'); +} + +function responseContainsPlanCollaborationMode(response: unknown): boolean { + const record = response && typeof response === 'object' ? response as Record : null; + const candidates = [ + Array.isArray(response) ? response : undefined, + Array.isArray(record?.data) ? record.data : undefined, + Array.isArray(record?.modes) ? record.modes : undefined, + Array.isArray(record?.collaborationModes) ? record.collaborationModes : undefined, + Array.isArray(record?.items) ? record.items : undefined + ]; + + for (const candidate of candidates) { + if (!candidate) continue; + for (const entry of candidate) { + if (entry === 'plan') { + return true; + } + if (!entry || typeof entry !== 'object') { + continue; + } + const entryRecord = entry as Record; + const mode = asString(entryRecord.mode) + ?? asString(entryRecord.name) + ?? asString(entryRecord.id); + if (mode === 'plan') { + return true; + } + } + } + + return false; +} + class CodexRemoteLauncher extends RemoteLauncherBase { private readonly session: CodexSession; private readonly appServerClient: CodexAppServerClient; @@ -225,6 +284,10 @@ class CodexRemoteLauncher extends RemoteLauncherBase { is_error: !approved, id: randomUUID() }); + if (approved && isExitPlanModeTool(toolName)) { + session.setCollaborationMode('default'); + logger.debug('[Codex] exit_plan_mode approved; collaborationMode reset to default'); + } } }); const reasoningProcessor = new ReasoningProcessor((message) => { @@ -583,6 +646,17 @@ class CodexRemoteLauncher extends RemoteLauncherBase { experimentalApi: true } }); + let supportsTurnCollaborationMode = true; + try { + const response = await appServerClient.listCollaborationModes(); + const hasPlanMode = responseContainsPlanCollaborationMode(response); + logger.debug(`[Codex] collaborationMode/list plan=${hasPlanMode}`); + if (!hasPlanMode) { + logger.debug('[Codex] collaborationMode/list did not report plan; will still attempt collaborationMode until rejected'); + } + } catch (error) { + logger.debug(`[Codex] collaborationMode/list failed: ${errorMessage(error)}`); + } let hasThread = false; let pending: QueuedMessage | null = null; @@ -694,21 +768,46 @@ class CodexRemoteLauncher extends RemoteLauncherBase { } } - const turnParams = buildTurnStartParams({ - threadId: this.currentThreadId, + turnInFlight = true; + allowAnonymousTerminalEvent = false; + + const buildParams = (suppressCollaborationMode: boolean) => buildTurnStartParams({ + threadId: this.currentThreadId!, message: message.message, cwd: session.path, mode: { ...message.mode, model: session.getModel() ?? message.mode.model }, - cliOverrides: session.codexCliOverrides - }); - turnInFlight = true; - allowAnonymousTerminalEvent = false; - const turnResponse = await appServerClient.startTurn(turnParams, { - signal: this.abortController.signal + cliOverrides: session.codexCliOverrides, + overrides: suppressCollaborationMode + ? { suppressCollaborationMode: true } + : undefined }); + + let turnResponse: unknown; + const shouldSendCollaborationMode = supportsTurnCollaborationMode && Boolean(message.mode.collaborationMode); + try { + turnResponse = await appServerClient.startTurn(buildParams(!shouldSendCollaborationMode), { + signal: this.abortController.signal + }); + } catch (error) { + if (shouldSendCollaborationMode && shouldRetryWithoutCollaborationMode(error)) { + supportsTurnCollaborationMode = false; + if (message.mode.collaborationMode === 'plan') { + const fallbackMessage = 'Plan mode is not supported by this Codex runtime. Sent as a normal turn instead.'; + logger.debug(`[Codex] ${fallbackMessage}`); + session.sendSessionEvent({ type: 'message', message: fallbackMessage }); + } else { + logger.debug('[Codex] collaborationMode is not supported by this Codex runtime; retrying without it'); + } + turnResponse = await appServerClient.startTurn(buildParams(true), { + signal: this.abortController.signal + }); + } else { + throw error; + } + } const turnRecord = asRecord(turnResponse); const turn = turnRecord ? asRecord(turnRecord.turn) : null; const turnId = asString(turn?.id); diff --git a/cli/src/codex/session.ts b/cli/src/codex/session.ts index 524d21d7dd..9618feedba 100644 --- a/cli/src/codex/session.ts +++ b/cli/src/codex/session.ts @@ -109,6 +109,7 @@ export class CodexSession extends AgentSessionBase { setCollaborationMode = (mode: EnhancedMode['collaborationMode']): void => { this.collaborationMode = mode; + this.client.keepAlive(this.thinking, this.mode, this.getKeepAliveRuntime()); }; recordLocalLaunchFailure = (message: string, exitReason: LocalLaunchExitReason): void => { diff --git a/cli/src/codex/utils/appServerConfig.test.ts b/cli/src/codex/utils/appServerConfig.test.ts index 0951b41331..2a240baaeb 100644 --- a/cli/src/codex/utils/appServerConfig.test.ts +++ b/cli/src/codex/utils/appServerConfig.test.ts @@ -236,4 +236,17 @@ describe('appServerConfig', () => { }); expect(params.model).toBeUndefined(); }); + + it('can suppress collaboration mode while preserving top-level model', () => { + const params = buildTurnStartParams({ + threadId: 'thread-1', + message: 'hello', + cwd: '/workspace/project', + mode: { permissionMode: 'default', model: 'o3', collaborationMode: 'plan' }, + overrides: { suppressCollaborationMode: true } + }); + + expect(params.collaborationMode).toBeUndefined(); + expect(params.model).toBe('o3'); + }); }); diff --git a/cli/src/codex/utils/appServerConfig.ts b/cli/src/codex/utils/appServerConfig.ts index 12565909f8..9491683af7 100644 --- a/cli/src/codex/utils/appServerConfig.ts +++ b/cli/src/codex/utils/appServerConfig.ts @@ -117,6 +117,7 @@ export function buildTurnStartParams(args: { approvalPolicy?: TurnStartParams['approvalPolicy']; sandboxPolicy?: TurnStartParams['sandboxPolicy']; model?: string; + suppressCollaborationMode?: boolean; }; }): TurnStartParams { const params: TurnStartParams = { @@ -141,7 +142,9 @@ export function buildTurnStartParams(args: { params.sandboxPolicy = sandboxPolicy; } - const collaborationMode = args.mode?.collaborationMode; + const collaborationMode = args.overrides?.suppressCollaborationMode + ? undefined + : args.mode?.collaborationMode; const model = args.overrides?.model ?? args.mode?.model; if (collaborationMode) { if (!model) { diff --git a/cli/src/codex/utils/appServerPermissionAdapter.test.ts b/cli/src/codex/utils/appServerPermissionAdapter.test.ts index f899111e02..22c4bfa057 100644 --- a/cli/src/codex/utils/appServerPermissionAdapter.test.ts +++ b/cli/src/codex/utils/appServerPermissionAdapter.test.ts @@ -77,4 +77,31 @@ describe('registerAppServerPermissionHandlers', () => { decision: 'cancel' }); }); + + it('forwards generic tool approval requests with the app-server tool name', async () => { + const { client, handlers } = createClient(); + const permissionHandler = { + handleToolCall: vi.fn(async () => ({ decision: 'approved' })) + }; + + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: permissionHandler as never + }); + + const handler = handlers.get('item/tool/requestApproval'); + expect(handler).toBeTypeOf('function'); + + await expect(handler?.({ + itemId: 'tool-123', + toolName: 'exit_plan_mode', + input: { plan: '1. Edit files' } + })).resolves.toEqual({ decision: 'accept' }); + + expect(permissionHandler.handleToolCall).toHaveBeenCalledWith( + 'tool-123', + 'exit_plan_mode', + { plan: '1. Edit files' } + ); + }); }); diff --git a/cli/src/codex/utils/appServerPermissionAdapter.ts b/cli/src/codex/utils/appServerPermissionAdapter.ts index 78fe9f1f5e..51594c79dc 100644 --- a/cli/src/codex/utils/appServerPermissionAdapter.ts +++ b/cli/src/codex/utils/appServerPermissionAdapter.ts @@ -21,6 +21,15 @@ function asString(value: unknown): string | undefined { return typeof value === 'string' && value.length > 0 ? value : undefined; } +function pickToolName(record: Record): string { + return asString(record.toolName) + ?? asString(record.tool_name) + ?? asString(record.tool) + ?? asString(record.name) + ?? asString(record.permission) + ?? 'CodexTool'; +} + function mapDecision(decision: PermissionDecision): { decision: string } { switch (decision) { case 'approved': @@ -82,6 +91,20 @@ export function registerAppServerPermissionHandlers(args: { return mapDecision(result.decision); }); + client.registerRequestHandler('item/tool/requestApproval', async (params) => { + const record = asRecord(params) ?? {}; + const toolCallId = asString(record.itemId) ?? asString(record.item_id) ?? randomUUID(); + const toolName = pickToolName(record); + + const result = await permissionHandler.handleToolCall( + toolCallId, + toolName, + record.input ?? record.arguments ?? params + ) as PermissionResult; + + return mapDecision(result.decision); + }); + client.registerRequestHandler('item/tool/requestUserInput', async (params) => { const record = asRecord(params) ?? {}; const requestId = asString(record.itemId) ?? randomUUID();