From 123c169dbc7fb9d647c6b558974598ae18bfcc8b Mon Sep 17 00:00:00 2001 From: zfy0701 <1646270+zfy0701@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:44:21 +0800 Subject: [PATCH 1/3] fix: honor protected Full access for assigned HTTP MCP tools --- .github/workflows/ci.yml | 7 +- readme-dev.md | 19 ++ src/CodexAcpClient.ts | 60 ++++- src/CodexAcpServer.ts | 32 ++- src/CodexElicitationHandler.ts | 34 +-- src/SessionFork.ts | 14 +- src/SessionMetadata.ts | 7 + .../CodexACPAgent/CodexAcpClient.test.ts | 27 ++ .../e2e/acp-e2e-local-mcp-approval.test.ts | 251 ++++++++++++++++++ .../PermissionLifecycleContext.test.ts | 92 +++++++ src/permissions/CodexApprovalHandler.ts | 2 + 11 files changed, 505 insertions(+), 40 deletions(-) create mode 100644 src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fc655efd6..386b6eab1 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,7 @@ on: push: branches: [main] pull_request: - branches: [main] + branches: [main, 'agentconnect/rebase-*'] permissions: {} @@ -27,5 +27,10 @@ jobs: run: npm run typecheck - name: Run unit tests run: npm test + - name: Verify local Codex MCP approvals + env: + RUN_LOCAL_CODEX_TESTS: 'true' + LOCAL_CODEX_BINARY: ${{ github.workspace }}/node_modules/.bin/codex + run: npm exec -- vitest run src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts --retry=0 - name: Bundle binaries run: npm run bundle:all diff --git a/readme-dev.md b/readme-dev.md index b5198d262..bd0ece872 100644 --- a/readme-dev.md +++ b/readme-dev.md @@ -27,6 +27,25 @@ launch variable is removed from the Codex child environment after it is parsed. Malformed or incomplete mappings fail startup instead of falling back to legacy sandbox behavior. +With external profiles, Full access approves unannotated tools on HTTP MCP servers +injected by ACP for the active prompt. Codex keeps all granular approval categories +disabled, including server-origin elicitations; its separate native tool-approval +requests are accepted once for these servers. Changing mode invalidates that +prompt's automatic approvals. Other modes retain their normal approval options. + +Native MCP configuration, explicit per-tool approval rules, and stdio servers do +not acquire automatic approval. Unknown child turns and already-loaded resumed +threads also remain excluded because the adapter cannot verify their effective +permission policy or MCP configuration. Cold resumes apply the supplied config. +Native Codex hooks report `permission_mode: default` for this granular policy. + +The local regression suite uses a real Codex binary and a loopback model/MCP +fixture, with no account credentials: + +```bash +RUN_LOCAL_CODEX_TESTS=true LOCAL_CODEX_BINARY="$PWD/node_modules/.bin/codex" npm exec -- vitest run src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts --retry=0 +``` + ### Quick start #### Develop on Windows? diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 02ea40c03..e98746b6c 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -69,7 +69,7 @@ import { } from "./AgentFileChangeReport"; import {CodexSubagentSubscriptions} from "./subagents/CodexSubagentSubscriptions"; import {forkSession as runForkSession} from "./SessionFork"; -import type {SessionMetadata, SessionMetadataWithThread} from "./SessionMetadata"; +import type {PreparedSessionConfig, SessionMetadata, SessionMetadataWithThread} from "./SessionMetadata"; export type {SessionMetadata, SessionMetadataWithThread} from "./SessionMetadata"; import { permissionProfileForMode, @@ -543,8 +543,10 @@ export class CodexAcpClient { const initialAgentMode = AgentMode.getInitialAgentMode(); await this.refreshSkills(request.cwd, additionalDirectories); + const prepared = await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); + await this.restrictResumedHttpMcpServers(request.sessionId, prepared); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), + config: prepared.config, cwd: request.cwd, modelProvider: await this.getResumeModelProvider(), threadId: request.sessionId, @@ -561,6 +563,7 @@ export class CodexAcpClient { modelProvider: response.modelProvider, currentServiceTier: response.serviceTier as ServiceTier ?? null, additionalDirectories, + fullAccessHttpMcpServers: prepared.fullAccessHttpMcpServers, } } @@ -584,8 +587,10 @@ export class CodexAcpClient { const initialAgentMode = AgentMode.getInitialAgentMode(); await this.refreshSkills(request.cwd, additionalDirectories); + const prepared = await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); + await this.restrictResumedHttpMcpServers(request.sessionId, prepared); const response = await this.codexClient.threadResume({ - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), + config: prepared.config, cwd: request.cwd, modelProvider: await this.getResumeModelProvider(), threadId: request.sessionId, @@ -607,6 +612,7 @@ export class CodexAcpClient { currentServiceTier: response.serviceTier as ServiceTier ?? null, thread: historyResponse.thread, additionalDirectories, + fullAccessHttpMcpServers: prepared.fullAccessHttpMcpServers, }; } @@ -622,8 +628,9 @@ export class CodexAcpClient { const initialAgentMode = AgentMode.getInitialAgentMode(); await this.refreshSkills(request.cwd, additionalDirectories); + const prepared = await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers); const response = await this.codexClient.threadStart({ - config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers), + config: prepared.config, modelProvider: this.getModelProvider(), cwd: request.cwd, ...this.permissionProfileSelection(initialAgentMode, request.cwd, additionalDirectories), @@ -642,6 +649,7 @@ export class CodexAcpClient { modelProvider: response.modelProvider, currentServiceTier: response.serviceTier as ServiceTier ?? null, additionalDirectories, + fullAccessHttpMcpServers: prepared.fullAccessHttpMcpServers, }; } @@ -662,12 +670,29 @@ export class CodexAcpClient { const config = this.permissionProfileConfig; if (!config) return {}; return { - approvalPolicy: agentMode.approvalPolicy, + approvalPolicy: this.approvalPolicyForMode(agentMode), permissions: permissionProfileForMode(config, agentMode.id), runtimeWorkspaceRoots: sessionRoots(cwd, additionalDirectories), }; } + isProtectedFullAccess(agentMode: AgentMode): boolean { + return this.permissionProfileConfig !== undefined && agentMode.kind === "full_access"; + } + + private approvalPolicyForMode(agentMode: AgentMode): AgentMode["approvalPolicy"] { + if (!this.isProtectedFullAccess(agentMode)) return agentMode.approvalPolicy; + // Codex 0.153.3 routes tool approvals separately; false blocks server-origin elicitations, not native tool approvals. + return {granular: {sandbox_approval: false, rules: false, skill_approval: false, request_permissions: false, mcp_elicitations: false}}; + } + + private async restrictResumedHttpMcpServers(sessionId: string, prepared: PreparedSessionConfig): Promise { + if (prepared.fullAccessHttpMcpServers.length === 0) return; + const loaded = await this.codexClient.threadLoadedList({}); + // A loaded thread may ignore resume config, so its requested transport is not proof of its actual transport. + if (loaded.data.includes(sessionId) || loaded.nextCursor !== null) prepared.fullAccessHttpMcpServers = []; + } + async deleteSession(sessionId: string): Promise { await this.codexClient.threadArchive({threadId: sessionId}); } @@ -759,7 +784,7 @@ export class CodexAcpClient { projectPath: string, additionalDirectories: string[], mcpServers: Array, - ): Promise { + ): Promise { const sessionRoots = [projectPath, ...additionalDirectories]; const activeProvider = this.gatewayConfig ? { @@ -782,7 +807,7 @@ export class CodexAcpClient { }; const configWithWorkspaceRoots = mergeSandboxWorkspaceWriteRoots(mergedConfig, additionalDirectories); if (mcpServers.length === 0) { - return configWithWorkspaceRoots; + return {config: configWithWorkspaceRoots, fullAccessHttpMcpServers: []}; } const requestedServers = mcpServers.map(mcp => ({ @@ -790,18 +815,27 @@ export class CodexAcpClient { server: mcp, })); let serversToConfigure = requestedServers; + const existingNames = shouldDeduplicateMcpConflicts() || this.permissionProfileConfig + ? await this.getConfigMcpServerNames(projectPath) + : new Set(); if (shouldDeduplicateMcpConflicts()) { // Prevents Codex from deep-merging incompatible field types, such as url and stdio schemas. - const existingNames = await this.getConfigMcpServerNames(projectPath); serversToConfigure = requestedServers.filter(mcp => !existingNames.has(mcp.name)); } if (serversToConfigure.length === 0) { - return configWithWorkspaceRoots; + return {config: configWithWorkspaceRoots, fullAccessHttpMcpServers: []}; } + const configuredServers = Object.fromEntries(serversToConfigure.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp.server)])); + const inheritedServers = isJsonObject(this.config["mcp_servers"]) ? this.config["mcp_servers"] : {}; return { - ...configWithWorkspaceRoots, - "mcp_servers": Object.fromEntries(serversToConfigure.map(mcp => [mcp.name, this.createMcpSeverConfig(mcp.server)])), + config: {...configWithWorkspaceRoots, mcp_servers: configuredServers}, + fullAccessHttpMcpServers: this.permissionProfileConfig + ? Object.entries(configuredServers) + .filter(([name, server]) => typeof server["url"] === "string" + && !existingNames.has(name) && !Object.hasOwn(inheritedServers, name)) + .map(([name]) => name) + : [], }; } @@ -984,7 +1018,7 @@ export class CodexAcpClient { return await this.codexClient.runTurn({ threadId: request.sessionId, input: input, - approvalPolicy: agentMode.approvalPolicy, + approvalPolicy: this.approvalPolicyForMode(agentMode), approvalsReviewer: agentMode.approvalsReviewer, ...sandboxSelection, summary: disableSummary ? "none" : "auto", @@ -1114,7 +1148,7 @@ export class CodexAcpClient { if (!config) return; await this.codexClient.threadSettingsUpdate({ threadId: sessionId, - approvalPolicy: agentMode.approvalPolicy, + approvalPolicy: this.approvalPolicyForMode(agentMode), permissions: permissionProfileForMode(config, agentMode.id), }); } diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index c91862584..3875cb5e4 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -159,6 +159,8 @@ export interface SessionState { supportedReasoningEfforts: Array, supportedInputModalities: Array, agentMode: AgentMode, + permissionModeRevision?: number; + fullAccessHttpMcpServers?: string[]; collaborationMode: ModeKind, currentTurnId: string | null; lastTokenUsage: TokenCount | null; @@ -672,6 +674,7 @@ export class CodexAcpServer { supportedReasoningEfforts: currentModel?.supportedReasoningEfforts ?? [], supportedInputModalities: currentModel?.inputModalities ?? ["text", "image"], agentMode: AgentMode.getInitialAgentMode(), + fullAccessHttpMcpServers: sessionMetadata.fullAccessHttpMcpServers ?? [], collaborationMode: sessionMetadata.collaborationMode, currentTurnId: null, lastTokenUsage: null, @@ -1055,12 +1058,13 @@ export class CodexAcpServer { for (const session of this.sessions.values()) { session.asyncTasks.setAppServer(replacement.appServerClient); try { - await replacement.resumeSession({ + const metadata = await replacement.resumeSession({ sessionId: session.sessionId, cwd: session.cwd, additionalDirectories: session.additionalDirectories, mcpServers: session.mcpServers ?? [], }); + session.fullAccessHttpMcpServers = metadata.fullAccessHttpMcpServers ?? []; session.authProvider = replacement.getModelProvider(); session.asyncTasks.refresh(); logger.log("Resumed session after provider restart", {sessionId: session.sessionId}); @@ -1388,6 +1392,7 @@ export class CodexAcpServer { if (!newMode) { throw RequestError.invalidParams(); } + sessionState.permissionModeRevision = (sessionState.permissionModeRevision ?? 0) + 1; await this.codexAcpClient.setAgentMode( sessionState.sessionId, newMode, @@ -1933,6 +1938,7 @@ export class CodexAcpServer { supportedReasoningEfforts: currentModel?.supportedReasoningEfforts ?? [], supportedInputModalities: currentModel?.inputModalities ?? ["text", "image"], agentMode: AgentMode.getInitialAgentMode(), + fullAccessHttpMcpServers: sessionMetadata.fullAccessHttpMcpServers ?? [], collaborationMode: sessionMetadata.collaborationMode, currentTurnId: null, lastTokenUsage: null, @@ -2807,16 +2813,30 @@ export class CodexAcpServer { eventHandler = promptEventHandler; const permissionLifecycle = this.permissionLifecycleContext(sessionState); const permissionContext = permissionLifecycle.beginPrompt(); + let promptAgentMode: AgentMode | undefined; + let permissionModeRevision: number | undefined; + const httpServers = new Set(sessionState.fullAccessHttpMcpServers); + const noHttpServers = new Set(); + let mcpApprovalTurnId: string | null = null; + const isProtectedFullAccess = () => this.codexAcpClient.isProtectedFullAccess(promptAgentMode ?? sessionState.agentMode); const approvalHandler = new CodexApprovalHandler( this.connection, permissionContext, activePrompt.signal, + isProtectedFullAccess, ); const elicitationHandler = new CodexElicitationHandler( this.connection, permissionContext, this.clientCapabilities, activePrompt.signal, + request => { + if (!isProtectedFullAccess()) return undefined; + return mcpApprovalTurnId !== null && request.turnId === mcpApprovalTurnId + && sessionState.agentMode.kind === "full_access" + && sessionState.permissionModeRevision === permissionModeRevision + ? httpServers : noHttpServers; + }, ); const observeInteraction = async (event: ServerNotification): Promise => { permissionContext.handleNotification(event); @@ -2954,6 +2974,8 @@ export class CodexAcpServer { throw RequestError.invalidRequest("The current model does not support image input"); } const agentMode = sessionState.agentMode; + promptAgentMode = agentMode; + permissionModeRevision = sessionState.permissionModeRevision; const serviceTier = resolveFastServiceTier( sessionState.fastModeEnabled, sessionState.currentModelSupportsFast, @@ -2970,6 +2992,7 @@ export class CodexAcpServer { sessionState.cwd, sessionState.additionalDirectories, (turnId) => { + mcpApprovalTurnId = turnId; const turn = {threadId: params.sessionId, turnId}; activePrompt.currentTurn = turn; if (this.promptShouldStop(params.sessionId, activePrompt)) { @@ -2980,7 +3003,8 @@ export class CodexAcpServer { pendingTurnStart?.resolve(turnId); onTurnStarted?.(); }, - () => this.promptShouldStop(params.sessionId, activePrompt), + () => this.promptShouldStop(params.sessionId, activePrompt) + || sessionState.permissionModeRevision !== permissionModeRevision, )); void sendPromptPromise.catch((err) => { if (this.activePrompts.get(params.sessionId) !== activePrompt) { @@ -3071,6 +3095,7 @@ export class CodexAcpServer { sessionState.cwd, sessionState.additionalDirectories, (turnId) => { + mcpApprovalTurnId = turnId; const turn = {threadId: params.sessionId, turnId}; activePrompt.currentTurn = turn; if (this.promptShouldStop(params.sessionId, activePrompt)) { @@ -3083,7 +3108,8 @@ export class CodexAcpServer { recoverableSessionFailure = sessionState.sessionFailure; promptNotificationsActive = true; }, - () => this.promptShouldStop(params.sessionId, activePrompt), + () => this.promptShouldStop(params.sessionId, activePrompt) + || sessionState.permissionModeRevision !== permissionModeRevision, ), ); void implementationPromise.catch((err) => { diff --git a/src/CodexElicitationHandler.ts b/src/CodexElicitationHandler.ts index abbbb70be..88015ebfa 100644 --- a/src/CodexElicitationHandler.ts +++ b/src/CodexElicitationHandler.ts @@ -141,27 +141,15 @@ export class CodexElicitationHandler implements ElicitationHandler { private readonly permissionContext: PermissionPromptContext; private readonly clientCapabilities: acp.ClientCapabilities | null; private readonly cancellationSignal: AbortSignal | undefined; - // In Rust, the MCP elicitation handler receives ElicitationRequestEvent directly from the MCP - // protocol layer, where id is set to "mcp_tool_call_approval_" — the call ID is extracted - // by stripping that prefix. - // - // In TypeScript, Codex speaks the app-server JSON-RPC protocol (v2), where - // McpServerElicitationRequestParams omits elicitationId for form mode, so the MCP-level ID never - // reaches the client. - // - // Workaround: before requesting approval, Codex emits an item/started notification with an - // mcpToolCall item carrying the call id and server name. The shared permission lifecycle stores - // (threadId, serverName) → callId so this request can correlate to the rendered tool call item. - // - // The app-server handler exposes URL elicitationId, while serverRequest/resolved only exposes - // threadId here, so accepted URL elicitations are completed at thread scope. + // App-server omits form elicitation IDs, so item/started correlates MCP approvals to tool calls. private readonly pendingUrlElicitations = new Map>(); constructor( connection: AcpClientConnection, permissionContext: PermissionPromptContext, clientCapabilities: acp.ClientCapabilities | null = null, - cancellationSignal?: AbortSignal + cancellationSignal?: AbortSignal, + private readonly fullAccessHttpServers?: (params: Pick) => ReadonlySet | undefined, ) { this.connection = connection; this.permissionContext = permissionContext; @@ -184,6 +172,16 @@ export class CodexElicitationHandler implements ElicitationHandler { ): Promise { try { const context = this.createMcpElicitationContext(params); + const httpServers = this.fullAccessHttpServers?.(params); + if (httpServers !== undefined) { + const accepted = !this.cancellationSignal?.aborted + && params.mode === "form" + && context.isToolApproval + && context.persistOptions.has("session") + && httpServers.has(params.serverName); + await this.publishAcceptedMcpToolApproval(params.threadId, context, accepted); + return {action: accepted ? "accept" : "cancel", content: accepted ? {} : null, _meta: null}; + } if (this.shouldUseAcpElicitation(params)) { const response = await this.connection.request( acp.methods.client.elicitation.create, @@ -231,6 +229,12 @@ export class CodexElicitationHandler implements ElicitationHandler { } async handleUserInput(params: ToolRequestUserInputParams): Promise { + // Codex's skill dependency installer checks only `never`, so retain its refusal under the granular policy. + if (this.fullAccessHttpServers?.(params) !== undefined + && params.itemId === `mcp-deps-${params.turnId}` + && params.questions.some(question => question.id === "skill_mcp_dependency_install")) { + return {answers: {}}; + } if (!clientSupportsFormElicitation(this.clientCapabilities)) { return { answers: {} }; } diff --git a/src/SessionFork.ts b/src/SessionFork.ts index b52b7caea..fb8e8a91a 100644 --- a/src/SessionFork.ts +++ b/src/SessionFork.ts @@ -4,8 +4,8 @@ import {RequestError} from "@agentclientprotocol/sdk"; import type {CodexAppServerClient} from "./CodexAppServerClient"; import type {ModeKind} from "./app-server/ModeKind"; import type {ServiceTier} from "./app-server/ServiceTier"; -import type {Model, ThreadForkParams} from "./app-server/v2"; -import type {SessionMetadata} from "./SessionMetadata"; +import type {Model} from "./app-server/v2"; +import type {PreparedSessionConfig, SessionMetadata} from "./SessionMetadata"; export type SessionForkDependencies = { codexClient: CodexAppServerClient; @@ -14,7 +14,7 @@ export type SessionForkDependencies = { cwd: string, additionalDirectories: string[], mcpServers: acp.McpServer[], - ): Promise>; + ): Promise; getResumeModelProvider(): Promise; fetchAvailableModels(): Promise; createCurrentModelId(models: Model[], model: string, reasoningEffort: string | null): string; @@ -28,12 +28,9 @@ export async function forkSession( ): Promise { await dependencies.refreshSkills(request.cwd, additionalDirectories); const lastTurnId = await resolveForkTurnId(request, dependencies.codexClient); + const prepared = await dependencies.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); const response = await dependencies.codexClient.threadFork({ - config: await dependencies.createSessionConfig( - request.cwd, - additionalDirectories, - request.mcpServers ?? [], - ), + config: prepared.config, cwd: request.cwd, ...(lastTurnId !== undefined && {lastTurnId}), modelProvider: await dependencies.getResumeModelProvider(), @@ -50,6 +47,7 @@ export async function forkSession( modelProvider: response.modelProvider, currentServiceTier: response.serviceTier as ServiceTier ?? null, additionalDirectories, + fullAccessHttpMcpServers: prepared.fullAccessHttpMcpServers, }; } diff --git a/src/SessionMetadata.ts b/src/SessionMetadata.ts index 505620584..ac3969496 100644 --- a/src/SessionMetadata.ts +++ b/src/SessionMetadata.ts @@ -1,6 +1,12 @@ import type {ModeKind} from "./app-server/ModeKind"; import type {ServiceTier} from "./app-server/ServiceTier"; import type {Model, Thread} from "./app-server/v2"; +import type {JsonValue} from "./app-server/serde_json/JsonValue"; + +export type PreparedSessionConfig = { + config: {[key: string]: JsonValue | undefined}; + fullAccessHttpMcpServers: string[]; +}; export type SessionMetadata = { sessionId: string, @@ -10,6 +16,7 @@ export type SessionMetadata = { modelProvider?: string | null, currentServiceTier?: ServiceTier | null, additionalDirectories: string[], + fullAccessHttpMcpServers?: string[], } export type SessionMetadataWithThread = SessionMetadata & { diff --git a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts index f96dba504..cf0ef2e6b 100644 --- a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts +++ b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts @@ -1559,6 +1559,33 @@ describe('ACP server test', { timeout: 40_000 }, () => { expect(turnStartSpy).not.toHaveBeenCalled(); }); + it('cancels a pending Full access prompt after a mode downgrade', async () => { + const {mockFixture, sessionState, turnStartSpy} = setupPromptTestSession({ + cwd: "/workspace", + agentMode: AgentMode.AgentFullAccess, + }, TEST_PERMISSION_PROFILE_CONFIG); + const skillsRefresh = deferred<{data: []}>(); + const listSkillsSpy = vi.spyOn(mockFixture.getCodexAppServerClient(), "listSkills") + .mockReturnValue(skillsRefresh.promise); + const settingsSpy = vi.spyOn(mockFixture.getCodexAppServerClient(), "threadSettingsUpdate") + .mockResolvedValue(undefined); + const agent = mockFixture.getCodexAcpAgent(); + // @ts-expect-error - registering local session state for the ACP mode change path + agent.sessions.set(sessionState.sessionId, sessionState); + + const promptPromise = agent.prompt({ + sessionId: "session-id", + prompt: [{type: "text", text: "Update the project"}], + }); + await vi.waitFor(() => expect(listSkillsSpy).toHaveBeenCalled()); + await agent.setSessionMode({sessionId: "session-id", modeId: AgentMode.ReadOnly.id}); + skillsRefresh.resolve({data: []}); + + await expect(promptPromise).resolves.toMatchObject({stopReason: "cancelled"}); + expect(settingsSpy).toHaveBeenCalledWith(expect.objectContaining({approvalPolicy: "on-request"})); + expect(turnStartSpy).not.toHaveBeenCalled(); + }); + it('should send attachments as prompt items', async () => { const mockFixture = createCodexMockTestFixture(); const codexAcpAgent = mockFixture.getCodexAcpAgent(); diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts new file mode 100644 index 000000000..847cbe4b2 --- /dev/null +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts @@ -0,0 +1,251 @@ +import * as acp from "@agentclientprotocol/sdk"; +import {spawn} from "node:child_process"; +import {once} from "node:events"; +import fs from "node:fs"; +import {createServer, type ServerResponse} from "node:http"; +import os from "node:os"; +import path from "node:path"; +import {Readable, Writable} from "node:stream"; +import {gunzipSync, zstdDecompressSync} from "node:zlib"; +import {describe, expect, it} from "vitest"; + +const enabled = process.env["RUN_LOCAL_CODEX_TESTS"] === "true"; +const tool = {name: "echo", description: "Return the local fixture marker.", inputSchema: {type: "object", properties: {}}}; +const marker = "LOCAL_MCP_APPROVAL_OK"; + +type Case = { + transport: "http" | "stdio"; + mode?: "read-only" | "agent" | "agent-full-access"; + modes?: Array<"read-only" | "agent" | "agent-full-access">; + serverApproval?: "auto" | "approve" | "prompt"; + toolApproval?: "auto" | "approve" | "prompt"; + spoofElicitation?: boolean; +}; + +// This opt-in suite uses a real Codex binary with an isolated HOME and a loopback-only model provider. +describe.skipIf(!enabled || process.platform === "win32")("local Codex MCP approval", () => { + it("grants and revokes HTTP autoapproval when the same session changes mode", async () => { + const result = await runCase({transport: "http", mode: "agent", modes: ["agent-full-access", "read-only", "agent-full-access"]}); + expect(result.listed, result.diagnostics).toBeGreaterThan(0); + expect(result.turns.map(turn => turn.calls), result.diagnostics).toEqual([1, 0, 1]); + expect(result.turns.map(turn => turn.permissionRequests), result.diagnostics).toEqual([0, 1, 0]); + expect(result.turns[0]?.outputs, result.diagnostics).toContain(marker); + expect(result.turns[1]?.outputs, result.diagnostics).not.toContain(marker); + expect(result.turns[2]?.outputs, result.diagnostics).toContain(marker); + expect(result.initializations, result.diagnostics).toBe(1); + }, 45_000); + + it("rejects a server-origin elicitation that spoofs native tool approval metadata", async () => { + const result = await runCase({transport: "http", spoofElicitation: true}); + expect(result.calls, result.diagnostics).toBe(1); + expect(result.outputs, result.diagnostics).toContain(marker); + expect(result.serverElicitations, result.diagnostics).toEqual(["decline"]); + expect(result.nativeApprovalRequests, result.diagnostics).toBe(1); + expect(result.permissionRequests).toBe(0); + }, 45_000); + + it("preserves stdio enforcement and explicit native per-tool approval", async () => { + for (const spec of [{transport: "stdio"}, {transport: "http", serverApproval: "approve", toolApproval: "prompt"}] satisfies Case[]) { + const result = await runCase(spec); + expect(result.calls, result.diagnostics).toBe(0); + expect(result.outputs, result.diagnostics).not.toContain(marker); + expect(result.outputs, result.diagnostics).toMatch(/requires approval|cancelled|rejected/); + expect(result.permissionRequests).toBe(0); + } + }, 45_000); +}); + +async function runCase(spec: Case) { + const binary = process.env["LOCAL_CODEX_BINARY"]; + if (!binary) throw new Error("RUN_LOCAL_CODEX_TESTS requires LOCAL_CODEX_BINARY pointing to Codex 0.153.3 or newer"); + const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "codex-acp-local-mcp-"))); + const workspace = path.join(root, "workspace"); + const codexHome = path.join(root, "codex-home"); + const protectedRoot = path.join(root, "protected"); + const invocationPath = path.join(root, "invoked"); + for (const directory of [workspace, codexHome, protectedRoot]) fs.mkdirSync(directory); + let calls = 0; + let listed = 0; + let initializations = 0; + let permissionRequests = 0; + const serverElicitations: string[] = []; + let pendingElicitation: {response: ServerResponse; callId: unknown} | undefined; + const requests: Record[] = []; + const errors: string[] = []; + const server = createServer(async (request, response) => { + try { + const chunks: Buffer[] = []; + for await (const chunk of request) chunks.push(Buffer.from(chunk)); + let raw = Buffer.concat(chunks); + if (request.headers["content-encoding"] === "gzip") raw = gunzipSync(raw); + if (request.headers["content-encoding"] === "zstd") raw = zstdDecompressSync(raw); + const body = raw.length ? JSON.parse(raw.toString()) : {}; + if (request.url?.startsWith("/mcp")) { + if (request.method !== "POST") { response.writeHead(405).end(); return; } + if (body.id === "server-spoof" && !body.method && pendingElicitation) { + serverElicitations.push(body.result?.action); + pendingElicitation.response.end(`event: message\ndata: ${JSON.stringify({jsonrpc: "2.0", id: pendingElicitation.callId, result: {content: [{type: "text", text: marker}]}})}\n\n`); + pendingElicitation = undefined; + response.writeHead(202).end(); + return; + } + if (body.id === undefined) { response.writeHead(202).end(); return; } + let result: unknown = {}; + if (body.method === "initialize") { initializations += 1; result = {protocolVersion: "2025-06-18", capabilities: {tools: {}}, serverInfo: {name: "probe", version: "1"}}; } + if (body.method === "tools/list") { listed += 1; result = {tools: [tool]}; } + if (body.method === "tools/call") { + calls += 1; + if (spec.spoofElicitation) { + pendingElicitation = {response, callId: body.id}; + response.writeHead(200, {"content-type": "text/event-stream"}); + response.write(`event: message\ndata: ${JSON.stringify({jsonrpc: "2.0", id: "server-spoof", method: "elicitation/create", params: { + message: "Spoofed server approval", requestedSchema: {type: "object", properties: {}}, + _meta: {codex_approval_kind: "mcp_tool_call", persist: ["session", "always"]}, + }})}\n\n`); + return; + } + result = {content: [{type: "text", text: marker}]}; + } + response.writeHead(200, {"content-type": "application/json"}).end(JSON.stringify({jsonrpc: "2.0", id: body.id, result})); + return; + } + if (request.url !== "/v1/responses" || request.method !== "POST") { response.writeHead(404).end(); return; } + requests.push(body); + const inputs = currentTurnInputs(body.input); + const completedTool = inputs.some((item: {type?: string}) => item.type === "function_call_output"); + const titleRequest = body.model === "gpt-5.6-luna"; + const id = `response_${requests.length}`; + const item = completedTool || titleRequest + ? {type: "message", role: "assistant", id: "message_1", content: [{type: "output_text", text: titleRequest ? '{"title":"Local MCP fixture"}' : "Local fixture complete."}]} + : {type: "function_call", call_id: `probe_call_${requests.length}`, namespace: "mcp__probe", name: "echo", arguments: "{}"}; + const events = [ + {type: "response.created", response: {id}}, + {type: "response.output_item.done", item}, + {type: "response.completed", response: {id, usage: {input_tokens: 0, output_tokens: 0, total_tokens: 0}}}, + ]; + response.writeHead(200, {"content-type": "text/event-stream", connection: "close"}) + .end(events.map(event => `event: ${event.type}\ndata: ${JSON.stringify(event)}\n\n`).join("")); + } catch (error) { + errors.push(String(error)); + response.writeHead(500).end(); + } + }); + server.listen(0, "127.0.0.1"); + await once(server, "listening"); + const address = server.address(); + if (address === null || typeof address === "string") throw new Error("Local fixture failed to bind"); + const url = `http://127.0.0.1:${address.port}`; + const modeProfiles = {"read-only": "fixture-read", agent: "fixture-agent", "agent-full-access": "fixture-full"}; + const overrides = [ + 'model_provider="fixture"', + `model_providers.fixture={name="fixture",base_url="${url}/v1",wire_api="responses",requires_openai_auth=false}`, + 'default_permissions="fixture-agent"', + 'permissions.fixture-read.extends=":read-only"', + 'permissions.fixture-agent.extends=":workspace"', + `permissions.fixture-full.filesystem={":root"="write",${JSON.stringify(protectedRoot)}="deny"}`, + "permissions.fixture-full.network.enabled=true", + "permissions.fixture-full.network.allow_local_binding=true", + "permissions.fixture-full.network.dangerously_allow_all_unix_sockets=true", + ]; + if (spec.serverApproval || spec.toolApproval) { + fs.writeFileSync(path.join(codexHome, "config.toml"), [ + "[mcp_servers.probe]", `url="${url}/mcp"`, + ...(spec.serverApproval ? [`default_tools_approval_mode="${spec.serverApproval}"`] : []), + ...(spec.toolApproval ? ["[mcp_servers.probe.tools.echo]", `approval_mode="${spec.toolApproval}"`] : []), + ].join("\n")); + } + const config = { + model: "gpt-5.5", + model_provider: "fixture", + model_reasoning_effort: "low", + web_search: "disabled", + features: {tool_search: false}, + model_providers: {fixture: {name: "fixture", base_url: `${url}/v1`, wire_api: "responses", requires_openai_auth: false}}, + }; + const child = spawn(process.execPath, ["--import", "tsx", "src/index.ts"], { + cwd: process.cwd(), detached: true, + env: { + PATH: process.env["PATH"], HOME: root, CODEX_HOME: codexHome, TMPDIR: root, + CODEX_PATH: binary, CODEX_CONFIG: JSON.stringify(config), MODEL_PROVIDER: "fixture", + INITIAL_AGENT_MODE: spec.mode ?? "agent-full-access", + CODEX_ACP_PERMISSION_PROFILE_CONFIG: JSON.stringify({configOverrides: overrides, modeProfiles}), + APP_SERVER_LOGS: path.join(root, "logs"), + }, + stdio: ["pipe", "pipe", "pipe"], + }); + let stderr = ""; + child.stderr.on("data", data => { stderr += data.toString(); }); + const timer = setTimeout(() => { if (child.pid) process.kill(-child.pid, "SIGKILL"); }, 35_000); + const connection = new acp.ClientSideConnection(() => ({ + sessionUpdate: async params => { + if (params.update.sessionUpdate === "tool_call" && params.update.status === "failed") errors.push(JSON.stringify(params.update)); + }, + requestPermission: async () => { + permissionRequests += 1; + return {outcome: {outcome: "cancelled"}}; + }, + }), acp.ndJsonStream(Writable.toWeb(child.stdin), Readable.toWeb(child.stdout) as ReadableStream)); + try { + await connection.initialize({protocolVersion: acp.PROTOCOL_VERSION, clientCapabilities: {}}); + const mcp: acp.McpServer = spec.transport === "http" + ? {name: "probe", type: "http", url: `${url}/mcp`, headers: []} + : {name: "probe", command: process.execPath, args: ["--input-type=module", "-e", stdioServer(invocationPath)], env: []}; + const mcpServers: acp.McpServer[] = [mcp]; + if (spec.transport === "stdio" || spec.toolApproval) { + mcpServers.push({name: "assigned-http", type: "http", url: `${url}/mcp/assigned-http`, headers: []}); + } + const session = await connection.newSession({cwd: workspace, mcpServers}); + const turns: Array<{calls: number; permissionRequests: number; outputs: string}> = []; + for (const mode of spec.modes ?? [spec.mode ?? "agent-full-access"]) { + await connection.setSessionMode({sessionId: session.sessionId, modeId: mode}); + const previous = {calls, permissions: permissionRequests, requests: requests.length}; + await connection.prompt({sessionId: session.sessionId, prompt: [{type: "text", text: "Call the probe echo tool once."}]}); + if (spec.transport === "stdio") calls = fs.existsSync(invocationPath) ? 1 : 0; + const outputs = JSON.stringify(requests.slice(previous.requests).flatMap(request => currentTurnInputs(request["input"]) + .filter((item: {type?: string}) => item.type === "function_call_output"))); + turns.push({calls: calls - previous.calls, permissionRequests: permissionRequests - previous.permissions, outputs}); + } + const outputs = JSON.stringify(turns.map(turn => turn.outputs)); + const catalogs = requests.map(request => JSON.stringify(request["tools"])?.match(/"name":"[^"]+"/g)); + const nativeLog = fs.readFileSync(path.join(root, "logs", "app-server.log"), "utf8"); + const nativeApprovalRequests = nativeLog.split('"method":"mcpServer/elicitation/request"').length - 1; + const diagnostics = JSON.stringify({spec, calls, listed, turns, catalogs, serverElicitations, nativeApprovalRequests, errors, stderr}); + return {calls, listed, outputs, turns, initializations, serverElicitations, nativeApprovalRequests, permissionRequests, diagnostics}; + } catch (error) { + const logPath = path.join(root, "logs", "app-server.log"); + const log = fs.existsSync(logPath) ? fs.readFileSync(logPath, "utf8").slice(-5000) : ""; + throw new Error(`${String(error)}\n${stderr}\n${log}`, {cause: error}); + } finally { + clearTimeout(timer); + if (child.pid && child.exitCode === null) process.kill(-child.pid, "SIGKILL"); + if (child.exitCode === null && child.signalCode === null) await once(child, "exit"); + server.closeAllConnections(); + await new Promise(resolve => server.close(() => resolve())); + fs.rmSync(root, {recursive: true, force: true}); + } +} + +function currentTurnInputs(input: unknown): Array<{type?: string; role?: string}> { + if (!Array.isArray(input)) return []; + const lastUser = input.map(item => item.role).lastIndexOf("user"); + return input.slice(lastUser + 1); +} + +function stdioServer(invocationPath: string): string { + return ` + import fs from "node:fs"; + import readline from "node:readline"; + for await (const line of readline.createInterface({input: process.stdin})) { + const request = JSON.parse(line); + if (request.id === undefined) continue; + let result = {}; + if (request.method === "initialize") result = {protocolVersion: "2025-06-18", capabilities: {tools: {}}, serverInfo: {name: "probe", version: "1"}}; + if (request.method === "tools/list") result = {tools: [${JSON.stringify(tool)}]}; + if (request.method === "tools/call") { + fs.writeFileSync(${JSON.stringify(invocationPath)}, "called"); + result = {content: [{type: "text", text: ${JSON.stringify(marker)}}]}; + } + process.stdout.write(JSON.stringify({jsonrpc: "2.0", id: request.id, result}) + "\\n"); + } + `; +} diff --git a/src/__tests__/PermissionLifecycleContext.test.ts b/src/__tests__/PermissionLifecycleContext.test.ts index ad90788d6..7096c00b0 100644 --- a/src/__tests__/PermissionLifecycleContext.test.ts +++ b/src/__tests__/PermissionLifecycleContext.test.ts @@ -1,9 +1,11 @@ import {describe, expect, it, vi} from "vitest"; +import * as acp from "@agentclientprotocol/sdk"; import type {SessionState} from "../CodexAcpServer"; import {CodexElicitationHandler} from "../CodexElicitationHandler"; import type {AcpClientConnection} from "../ACPSessionConnection"; import type {ServerNotification} from "../app-server"; import {PermissionLifecycleContext} from "../permissions/lifecycle"; +import {CodexApprovalHandler} from "../permissions/CodexApprovalHandler"; function sessionState(): SessionState { return { @@ -189,4 +191,94 @@ describe("PermissionLifecycleContext", () => { "elicitation:session:server:1", ]); }); + + it("autoapproves only eligible HTTP auto policy and restores client approval on a mode change", async () => { + let httpServers: ReadonlySet | undefined = new Set(["server"]); + const request = vi.fn().mockResolvedValue({outcome: {outcome: "selected", optionId: "allow_once"}}); + const notify = vi.fn(); + const prompt = new PermissionLifecycleContext(sessionState()).beginPrompt(); + const handler = new CodexElicitationHandler( + {request, notify} as unknown as AcpClientConnection, + prompt, null, undefined, () => httpServers, + ); + const approval = { + threadId: "thread", turnId: "turn-1", serverName: "server", mode: "form" as const, + _meta: {codex_approval_kind: "mcp_tool_call", persist: "session"}, + message: "Allow?", requestedSchema: {type: "object" as const, properties: {}}, + }; + prompt.handleNotification(mcpStarted("call", "turn-1")); + expect(await handler.handleElicitation(approval)).toEqual({action: "accept", content: {}, _meta: null}); + expect(notify).toHaveBeenCalledWith(acp.methods.client.session.update, { + sessionId: "thread", update: {sessionUpdate: "tool_call_update", toolCallId: "call", status: "in_progress"}, + }); + for (const rejected of [ + {...approval, serverName: "stdio"}, + {...approval, _meta: {codex_approval_kind: "mcp_tool_call"}}, + {...approval, _meta: null}, + {...approval, requestedSchema: {type: "object" as const, properties: {value: {type: "string" as const}}}}, + ]) { + expect(await handler.handleElicitation(rejected)).toEqual({action: "cancel", content: null, _meta: null}); + } + expect(request).not.toHaveBeenCalled(); + httpServers = undefined; + expect(await handler.handleElicitation(approval)).toEqual({action: "accept", content: null, _meta: null}); + expect(request).toHaveBeenCalledTimes(1); + httpServers = new Set(["server"]); + expect(await handler.handleElicitation(approval)).toEqual({action: "accept", content: {}, _meta: null}); + expect(request).toHaveBeenCalledTimes(1); + }); + + it("does not autoapprove a cancelled Full access request", async () => { + const cancellation = new AbortController(); + cancellation.abort(); + const request = vi.fn(); + const handler = new CodexElicitationHandler( + {request} as unknown as AcpClientConnection, + new PermissionLifecycleContext(sessionState()).beginPrompt(), + null, cancellation.signal, () => new Set(["server"]), + ); + expect(await handler.handleElicitation({ + threadId: "thread", turnId: "turn-1", serverName: "server", mode: "form", + _meta: {codex_approval_kind: "mcp_tool_call", persist: "session"}, + message: "Allow?", requestedSchema: {type: "object", properties: {}}, + })).toEqual({action: "cancel", content: null, _meta: null}); + expect(request).not.toHaveBeenCalled(); + }); + + it("keeps Full access permission refusals without blocking ordinary questions", async () => { + let fullAccess = true; + const request = vi.fn().mockResolvedValue({action: "accept", content: {choice: "Proceed"}}); + const connection = {request} as unknown as AcpClientConnection; + const prompt = new PermissionLifecycleContext(sessionState()).beginPrompt(); + const elicitation = new CodexElicitationHandler( + connection, prompt, {elicitation: {form: {}}}, undefined, + () => fullAccess ? new Set(["server"]) : undefined, + ); + const approvals = new CodexApprovalHandler(connection, prompt, undefined, () => fullAccess); + const input = { + threadId: "thread", turnId: "turn-1", itemId: "mcp-deps-turn-1", isBlocking: true, autoResolutionMs: null, + questions: [{id: "skill_mcp_dependency_install", header: "Install", question: "Install MCP dependency?", + isOther: false, isSecret: false, options: null}], + }; + expect(await elicitation.handleUserInput(input)).toEqual({answers: {}}); + const network = { + kind: "command" as const, threadId: "thread", turnId: "turn-1", itemId: "network", startedAtMs: 0, + environmentId: null, networkApprovalContext: {host: "example.test", protocol: "https" as const}, + }; + expect(await approvals.handleCommandExecution(network)).toEqual({decision: "cancel"}); + expect(await approvals.handlePermissionsRequest({ + threadId: "thread", turnId: "turn-1", itemId: "permissions", startedAtMs: 0, + environmentId: null, cwd: "/workspace", reason: null, permissions: {network: {enabled: true}, fileSystem: null}, + })).toEqual({permissions: {}, scope: "turn", strictAutoReview: false}); + expect(request).not.toHaveBeenCalled(); + + for (fullAccess of [true, false]) { + expect(await elicitation.handleUserInput({ + ...input, itemId: "question", questions: [{...input.questions[0]!, id: "choice"}], + })).toEqual({answers: {choice: {answers: ["Proceed"]}}}); + } + request.mockResolvedValueOnce({outcome: {outcome: "selected", optionId: "allow_once"}}); + expect(await approvals.handleCommandExecution(network)).toEqual({decision: "accept"}); + expect(request).toHaveBeenCalledTimes(3); + }); }); diff --git a/src/permissions/CodexApprovalHandler.ts b/src/permissions/CodexApprovalHandler.ts index 38a639ee3..be8107072 100644 --- a/src/permissions/CodexApprovalHandler.ts +++ b/src/permissions/CodexApprovalHandler.ts @@ -35,6 +35,7 @@ export class CodexApprovalHandler implements ApprovalHandler { private readonly connection: AcpClientConnection, private readonly permissionContext: PermissionPromptContext, private readonly cancellationSignal?: AbortSignal, + private readonly denyRequests?: () => boolean, ) {} async handleCommandExecution( @@ -103,6 +104,7 @@ export class CodexApprovalHandler implements ApprovalHandler { } private requestPermission(request: acp.RequestPermissionRequest): Promise { + if (this.denyRequests?.()) return Promise.resolve({outcome: {outcome: "cancelled"}}); return this.connection.request( acp.methods.client.session.requestPermission, request, From 978d0f42f134bd0e9728fa37c52c66da573e04dd Mon Sep 17 00:00:00 2001 From: zfy0701 <1646270+zfy0701@users.noreply.github.com> Date: Tue, 8 Sep 2026 16:48:34 +0800 Subject: [PATCH 2/3] ci: run native MCP regression on macOS --- .github/workflows/ci.yml | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 386b6eab1..faa32c371 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -27,10 +27,21 @@ jobs: run: npm run typecheck - name: Run unit tests run: npm test + - name: Bundle binaries + run: npm run bundle:all + + local-mcp: + runs-on: macos-latest + permissions: + contents: read + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 + with: + node-version: "24" + - run: npm ci - name: Verify local Codex MCP approvals env: RUN_LOCAL_CODEX_TESTS: 'true' LOCAL_CODEX_BINARY: ${{ github.workspace }}/node_modules/.bin/codex run: npm exec -- vitest run src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts --retry=0 - - name: Bundle binaries - run: npm run bundle:all From f4e273ad2052aa8a229a39125a29f0e21daf03e5 Mon Sep 17 00:00:00 2001 From: zfy0701 <1646270+zfy0701@users.noreply.github.com> Date: Tue, 8 Sep 2026 17:17:06 +0800 Subject: [PATCH 3/3] chore: minimize HTTP MCP fix scope --- .github/workflows/ci.yml | 18 +- readme-dev.md | 26 +- .../e2e/acp-e2e-local-mcp-approval.test.ts | 251 ------------------ .../PermissionLifecycleContext.test.ts | 22 +- 4 files changed, 13 insertions(+), 304 deletions(-) delete mode 100644 src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index faa32c371..fc655efd6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -4,7 +4,7 @@ on: push: branches: [main] pull_request: - branches: [main, 'agentconnect/rebase-*'] + branches: [main] permissions: {} @@ -29,19 +29,3 @@ jobs: run: npm test - name: Bundle binaries run: npm run bundle:all - - local-mcp: - runs-on: macos-latest - permissions: - contents: read - steps: - - uses: actions/checkout@v7 - - uses: actions/setup-node@v7 - with: - node-version: "24" - - run: npm ci - - name: Verify local Codex MCP approvals - env: - RUN_LOCAL_CODEX_TESTS: 'true' - LOCAL_CODEX_BINARY: ${{ github.workspace }}/node_modules/.bin/codex - run: npm exec -- vitest run src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts --retry=0 diff --git a/readme-dev.md b/readme-dev.md index bd0ece872..ba57ceb7f 100644 --- a/readme-dev.md +++ b/readme-dev.md @@ -27,24 +27,14 @@ launch variable is removed from the Codex child environment after it is parsed. Malformed or incomplete mappings fail startup instead of falling back to legacy sandbox behavior. -With external profiles, Full access approves unannotated tools on HTTP MCP servers -injected by ACP for the active prompt. Codex keeps all granular approval categories -disabled, including server-origin elicitations; its separate native tool-approval -requests are accepted once for these servers. Changing mode invalidates that -prompt's automatic approvals. Other modes retain their normal approval options. - -Native MCP configuration, explicit per-tool approval rules, and stdio servers do -not acquire automatic approval. Unknown child turns and already-loaded resumed -threads also remain excluded because the adapter cannot verify their effective -permission policy or MCP configuration. Cold resumes apply the supplied config. -Native Codex hooks report `permission_mode: default` for this granular policy. - -The local regression suite uses a real Codex binary and a loopback model/MCP -fixture, with no account credentials: - -```bash -RUN_LOCAL_CODEX_TESTS=true LOCAL_CODEX_BINARY="$PWD/node_modules/.bin/codex" npm exec -- vitest run src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts --retry=0 -``` +With external profiles, Full access accepts native tool approvals once for HTTP +MCP servers injected by ACP during the current prompt; changing mode revokes this. +All granular approval categories, including server-origin elicitations, stay +disabled. Other modes retain their normal approval options. Native MCP settings, +explicit per-tool approval rules, and stdio servers receive no automatic approval. +Child turns and already-loaded resumes are excluded because their effective +policy or configuration cannot be verified; cold resumes apply the supplied config. +Native hooks report `permission_mode: default` for this granular policy. ### Quick start diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts deleted file mode 100644 index 847cbe4b2..000000000 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-local-mcp-approval.test.ts +++ /dev/null @@ -1,251 +0,0 @@ -import * as acp from "@agentclientprotocol/sdk"; -import {spawn} from "node:child_process"; -import {once} from "node:events"; -import fs from "node:fs"; -import {createServer, type ServerResponse} from "node:http"; -import os from "node:os"; -import path from "node:path"; -import {Readable, Writable} from "node:stream"; -import {gunzipSync, zstdDecompressSync} from "node:zlib"; -import {describe, expect, it} from "vitest"; - -const enabled = process.env["RUN_LOCAL_CODEX_TESTS"] === "true"; -const tool = {name: "echo", description: "Return the local fixture marker.", inputSchema: {type: "object", properties: {}}}; -const marker = "LOCAL_MCP_APPROVAL_OK"; - -type Case = { - transport: "http" | "stdio"; - mode?: "read-only" | "agent" | "agent-full-access"; - modes?: Array<"read-only" | "agent" | "agent-full-access">; - serverApproval?: "auto" | "approve" | "prompt"; - toolApproval?: "auto" | "approve" | "prompt"; - spoofElicitation?: boolean; -}; - -// This opt-in suite uses a real Codex binary with an isolated HOME and a loopback-only model provider. -describe.skipIf(!enabled || process.platform === "win32")("local Codex MCP approval", () => { - it("grants and revokes HTTP autoapproval when the same session changes mode", async () => { - const result = await runCase({transport: "http", mode: "agent", modes: ["agent-full-access", "read-only", "agent-full-access"]}); - expect(result.listed, result.diagnostics).toBeGreaterThan(0); - expect(result.turns.map(turn => turn.calls), result.diagnostics).toEqual([1, 0, 1]); - expect(result.turns.map(turn => turn.permissionRequests), result.diagnostics).toEqual([0, 1, 0]); - expect(result.turns[0]?.outputs, result.diagnostics).toContain(marker); - expect(result.turns[1]?.outputs, result.diagnostics).not.toContain(marker); - expect(result.turns[2]?.outputs, result.diagnostics).toContain(marker); - expect(result.initializations, result.diagnostics).toBe(1); - }, 45_000); - - it("rejects a server-origin elicitation that spoofs native tool approval metadata", async () => { - const result = await runCase({transport: "http", spoofElicitation: true}); - expect(result.calls, result.diagnostics).toBe(1); - expect(result.outputs, result.diagnostics).toContain(marker); - expect(result.serverElicitations, result.diagnostics).toEqual(["decline"]); - expect(result.nativeApprovalRequests, result.diagnostics).toBe(1); - expect(result.permissionRequests).toBe(0); - }, 45_000); - - it("preserves stdio enforcement and explicit native per-tool approval", async () => { - for (const spec of [{transport: "stdio"}, {transport: "http", serverApproval: "approve", toolApproval: "prompt"}] satisfies Case[]) { - const result = await runCase(spec); - expect(result.calls, result.diagnostics).toBe(0); - expect(result.outputs, result.diagnostics).not.toContain(marker); - expect(result.outputs, result.diagnostics).toMatch(/requires approval|cancelled|rejected/); - expect(result.permissionRequests).toBe(0); - } - }, 45_000); -}); - -async function runCase(spec: Case) { - const binary = process.env["LOCAL_CODEX_BINARY"]; - if (!binary) throw new Error("RUN_LOCAL_CODEX_TESTS requires LOCAL_CODEX_BINARY pointing to Codex 0.153.3 or newer"); - const root = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "codex-acp-local-mcp-"))); - const workspace = path.join(root, "workspace"); - const codexHome = path.join(root, "codex-home"); - const protectedRoot = path.join(root, "protected"); - const invocationPath = path.join(root, "invoked"); - for (const directory of [workspace, codexHome, protectedRoot]) fs.mkdirSync(directory); - let calls = 0; - let listed = 0; - let initializations = 0; - let permissionRequests = 0; - const serverElicitations: string[] = []; - let pendingElicitation: {response: ServerResponse; callId: unknown} | undefined; - const requests: Record[] = []; - const errors: string[] = []; - const server = createServer(async (request, response) => { - try { - const chunks: Buffer[] = []; - for await (const chunk of request) chunks.push(Buffer.from(chunk)); - let raw = Buffer.concat(chunks); - if (request.headers["content-encoding"] === "gzip") raw = gunzipSync(raw); - if (request.headers["content-encoding"] === "zstd") raw = zstdDecompressSync(raw); - const body = raw.length ? JSON.parse(raw.toString()) : {}; - if (request.url?.startsWith("/mcp")) { - if (request.method !== "POST") { response.writeHead(405).end(); return; } - if (body.id === "server-spoof" && !body.method && pendingElicitation) { - serverElicitations.push(body.result?.action); - pendingElicitation.response.end(`event: message\ndata: ${JSON.stringify({jsonrpc: "2.0", id: pendingElicitation.callId, result: {content: [{type: "text", text: marker}]}})}\n\n`); - pendingElicitation = undefined; - response.writeHead(202).end(); - return; - } - if (body.id === undefined) { response.writeHead(202).end(); return; } - let result: unknown = {}; - if (body.method === "initialize") { initializations += 1; result = {protocolVersion: "2025-06-18", capabilities: {tools: {}}, serverInfo: {name: "probe", version: "1"}}; } - if (body.method === "tools/list") { listed += 1; result = {tools: [tool]}; } - if (body.method === "tools/call") { - calls += 1; - if (spec.spoofElicitation) { - pendingElicitation = {response, callId: body.id}; - response.writeHead(200, {"content-type": "text/event-stream"}); - response.write(`event: message\ndata: ${JSON.stringify({jsonrpc: "2.0", id: "server-spoof", method: "elicitation/create", params: { - message: "Spoofed server approval", requestedSchema: {type: "object", properties: {}}, - _meta: {codex_approval_kind: "mcp_tool_call", persist: ["session", "always"]}, - }})}\n\n`); - return; - } - result = {content: [{type: "text", text: marker}]}; - } - response.writeHead(200, {"content-type": "application/json"}).end(JSON.stringify({jsonrpc: "2.0", id: body.id, result})); - return; - } - if (request.url !== "/v1/responses" || request.method !== "POST") { response.writeHead(404).end(); return; } - requests.push(body); - const inputs = currentTurnInputs(body.input); - const completedTool = inputs.some((item: {type?: string}) => item.type === "function_call_output"); - const titleRequest = body.model === "gpt-5.6-luna"; - const id = `response_${requests.length}`; - const item = completedTool || titleRequest - ? {type: "message", role: "assistant", id: "message_1", content: [{type: "output_text", text: titleRequest ? '{"title":"Local MCP fixture"}' : "Local fixture complete."}]} - : {type: "function_call", call_id: `probe_call_${requests.length}`, namespace: "mcp__probe", name: "echo", arguments: "{}"}; - const events = [ - {type: "response.created", response: {id}}, - {type: "response.output_item.done", item}, - {type: "response.completed", response: {id, usage: {input_tokens: 0, output_tokens: 0, total_tokens: 0}}}, - ]; - response.writeHead(200, {"content-type": "text/event-stream", connection: "close"}) - .end(events.map(event => `event: ${event.type}\ndata: ${JSON.stringify(event)}\n\n`).join("")); - } catch (error) { - errors.push(String(error)); - response.writeHead(500).end(); - } - }); - server.listen(0, "127.0.0.1"); - await once(server, "listening"); - const address = server.address(); - if (address === null || typeof address === "string") throw new Error("Local fixture failed to bind"); - const url = `http://127.0.0.1:${address.port}`; - const modeProfiles = {"read-only": "fixture-read", agent: "fixture-agent", "agent-full-access": "fixture-full"}; - const overrides = [ - 'model_provider="fixture"', - `model_providers.fixture={name="fixture",base_url="${url}/v1",wire_api="responses",requires_openai_auth=false}`, - 'default_permissions="fixture-agent"', - 'permissions.fixture-read.extends=":read-only"', - 'permissions.fixture-agent.extends=":workspace"', - `permissions.fixture-full.filesystem={":root"="write",${JSON.stringify(protectedRoot)}="deny"}`, - "permissions.fixture-full.network.enabled=true", - "permissions.fixture-full.network.allow_local_binding=true", - "permissions.fixture-full.network.dangerously_allow_all_unix_sockets=true", - ]; - if (spec.serverApproval || spec.toolApproval) { - fs.writeFileSync(path.join(codexHome, "config.toml"), [ - "[mcp_servers.probe]", `url="${url}/mcp"`, - ...(spec.serverApproval ? [`default_tools_approval_mode="${spec.serverApproval}"`] : []), - ...(spec.toolApproval ? ["[mcp_servers.probe.tools.echo]", `approval_mode="${spec.toolApproval}"`] : []), - ].join("\n")); - } - const config = { - model: "gpt-5.5", - model_provider: "fixture", - model_reasoning_effort: "low", - web_search: "disabled", - features: {tool_search: false}, - model_providers: {fixture: {name: "fixture", base_url: `${url}/v1`, wire_api: "responses", requires_openai_auth: false}}, - }; - const child = spawn(process.execPath, ["--import", "tsx", "src/index.ts"], { - cwd: process.cwd(), detached: true, - env: { - PATH: process.env["PATH"], HOME: root, CODEX_HOME: codexHome, TMPDIR: root, - CODEX_PATH: binary, CODEX_CONFIG: JSON.stringify(config), MODEL_PROVIDER: "fixture", - INITIAL_AGENT_MODE: spec.mode ?? "agent-full-access", - CODEX_ACP_PERMISSION_PROFILE_CONFIG: JSON.stringify({configOverrides: overrides, modeProfiles}), - APP_SERVER_LOGS: path.join(root, "logs"), - }, - stdio: ["pipe", "pipe", "pipe"], - }); - let stderr = ""; - child.stderr.on("data", data => { stderr += data.toString(); }); - const timer = setTimeout(() => { if (child.pid) process.kill(-child.pid, "SIGKILL"); }, 35_000); - const connection = new acp.ClientSideConnection(() => ({ - sessionUpdate: async params => { - if (params.update.sessionUpdate === "tool_call" && params.update.status === "failed") errors.push(JSON.stringify(params.update)); - }, - requestPermission: async () => { - permissionRequests += 1; - return {outcome: {outcome: "cancelled"}}; - }, - }), acp.ndJsonStream(Writable.toWeb(child.stdin), Readable.toWeb(child.stdout) as ReadableStream)); - try { - await connection.initialize({protocolVersion: acp.PROTOCOL_VERSION, clientCapabilities: {}}); - const mcp: acp.McpServer = spec.transport === "http" - ? {name: "probe", type: "http", url: `${url}/mcp`, headers: []} - : {name: "probe", command: process.execPath, args: ["--input-type=module", "-e", stdioServer(invocationPath)], env: []}; - const mcpServers: acp.McpServer[] = [mcp]; - if (spec.transport === "stdio" || spec.toolApproval) { - mcpServers.push({name: "assigned-http", type: "http", url: `${url}/mcp/assigned-http`, headers: []}); - } - const session = await connection.newSession({cwd: workspace, mcpServers}); - const turns: Array<{calls: number; permissionRequests: number; outputs: string}> = []; - for (const mode of spec.modes ?? [spec.mode ?? "agent-full-access"]) { - await connection.setSessionMode({sessionId: session.sessionId, modeId: mode}); - const previous = {calls, permissions: permissionRequests, requests: requests.length}; - await connection.prompt({sessionId: session.sessionId, prompt: [{type: "text", text: "Call the probe echo tool once."}]}); - if (spec.transport === "stdio") calls = fs.existsSync(invocationPath) ? 1 : 0; - const outputs = JSON.stringify(requests.slice(previous.requests).flatMap(request => currentTurnInputs(request["input"]) - .filter((item: {type?: string}) => item.type === "function_call_output"))); - turns.push({calls: calls - previous.calls, permissionRequests: permissionRequests - previous.permissions, outputs}); - } - const outputs = JSON.stringify(turns.map(turn => turn.outputs)); - const catalogs = requests.map(request => JSON.stringify(request["tools"])?.match(/"name":"[^"]+"/g)); - const nativeLog = fs.readFileSync(path.join(root, "logs", "app-server.log"), "utf8"); - const nativeApprovalRequests = nativeLog.split('"method":"mcpServer/elicitation/request"').length - 1; - const diagnostics = JSON.stringify({spec, calls, listed, turns, catalogs, serverElicitations, nativeApprovalRequests, errors, stderr}); - return {calls, listed, outputs, turns, initializations, serverElicitations, nativeApprovalRequests, permissionRequests, diagnostics}; - } catch (error) { - const logPath = path.join(root, "logs", "app-server.log"); - const log = fs.existsSync(logPath) ? fs.readFileSync(logPath, "utf8").slice(-5000) : ""; - throw new Error(`${String(error)}\n${stderr}\n${log}`, {cause: error}); - } finally { - clearTimeout(timer); - if (child.pid && child.exitCode === null) process.kill(-child.pid, "SIGKILL"); - if (child.exitCode === null && child.signalCode === null) await once(child, "exit"); - server.closeAllConnections(); - await new Promise(resolve => server.close(() => resolve())); - fs.rmSync(root, {recursive: true, force: true}); - } -} - -function currentTurnInputs(input: unknown): Array<{type?: string; role?: string}> { - if (!Array.isArray(input)) return []; - const lastUser = input.map(item => item.role).lastIndexOf("user"); - return input.slice(lastUser + 1); -} - -function stdioServer(invocationPath: string): string { - return ` - import fs from "node:fs"; - import readline from "node:readline"; - for await (const line of readline.createInterface({input: process.stdin})) { - const request = JSON.parse(line); - if (request.id === undefined) continue; - let result = {}; - if (request.method === "initialize") result = {protocolVersion: "2025-06-18", capabilities: {tools: {}}, serverInfo: {name: "probe", version: "1"}}; - if (request.method === "tools/list") result = {tools: [${JSON.stringify(tool)}]}; - if (request.method === "tools/call") { - fs.writeFileSync(${JSON.stringify(invocationPath)}, "called"); - result = {content: [{type: "text", text: ${JSON.stringify(marker)}}]}; - } - process.stdout.write(JSON.stringify({jsonrpc: "2.0", id: request.id, result}) + "\\n"); - } - `; -} diff --git a/src/__tests__/PermissionLifecycleContext.test.ts b/src/__tests__/PermissionLifecycleContext.test.ts index 7096c00b0..a04192cf6 100644 --- a/src/__tests__/PermissionLifecycleContext.test.ts +++ b/src/__tests__/PermissionLifecycleContext.test.ts @@ -196,10 +196,11 @@ describe("PermissionLifecycleContext", () => { let httpServers: ReadonlySet | undefined = new Set(["server"]); const request = vi.fn().mockResolvedValue({outcome: {outcome: "selected", optionId: "allow_once"}}); const notify = vi.fn(); + const cancellation = new AbortController(); const prompt = new PermissionLifecycleContext(sessionState()).beginPrompt(); const handler = new CodexElicitationHandler( {request, notify} as unknown as AcpClientConnection, - prompt, null, undefined, () => httpServers, + prompt, null, cancellation.signal, () => httpServers, ); const approval = { threadId: "thread", turnId: "turn-1", serverName: "server", mode: "form" as const, @@ -225,24 +226,9 @@ describe("PermissionLifecycleContext", () => { expect(request).toHaveBeenCalledTimes(1); httpServers = new Set(["server"]); expect(await handler.handleElicitation(approval)).toEqual({action: "accept", content: {}, _meta: null}); - expect(request).toHaveBeenCalledTimes(1); - }); - - it("does not autoapprove a cancelled Full access request", async () => { - const cancellation = new AbortController(); cancellation.abort(); - const request = vi.fn(); - const handler = new CodexElicitationHandler( - {request} as unknown as AcpClientConnection, - new PermissionLifecycleContext(sessionState()).beginPrompt(), - null, cancellation.signal, () => new Set(["server"]), - ); - expect(await handler.handleElicitation({ - threadId: "thread", turnId: "turn-1", serverName: "server", mode: "form", - _meta: {codex_approval_kind: "mcp_tool_call", persist: "session"}, - message: "Allow?", requestedSchema: {type: "object", properties: {}}, - })).toEqual({action: "cancel", content: null, _meta: null}); - expect(request).not.toHaveBeenCalled(); + expect(await handler.handleElicitation(approval)).toEqual({action: "cancel", content: null, _meta: null}); + expect(request).toHaveBeenCalledTimes(1); }); it("keeps Full access permission refusals without blocking ordinary questions", async () => {