From 1f02bb7de1e736a723ddc89b25fc426e87943d7b Mon Sep 17 00:00:00 2001 From: weishu Date: Mon, 16 Mar 2026 13:00:09 +0800 Subject: [PATCH] Fix codex approve policy --- cli/src/codex/codexLocalLauncher.test.ts | 141 ++++++++++++++++++ cli/src/codex/codexLocalLauncher.ts | 14 +- cli/src/codex/codexRemoteLauncher.ts | 7 +- cli/src/codex/utils/appServerConfig.test.ts | 10 ++ cli/src/codex/utils/appServerConfig.ts | 31 +--- cli/src/codex/utils/codexCliOverrides.test.ts | 23 ++- cli/src/codex/utils/codexCliOverrides.ts | 35 +++++ cli/src/codex/utils/codexStartConfig.test.ts | 12 ++ cli/src/codex/utils/codexStartConfig.ts | 21 +-- cli/src/codex/utils/permissionHandler.test.ts | 119 +++++++++++++++ cli/src/codex/utils/permissionHandler.ts | 51 ++++++- cli/src/codex/utils/permissionModeConfig.ts | 46 ++++++ 12 files changed, 461 insertions(+), 49 deletions(-) create mode 100644 cli/src/codex/codexLocalLauncher.test.ts create mode 100644 cli/src/codex/utils/permissionHandler.test.ts create mode 100644 cli/src/codex/utils/permissionModeConfig.ts diff --git a/cli/src/codex/codexLocalLauncher.test.ts b/cli/src/codex/codexLocalLauncher.test.ts new file mode 100644 index 00000000..66866d03 --- /dev/null +++ b/cli/src/codex/codexLocalLauncher.test.ts @@ -0,0 +1,141 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const harness = vi.hoisted(() => ({ + launches: [] as Array> +})); + +vi.mock('./codexLocal', () => ({ + codexLocal: async (opts: Record) => { + harness.launches.push(opts); + } +})); + +vi.mock('./utils/buildHapiMcpBridge', () => ({ + buildHapiMcpBridge: async () => ({ + server: { + url: 'http://localhost:0', + stop: () => {} + }, + mcpServers: {} + }) +})); + +vi.mock('./utils/codexSessionScanner', () => ({ + createCodexSessionScanner: async () => ({ + cleanup: async () => {}, + onNewSession: () => {} + }) +})); + +vi.mock('@/modules/common/launcher/BaseLocalLauncher', () => ({ + BaseLocalLauncher: class { + readonly control = { + requestExit: () => {} + }; + + constructor(private readonly opts: { launch: (signal: AbortSignal) => Promise }) {} + + async run(): Promise<'exit'> { + await this.opts.launch(new AbortController().signal); + return 'exit'; + } + } +})); + +import { codexLocalLauncher } from './codexLocalLauncher'; + +function createSessionStub(permissionMode: 'default' | 'read-only' | 'safe-yolo' | 'yolo', codexArgs?: string[]) { + return { + sessionId: null, + path: '/tmp/worktree', + startedBy: 'terminal' as const, + startingMode: 'local' as const, + codexArgs, + client: { + rpcHandlerManager: {} + }, + getPermissionMode: () => permissionMode, + onSessionFound: () => {}, + sendSessionEvent: () => {}, + recordLocalLaunchFailure: () => {}, + sendUserMessage: () => {}, + sendCodexMessage: () => {}, + queue: {} + }; +} + +describe('codexLocalLauncher', () => { + afterEach(() => { + harness.launches = []; + }); + + it('rebuilds approval and sandbox args from yolo mode', async () => { + const session = createSessionStub('yolo', [ + '--sandbox', + 'read-only', + '--ask-for-approval', + 'untrusted', + '--model', + 'o3', + '--full-auto' + ]); + + await codexLocalLauncher(session as never); + + expect(harness.launches).toHaveLength(1); + expect(harness.launches[0]?.codexArgs).toEqual([ + '--ask-for-approval', + 'never', + '--sandbox', + 'danger-full-access', + '--model', + 'o3' + ]); + }); + + it('preserves raw Codex approval flags in default mode', async () => { + const session = createSessionStub('default', [ + '--ask-for-approval', + 'on-request', + '--sandbox', + 'workspace-write', + '--model', + 'o3' + ]); + + await codexLocalLauncher(session as never); + + expect(harness.launches).toHaveLength(1); + expect(harness.launches[0]?.codexArgs).toEqual([ + '--ask-for-approval', + 'on-request', + '--sandbox', + 'workspace-write', + '--model', + 'o3' + ]); + }); + + it('keeps sandbox escalation available in safe-yolo mode', async () => { + const session = createSessionStub('safe-yolo', [ + '--ask-for-approval', + 'never', + '--sandbox', + 'danger-full-access', + '--model', + 'o3' + ]); + + await codexLocalLauncher(session as never); + + expect(harness.launches).toHaveLength(1); + expect(harness.launches[0]?.codexArgs).toEqual([ + '--ask-for-approval', + 'on-failure', + '--sandbox', + 'workspace-write', + '--model', + 'o3' + ]); + }); +}); diff --git a/cli/src/codex/codexLocalLauncher.ts b/cli/src/codex/codexLocalLauncher.ts index 482803c2..d44b2d08 100644 --- a/cli/src/codex/codexLocalLauncher.ts +++ b/cli/src/codex/codexLocalLauncher.ts @@ -4,11 +4,23 @@ import { CodexSession } from './session'; import { createCodexSessionScanner } from './utils/codexSessionScanner'; import { convertCodexEvent } from './utils/codexEventConverter'; import { buildHapiMcpBridge } from './utils/buildHapiMcpBridge'; +import { stripCodexCliOverrides } from './utils/codexCliOverrides'; +import { buildCodexPermissionModeCliArgs } from './utils/permissionModeConfig'; import { BaseLocalLauncher } from '@/modules/common/launcher/BaseLocalLauncher'; export async function codexLocalLauncher(session: CodexSession): Promise<'switch' | 'exit'> { const resumeSessionId = session.sessionId; let scanner: Awaited> | null = null; + const permissionMode = session.getPermissionMode(); + const managedPermissionMode = permissionMode === 'read-only' || permissionMode === 'safe-yolo' || permissionMode === 'yolo' + ? permissionMode + : null; + const codexArgs = managedPermissionMode + ? [ + ...buildCodexPermissionModeCliArgs(managedPermissionMode), + ...stripCodexCliOverrides(session.codexArgs) + ] + : session.codexArgs; // Start hapi hub for MCP bridge (same as remote mode) const { server: happyServer, mcpServers } = await buildHapiMcpBridge(session.client); @@ -32,7 +44,7 @@ export async function codexLocalLauncher(session: CodexSession): Promise<'switch sessionId: resumeSessionId, onSessionFound: handleSessionFound, abort: abortSignal, - codexArgs: session.codexArgs, + codexArgs, mcpServers }); }, diff --git a/cli/src/codex/codexRemoteLauncher.ts b/cli/src/codex/codexRemoteLauncher.ts index 0326320e..5dc2f27e 100644 --- a/cli/src/codex/codexRemoteLauncher.ts +++ b/cli/src/codex/codexRemoteLauncher.ts @@ -176,7 +176,12 @@ class CodexRemoteLauncher extends RemoteLauncherBase { } }; - const permissionHandler = new CodexPermissionHandler(session.client, { + const permissionHandler = new CodexPermissionHandler(session.client, () => { + const mode = session.getPermissionMode(); + return mode === 'default' || mode === 'read-only' || mode === 'safe-yolo' || mode === 'yolo' + ? mode + : undefined; + }, { onRequest: ({ id, toolName, input }) => { const inputRecord = input && typeof input === 'object' ? input as Record : {}; const message = typeof inputRecord.message === 'string' ? inputRecord.message : undefined; diff --git a/cli/src/codex/utils/appServerConfig.test.ts b/cli/src/codex/utils/appServerConfig.test.ts index 87ffc62c..a1f1bd43 100644 --- a/cli/src/codex/utils/appServerConfig.test.ts +++ b/cli/src/codex/utils/appServerConfig.test.ts @@ -33,6 +33,16 @@ describe('appServerConfig', () => { }); expect(params.sandbox).toBe('danger-full-access'); + expect(params.approvalPolicy).toBe('never'); + }); + + it('keeps on-failure approvals for safe-yolo threads', () => { + const params = buildThreadStartParams({ + mode: { permissionMode: 'safe-yolo' }, + mcpServers + }); + + expect(params.sandbox).toBe('workspace-write'); expect(params.approvalPolicy).toBe('on-failure'); }); diff --git a/cli/src/codex/utils/appServerConfig.ts b/cli/src/codex/utils/appServerConfig.ts index 0804c9e5..2630ab5e 100644 --- a/cli/src/codex/utils/appServerConfig.ts +++ b/cli/src/codex/utils/appServerConfig.ts @@ -9,41 +9,18 @@ import type { ThreadStartParams, TurnStartParams } from '../appServerTypes'; +import { resolveCodexPermissionModeConfig } from './permissionModeConfig'; function resolveApprovalPolicy(mode: EnhancedMode): ApprovalPolicy { - switch (mode.permissionMode) { - case 'default': return 'untrusted'; - case 'read-only': return 'never'; - case 'safe-yolo': return 'on-failure'; - case 'yolo': return 'on-failure'; - default: { - throw new Error(`Unknown permission mode: ${mode.permissionMode}`); - } - } + return resolveCodexPermissionModeConfig(mode.permissionMode).approvalPolicy; } function resolveSandbox(mode: EnhancedMode): SandboxMode { - switch (mode.permissionMode) { - case 'default': return 'workspace-write'; - case 'read-only': return 'read-only'; - case 'safe-yolo': return 'workspace-write'; - case 'yolo': return 'danger-full-access'; - default: { - throw new Error(`Unknown permission mode: ${mode.permissionMode}`); - } - } + return resolveCodexPermissionModeConfig(mode.permissionMode).sandbox; } function resolveSandboxPolicy(mode: EnhancedMode): SandboxPolicy { - switch (mode.permissionMode) { - case 'default': return { type: 'workspaceWrite' }; - case 'read-only': return { type: 'readOnly' }; - case 'safe-yolo': return { type: 'workspaceWrite' }; - case 'yolo': return { type: 'dangerFullAccess' }; - default: { - throw new Error(`Unknown permission mode: ${mode.permissionMode}`); - } - } + return resolveCodexPermissionModeConfig(mode.permissionMode).sandboxPolicy; } function resolveSandboxPolicyOverride(value: CodexCliOverrides['sandbox'] | undefined): SandboxPolicy | undefined { diff --git a/cli/src/codex/utils/codexCliOverrides.test.ts b/cli/src/codex/utils/codexCliOverrides.test.ts index bc2ee40a..c236bf86 100644 --- a/cli/src/codex/utils/codexCliOverrides.test.ts +++ b/cli/src/codex/utils/codexCliOverrides.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { parseCodexCliOverrides } from './codexCliOverrides'; +import { parseCodexCliOverrides, stripCodexCliOverrides } from './codexCliOverrides'; describe('parseCodexCliOverrides', () => { it('parses sandbox and approval flags', () => { @@ -50,4 +50,25 @@ describe('parseCodexCliOverrides', () => { sandbox: 'read-only' }); }); + + it('strips approval and sandbox overrides while keeping unrelated args', () => { + expect(stripCodexCliOverrides([ + '--sandbox', + 'read-only', + '--ask-for-approval=never', + '--model', + 'o3', + '--full-auto', + '--dangerously-bypass-approvals-and-sandbox', + '--', + '--sandbox', + 'danger-full-access' + ])).toEqual([ + '--model', + 'o3', + '--', + '--sandbox', + 'danger-full-access' + ]); + }); }); diff --git a/cli/src/codex/utils/codexCliOverrides.ts b/cli/src/codex/utils/codexCliOverrides.ts index 53a1cfc6..f2d6eeed 100644 --- a/cli/src/codex/utils/codexCliOverrides.ts +++ b/cli/src/codex/utils/codexCliOverrides.ts @@ -86,3 +86,38 @@ export function parseCodexCliOverrides(args?: string[]): CodexCliOverrides { export function hasCodexCliOverrides(overrides?: CodexCliOverrides): boolean { return Boolean(overrides?.sandbox || overrides?.approvalPolicy); } + +export function stripCodexCliOverrides(args?: string[]): string[] { + if (!args || args.length === 0) { + return []; + } + + const filtered: string[] = []; + + for (let i = 0; i < args.length; i++) { + const arg = args[i]; + if (arg === '--') { + filtered.push(...args.slice(i)); + break; + } + + if ( + arg === '--full-auto' + || arg === '--yolo' + || arg === '--dangerously-bypass-approvals-and-sandbox' + || arg.startsWith('--sandbox=') + || arg.startsWith('--ask-for-approval=') + ) { + continue; + } + + if (arg === '-s' || arg === '--sandbox' || arg === '-a' || arg === '--ask-for-approval') { + i += 1; + continue; + } + + filtered.push(arg); + } + + return filtered; +} diff --git a/cli/src/codex/utils/codexStartConfig.test.ts b/cli/src/codex/utils/codexStartConfig.test.ts index c6e547d7..54fdbef6 100644 --- a/cli/src/codex/utils/codexStartConfig.test.ts +++ b/cli/src/codex/utils/codexStartConfig.test.ts @@ -32,6 +32,18 @@ describe('buildCodexStartConfig', () => { }); expect(config.sandbox).toBe('danger-full-access'); + expect(config['approval-policy']).toBe('never'); + }); + + it('keeps on-failure approvals for safe-yolo', () => { + const config = buildCodexStartConfig({ + message: 'hello', + mode: { permissionMode: 'safe-yolo' }, + first: false, + mcpServers + }); + + expect(config.sandbox).toBe('workspace-write'); expect(config['approval-policy']).toBe('on-failure'); }); diff --git a/cli/src/codex/utils/codexStartConfig.ts b/cli/src/codex/utils/codexStartConfig.ts index 45560afc..7ba9556f 100644 --- a/cli/src/codex/utils/codexStartConfig.ts +++ b/cli/src/codex/utils/codexStartConfig.ts @@ -2,29 +2,14 @@ import type { CodexSessionConfig } from '../types'; import type { EnhancedMode } from '../loop'; import type { CodexCliOverrides } from './codexCliOverrides'; import { codexSystemPrompt } from './systemPrompt'; +import { resolveCodexPermissionModeConfig } from './permissionModeConfig'; function resolveApprovalPolicy(mode: EnhancedMode): CodexSessionConfig['approval-policy'] { - switch (mode.permissionMode) { - case 'default': return 'untrusted'; - case 'read-only': return 'never'; - case 'safe-yolo': return 'on-failure'; - case 'yolo': return 'on-failure'; - default: { - throw new Error(`Unknown permission mode: ${mode.permissionMode}`); - } - } + return resolveCodexPermissionModeConfig(mode.permissionMode).approvalPolicy; } function resolveSandbox(mode: EnhancedMode): CodexSessionConfig['sandbox'] { - switch (mode.permissionMode) { - case 'default': return 'workspace-write'; - case 'read-only': return 'read-only'; - case 'safe-yolo': return 'workspace-write'; - case 'yolo': return 'danger-full-access'; - default: { - throw new Error(`Unknown permission mode: ${mode.permissionMode}`); - } - } + return resolveCodexPermissionModeConfig(mode.permissionMode).sandbox; } export function buildCodexStartConfig(args: { diff --git a/cli/src/codex/utils/permissionHandler.test.ts b/cli/src/codex/utils/permissionHandler.test.ts new file mode 100644 index 00000000..49c28014 --- /dev/null +++ b/cli/src/codex/utils/permissionHandler.test.ts @@ -0,0 +1,119 @@ +import { describe, expect, it } from 'vitest'; +import type { ApiSessionClient } from '@/api/apiSession'; +import { CodexPermissionHandler } from './permissionHandler'; + +type FakeAgentState = { + requests: Record; + completedRequests: Record; +}; + +function createHarness(mode: 'default' | 'read-only' | 'safe-yolo' | 'yolo') { + let agentState: FakeAgentState = { + requests: {}, + completedRequests: {} + }; + + const rpcHandlers = new Map Promise | unknown>(); + const session = { + rpcHandlerManager: { + registerHandler(method: string, handler: (params: unknown) => Promise | unknown) { + rpcHandlers.set(method, handler); + } + }, + updateAgentState(handler: (state: FakeAgentState) => FakeAgentState) { + agentState = handler(agentState); + } + } as unknown as ApiSessionClient; + + const handler = new CodexPermissionHandler(session, () => mode); + + return { + handler, + rpcHandlers, + getAgentState: () => agentState + }; +} + +describe('CodexPermissionHandler', () => { + it('auto-approves yolo requests for the session', async () => { + const { handler, getAgentState } = createHarness('yolo'); + + await expect(handler.handleToolCall('perm-1', 'CodexPatch', { grantRoot: '/tmp' })).resolves.toEqual({ + decision: 'approved_for_session' + }); + + expect(getAgentState().requests).toEqual({}); + expect(getAgentState().completedRequests).toMatchObject({ + 'perm-1': { + tool: 'CodexPatch', + status: 'approved', + decision: 'approved_for_session' + } + }); + }); + + it('auto-approves safe-yolo requests once', async () => { + const { handler, getAgentState } = createHarness('safe-yolo'); + + await expect(handler.handleToolCall('perm-1', 'CodexBash', { command: 'pwd' })).resolves.toEqual({ + decision: 'approved' + }); + + expect(getAgentState().requests).toEqual({}); + expect(getAgentState().completedRequests).toMatchObject({ + 'perm-1': { + tool: 'CodexBash', + status: 'approved', + decision: 'approved' + } + }); + }); + + it('keeps default mode requests pending until a permission RPC arrives', async () => { + const { handler, rpcHandlers, getAgentState } = createHarness('default'); + const resultPromise = handler.handleToolCall('perm-1', 'CodexPatch', { grantRoot: '/tmp' }); + + expect(getAgentState().requests).toMatchObject({ + 'perm-1': { + tool: 'CodexPatch' + } + }); + + const permissionRpc = rpcHandlers.get('permission'); + expect(permissionRpc).toBeTypeOf('function'); + + await permissionRpc?.({ id: 'perm-1', approved: true, decision: 'approved' }); + + await expect(resultPromise).resolves.toEqual({ + decision: 'approved', + reason: undefined + }); + + expect(getAgentState().requests).toEqual({}); + expect(getAgentState().completedRequests).toMatchObject({ + 'perm-1': { + tool: 'CodexPatch', + status: 'approved', + decision: 'approved' + } + }); + }); + + it('auto-approves read-only non-write tools but not patches', async () => { + const { handler, getAgentState } = createHarness('read-only'); + + await expect(handler.handleToolCall('read-1', 'Read', { file: 'README.md' })).resolves.toEqual({ + decision: 'approved' + }); + + const patchPromise = handler.handleToolCall('patch-1', 'CodexPatch', { grantRoot: '/tmp' }); + expect(getAgentState().requests).toMatchObject({ + 'patch-1': { + tool: 'CodexPatch' + } + }); + + handler.reset(); + await expect(patchPromise).rejects.toThrow('Session reset'); + }); +}); diff --git a/cli/src/codex/utils/permissionHandler.ts b/cli/src/codex/utils/permissionHandler.ts index 768833fa..2b50decf 100644 --- a/cli/src/codex/utils/permissionHandler.ts +++ b/cli/src/codex/utils/permissionHandler.ts @@ -7,8 +7,10 @@ import { logger } from "@/ui/logger"; import { ApiSessionClient } from "@/api/apiSession"; +import type { CodexPermissionMode } from "@hapi/protocol/types"; import { BasePermissionHandler, + type AutoApprovalDecision, type PendingPermissionRequest, type PermissionCompletion } from "@/modules/common/permission/BasePermissionHandler"; @@ -38,7 +40,11 @@ type CodexPermissionHandlerOptions = { }; export class CodexPermissionHandler extends BasePermissionHandler { - constructor(session: ApiSessionClient, private readonly options?: CodexPermissionHandlerOptions) { + constructor( + session: ApiSessionClient, + private readonly getPermissionMode: () => CodexPermissionMode | undefined, + private readonly options?: CodexPermissionHandlerOptions + ) { super(session); } @@ -46,6 +52,43 @@ export class CodexPermissionHandler extends BasePermissionHandler ({ + ...currentState, + completedRequests: { + ...currentState.completedRequests, + [id]: { + tool: toolName, + arguments: input, + createdAt: timestamp, + completedAt: timestamp, + status: 'approved', + decision + } + } + })); + + logger.debug(`[Codex] Auto-approved ${toolName} (${id}) with decision=${decision}`); + + return { decision }; + } + /** * Handle a tool permission request * @param toolCallId - The unique ID of the tool call @@ -58,6 +101,12 @@ export class CodexPermissionHandler extends BasePermissionHandler { + const mode = this.getPermissionMode() ?? 'default'; + const autoDecision = this.resolveAutoApprovalDecision(mode, toolName, toolCallId); + if (autoDecision) { + return Promise.resolve(this.completeAutoApproval(toolCallId, toolName, input, autoDecision)); + } + return new Promise((resolve, reject) => { // Store the pending request this.addPendingRequest(toolCallId, toolName, input, { resolve, reject }); diff --git a/cli/src/codex/utils/permissionModeConfig.ts b/cli/src/codex/utils/permissionModeConfig.ts new file mode 100644 index 00000000..30089409 --- /dev/null +++ b/cli/src/codex/utils/permissionModeConfig.ts @@ -0,0 +1,46 @@ +import type { CodexPermissionMode } from '@hapi/protocol/types'; +import type { ApprovalPolicy, SandboxMode, SandboxPolicy } from '../appServerTypes'; + +export type CodexPermissionModeConfig = { + approvalPolicy: ApprovalPolicy; + sandbox: SandboxMode; + sandboxPolicy: SandboxPolicy; +}; + +export function resolveCodexPermissionModeConfig(mode: CodexPermissionMode): CodexPermissionModeConfig { + switch (mode) { + case 'default': + return { + approvalPolicy: 'untrusted', + sandbox: 'workspace-write', + sandboxPolicy: { type: 'workspaceWrite' } + }; + case 'read-only': + return { + approvalPolicy: 'never', + sandbox: 'read-only', + sandboxPolicy: { type: 'readOnly' } + }; + case 'safe-yolo': + return { + // Keep escalation available when the workspace-write sandbox blocks a command. + approvalPolicy: 'on-failure', + sandbox: 'workspace-write', + sandboxPolicy: { type: 'workspaceWrite' } + }; + case 'yolo': + return { + approvalPolicy: 'never', + sandbox: 'danger-full-access', + sandboxPolicy: { type: 'dangerFullAccess' } + }; + } + + const unexpectedMode: never = mode; + throw new Error(`Unknown permission mode: ${unexpectedMode}`); +} + +export function buildCodexPermissionModeCliArgs(mode: Exclude): string[] { + const config = resolveCodexPermissionModeConfig(mode); + return ['--ask-for-approval', config.approvalPolicy, '--sandbox', config.sandbox]; +}