Skip to content

Fix Codex mobile approval request handling #833

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SharingMan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix Codex mobile approval request handling #833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 127 additions & 2 deletions packages/happy-cli/src/codex/__tests__/codexMcpClient.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const {
mockClientConnect,
mockClientClose,
mockStdioCtor,
mockNotificationHandler,
mockRequestHandler,
} = vi.hoisted(() => ({
mockExecSync: vi.fn(),
mockInitializeSandbox: vi.fn(),
Expand All @@ -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<any>) | null,
},
}));

vi.mock('child_process', () => ({
Expand All @@ -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<any>) => {
mockRequestHandler.current = handler;
});
connect = mockClientConnect;
close = mockClientClose;
callTool = vi.fn();
Expand Down Expand Up @@ -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(() => {
Expand Down Expand Up @@ -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' });
});
});
80 changes: 69 additions & 11 deletions packages/happy-cli/src/codex/codexMcpClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>;
autoApproved?: boolean;
} | null = null;
private sandboxConfig?: SandboxConfig;
private sandboxCleanup: (() => Promise<void>) | null = null;
public sandboxEnabled: boolean = false;
Expand All @@ -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<string, unknown>
: undefined,
autoApproved: typeof msg.auto_approved === 'boolean' ? msg.auto_approved : undefined,
};
}
}
this.updateIdentifiersFromEvent(msg);
this.handler?.(msg);
});
Expand Down Expand Up @@ -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<string, unknown>,
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,
};
}
}
Expand Down Expand Up @@ -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);
}

Expand Down