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 c58e338d7..3a06fdb0d 100644 --- a/packages/server/src/server/agent/providers/codex-mcp-agent.ts +++ b/packages/server/src/server/agent/providers/codex-mcp-agent.ts @@ -307,6 +307,8 @@ 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; private historyPending = false; @@ -550,6 +552,7 @@ class CodexMcpAgentSession implements AgentSession { } this.pendingPermissionHandlers.delete(requestId); this.pendingPermissions.delete(requestId); + this.resolvedPermissionRequests.add(requestId); const status = response.behavior === "allow" ? "granted" : "denied"; this.emitEvent({ @@ -580,6 +583,7 @@ class CodexMcpAgentSession implements AgentSession { ? "abort" : "denied"; pending.resolve({ decision, reason: response.message }); + this.flushQueuedToolEvents(requestId, response.behavior === "allow"); } describePersistence(): AgentPersistenceHandle | null { @@ -608,6 +612,8 @@ class CodexMcpAgentSession implements AgentSession { } this.pendingPermissionHandlers.clear(); this.pendingPermissions.clear(); + this.pendingToolEvents.clear(); + this.resolvedPermissionRequests.clear(); this.eventQueue?.end(); this.eventQueue = null; @@ -776,6 +782,97 @@ 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) ?? []; @@ -929,19 +1026,25 @@ class CodexMcpAgentSession implements AgentSession { const callId = normalizeCallId((event as { call_id?: string }).call_id); const command = (event as { command?: unknown }).command; const cwd = (event as { cwd?: string }).cwd; - this.emitEvent({ - type: "timeline", - provider: "codex-mcp", - item: createToolCallTimelineItem({ - server: "command", - tool: "shell", - status: "running", - callId, - displayName: buildCommandDisplayName(command), - kind: "execute", - input: { command, cwd }, - }), - }); + const emitEvent = () => { + this.emitEvent({ + type: "timeline", + provider: "codex-mcp", + item: createToolCallTimelineItem({ + server: "command", + tool: "shell", + status: "running", + callId, + displayName: buildCommandDisplayName(command), + kind: "execute", + input: { command, cwd }, + }), + }); + }; + if (this.queuePermissionGatedEvent(callId, event as Record, emitEvent)) { + break; + } + emitEvent(); break; } case "exec_command_end": { @@ -965,29 +1068,41 @@ class CodexMcpAgentSession implements AgentSession { cwd, } : undefined; - if (typeof exitCode === "number" && exitCode !== 0) { - this.turnState && (this.turnState.sawError = true); - } - this.emitEvent({ - type: "timeline", - provider: "codex-mcp", - item: createToolCallTimelineItem({ - server: "command", - tool: "shell", - status: exitCode && exitCode !== 0 ? "failed" : "completed", - callId, - displayName: buildCommandDisplayName(command), - kind: "execute", - input: { command, cwd }, - output: structuredOutput, - }), - }); - if (typeof exitCode === "number" && exitCode !== 0) { + const emitEvent = () => { + if (typeof exitCode === "number" && exitCode !== 0) { + this.turnState && (this.turnState.sawError = true); + } this.emitEvent({ type: "timeline", provider: "codex-mcp", - item: { type: "error", message: `Command failed with exit code ${exitCode}` }, + item: createToolCallTimelineItem({ + server: "command", + tool: "shell", + status: exitCode && exitCode !== 0 ? "failed" : "completed", + callId, + displayName: buildCommandDisplayName(command), + kind: "execute", + input: { command, cwd }, + output: structuredOutput, + }), }); + if (typeof exitCode === "number" && exitCode !== 0) { + this.emitEvent({ + type: "timeline", + provider: "codex-mcp", + item: { type: "error", message: `Command failed with exit code ${exitCode}` }, + }); + } + }; + if (this.queuePermissionGatedEvent(callId, event as Record, emitEvent)) { + break; + } + emitEvent(); + break; + } + case "exec_approval_request": { + if (this.shouldGatePermissions()) { + this.ensurePermissionRequestFromEvent(event as Record); } break; } diff --git a/plan.md b/plan.md index 82740eccb..c5e6cec44 100644 --- a/plan.md +++ b/plan.md @@ -49,26 +49,26 @@ Build a new Codex MCP provider side‑by‑side with the existing Codex SDK prov - [x] **Fix**: Handle Codex CLI model availability mismatch (gpt-4.1 rejected for ChatGPT accounts) in Codex MCP tests/provider. - **Done (2025-12-24 18:40)**: Added model-rejection fallback for ChatGPT accounts, track runtime model from responses, and default to a placeholder when a configured model is rejected. -- [ ] **Fix**: Investigate Codex MCP permission elicitation behavior for `approval-policy=on-request` and `untrusted` (no permission_requested events). +- [x] **Fix**: Investigate Codex MCP permission elicitation behavior for `approval-policy=on-request` and `untrusted` (no permission_requested events). + - **Done (2025-12-24 19:16)**: Added permission gating fallback for exec approval events, queued command events until approval, and flush/dropped queued events on resolution. -- [ ] **Fix**: Resolve Codex MCP provider typecheck errors (`AgentPermissionResponse.message` usage, unused locals). +- [ ] **Fix**: Compare permission elicitation with happy-cli reference implementation. -- [ ] **Refactor**: Reduce casting/`unknown` usage in Codex MCP provider per type/quality report. + - Read `/Users/moboudra/dev/voice-dev/.tmp/happy-cli/src/codex/` to understand how elicitation works there. + - Identify what's different in `codex-mcp-agent.ts` vs the reference. + - The reference supports permissions - copy the working approach. -- [ ] **Review**: Check implementation + edge cases. +- [ ] **Fix**: Use valid model instead of gpt-4.1. - - If issues: add fix tasks + re-review. + - gpt-4.1 does not exist and is rejected by Codex CLI. + - Check what models are actually available (run `codex --help` or check docs). + - Update tests and provider to use a valid default model. -- [ ] **Review**: Strong type/quality review of provider + tests. +- [ ] **Fix**: Resolve typecheck errors in `codex-mcp-agent.ts`. - - Identify type safety issues, missing error handling, and brittle assumptions. - - Add fix tasks and re-review. + - Run `npm run typecheck --workspace=@paseo/server`. + - Fix `AgentPermissionResponse.message` and unused locals. -- [ ] **Test (E2E)**: Final verification (full scenario matrix). +- [ ] **Test (E2E)**: Run tests and verify fixes work. - - read-only + on-request - - read-only + deny - - workspace-write + untrusted - - full-access - -- [ ] **Plan**: Re-audit and add any follow-up tasks. +- [ ] **Plan**: Re-audit based on test results.