From 688dbdc7def1940d924cf9ed1b7d865a5b2e7474 Mon Sep 17 00:00:00 2001 From: SharingMan <863155964@qq.com> Date: Mon, 9 Mar 2026 23:11:40 +0800 Subject: [PATCH] Fix Codex mobile approval request handling --- .../codex/__tests__/codexMcpClient.test.ts | 129 +++++++++++++++++- .../happy-cli/src/codex/codexMcpClient.ts | 80 +++++++++-- 2 files changed, 196 insertions(+), 13 deletions(-) diff --git a/packages/happy-cli/src/codex/__tests__/codexMcpClient.test.ts b/packages/happy-cli/src/codex/__tests__/codexMcpClient.test.ts index 5070dacb14..94f7536514 100644 --- a/packages/happy-cli/src/codex/__tests__/codexMcpClient.test.ts +++ b/packages/happy-cli/src/codex/__tests__/codexMcpClient.test.ts @@ -10,6 +10,8 @@ const { mockClientConnect, mockClientClose, mockStdioCtor, + mockNotificationHandler, + mockRequestHandler, } = vi.hoisted(() => ({ mockExecSync: vi.fn(), mockInitializeSandbox: vi.fn(), @@ -18,6 +20,12 @@ const { mockClientConnect: vi.fn(), mockClientClose: vi.fn(), mockStdioCtor: vi.fn(), + mockNotificationHandler: { + current: null as ((data: any) => void) | null, + }, + mockRequestHandler: { + current: null as ((request: any) => Promise) | null, + }, })); vi.mock('child_process', () => ({ @@ -39,8 +47,12 @@ vi.mock('@/ui/logger', () => ({ vi.mock('@modelcontextprotocol/sdk/client/index.js', () => ({ Client: class MockClient { - setNotificationHandler = vi.fn(); - setRequestHandler = vi.fn(); + setNotificationHandler = vi.fn((_schema: any, handler: (data: any) => void) => { + mockNotificationHandler.current = handler; + }); + setRequestHandler = vi.fn((_schema: any, handler: (request: any) => Promise) => { + mockRequestHandler.current = handler; + }); connect = mockClientConnect; close = mockClientClose; callTool = vi.fn(); @@ -83,6 +95,8 @@ describe('CodexMcpClient sandbox integration', () => { mockClientClose.mockResolvedValue(undefined); mockInitializeSandbox.mockResolvedValue(mockSandboxCleanup); mockWrapForMcpTransport.mockResolvedValue({ command: 'sh', args: ['-c', 'wrapped codex mcp'] }); + mockNotificationHandler.current = null; + mockRequestHandler.current = null; }); afterAll(() => { @@ -152,4 +166,115 @@ describe('CodexMcpClient sandbox integration', () => { }), ); }); + + it('maps permission decisions to Codex elicitation actions', async () => { + const client = new CodexMcpClient(); + client.setPermissionHandler({ + handleToolCall: vi.fn().mockResolvedValue({ decision: 'approved_for_session' }), + } as any); + + await client.connect(); + + expect(mockRequestHandler.current).toBeTruthy(); + const response = await mockRequestHandler.current!({ + params: { + codex_call_id: 'call_123', + codex_command: ['echo', 'hi'], + codex_cwd: '/tmp/project', + }, + }); + + expect(response).toEqual({ action: 'accept' }); + }); + + it('falls back to exec_approval_request metadata when elicitation params omit Codex fields', async () => { + const permissionHandler = { + handleToolCall: vi.fn().mockResolvedValue({ decision: 'approved' }), + }; + const client = new CodexMcpClient(); + client.setPermissionHandler(permissionHandler as any); + + await client.connect(); + + expect(mockNotificationHandler.current).toBeTruthy(); + expect(mockRequestHandler.current).toBeTruthy(); + + mockNotificationHandler.current!({ + params: { + msg: { + type: 'exec_approval_request', + call_id: 'call_from_event', + command: ['/bin/zsh', '-lc', 'mkdir -p ../02-mobliecoding'], + cwd: '/tmp/project', + }, + }, + }); + + const response = await mockRequestHandler.current!({ + params: { + message: 'Allow Codex to run mkdir?', + requestedSchema: { type: 'object', properties: {} }, + }, + }); + + expect(permissionHandler.handleToolCall).toHaveBeenCalledWith( + 'call_from_event', + 'CodexBash', + { + command: ['/bin/zsh', '-lc', 'mkdir -p ../02-mobliecoding'], + cwd: '/tmp/project', + }, + ); + expect(response).toEqual({ action: 'accept' }); + }); + + it('falls back to patch approval metadata when elicitation params omit Codex fields', async () => { + const permissionHandler = { + handleToolCall: vi.fn().mockResolvedValue({ decision: 'approved' }), + }; + const client = new CodexMcpClient(); + client.setPermissionHandler(permissionHandler as any); + + await client.connect(); + + expect(mockNotificationHandler.current).toBeTruthy(); + expect(mockRequestHandler.current).toBeTruthy(); + + mockNotificationHandler.current!({ + params: { + msg: { + type: 'apply_patch_approval_request', + call_id: 'call_patch_event', + changes: { + '/tmp/project/README.md': { + type: 'add', + content: '# hello\n', + }, + }, + }, + }, + }); + + const response = await mockRequestHandler.current!({ + params: { + message: 'Allow Codex to apply patch?', + requestedSchema: { type: 'object', properties: {} }, + }, + }); + + expect(permissionHandler.handleToolCall).toHaveBeenCalledWith( + 'call_patch_event', + 'CodexPatch', + { + changes: { + '/tmp/project/README.md': { + type: 'add', + content: '# hello\n', + }, + }, + autoApproved: undefined, + }, + ); + expect(response).toEqual({ action: 'accept' }); + }); }); diff --git a/packages/happy-cli/src/codex/codexMcpClient.ts b/packages/happy-cli/src/codex/codexMcpClient.ts index 25f2e6854d..737a8f2158 100644 --- a/packages/happy-cli/src/codex/codexMcpClient.ts +++ b/packages/happy-cli/src/codex/codexMcpClient.ts @@ -57,6 +57,14 @@ export class CodexMcpClient { private conversationId: string | null = null; private handler: ((event: any) => void) | null = null; private permissionHandler: CodexPermissionHandler | null = null; + private lastApprovalRequest: { + callId: string; + toolName: 'CodexBash' | 'CodexPatch'; + command?: string[]; + cwd?: string; + changes?: Record; + autoApproved?: boolean; + } | null = null; private sandboxConfig?: SandboxConfig; private sandboxCleanup: (() => Promise) | null = null; public sandboxEnabled: boolean = false; @@ -75,6 +83,26 @@ export class CodexMcpClient { }) }).passthrough(), (data) => { const msg = data.params.msg; + if (typeof msg?.call_id === 'string') { + if (msg.type === 'exec_approval_request') { + this.lastApprovalRequest = { + callId: msg.call_id, + toolName: 'CodexBash', + command: Array.isArray(msg.command) ? msg.command : undefined, + cwd: typeof msg.cwd === 'string' ? msg.cwd : undefined, + }; + } + if (msg.type === 'patch_apply_begin' || msg.type === 'apply_patch_approval_request') { + this.lastApprovalRequest = { + callId: msg.call_id, + toolName: 'CodexPatch', + changes: msg.changes && typeof msg.changes === 'object' + ? msg.changes as Record + : undefined, + autoApproved: typeof msg.auto_approved === 'boolean' ? msg.auto_approved : undefined, + }; + } + } this.updateIdentifiersFromEvent(msg); this.handler?.(msg); }); @@ -200,38 +228,67 @@ export class CodexMcpClient { codex_event_id: string, codex_call_id: string, codex_command: string[], - codex_cwd: string + codex_cwd: string, + codex_changes?: Record, + codex_auto_approved?: boolean, } - const toolName = 'CodexBash'; + const toolName = this.lastApprovalRequest?.toolName + ?? (params.codex_changes ? 'CodexPatch' : 'CodexBash'); + const permissionRequestId = params.codex_call_id + || params.codex_mcp_tool_call_id + || this.lastApprovalRequest?.callId; + const command = params.codex_command ?? this.lastApprovalRequest?.command; + const cwd = params.codex_cwd ?? this.lastApprovalRequest?.cwd; + const changes = params.codex_changes ?? this.lastApprovalRequest?.changes; + const autoApproved = params.codex_auto_approved ?? this.lastApprovalRequest?.autoApproved; + + const mapDecisionToAction = (decision: 'approved' | 'approved_for_session' | 'denied' | 'abort') => { + if (decision === 'approved' || decision === 'approved_for_session') return 'accept' as const; + if (decision === 'denied') return 'decline' as const; + return 'cancel' as const; + }; // If no permission handler set, deny by default if (!this.permissionHandler) { logger.debug('[CodexMCP] No permission handler set, denying by default'); return { - decision: 'denied' as const, + action: 'decline' as const, + }; + } + + if (!permissionRequestId) { + logger.debug('[CodexMCP] Missing approval request identifier in elicitation payload'); + return { + action: 'decline' as const, }; } try { // Request permission through the handler const result = await this.permissionHandler.handleToolCall( - params.codex_call_id, + permissionRequestId, toolName, - { - command: params.codex_command, - cwd: params.codex_cwd - } + toolName === 'CodexPatch' + ? { + changes, + autoApproved, + } + : { + command, + cwd, + } ); + this.lastApprovalRequest = null; logger.debug('[CodexMCP] Permission result:', result); return { - decision: result.decision + action: mapDecisionToAction(result.decision), } } catch (error) { + this.lastApprovalRequest = null; logger.debug('[CodexMCP] Error handling permission request:', error); return { - decision: 'denied' as const, - reason: error instanceof Error ? error.message : 'Permission request failed' + action: 'decline' as const, }; } } @@ -363,6 +420,7 @@ export class CodexMcpClient { const previousSessionId = this.sessionId; this.sessionId = null; this.conversationId = null; + this.lastApprovalRequest = null; logger.debug('[CodexMCP] Session cleared, previous sessionId:', previousSessionId); }