From bbf3d0f9ccef3785d96b98ddcde2238a795a726a Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Tue, 28 Jul 2026 17:10:32 +0200 Subject: [PATCH] Keep plan approval focused on the latest proposal (#2534) * fix(server): replace stale plan approvals Synthetic plan approvals accumulated across planning turns because each proposal used a new permission ID. Dismiss the current proposal only after a follow-up prompt is accepted, and enforce one pending proposal when a newer plan arrives. * fix(server): close plan approval submission race Deactivate the existing proposal before starting a revision so it cannot be implemented concurrently. Restore it when submission fails, while preserving any newer approval emitted during the request. * fix(server): deactivate plans before prompt setup Start the plan-dismissal transaction before any asynchronous prompt setup so a stale proposal cannot be implemented from another client while the revision is preparing. * fix(server): make prompt dismissal final Treat prompt submission as the dismissal event instead of compensating after failures. This avoids resurrecting stale plans across ambiguous provider outcomes and manager failure cleanup. --- .../providers/codex-app-server-agent.test.ts | 214 +++++++++++++++++- .../agent/providers/codex-app-server-agent.ts | 100 ++++---- 2 files changed, 267 insertions(+), 47 deletions(-) diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts index 2a4783534..57235d377 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.test.ts @@ -3914,8 +3914,8 @@ describe("Codex app-server provider", () => { }, actions: [ expect.objectContaining({ - id: "reject", - label: "Reject", + id: "dismiss", + label: "Dismiss", behavior: "deny", }), expect.objectContaining({ @@ -3979,6 +3979,216 @@ describe("Codex app-server provider", () => { }); }); + test("replaces a pending synthetic plan approval when a later plan completes", () => { + const session = createSession({ + featureValues: { plan_mode: true }, + }); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-first" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the first plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-second" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the revised plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + + expect(session.getPendingPermissions()).toEqual([ + expect.objectContaining({ + kind: "plan", + input: { plan: "- Implement the revised plan" }, + }), + ]); + }); + + test("dismisses a pending synthetic plan approval after a new prompt is accepted", async () => { + const session = createSession({ + featureValues: { plan_mode: true }, + }); + const events: AgentStreamEvent[] = []; + session.subscribe((event) => events.push(event)); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-pending" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the original plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + const pendingPlan = session.getPendingPermissions()[0]; + expect(pendingPlan).toBeDefined(); + + session.activeForegroundTurnId = null; + session.client = createStub({ + request: async (method) => { + if (method === "thread/loaded/list") return { data: ["test-thread"] }; + if (method === "turn/start") return {}; + throw new Error(`Unexpected request: ${method}`); + }, + }); + + await session.startTurn("Revise the plan to include tests"); + + expect(session.getPendingPermissions()).toEqual([]); + expect(events).toContainEqual({ + type: "permission_resolved", + provider: "codex", + requestId: pendingPlan!.id, + resolution: { + behavior: "deny", + message: "Dismissed by a new prompt", + }, + }); + }); + + test("makes the old plan non-actionable while a new prompt is being prepared", async () => { + const session = createSession({ + featureValues: { plan_mode: true }, + }); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-pending" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the original plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + const pendingPlan = session.getPendingPermissions()[0]; + expect(pendingPlan).toBeDefined(); + + let continuePromptSetup: (() => void) | undefined; + let markPromptSetupStarted: (() => void) | undefined; + const promptSetupStarted = new Promise((resolve) => { + markPromptSetupStarted = resolve; + }); + session.activeForegroundTurnId = null; + session.client = createStub({ + request: async (method) => { + if (method === "thread/loaded/list") { + markPromptSetupStarted?.(); + await new Promise((resolve) => { + continuePromptSetup = resolve; + }); + return { data: ["test-thread"] }; + } + if (method === "turn/start") return {}; + throw new Error(`Unexpected request: ${method}`); + }, + }); + + const startTurn = session.startTurn("Revise the plan"); + await promptSetupStarted; + + expect(session.getPendingPermissions()).toEqual([]); + await expect( + session.respondToPermission(pendingPlan!.id, { + behavior: "allow", + selectedActionId: "implement", + }), + ).rejects.toThrow( + `No pending Codex app-server permission request with id '${pendingPlan!.id}'`, + ); + + continuePromptSetup?.(); + await startTurn; + }); + + test("does not dismiss a new plan approval emitted while a prompt is being accepted", async () => { + const session = createSession({ + featureValues: { plan_mode: true }, + }); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-pending" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the original plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + + let acceptPrompt: (() => void) | undefined; + let markPromptRequested: (() => void) | undefined; + const promptRequested = new Promise((resolve) => { + markPromptRequested = resolve; + }); + session.activeForegroundTurnId = null; + session.client = createStub({ + request: async (method) => { + if (method === "thread/loaded/list") return { data: ["test-thread"] }; + if (method === "turn/start") { + markPromptRequested?.(); + return await new Promise((resolve) => { + acceptPrompt = resolve; + }); + } + throw new Error(`Unexpected request: ${method}`); + }, + }); + + const startTurn = session.startTurn("Revise the plan"); + await promptRequested; + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the newer plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + acceptPrompt?.(); + await startTurn; + + expect(session.getPendingPermissions()).toEqual([ + expect.objectContaining({ input: { plan: "- Implement the newer plan" } }), + ]); + }); + + test("keeps a synthetic plan dismissed when a new prompt is rejected", async () => { + const session = createSession({ + featureValues: { plan_mode: true }, + }); + + asInternals(session).handleNotification("turn/started", { + turn: { id: "turn-plan-pending" }, + }); + asInternals(session).handleNotification("turn/plan/updated", { + plan: [{ step: "Implement the original plan", status: "pending" }], + }); + asInternals(session).handleNotification("turn/completed", { + turn: { status: "completed", error: null }, + }); + const pendingPlan = session.getPendingPermissions()[0]; + expect(pendingPlan).toBeDefined(); + + session.activeForegroundTurnId = null; + session.client = createStub({ + request: async (method) => { + if (method === "thread/loaded/list") return { data: ["test-thread"] }; + if (method === "turn/start") throw new Error("Prompt rejected"); + throw new Error(`Unexpected request: ${method}`); + }, + }); + + await expect(session.startTurn("Revise the plan")).rejects.toThrow("Prompt rejected"); + + expect(session.getPendingPermissions()).toEqual([]); + }); + test("emits imageView paths with spaces as valid assistant markdown images", () => { const session = createSession(); const events: AgentStreamEvent[] = []; diff --git a/packages/server/src/server/agent/providers/codex-app-server-agent.ts b/packages/server/src/server/agent/providers/codex-app-server-agent.ts index fc26e7a47..fe1e83406 100644 --- a/packages/server/src/server/agent/providers/codex-app-server-agent.ts +++ b/packages/server/src/server/agent/providers/codex-app-server-agent.ts @@ -963,8 +963,8 @@ function buildPlanPermissionActions(options?: { }): AgentPermissionAction[] { const actions: AgentPermissionAction[] = [ { - id: "reject", - label: "Reject", + id: "dismiss", + label: "Dismiss", behavior: "deny", variant: "danger", intent: "dismiss", @@ -3092,6 +3092,13 @@ interface CodexSubAgentCallState { childThreadIds: Set; } +interface CodexPendingPermissionHandler { + resolve: (value: unknown) => void; + kind: "command" | "file" | "question" | "mcp_elicitation" | "plan"; + questions?: CodexQuestionPrompt[]; + planText?: string; +} + export class CodexAppServerAgentSession implements AgentSession { readonly provider = CODEX_PROVIDER; readonly capabilities = CODEX_APP_SERVER_CAPABILITIES; @@ -3115,15 +3122,7 @@ export class CodexAppServerAgentSession implements AgentSession { private persistedProviderSubagentEvents: AgentStreamEvent[] = []; private pendingPermissions = new Map(); private mcpElicitationPermissionIds = new Map(); - private pendingPermissionHandlers = new Map< - string, - { - resolve: (value: unknown) => void; - kind: "command" | "file" | "question" | "mcp_elicitation" | "plan"; - questions?: CodexQuestionPrompt[]; - planText?: string; - } - >(); + private pendingPermissionHandlers = new Map(); private resolvedPermissionRequests = new Set(); private pendingAgentMessages = new Map(); private pendingReasoning = new Map(); @@ -3430,6 +3429,8 @@ export class CodexAppServerAgentSession implements AgentSession { } private emitSyntheticPlanApprovalRequest(planText: string): void { + this.dismissPendingPlanApprovals("Superseded by a newer plan"); + const requestId = `permission-${randomUUID()}`; const request: AgentPermissionRequest = { id: requestId, @@ -3837,30 +3838,31 @@ export class CodexAppServerAgentSession implements AgentSession { throw new Error("A foreground turn is already active"); } - await this.connect(); - if (!this.client) { - throw new Error("Codex client not initialized"); - } - - const slashCommand = await this.resolveSlashCommandInvocation(prompt); - const effectivePrompt = slashCommand - ? await this.buildCommandPromptInput(slashCommand.commandName, slashCommand.args) - : prompt; - - if (this.currentThreadId) { - await this.ensureThreadLoaded(); - } else { - await this.ensureThread(); - } - - const turnStart = await this.buildTurnStartParams(effectivePrompt, options); - - const turnId = this.createTurnId(); - this.activeForegroundTurnId = turnId; - this.activeClientMessageId = options?.clientMessageId ?? null; - this.currentTurnId = null; + this.dismissPendingPlanApprovals("Dismissed by a new prompt"); try { + await this.connect(); + if (!this.client) { + throw new Error("Codex client not initialized"); + } + + const slashCommand = await this.resolveSlashCommandInvocation(prompt); + const effectivePrompt = slashCommand + ? await this.buildCommandPromptInput(slashCommand.commandName, slashCommand.args) + : prompt; + + if (this.currentThreadId) { + await this.ensureThreadLoaded(); + } else { + await this.ensureThread(); + } + + const turnStart = await this.buildTurnStartParams(effectivePrompt, options); + const turnId = this.createTurnId(); + this.activeForegroundTurnId = turnId; + this.activeClientMessageId = options?.clientMessageId ?? null; + this.currentTurnId = null; + this.logTurnStartSummary({ turnId, thinkingOptionId: turnStart.thinkingOptionId, @@ -3871,13 +3873,12 @@ export class CodexAppServerAgentSession implements AgentSession { hasCodexConfig: turnStart.hasCodexConfig, }); await this.client.request("turn/start", turnStart.params, TURN_START_TIMEOUT_MS); + return { turnId }; } catch (error) { this.activeForegroundTurnId = null; this.activeClientMessageId = null; throw error; } - - return { turnId }; } private rememberCodexUserMessageTurn(messageId: string | null | undefined): boolean { @@ -4128,12 +4129,7 @@ export class CodexAppServerAgentSession implements AgentSession { private handlePlanPermissionResponse(params: { requestId: string; response: AgentPermissionResponse; - pending: { - resolve: (value: unknown) => void; - kind: "command" | "file" | "question" | "mcp_elicitation" | "plan"; - questions?: CodexQuestionPrompt[]; - planText?: string; - }; + pending: CodexPendingPermissionHandler; pendingRequest: AgentPermissionRequest | null; }): AgentPermissionResult | void { const { requestId, response, pending, pendingRequest } = params; @@ -4144,6 +4140,23 @@ export class CodexAppServerAgentSession implements AgentSession { }); } + this.resolvePlanPermission(requestId, response); + if (followUpPrompt) { + return { followUpPrompt }; + } + } + + private dismissPendingPlanApprovals(message: string): void { + const requestIds = Array.from(this.pendingPermissionHandlers) + .filter(([, pending]) => pending.kind === "plan") + .map(([requestId]) => requestId); + + for (const requestId of requestIds) { + this.resolvePlanPermission(requestId, { behavior: "deny", message }); + } + } + + private resolvePlanPermission(requestId: string, resolution: AgentPermissionResponse): void { this.pendingPermissionHandlers.delete(requestId); this.pendingPermissions.delete(requestId); this.resolvedPermissionRequests.add(requestId); @@ -4151,11 +4164,8 @@ export class CodexAppServerAgentSession implements AgentSession { type: "permission_resolved", provider: CODEX_PROVIDER, requestId, - resolution: response, + resolution, }); - if (followUpPrompt) { - return { followUpPrompt }; - } } private emitDeniedToolCallTimelineEvent(params: {