From d97b270ba8aceb6767dfd9d0f613235c396f309d Mon Sep 17 00:00:00 2001 From: SSU-WEI HUANG Date: Sun, 12 Jul 2026 18:42:55 +0800 Subject: [PATCH] fix(codex): bridge MCP elicitation through user input (#1008) * fix(codex): bridge MCP elicitation through user input * fix(codex): allow MCP elicitation in yolo mode * fix(codex): preserve MCP form semantics * fix(codex): accept implicit MCP form mode * fix(codex): harden MCP elicitation prompts * fix(codex): require valid MCP choice answers * fix(codex): round-trip MCP array elicitation * fix(web): require explicit MCP URL confirmation * fix(codex): preserve MCP array item types * fix(codex): support multi-select MCP elicitation * fix(codex): allow MCP elicitation in read-only mode * fix(codex): route MCP tool approvals through permissions --- cli/src/codex/appServerTypes.ts | 14 +- cli/src/codex/codexRemoteLauncher.test.ts | 32 +- cli/src/codex/utils/appServerConfig.test.ts | 39 +- cli/src/codex/utils/appServerConfig.ts | 16 + .../utils/appServerPermissionAdapter.test.ts | 418 +++++++++++++++++- .../codex/utils/appServerPermissionAdapter.ts | 267 ++++++++++- cli/src/codex/utils/permissionModeConfig.ts | 4 +- .../ToolCard/RequestUserInputFooter.tsx | 46 +- .../components/ToolCard/knownTools.test.tsx | 42 ++ web/src/components/ToolCard/knownTools.tsx | 6 +- .../ToolCard/requestUserInput.test.ts | 122 +++++ .../components/ToolCard/requestUserInput.ts | 87 +++- .../ToolCard/views/RequestUserInputView.tsx | 2 +- web/src/lib/locales/en.ts | 1 + web/src/lib/locales/zh-CN.ts | 1 + 15 files changed, 1006 insertions(+), 91 deletions(-) create mode 100644 web/src/components/ToolCard/requestUserInput.test.ts diff --git a/cli/src/codex/appServerTypes.ts b/cli/src/codex/appServerTypes.ts index 5fc0262c..2978aab4 100644 --- a/cli/src/codex/appServerTypes.ts +++ b/cli/src/codex/appServerTypes.ts @@ -1,4 +1,16 @@ -export type ApprovalPolicy = 'untrusted' | 'on-failure' | 'on-request' | 'never'; +export type ApprovalPolicyPreset = 'untrusted' | 'on-failure' | 'on-request' | 'never'; + +export type ApprovalPolicy = + | ApprovalPolicyPreset + | { + granular: { + sandbox_approval: boolean; + rules: boolean; + skill_approval?: boolean; + request_permissions?: boolean; + mcp_elicitations: boolean; + }; + }; export type SandboxMode = 'read-only' | 'workspace-write' | 'danger-full-access'; export interface InitializeCapabilities { diff --git a/cli/src/codex/codexRemoteLauncher.test.ts b/cli/src/codex/codexRemoteLauncher.test.ts index 731b030d..f389b4f2 100644 --- a/cli/src/codex/codexRemoteLauncher.test.ts +++ b/cli/src/codex/codexRemoteLauncher.test.ts @@ -1129,8 +1129,8 @@ describe('codexRemoteLauncher', () => { expect(session.thinking).toBe(false); }); - it('uses live permission mode for app-server MCP elicitation handlers', async () => { - const { session, setPermissionMode } = createSessionStub(); + it('routes app-server MCP elicitation through the existing user-input transport', async () => { + const { session, codexMessages, rpcHandlers, setPermissionMode } = createSessionStub(); const exitReason = await codexRemoteLauncher(session as never); @@ -1156,14 +1156,24 @@ describe('codexRemoteLauncher', () => { } }; - await expect(handler?.(request)).resolves.toEqual({ - action: 'cancel', - content: null, - _meta: null + setPermissionMode('yolo'); + const response = handler?.(request); + await vi.waitFor(() => { + expect(codexMessages).toContainEqual(expect.objectContaining({ + type: 'tool-call', + name: 'request_user_input' + })); + }); + const requestMessage = codexMessages.find((message) => ( + typeof message === 'object' && message !== null && 'name' in message && message.name === 'request_user_input' + )) as { callId: string }; + await rpcHandlers.get('permission')?.({ + id: requestMessage.callId, + approved: true, + answers: { approval: { answers: ['allow'] } } }); - setPermissionMode('yolo'); - await expect(handler?.(request)).resolves.toEqual({ + await expect(response).resolves.toEqual({ action: 'accept', content: { approval: 'allow' @@ -1171,12 +1181,6 @@ describe('codexRemoteLauncher', () => { _meta: null }); - setPermissionMode('default'); - await expect(handler?.(request)).resolves.toEqual({ - action: 'cancel', - content: null, - _meta: null - }); }); it('sends Codex plan collaboration mode when the app-server advertises it', async () => { diff --git a/cli/src/codex/utils/appServerConfig.test.ts b/cli/src/codex/utils/appServerConfig.test.ts index 2142e18e..ebf128fb 100644 --- a/cli/src/codex/utils/appServerConfig.test.ts +++ b/cli/src/codex/utils/appServerConfig.test.ts @@ -87,7 +87,15 @@ describe('appServerConfig', () => { }); expect(params.sandbox).toBe('danger-full-access'); - expect(params.approvalPolicy).toBe('never'); + expect(params.approvalPolicy).toEqual({ + granular: { + sandbox_approval: false, + rules: false, + skill_approval: false, + request_permissions: false, + mcp_elicitations: true + } + }); }); it('keeps on-failure approvals for safe-yolo threads', () => { @@ -101,6 +109,25 @@ describe('appServerConfig', () => { expect(params.approvalPolicy).toBe('on-failure'); }); + it('allows MCP elicitation without enabling sandbox prompts for read-only threads', () => { + const params = buildThreadStartParams({ + cwd: '/workspace/project', + mode: { permissionMode: 'read-only', collaborationMode: 'default' }, + mcpServers + }); + + expect(params.sandbox).toBe('read-only'); + expect(params.approvalPolicy).toEqual({ + granular: { + sandbox_approval: false, + rules: false, + skill_approval: false, + request_permissions: false, + mcp_elicitations: true + } + }); + }); + it('concatenates custom developer instructions after base instructions', () => { const params = buildThreadStartParams({ cwd: '/workspace/project', @@ -229,7 +256,15 @@ describe('appServerConfig', () => { expect(params.threadId).toBe('thread-1'); expect(params.cwd).toBe('/workspace/project'); expect(params.input).toEqual([{ type: 'text', text: 'hello' }]); - expect(params.approvalPolicy).toBe('never'); + expect(params.approvalPolicy).toEqual({ + granular: { + sandbox_approval: false, + rules: false, + skill_approval: false, + request_permissions: false, + mcp_elicitations: true + } + }); expect(params.sandboxPolicy).toEqual({ type: 'readOnly' }); expect(params.effort).toBe('high'); expect(params.summary).toBeUndefined(); diff --git a/cli/src/codex/utils/appServerConfig.ts b/cli/src/codex/utils/appServerConfig.ts index 9a694133..13bbc8c0 100644 --- a/cli/src/codex/utils/appServerConfig.ts +++ b/cli/src/codex/utils/appServerConfig.ts @@ -23,7 +23,23 @@ const MODELS_WITHOUT_REASONING_SUMMARY = new Set([ 'gpt-5.3-codex-spark' ]); +const MCP_ELICITATION_ONLY_APPROVAL_POLICY = { + granular: { + sandbox_approval: false, + rules: false, + skill_approval: false, + request_permissions: false, + mcp_elicitations: true + } +} as const satisfies ApprovalPolicy; + function resolveApprovalPolicy(mode: EnhancedMode): ApprovalPolicy { + if (mode.permissionMode === 'yolo' || mode.permissionMode === 'read-only') { + // Codex's `never` policy auto-declines MCP elicitations before app-server + // can forward them. Keep command/sandbox prompts disabled for Yolo and + // read-only while allowing auth and structured input to reach HAPI's UI. + return MCP_ELICITATION_ONLY_APPROVAL_POLICY; + } return resolveCodexPermissionModeConfig(mode.permissionMode).approvalPolicy; } diff --git a/cli/src/codex/utils/appServerPermissionAdapter.test.ts b/cli/src/codex/utils/appServerPermissionAdapter.test.ts index 7d0df706..125b88b7 100644 --- a/cli/src/codex/utils/appServerPermissionAdapter.test.ts +++ b/cli/src/codex/utils/appServerPermissionAdapter.test.ts @@ -189,17 +189,21 @@ describe('registerAppServerPermissionHandlers', () => { }); }); - it('accepts non-HAPI MCP elicitation requests when live permission mode is yolo', async () => { + it('keeps structured MCP tool approval forms interactive even in yolo mode', async () => { const { client, handlers } = createClient(); - let permissionMode: 'default' | 'read-only' | 'safe-yolo' | 'yolo' = 'default'; const permissionHandler = { handleToolCall: vi.fn() }; + const onUserInputRequest = vi.fn(async () => ({ + decision: 'accept' as const, + answers: { approval: { answers: ['allow'] } } + })); registerAppServerPermissionHandlers({ client: client as never, permissionHandler: permissionHandler as never, - getPermissionMode: () => permissionMode + getPermissionMode: () => 'yolo', + onUserInputRequest }); const handler = handlers.get('mcpServer/elicitation/request'); @@ -211,26 +215,26 @@ describe('registerAppServerPermissionHandlers', () => { serverName: 'qmd', mode: 'form', message: 'Allow the qmd MCP server to run tool "status"?', - _meta: null, + _meta: { + codex_approval_kind: 'mcp_tool_call', + tool_name: 'status' + }, requestedSchema: { type: 'object', properties: { approval: { type: 'string', enum: ['allow', 'deny'] + }, + comment: { + type: 'string', + title: 'Optional comment' } }, required: ['approval'] } }; - await expect(handler?.(request)).resolves.toEqual({ - action: 'cancel', - content: null, - _meta: null - }); - - permissionMode = 'yolo'; await expect(handler?.(request)).resolves.toEqual({ action: 'accept', content: { @@ -239,15 +243,369 @@ describe('registerAppServerPermissionHandlers', () => { _meta: null }); - permissionMode = 'default'; - await expect(handler?.(request)).resolves.toEqual({ - action: 'cancel', + expect(onUserInputRequest).toHaveBeenCalledWith({ + id: expect.any(String), + input: { + questions: [{ + id: 'approval', + header: 'approval', + question: 'Allow the qmd MCP server to run tool "status"?\n\napproval', + required: true, + options: [{ label: 'allow', description: '' }, { label: 'deny', description: '' }] + }, { + id: 'comment', + header: 'Optional comment', + question: 'Allow the qmd MCP server to run tool "status"?\n\nOptional comment', + required: false, + options: [] + }] + } + }); + }); + + it('routes message-only MCP tool approvals through the permission handler in yolo mode', async () => { + const { client, handlers } = createClient(); + const permissionHandler = { + handleToolCall: vi.fn(async () => ({ decision: 'approved_for_session' as const })) + }; + const onUserInputRequest = vi.fn(); + + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: permissionHandler as never, + getPermissionMode: () => 'yolo', + onUserInputRequest + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'github', + request: { + elicitationId: 'approval-1', + mode: 'form', + message: 'Allow GitHub search?', + requestedSchema: { + type: 'object', + properties: {} + }, + _meta: { + codex_approval_kind: 'mcp_tool_call', + tool_name: 'search_issues', + tool_title: 'Search issues', + tool_description: 'Search GitHub issues', + tool_params: { query: 'is:open bug' }, + tool_params_display: { query: 'is:open bug' }, + persist: ['session', 'always'] + } + } + })).resolves.toEqual({ + action: 'accept', + content: null, + _meta: { persist: 'session' } + }); + + expect(permissionHandler.handleToolCall).toHaveBeenCalledWith( + 'approval-1', + 'search_issues', + { + message: 'Allow GitHub search?', + serverName: 'github', + toolTitle: 'Search issues', + toolDescription: 'Search GitHub issues', + toolParams: { query: 'is:open bug' }, + toolParamsDisplay: { query: 'is:open bug' } + } + ); + expect(onUserInputRequest).not.toHaveBeenCalled(); + }); + + it('does not persist a yolo MCP tool approval when session persistence is unavailable', async () => { + const { client, handlers } = createClient(); + const permissionHandler = { + handleToolCall: vi.fn(async () => ({ decision: 'approved_for_session' as const })) + }; + + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: permissionHandler as never, + getPermissionMode: () => 'yolo' + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Allow tool?', + elicitationId: 'approval-2', + requestedSchema: null, + _meta: { + codex_approval_kind: 'mcp_tool_call', + tool_name: 'external_tool', + persist: 'always' + } + })).resolves.toEqual({ + action: 'accept', content: null, _meta: null }); }); - it('does not auto-accept non-HAPI MCP elicitation requests in safe-yolo mode', async () => { + it.each([ + ['approved', { action: 'accept', content: null, _meta: null }], + ['denied', { action: 'decline', content: null, _meta: null }], + ['abort', { action: 'cancel', content: null, _meta: null }] + ] as const)('maps %s MCP tool approval decisions without request_user_input', async (decision, expected) => { + const { client, handlers } = createClient(); + const permissionHandler = { + handleToolCall: vi.fn(async () => ({ decision })) + }; + const onUserInputRequest = vi.fn(); + + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: permissionHandler as never, + onUserInputRequest + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + message: 'Allow tool?', + elicitationId: 'approval-3', + _meta: { + codex_approval_kind: 'mcp_tool_call', + tool_name: 'external_tool', + persist: ['session'] + } + })).resolves.toEqual(expected); + expect(onUserInputRequest).not.toHaveBeenCalled(); + }); + + it('keeps a selected MCP answer when the user also adds a note', async () => { + const { client, handlers } = createClient(); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest: vi.fn(async () => ({ + decision: 'accept' as const, + answers: { approval: { answers: ['allow', 'user_note: approved for this task'] } } + })) + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Approve?', + requestedSchema: { + type: 'object', + properties: { + approval: { type: 'string', enum: ['allow', 'deny'] } + }, + required: ['approval'] + } + })).resolves.toEqual({ + action: 'accept', + content: { approval: 'allow' }, + _meta: null + }); + }); + + it('does not coerce a note-only MCP choice into a schema value', async () => { + const { client, handlers } = createClient(); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest: vi.fn(async () => ({ + decision: 'accept' as const, + answers: { approved: { answers: ['user_note: please approve'] } } + })) + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Approve?', + requestedSchema: { + type: 'object', + properties: { approved: { type: 'boolean' } }, + required: ['approved'] + } + })).resolves.toEqual({ + action: 'accept', + content: {}, + _meta: null + }); + }); + + it('round-trips array choices and free-text array input', async () => { + const { client, handlers } = createClient(); + const onUserInputRequest = vi.fn(async () => ({ + decision: 'accept' as const, + answers: { + tags: { answers: ['bug'] }, + paths: { answers: ['user_note: src/index.ts'] } + } + })); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Choose metadata', + requestedSchema: { + type: 'object', + properties: { + tags: { type: 'array', items: { type: 'string', enum: ['bug', 'feature'] } }, + paths: { type: 'array', items: { type: 'string' } } + }, + required: ['tags', 'paths'] + } + })).resolves.toEqual({ + action: 'accept', + content: { + tags: ['bug'], + paths: ['src/index.ts'] + }, + _meta: null + }); + expect(onUserInputRequest).toHaveBeenCalledWith({ + id: expect.any(String), + input: { + questions: [{ + id: 'tags', + header: 'tags', + question: 'Choose metadata\n\ntags', + required: true, + multiple: true, + options: [{ label: 'bug', description: '' }, { label: 'feature', description: '' }] + }, { + id: 'paths', + header: 'paths', + question: 'Choose metadata\n\npaths', + required: true, + multiple: true, + options: [] + }] + } + }); + }); + + it('preserves number, integer, and boolean array item types', async () => { + const { client, handlers } = createClient(); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest: vi.fn(async () => ({ + decision: 'accept' as const, + answers: { + scores: { answers: ['2.5'] }, + indices: { answers: ['3'] }, + flags: { answers: ['true'] } + } + })) + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Choose typed arrays', + requestedSchema: { + type: 'object', + properties: { + scores: { type: 'array', items: { type: 'number', enum: [1.5, 2.5] } }, + indices: { type: 'array', items: { type: 'integer', enum: [2, 3] } }, + flags: { type: 'array', items: { type: 'boolean' } } + }, + required: ['scores', 'indices', 'flags'] + } + })).resolves.toEqual({ + action: 'accept', + content: { + scores: [2.5], + indices: [3], + flags: [true] + }, + _meta: null + }); + }); + + it('treats an omitted MCP elicitation mode as a form request', async () => { + const { client, handlers } = createClient(); + const onUserInputRequest = vi.fn(async () => ({ + decision: 'accept' as const, + answers: { nickname: { answers: ['user_note: Codex'] } } + })); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + message: 'Choose a nickname', + requestedSchema: { + type: 'object', + properties: { nickname: { type: 'string' } }, + required: ['nickname'] + } + })).resolves.toEqual({ + action: 'accept', + content: { nickname: 'Codex' }, + _meta: null + }); + expect(onUserInputRequest).toHaveBeenCalledOnce(); + }); + + it('gives generic message-only MCP forms a display header instead of exposing the internal id', async () => { + const { client, handlers } = createClient(); + const onUserInputRequest = vi.fn(async () => ({ + decision: 'accept' as const, + answers: { __mcp_form_confirmation: { answers: ['Continue'] } } + })); + registerAppServerPermissionHandlers({ + client: client as never, + permissionHandler: { handleToolCall: vi.fn() } as never, + onUserInputRequest + }); + + const handler = handlers.get('mcpServer/elicitation/request'); + await expect(handler?.({ + serverName: 'external', + mode: 'form', + message: 'Continue with connector setup?', + requestedSchema: { + type: 'object', + properties: {} + } + })).resolves.toEqual({ + action: 'accept', + content: {}, + _meta: null + }); + expect(onUserInputRequest).toHaveBeenCalledWith({ + id: expect.any(String), + input: { + questions: [{ + id: '__mcp_form_confirmation', + header: 'Confirmation', + question: 'Continue with connector setup?', + options: [{ label: 'Continue', description: '' }] + }] + } + }); + }); + + it('forwards URL MCP elicitation and preserves a declined response', async () => { const { client, handlers } = createClient(); const permissionHandler = { handleToolCall: vi.fn() @@ -256,7 +614,22 @@ describe('registerAppServerPermissionHandlers', () => { registerAppServerPermissionHandlers({ client: client as never, permissionHandler: permissionHandler as never, - getPermissionMode: () => 'safe-yolo' + getPermissionMode: () => 'yolo', + onUserInputRequest: vi.fn(async ({ input }) => { + expect(input).toEqual({ + url: 'https://example.com/login', + questions: [{ + id: '__mcp_url_confirmation', + header: 'Sign in', + question: 'Sign in to continue', + options: [{ + label: 'Open sign-in page and continue', + description: 'https://example.com/login' + }] + }] + }); + return { decision: 'decline' as const }; + }) }); const handler = handlers.get('mcpServer/elicitation/request'); @@ -266,15 +639,14 @@ describe('registerAppServerPermissionHandlers', () => { threadId: 'thread-1', turnId: 'turn-1', serverName: 'external', - mode: 'form', - message: 'Collect data', - _meta: null, - requestedSchema: { - type: 'object', - properties: {}, + request: { + mode: 'url', + message: 'Sign in to continue', + url: 'https://example.com/login', + elicitationId: 'auth-1' } })).resolves.toEqual({ - action: 'cancel', + action: 'decline', content: null, _meta: null }); diff --git a/cli/src/codex/utils/appServerPermissionAdapter.ts b/cli/src/codex/utils/appServerPermissionAdapter.ts index b62514c4..08294000 100644 --- a/cli/src/codex/utils/appServerPermissionAdapter.ts +++ b/cli/src/codex/utils/appServerPermissionAdapter.ts @@ -12,6 +12,8 @@ type PermissionResult = { }; type ElicitationSchemaProperty = { + title?: unknown; + description?: unknown; type?: unknown; default?: unknown; enum?: unknown; @@ -19,6 +21,8 @@ type ElicitationSchemaProperty = { items?: unknown; }; +type UserInputAnswer = Record | Record; + function asRecord(value: unknown): Record | null { if (!value || typeof value !== 'object') { return null; @@ -30,6 +34,17 @@ function asString(value: unknown): string | undefined { return typeof value === 'string' && value.length > 0 ? value : undefined; } +function asWebUrl(value: unknown): string | undefined { + const raw = asString(value); + if (!raw) return undefined; + try { + const url = new URL(raw); + return url.protocol === 'https:' || url.protocol === 'http:' ? url.toString() : undefined; + } catch { + return undefined; + } +} + function pickToolName(record: Record): string { return asString(record.toolName) ?? asString(record.tool_name) @@ -150,6 +165,211 @@ function buildAcceptedElicitationContent(params: unknown): Record { + const record = asRecord(params) ?? {}; + return asRecord(record.request) ?? record; +} + +function getMcpToolApprovalMeta(params: unknown): Record | null { + const record = asRecord(params) ?? {}; + const request = unwrapElicitationRequest(params); + const meta = asRecord(request._meta) ?? asRecord(record._meta); + if (meta?.codex_approval_kind !== 'mcp_tool_call') return null; + + const mode = asString(request.mode) ?? 'form'; + if (mode !== 'form') return null; + + const schema = asRecord(request.requestedSchema); + const properties = asRecord(schema?.properties); + if (properties && Object.keys(properties).length > 0) return null; + + return meta; +} + +function mcpApprovalSupportsSessionPersistence(meta: Record): boolean { + if (meta.persist === 'session') return true; + return Array.isArray(meta.persist) && meta.persist.includes('session'); +} + +function buildMcpToolApprovalInput( + params: unknown, + meta: Record +): { toolName: string; input: Record } { + const record = asRecord(params) ?? {}; + const request = unwrapElicitationRequest(params); + const serverName = asString(record.serverName) ?? asString(request.serverName); + const toolTitle = asString(meta.tool_title); + const toolName = asString(meta.tool_name) ?? toolTitle ?? serverName ?? 'MCP tool'; + const input: Record = { + message: asString(request.message) ?? 'Allow MCP tool call?' + }; + + if (serverName) input.serverName = serverName; + if (toolTitle) input.toolTitle = toolTitle; + const toolDescription = asString(meta.tool_description); + if (toolDescription) input.toolDescription = toolDescription; + if (meta.tool_params !== undefined) input.toolParams = meta.tool_params; + if (meta.tool_params_display !== undefined) input.toolParamsDisplay = meta.tool_params_display; + + return { toolName, input }; +} + +function mapMcpToolApprovalDecision( + decision: PermissionDecision, + meta: Record +): { action: 'accept' | 'decline' | 'cancel'; content: null; _meta: { persist: 'session' } | null } { + if (decision === 'denied') { + return { action: 'decline', content: null, _meta: null }; + } + if (decision === 'abort') { + return { action: 'cancel', content: null, _meta: null }; + } + + return { + action: 'accept', + content: null, + _meta: decision === 'approved_for_session' && mcpApprovalSupportsSessionPersistence(meta) + ? { persist: 'session' } + : null + }; +} + +function elicitationChoiceValues(property: Record): unknown[] { + if (Array.isArray(property.enum)) return property.enum; + if (Array.isArray(property.oneOf)) { + return property.oneOf.map((item) => asRecord(item)?.const).filter((item) => item !== undefined); + } + if (property.type === 'array') { + const items = asRecord(property.items); + if (Array.isArray(items?.enum)) return items.enum; + if (Array.isArray(items?.oneOf)) { + return items.oneOf.map((item) => asRecord(item)?.const).filter((item) => item !== undefined); + } + if (items?.type === 'boolean') return [true, false]; + } + if (property.type === 'boolean') return [true, false]; + return []; +} + +function elicitationOptions(property: Record): Array<{ label: string; description: string }> { + return elicitationChoiceValues(property).map((value) => ({ + label: String(value), + description: '' + })); +} + +function buildElicitationUserInput(params: unknown): { questions: unknown[]; url?: string } | null { + const request = unwrapElicitationRequest(params); + const mode = asString(request.mode) ?? 'form'; + const message = asString(request.message) ?? 'MCP server requires input'; + + if (mode === 'url') { + const url = asWebUrl(request.url); + if (!url) return null; + return { + url, + questions: [{ + id: '__mcp_url_confirmation', + header: 'Sign in', + question: message, + options: [{ label: 'Open sign-in page and continue', description: url }] + }] + }; + } + + if (mode !== 'form') return null; + const schema = asRecord(request.requestedSchema); + const properties = asRecord(schema?.properties); + if (!properties) return null; + const required = new Set( + Array.isArray(schema?.required) + ? schema.required.filter((value): value is string => typeof value === 'string') + : [] + ); + + const questions = Object.entries(properties).map(([id, rawProperty]) => { + const property = asRecord(rawProperty) ?? {}; + const fieldQuestion = asString(property.title) ?? asString(property.description) ?? id; + return { + id, + header: fieldQuestion, + question: `${message}\n\n${fieldQuestion}`, + required: required.has(id), + ...(property.type === 'array' ? { multiple: true } : {}), + options: elicitationOptions(property) + }; + }); + return { + questions: questions.length > 0 ? questions : [{ + id: '__mcp_form_confirmation', + header: 'Confirmation', + question: message, + options: [{ label: 'Continue', description: '' }] + }] + }; +} + +function answerValues(answers: UserInputAnswer, id: string): string[] { + const value = answers[id]; + if (Array.isArray(value)) return value; + return asRecord(value)?.answers instanceof Array + ? (asRecord(value)?.answers as unknown[]).filter((item): item is string => typeof item === 'string') + : []; +} + +function coerceSchemaValue(value: string, schema: Record): unknown { + if (schema.type === 'boolean') { + if (value === 'true') return true; + if (value === 'false') return false; + return undefined; + } + if (schema.type === 'number') { + const number = Number(value); + return Number.isFinite(number) ? number : undefined; + } + if (schema.type === 'integer') { + const number = Number(value); + return Number.isInteger(number) ? number : undefined; + } + return value; +} + +function buildElicitationContent(params: unknown, answers: UserInputAnswer): Record { + const request = unwrapElicitationRequest(params); + if (request.mode === 'url') return {}; + const properties = asRecord(asRecord(request.requestedSchema)?.properties) ?? {}; + const content: Record = {}; + + for (const [id, rawProperty] of Object.entries(properties)) { + const property = asRecord(rawProperty) ?? {}; + const values = answerValues(answers, id); + const selectedValues = values.filter((value) => !value.startsWith('user_note: ')); + const selected = selectedValues[0]; + const note = values.find((value) => value.startsWith('user_note: '))?.slice('user_note: '.length); + const hasSchemaChoices = elicitationChoiceValues(property).length > 0; + if (hasSchemaChoices && selected === undefined) continue; + const value = selected ?? note; + if (value === undefined) continue; + + if (property.type === 'array') { + const itemSchema = asRecord(property.items) ?? {}; + const rawValues = selectedValues.length > 0 + ? selectedValues + : note !== undefined + ? [note] + : []; + content[id] = rawValues + .map((item) => coerceSchemaValue(item, itemSchema)) + .filter((item) => item !== undefined); + } else { + const coerced = coerceSchemaValue(value, property); + if (coerced !== undefined) content[id] = coerced; + } + } + + return content; +} + function isHapiBridgeElicitation(params: unknown): boolean { const record = asRecord(params); return record?.serverName === 'hapi'; @@ -160,7 +380,7 @@ export function registerAppServerPermissionHandlers(args: { permissionHandler: CodexPermissionHandler; getPermissionMode?: () => CodexPermissionMode | undefined; onUserInputRequest?: (request: { id: string; input: unknown }) => Promise< - | { decision: 'accept'; answers: Record | Record } + | { decision: 'accept'; answers: UserInputAnswer } | { decision: 'decline' | 'cancel' } >; }): void { @@ -259,16 +479,36 @@ export function registerAppServerPermissionHandlers(args: { client.registerRequestHandler('mcpServer/elicitation/request', async (params) => { const record = asRecord(params) ?? {}; + const request = unwrapElicitationRequest(params); - const currentPermissionMode = getPermissionMode?.(); - const shouldAccept = isHapiBridgeElicitation(params) || currentPermissionMode === 'yolo'; + // HAPI's own bridge only asks for values whose safe defaults are defined by HAPI. + if (isHapiBridgeElicitation(params)) { + return { + action: 'accept', + content: buildAcceptedElicitationContent(request), + _meta: null + }; + } - if (!shouldAccept) { + const approvalMeta = getMcpToolApprovalMeta(params); + if (approvalMeta) { + const requestId = asString(request.elicitationId) ?? randomUUID(); + const approval = buildMcpToolApprovalInput(params, approvalMeta); + const result = await permissionHandler.handleToolCall( + requestId, + approval.toolName, + approval.input + ) as PermissionResult; + return mapMcpToolApprovalDecision(result.decision, approvalMeta); + } + + const input = buildElicitationUserInput(params); + if (!onUserInputRequest || !input) { logger.debug('[CodexAppServer] Cancelling unsupported MCP elicitation request', { serverName: record.serverName, - mode: record.mode, - message: record.message, - permissionMode: currentPermissionMode ?? 'unknown' + mode: request.mode, + message: request.message, + permissionMode: getPermissionMode?.() ?? 'unknown' }); return { @@ -278,16 +518,15 @@ export function registerAppServerPermissionHandlers(args: { }; } - logger.debug('[CodexAppServer] Accepting MCP elicitation request', { - serverName: record.serverName, - mode: record.mode, - message: record.message, - permissionMode: currentPermissionMode ?? 'unknown' - }); + const requestId = asString(request.elicitationId) ?? randomUUID(); + const result = await onUserInputRequest({ id: requestId, input }); + if (result.decision !== 'accept') { + return { action: result.decision === 'decline' ? 'decline' : 'cancel', content: null, _meta: null }; + } return { action: 'accept', - content: buildAcceptedElicitationContent(params), + content: buildElicitationContent(params, result.answers), _meta: null }; }); diff --git a/cli/src/codex/utils/permissionModeConfig.ts b/cli/src/codex/utils/permissionModeConfig.ts index e0838834..7adc3c1b 100644 --- a/cli/src/codex/utils/permissionModeConfig.ts +++ b/cli/src/codex/utils/permissionModeConfig.ts @@ -1,8 +1,8 @@ import type { CodexPermissionMode } from '@hapi/protocol/types'; -import type { ApprovalPolicy, SandboxMode, SandboxPolicy } from '../appServerTypes'; +import type { ApprovalPolicyPreset, SandboxMode, SandboxPolicy } from '../appServerTypes'; export type CodexPermissionModeConfig = { - approvalPolicy: ApprovalPolicy; + approvalPolicy: ApprovalPolicyPreset; sandbox: SandboxMode; sandboxPolicy: SandboxPolicy; }; diff --git a/web/src/components/ToolCard/RequestUserInputFooter.tsx b/web/src/components/ToolCard/RequestUserInputFooter.tsx index 4942a5e5..99ea2c31 100644 --- a/web/src/components/ToolCard/RequestUserInputFooter.tsx +++ b/web/src/components/ToolCard/RequestUserInputFooter.tsx @@ -8,6 +8,9 @@ import { isRequestUserInputToolName, parseRequestUserInputInput, formatRequestUserInputAnswers, + isRequestUserInputQuestionAnswered, + isRequestUserInputUrlConfirmed, + openRequestUserInputUrl, type RequestUserInputQuestion } from '@/components/ToolCard/requestUserInput' import { cn } from '@/lib/utils' @@ -57,7 +60,7 @@ function OptionRow(props: { } type QuestionState = { - selected: string | null + selected: string[] userNote: string } @@ -84,7 +87,7 @@ export function RequestUserInputFooter(props: { setStep(0) const initial: Record = {} for (const q of questions) { - initial[q.id] = { selected: null, userNote: '' } + initial[q.id] = { selected: [], userNote: '' } } setStateByQuestion(initial) setLoading(false) @@ -112,16 +115,7 @@ export function RequestUserInputFooter(props: { const currentQuestion = questions[clampedStep] as RequestUserInputQuestion | undefined const validateQuestion = (question: RequestUserInputQuestion): boolean => { - const state = stateByQuestion[question.id] - if (!state) return false - - // For questions with options, require a selection OR user note - if (question.options.length > 0) { - return state.selected !== null || state.userNote.trim().length > 0 - } - - // For pure text questions (no options), require user note - return state.userNote.trim().length > 0 + return isRequestUserInputQuestionAnswered(question, stateByQuestion[question.id]) } const submit = async () => { @@ -140,6 +134,18 @@ export function RequestUserInputFooter(props: { // Format answers for submission const formattedAnswers = formatRequestUserInputAnswers(stateByQuestion) + if (parsed.url) { + if (!isRequestUserInputUrlConfirmed(parsed, stateByQuestion)) { + setError(t('tool.selectOption')) + return + } + if (!openRequestUserInputUrl(parsed.url)) { + setError(t('tool.requestUserInput.popupBlocked')) + haptic.notification('error') + return + } + } + setLoading(true) await run(() => props.api.approvePermission(props.sessionId, permission.id, formattedAnswers), 'success') setLoading(false) @@ -160,13 +166,17 @@ export function RequestUserInputFooter(props: { setStep((s) => Math.max(s - 1, 0)) } - const selectOption = (questionId: string, optionLabel: string) => { + const selectOption = (question: RequestUserInputQuestion, optionLabel: string) => { haptic.selection() setStateByQuestion((prev) => ({ ...prev, - [questionId]: { - ...prev[questionId], - selected: optionLabel + [question.id]: { + ...prev[question.id], + selected: question.multiple + ? prev[question.id]?.selected.includes(optionLabel) + ? prev[question.id].selected.filter((value) => value !== optionLabel) + : [...(prev[question.id]?.selected ?? []), optionLabel] + : [optionLabel] } })) } @@ -227,7 +237,7 @@ export function RequestUserInputFooter(props: { <>
{currentQuestion.options.map((opt, optIdx) => { - const isSelected = currentState?.selected === opt.label + const isSelected = currentState?.selected.includes(opt.label) ?? false return ( selectOption(currentQuestion.id, opt.label)} + onClick={() => selectOption(currentQuestion, opt.label)} /> ) })} diff --git a/web/src/components/ToolCard/knownTools.test.tsx b/web/src/components/ToolCard/knownTools.test.tsx index 8328b848..8954403e 100644 --- a/web/src/components/ToolCard/knownTools.test.tsx +++ b/web/src/components/ToolCard/knownTools.test.tsx @@ -215,3 +215,45 @@ describe('getToolPresentation — Codex agent tools', () => { expect(presentation.minimal).toBe(true) }) }) + +describe('getToolPresentation — request_user_input', () => { + it('uses the question header instead of exposing its protocol id', () => { + const presentation = getToolPresentation({ + toolName: 'request_user_input', + input: { + questions: [{ + id: '__mcp_url_confirmation', + header: 'Sign in', + question: 'Sign in to continue' + }] + }, + result: null, + childrenCount: 0, + description: null, + metadata: null, + }) + + expect(presentation.title).toBe('Sign in') + expect(presentation.title).not.toContain('__mcp_url_confirmation') + expect(presentation.subtitle).toBe('Sign in to continue') + }) + + it('falls back to Question rather than exposing an id when no header is present', () => { + const presentation = getToolPresentation({ + toolName: 'request_user_input', + input: { + questions: [{ + id: '__mcp_form_confirmation', + question: 'Continue?' + }] + }, + result: null, + childrenCount: 0, + description: null, + metadata: null, + }) + + expect(presentation.title).toBe('Question') + expect(presentation.subtitle).toBe('Continue?') + }) +}) diff --git a/web/src/components/ToolCard/knownTools.tsx b/web/src/components/ToolCard/knownTools.tsx index c98d13fb..99bafe25 100644 --- a/web/src/components/ToolCard/knownTools.tsx +++ b/web/src/components/ToolCard/knownTools.tsx @@ -508,13 +508,13 @@ export const knownTools: Record 1) { return `${count} Questions` } - return id.length > 0 ? id : 'Question' + return header.length > 0 ? header : 'Question' }, subtitle: (opts) => { const questions = isObject(opts.input) && Array.isArray(opts.input.questions) diff --git a/web/src/components/ToolCard/requestUserInput.test.ts b/web/src/components/ToolCard/requestUserInput.test.ts new file mode 100644 index 00000000..8a11948d --- /dev/null +++ b/web/src/components/ToolCard/requestUserInput.test.ts @@ -0,0 +1,122 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { + isRequestUserInputQuestionAnswered, + isRequestUserInputUrlConfirmed, + formatRequestUserInputAnswers, + openRequestUserInputUrl, + parseRequestUserInputInput +} from './requestUserInput' + +describe('MCP URL request user input', () => { + afterEach(() => vi.restoreAllMocks()) + + it('only exposes http(s) URLs to the approval UI', () => { + expect(parseRequestUserInputInput({ url: 'https://example.com/login', questions: [] }).url) + .toBe('https://example.com/login') + expect(parseRequestUserInputInput({ url: 'javascript:alert(1)', questions: [] }).url) + .toBeNull() + }) + + it('reports popup failures instead of treating the URL as opened', () => { + const open = vi.spyOn(window, 'open').mockReturnValue(null) + expect(openRequestUserInputUrl('https://example.com/login')).toBe(false) + expect(open).toHaveBeenCalledWith('about:blank', '_blank') + }) + + it('severs opener access before navigating to an external MCP URL', () => { + const replace = vi.fn() + const popup = { + opener: window, + location: { replace } + } as unknown as Window + const open = vi.spyOn(window, 'open').mockReturnValue(popup) + + expect(openRequestUserInputUrl('https://example.com/login')).toBe(true) + expect(open).toHaveBeenCalledWith('about:blank', '_blank') + expect(popup.opener).toBeNull() + expect(replace).toHaveBeenCalledWith('https://example.com/login') + }) + + it('preserves optional form questions and allows them to stay empty', () => { + const parsed = parseRequestUserInputInput({ + questions: [{ id: 'comment', question: 'Comment', required: false, options: [] }] + }) + + expect(parsed.questions[0]).toEqual({ + id: 'comment', + question: 'Comment', + required: false, + multiple: false, + options: [] + }) + expect(isRequestUserInputQuestionAnswered(parsed.questions[0]!, { + selected: [], + userNote: '' + })).toBe(true) + }) + + it('requires an actual selection for required choice questions', () => { + const question = parseRequestUserInputInput({ + questions: [{ + id: 'approved', + question: 'Approved?', + required: true, + options: [{ label: 'true', description: '' }, { label: 'false', description: '' }] + }] + }).questions[0]! + + expect(isRequestUserInputQuestionAnswered(question, { + selected: [], + userNote: 'please approve' + })).toBe(false) + expect(isRequestUserInputQuestionAnswered(question, { + selected: ['true'], + userNote: 'please approve' + })).toBe(true) + }) + + it('opens an MCP URL only after selecting its explicit confirmation option', () => { + const url = 'https://example.com/login' + const hidden = parseRequestUserInputInput({ + url, + questions: [{ id: 'unrelated', question: 'Continue?', options: [] }] + }) + expect(isRequestUserInputUrlConfirmed(hidden, { + unrelated: { selected: [], userNote: 'yes' } + })).toBe(false) + + const explicit = parseRequestUserInputInput({ + url, + questions: [{ + id: '__mcp_url_confirmation', + question: 'Sign in', + options: [{ label: 'Open', description: url }] + }] + }) + expect(isRequestUserInputUrlConfirmed(explicit, { + __mcp_url_confirmation: { selected: [], userNote: '' } + })).toBe(false) + expect(isRequestUserInputUrlConfirmed(explicit, { + __mcp_url_confirmation: { selected: ['Open'], userNote: '' } + })).toBe(true) + }) + + it('serializes every selected value for multiple-choice questions', () => { + const parsed = parseRequestUserInputInput({ + questions: [{ + id: 'tags', + question: 'Tags', + required: true, + multiple: true, + options: [{ label: 'bug' }, { label: 'feature' }] + }] + }) + + expect(parsed.questions[0]?.multiple).toBe(true) + expect(formatRequestUserInputAnswers({ + tags: { selected: ['bug', 'feature'], userNote: '' } + })).toEqual({ + answers: { tags: { answers: ['bug', 'feature'] } } + }) + }) +}) diff --git a/web/src/components/ToolCard/requestUserInput.ts b/web/src/components/ToolCard/requestUserInput.ts index 19a7f87a..f5bf54c4 100644 --- a/web/src/components/ToolCard/requestUserInput.ts +++ b/web/src/components/ToolCard/requestUserInput.ts @@ -8,9 +8,21 @@ export type RequestUserInputOption = { export type RequestUserInputQuestion = { id: string question: string + required: boolean + multiple: boolean options: RequestUserInputOption[] } +export type RequestUserInputQuestionAnswer = { + selected: string[] + userNote: string +} + +export type ParsedRequestUserInput = { + questions: RequestUserInputQuestion[] + url: string | null +} + export type RequestUserInputQuestionInfo = { id: string question: string | null @@ -23,11 +35,34 @@ export function isRequestUserInputToolName(toolName: string): boolean { return toolName === 'request_user_input' } -export function parseRequestUserInputInput(input: unknown): { questions: RequestUserInputQuestion[] } { - if (!isObject(input)) return { questions: [] } +export function openRequestUserInputUrl(url: string): boolean { + const opened = window.open('about:blank', '_blank') + if (!opened) return false + try { + opened.opener = null + opened.location.replace(url) + return true + } catch { + opened.close() + return false + } +} + +export function parseRequestUserInputInput(input: unknown): ParsedRequestUserInput { + if (!isObject(input)) return { questions: [], url: null } + + let url: string | null = null + if (typeof input.url === 'string') { + try { + const parsed = new URL(input.url) + if (parsed.protocol === 'https:' || parsed.protocol === 'http:') url = parsed.toString() + } catch { + // Invalid and non-web URLs must never be opened by the approval UI. + } + } const rawQuestions = input.questions - if (!Array.isArray(rawQuestions)) return { questions: [] } + if (!Array.isArray(rawQuestions)) return { questions: [], url } const questions: RequestUserInputQuestion[] = [] for (const raw of rawQuestions) { @@ -52,11 +87,39 @@ export function parseRequestUserInputInput(input: unknown): { questions: Request questions.push({ id, question, + required: raw.required !== false, + multiple: raw.multiple === true, options }) } - return { questions } + return { questions, url } +} + +export function isRequestUserInputUrlConfirmed( + parsed: ParsedRequestUserInput, + answersByQuestion: Record +): boolean { + if (!parsed.url) return false + return parsed.questions.some((question) => { + if (question.id !== '__mcp_url_confirmation') return false + const selected = answersByQuestion[question.id]?.selected ?? [] + return question.options.some((option) => ( + selected.includes(option.label) && option.description === parsed.url + )) + }) +} + +export function isRequestUserInputQuestionAnswered( + question: RequestUserInputQuestion, + answer: RequestUserInputQuestionAnswer | undefined +): boolean { + if (!question.required) return true + if (!answer) return false + if (question.options.length > 0) { + return answer.selected.length > 0 + } + return answer.userNote.trim().length > 0 } export function extractRequestUserInputQuestionsInfo(input: unknown): RequestUserInputQuestionInfo[] | null { @@ -83,16 +146,14 @@ export function extractRequestUserInputQuestionsInfo(input: unknown): RequestUse * Format: { answers: { [id]: { answers: ["option", "user_note: note text"] } } } */ export function formatRequestUserInputAnswers( - answersByQuestion: Record + answersByQuestion: Record ): { answers: RequestUserInputAnswers } { const answers: RequestUserInputAnswers = {} for (const [id, answer] of Object.entries(answersByQuestion)) { const answerArray: string[] = [] - if (answer.selected) { - answerArray.push(answer.selected) - } + answerArray.push(...answer.selected) const note = answer.userNote.trim() if (note.length > 0) { @@ -110,13 +171,13 @@ export function formatRequestUserInputAnswers( */ export function parseRequestUserInputAnswers( answers: unknown -): Record | null { +): Record | null { if (!isObject(answers)) return null // Handle nested format: { answers: { [id]: { answers: string[] } } } const answersObj = isObject(answers.answers) ? answers.answers : answers - const parsed: Record = {} + const parsed: Record = {} for (const [id, value] of Object.entries(answersObj)) { let answerArray: string[] = [] @@ -127,15 +188,15 @@ export function parseRequestUserInputAnswers( answerArray = value.filter((a): a is string => typeof a === 'string') } - let selected: string | null = null + const selected: string[] = [] let userNote: string | null = null for (const item of answerArray) { if (item.startsWith('user_note: ')) { userNote = item.slice('user_note: '.length).trim() - } else if (!selected) { + } else { // Trim to match option labels which are also trimmed - selected = item.trim() + selected.push(item.trim()) } } diff --git a/web/src/components/ToolCard/views/RequestUserInputView.tsx b/web/src/components/ToolCard/views/RequestUserInputView.tsx index 45e1eb01..5b8a47e9 100644 --- a/web/src/components/ToolCard/views/RequestUserInputView.tsx +++ b/web/src/components/ToolCard/views/RequestUserInputView.tsx @@ -68,7 +68,7 @@ export function RequestUserInputView(props: ToolViewProps) { // Question with options
{q.options.map((opt, optIdx) => { - const isSelected = answer?.selected === opt.label + const isSelected = answer?.selected.includes(opt.label) ?? false return (