diff --git a/packages/server/src/server/agent/providers/codex-mcp-agent.ts b/packages/server/src/server/agent/providers/codex-mcp-agent.ts index 3c45a3333..e6691122f 100644 --- a/packages/server/src/server/agent/providers/codex-mcp-agent.ts +++ b/packages/server/src/server/agent/providers/codex-mcp-agent.ts @@ -319,7 +319,6 @@ class CodexMcpAgentSession implements AgentSession { private cachedRuntimeInfo: AgentRuntimeInfo | null = null; private pendingPermissions = new Map(); private pendingPermissionHandlers = new Map(); - private pendingToolEvents = new Map void>>(); private resolvedPermissionRequests = new Set(); private eventQueue: Pushable | null = null; private currentAbortController: AbortController | null = null; @@ -613,7 +612,6 @@ class CodexMcpAgentSession implements AgentSession { : "denied"; const reason = response.behavior === "deny" ? response.message : undefined; pending.resolve({ decision, reason }); - this.flushQueuedToolEvents(requestId, response.behavior === "allow"); } describePersistence(): AgentPersistenceHandle | null { @@ -647,7 +645,6 @@ class CodexMcpAgentSession implements AgentSession { } this.pendingPermissionHandlers.clear(); this.pendingPermissions.clear(); - this.pendingToolEvents.clear(); this.resolvedPermissionRequests.clear(); this.eventQueue?.end(); this.eventQueue = null; @@ -811,97 +808,6 @@ class CodexMcpAgentSession implements AgentSession { }); } - private getEffectiveApprovalPolicy(): string { - const modeId = this.currentMode ?? this.config.modeId ?? DEFAULT_CODEX_MODE_ID; - const preset = MODE_PRESETS[modeId] ?? MODE_PRESETS[DEFAULT_CODEX_MODE_ID]; - return this.config.approvalPolicy ?? preset.approvalPolicy; - } - - private shouldGatePermissions(): boolean { - const approvalPolicy = this.getEffectiveApprovalPolicy(); - return approvalPolicy === "on-request" || approvalPolicy === "untrusted"; - } - - private queuePermissionGatedEvent( - callId: string | undefined, - event: Record, - emitEvent: () => void - ): boolean { - if (!this.shouldGatePermissions()) { - return false; - } - if (!callId) { - return false; - } - const requestId = `permission-${callId}`; - if (this.resolvedPermissionRequests.has(requestId)) { - return false; - } - if (!this.pendingPermissions.has(requestId) && !this.pendingPermissionHandlers.has(requestId)) { - const permission = this.buildPermissionRequest(event); - if (!permission) { - return false; - } - this.pendingPermissions.set(permission.id, permission); - this.pendingPermissionHandlers.set(permission.id, { - request: permission, - resolve: () => {}, - reject: () => {}, - }); - this.emitPermissionRequested(permission); - } - this.queueToolEvent(requestId, emitEvent); - return true; - } - - private ensurePermissionRequestFromEvent(event: Record): void { - const callId = normalizeCallId(event.call_id as string | undefined); - if (!callId) { - return; - } - const requestId = `permission-${callId}`; - if (this.resolvedPermissionRequests.has(requestId)) { - return; - } - if (this.pendingPermissions.has(requestId) || this.pendingPermissionHandlers.has(requestId)) { - return; - } - const permission = this.buildPermissionRequest(event); - if (!permission) { - return; - } - this.pendingPermissions.set(permission.id, permission); - this.pendingPermissionHandlers.set(permission.id, { - request: permission, - resolve: () => {}, - reject: () => {}, - }); - this.emitPermissionRequested(permission); - } - - private queueToolEvent(requestId: string, emitEvent: () => void): void { - const queued = this.pendingToolEvents.get(requestId); - if (queued) { - queued.push(emitEvent); - return; - } - this.pendingToolEvents.set(requestId, [emitEvent]); - } - - private flushQueuedToolEvents(requestId: string, allow: boolean): void { - const queued = this.pendingToolEvents.get(requestId); - if (!queued) { - return; - } - this.pendingToolEvents.delete(requestId); - if (!allow) { - return; - } - for (const emitEvent of queued) { - emitEvent(); - } - } - private recordHistory(item: AgentTimelineItem): void { if (this.sessionId) { const history = SESSION_HISTORY.get(this.sessionId) ?? []; @@ -1079,9 +985,6 @@ class CodexMcpAgentSession implements AgentSession { }), }); }; - if (this.queuePermissionGatedEvent(callId, event as Record, emitEvent)) { - break; - } emitEvent(); break; } @@ -1141,18 +1044,9 @@ class CodexMcpAgentSession implements AgentSession { }); } }; - if (this.queuePermissionGatedEvent(callId, event as Record, emitEvent)) { - break; - } emitEvent(); break; } - case "exec_approval_request": { - if (this.shouldGatePermissions()) { - this.ensurePermissionRequestFromEvent(event as Record); - } - break; - } case "patch_apply_begin": { const callId = normalizeCallId((event as { call_id?: string }).call_id); const changes = (event as { changes?: Record }).changes ?? {}; diff --git a/plan.md b/plan.md index f96218857..1564dd5a9 100644 --- a/plan.md +++ b/plan.md @@ -245,14 +245,51 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov ``` - **Done (2025-12-24)**: Verified via debug script. -- [ ] **Fix**: Update MODE_PRESETS to use `on-request` instead of `untrusted`. +- [x] **Fix**: Update MODE_PRESETS to use `on-request` instead of `untrusted`. - Change `codex-mcp-agent.ts` MODE_PRESETS: - `read-only`: `approvalPolicy: "on-request"` (was `untrusted`) - `auto`: `approvalPolicy: "on-request"` (was `untrusted`) - Ensure elicitation handler returns `{ decision: "approved" | "denied" | ... }` format - Remove any workarounds that were compensating for missing elicitation + - **Done (2025-12-24 20:20)**: Removed synthetic permission gating/exec approval workarounds now that on-request elicitation is the default. - [ ] **Test (E2E)**: Rerun server vitest after fixes. - If failures: add follow-up fix tasks immediately after this item. + +- [ ] **Test (E2E)**: Permission flow parity - test both Codex MCP and Claude providers. + + - Create/update E2E tests that verify permissions work for BOTH providers + - Test cases for each provider: + - Permission requested event fires when tool needs approval + - Permission granted → tool executes + - Permission denied → tool blocked + - Permission abort/interrupt → session handles gracefully + - Ensure test structure allows easy comparison between providers + +- [ ] **Audit**: Feature parity checklist for Codex MCP provider vs Claude provider. + + - Document all capabilities the Claude provider supports + - Verify Codex MCP provider supports each one or document gaps + - Key areas to check: + - Streaming events (reasoning, text, tool calls) + - Session persistence/resume + - Abort/interrupt handling + - Runtime info reporting + - Mode switching + +- [ ] **Test (E2E)**: Comprehensive tool call coverage for Codex MCP provider. + + - All tool call types must be tested and emit proper timeline events: + - **Command runs**: `shell_command` / `exec_command` → exit code, stdout, stderr + - **File edits**: `apply_patch` / file modifications → before/after content + - **File creations**: new file writes → file path, content + - **MCP tool calls**: external MCP server tools → tool name, input, output + - **Web search**: if supported → query, results + - **File reads**: read operations → file path, content snippet + - Each test should verify: + - Timeline item is emitted with correct `type` and `status` + - Tool `input` and `output` are captured + - `callId` is consistent across events + - Permission flow triggers when expected (for unsafe operations)