From 27f7998ec0bed33c4e0720c9f256421df909a219 Mon Sep 17 00:00:00 2001 From: Motonari Tsuzuki Date: Sat, 15 Nov 2025 23:29:22 +0900 Subject: [PATCH] fix: resolve codex approval response ID mismatch When executing commands like `ls` in Codex, approval requests were sent but selecting YES didn't work because the permission response ID didn't match the registered permission request ID. The issue was that the elicitation handler registered permission requests using `codex_call_id`, but the mobile app sends permission responses using the `call_id` from `exec_approval_request` events, which corresponds to `codex_mcp_tool_call_id`. This fix changes the permission request registration to use `codex_mcp_tool_call_id` (with fallback to `codex_call_id` for compatibility) to match the ID that will be used in the tool-call message and permission response. Also adds comprehensive tests for the permission approval flow. --- .../__tests__/permissionApprovalFlow.test.ts | 170 ++++++++++++++++++ src/codex/codexMcpClient.ts | 9 +- 2 files changed, 177 insertions(+), 2 deletions(-) create mode 100644 src/codex/__tests__/permissionApprovalFlow.test.ts diff --git a/src/codex/__tests__/permissionApprovalFlow.test.ts b/src/codex/__tests__/permissionApprovalFlow.test.ts new file mode 100644 index 00000000..ccda2eb4 --- /dev/null +++ b/src/codex/__tests__/permissionApprovalFlow.test.ts @@ -0,0 +1,170 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { CodexPermissionHandler } from '../utils/permissionHandler'; +import type { ApiSessionClient } from '@/api/apiSession'; + +// Mock ApiSessionClient +const createMockSession = (): ApiSessionClient => { + const rpcHandlers = new Map Promise>(); + + return { + sessionId: 'test-session-id', + updateAgentState: vi.fn((updater: any) => { + const currentState = { requests: {}, completedRequests: {} }; + const newState = updater(currentState); + return newState; + }), + rpcHandlerManager: { + registerHandler: vi.fn((method: string, handler: (data: any) => Promise) => { + rpcHandlers.set(method, handler); + }), + call: vi.fn(), + }, + sendCodexMessage: vi.fn(), + sendSessionEvent: vi.fn(), + onUserMessage: vi.fn(), + keepAlive: vi.fn(), + flush: vi.fn(), + close: vi.fn(), + sendSessionDeath: vi.fn(), + } as unknown as ApiSessionClient; +}; + +describe('Codex Permission Approval Flow', () => { + let session: ApiSessionClient; + let permissionHandler: CodexPermissionHandler; + + beforeEach(() => { + session = createMockSession(); + permissionHandler = new CodexPermissionHandler(session); + }); + + it('should register permission request with correct ID and resolve when permission response matches', async () => { + const toolCallId = 'test-call-id-123'; + const toolName = 'CodexBash'; + const input = { command: ['ls'], cwd: '/tmp' }; + + // Register permission request + const permissionPromise = permissionHandler.handleToolCall(toolCallId, toolName, input); + + // Simulate permission response from mobile app + const rpcHandler = (session.rpcHandlerManager.registerHandler as any).mock.calls[0][1]; + + // Wait a bit to ensure the request is registered + await new Promise(resolve => setTimeout(resolve, 10)); + + // Send permission response with matching ID + await rpcHandler({ + id: toolCallId, + approved: true, + decision: 'approved' as const + }); + + // Wait for permission to be resolved + const result = await permissionPromise; + + expect(result.decision).toBe('approved'); + expect(session.updateAgentState).toHaveBeenCalled(); + }); + + it('should not resolve permission request when response ID does not match', async () => { + const toolCallId = 'test-call-id-123'; + const wrongId = 'wrong-call-id-456'; + const toolName = 'CodexBash'; + const input = { command: ['ls'], cwd: '/tmp' }; + + // Register permission request + const permissionPromise = permissionHandler.handleToolCall(toolCallId, toolName, input); + + // Simulate permission response from mobile app with wrong ID + const rpcHandler = (session.rpcHandlerManager.registerHandler as any).mock.calls[0][1]; + + // Wait a bit to ensure the request is registered + await new Promise(resolve => setTimeout(resolve, 10)); + + // Send permission response with non-matching ID + await rpcHandler({ + id: wrongId, + approved: true, + decision: 'approved' as const + }); + + // Permission should not be resolved (still pending) + // Use a timeout to verify it doesn't resolve + let resolved = false; + permissionPromise.then(() => { + resolved = true; + }); + + await new Promise(resolve => setTimeout(resolve, 100)); + + // The permission should still be pending since IDs don't match + expect(resolved).toBe(false); + }); + + it('should handle permission denial correctly', async () => { + const toolCallId = 'test-call-id-123'; + const toolName = 'CodexBash'; + const input = { command: ['rm', '-rf', '/'], cwd: '/' }; + + // Register permission request + const permissionPromise = permissionHandler.handleToolCall(toolCallId, toolName, input); + + // Simulate permission response denying the request + const rpcHandler = (session.rpcHandlerManager.registerHandler as any).mock.calls[0][1]; + + // Wait a bit to ensure the request is registered + await new Promise(resolve => setTimeout(resolve, 10)); + + // Send permission response denying + await rpcHandler({ + id: toolCallId, + approved: false, + decision: 'denied' as const + }); + + // Wait for permission to be resolved + const result = await permissionPromise; + + expect(result.decision).toBe('denied'); + }); + + it('should handle multiple permission requests with different IDs', async () => { + const toolCallId1 = 'test-call-id-123'; + const toolCallId2 = 'test-call-id-456'; + const toolName = 'CodexBash'; + const input1 = { command: ['ls'], cwd: '/tmp' }; + const input2 = { command: ['pwd'], cwd: '/tmp' }; + + // Register two permission requests + const permissionPromise1 = permissionHandler.handleToolCall(toolCallId1, toolName, input1); + const permissionPromise2 = permissionHandler.handleToolCall(toolCallId2, toolName, input2); + + // Simulate permission responses + const rpcHandler = (session.rpcHandlerManager.registerHandler as any).mock.calls[0][1]; + + // Wait a bit to ensure requests are registered + await new Promise(resolve => setTimeout(resolve, 10)); + + // Send permission response for first request + await rpcHandler({ + id: toolCallId1, + approved: true, + decision: 'approved' as const + }); + + // Send permission response for second request + await rpcHandler({ + id: toolCallId2, + approved: true, + decision: 'approved' as const + }); + + // Both permissions should be resolved + const result1 = await permissionPromise1; + const result2 = await permissionPromise2; + + expect(result1.decision).toBe('approved'); + expect(result2.decision).toBe('approved'); + }); +}); + diff --git a/src/codex/codexMcpClient.ts b/src/codex/codexMcpClient.ts index 795bc05d..e8aca634 100644 --- a/src/codex/codexMcpClient.ts +++ b/src/codex/codexMcpClient.ts @@ -134,9 +134,14 @@ export class CodexMcpClient { } try { - // Request permission through the handler + // Use codex_mcp_tool_call_id for permission request registration + // to match the call_id that will be used in exec_approval_request event + const actualCallId = params.codex_mcp_tool_call_id || params.codex_call_id; + + // Request permission through the handler using the actual call_id + // that will match the call_id in exec_approval_request event const result = await this.permissionHandler.handleToolCall( - params.codex_call_id, + actualCallId, toolName, { command: params.codex_command,