From 1f64e689872eabdefa54d9906d8c783d14a9de04 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Mon, 2 Nov 2026 23:01:24 +0000 Subject: [PATCH] fix(codex): show the file and diff before patch approval (#8585) * fix(codex): include patch details in file change approvals * fix(codex): name the file in patch approvals The approval detail renderer shows the diff without its filePath. Include the file in the approval title so the pending decision identifies the change being reviewed. Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> * fix(codex): await patch notifications before approval Approval requests bypass the session's notification conversion queue. Drain earlier notifications before reading the patch preview so an approval in the same stdout batch receives its matching file and diff. Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> * test(codex): express patch approval through provider actions Keep protocol framing in the fake app-server and precompute the approval title. Streamed completion already requires an item ID; expose that narrowing so completion cleanup stays inline without repeating its guard. --------- Co-authored-by: Okan YILDIRIM <63511519+ragokan@users.noreply.github.com> Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- .../providers/codex-app-server-agent.test.ts | 51 +++++++++++++++++++ .../agent/providers/codex-app-server-agent.ts | 39 +++++++++----- .../codex/test-utils/fake-app-server.ts | 22 ++++++++ 3 files changed, 98 insertions(+), 14 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 9986357cf4..9b5263e474 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 @@ -227,6 +227,57 @@ function deferred() { return { promise, resolve }; } +describe("Codex file-change approvals", () => { + test("file-change approval includes the matching patch when notifications are still queued", async () => { + const appServer = createFakeCodexAppServer(); + const { session } = await startPublicSteeringSession(appServer); + try { + for (const [id, filePath, diff] of [ + ["file-alpha", "/workspace/project/alpha.txt", "@@ -1 +1 @@\n-blue\n+green\n"], + ["file-beta", "/workspace/project/beta.txt", "@@ -1 +1 @@\n-small\n+large\n"], + ]) { + appServer.startsFileChange({ + threadId: "thread-1", + turnId: "native-A", + callId: id, + path: filePath, + diff, + }); + } + const permission = waitForNextPermission(session); + appServer.requestFileChangeApproval({ + itemId: "file-alpha", + threadId: "thread-1", + turnId: "native-A", + reason: "Apply the patch", + }); + + expect((await permission).request).toEqual({ + id: "permission-file-alpha", + provider: "codex", + name: "CodexFileChange", + kind: "tool", + title: "Apply file changes: alpha.txt", + description: "Apply the patch", + detail: { + type: "edit", + filePath: "alpha.txt", + unifiedDiff: "@@ -1 +1 @@\n-blue\n+green\n", + }, + metadata: { itemId: "file-alpha", threadId: "thread-1", turnId: "native-A" }, + }); + + await session.respondToPermission("permission-file-alpha", { behavior: "allow" }); + await expect(appServer.waitForApprovalDecision("file-alpha")).resolves.toEqual({ + decision: "accept", + }); + appServer.assertNoErrors(); + } finally { + await session.close(); + } + }); +}); + describe("Codex active-turn steering admission", () => { test("steers an accepted turn whose native id arrives after startTurn returns", async () => { const appServer = createFakeCodexAppServer({ 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 cc667b2494..f349978996 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 @@ -3483,6 +3483,7 @@ export class CodexAppServerAgentSession implements AgentSession { private pendingReasoning = new Map(); private pendingCommandOutputDeltas = new Map(); private pendingFileChangeOutputDeltas = new Map(); + private pendingToolCallDetails = new Map(); private pendingAssistantMessageBoundary = false; private terminalCommandByProcessId = new Map(); // The root thread's still-running command process ids, keyed by the native turn that @@ -5251,6 +5252,7 @@ export class CodexAppServerAgentSession implements AgentSession { async close(): Promise { this.closed = true; this.clearPendingPermissions(); + this.pendingToolCallDetails.clear(); this.pendingSubAgentNotificationsByThreadId.clear(); this.subscribers.clear(); this.startupEvents = null; @@ -6416,6 +6418,7 @@ export class CodexAppServerAgentSession implements AgentSession { this.pendingReasoning.clear(); this.pendingCommandOutputDeltas.clear(); this.pendingFileChangeOutputDeltas.clear(); + this.pendingToolCallDetails.clear(); this.pendingAssistantMessageBoundary = false; this.warnedIncompleteEditToolCallIds.clear(); this.pendingRootCompactionItemIds.clear(); @@ -6789,6 +6792,9 @@ export class CodexAppServerAgentSession implements AgentSession { if (shouldIgnoreMirroredLifecycleItem(parsed.source, parsed.item)) { return; } + if (parsed.item.id) { + this.pendingToolCallDetails.delete(parsed.item.id); + } this.receiveAsyncQuestion(parsed.threadId, parsed.item); if (this.isUserMessageItem(parsed.item)) { await this.handleUserMessageItem(parsed); @@ -6836,10 +6842,8 @@ export class CodexAppServerAgentSession implements AgentSession { if (timelineItem.type === "assistant_message") { this.pendingAssistantMessageBoundary = true; } - if (itemId) { - this.emittedItemCompletedIds.add(itemId); - this.emittedItemStartedIds.delete(itemId); - } + this.emittedItemCompletedIds.add(itemId); + this.emittedItemStartedIds.delete(itemId); await this.replayPendingSubAgentNotifications(registeredChildThreadIds); return; } @@ -6875,7 +6879,7 @@ export class CodexAppServerAgentSession implements AgentSession { private consumeStreamedTextCompletion( timelineItem: AgentTimelineItem, itemId: string | null | undefined, - ): boolean { + ): itemId is string { if (!itemId) { return false; } @@ -6984,6 +6988,7 @@ export class CodexAppServerAgentSession implements AgentSession { if (!timelineItem || timelineItem.type !== "tool_call") { return; } + this.pendingToolCallDetails.set(timelineItem.callId, timelineItem.detail); const registeredChildThreadIds = this.registerSubAgentToolCall({ timelineItem, rawItem: parsed.item, @@ -7253,7 +7258,8 @@ export class CodexAppServerAgentSession implements AgentSession { }); } - private handleFileChangeApprovalRequest(params: unknown): Promise { + private async handleFileChangeApprovalRequest(params: unknown): Promise { + await this.pendingNotifications; const parsed = z .object({ itemId: z.string(), @@ -7263,20 +7269,25 @@ export class CodexAppServerAgentSession implements AgentSession { }) .parse(params); const requestId = `permission-${parsed.itemId}`; + const detail: ToolCallTimelineItem["detail"] = this.pendingToolCallDetails.get( + parsed.itemId, + ) ?? { + type: "unknown", + input: { reason: parsed.reason ?? null }, + output: null, + }; + const title = + detail.type === "edit" && detail.filePath + ? `Apply file changes: ${detail.filePath}` + : "Apply file changes"; const request: AgentPermissionRequest = { id: requestId, provider: CODEX_PROVIDER, name: "CodexFileChange", kind: "tool", - title: "Apply file changes", + title, description: parsed.reason ?? undefined, - detail: { - type: "unknown", - input: { - reason: parsed.reason ?? null, - }, - output: null, - }, + detail, metadata: { itemId: parsed.itemId, threadId: parsed.threadId, diff --git a/packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts b/packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts index 99e6a73050..ec14a773e4 100644 --- a/packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts +++ b/packages/server/src/server/agent/providers/codex/test-utils/fake-app-server.ts @@ -43,6 +43,13 @@ interface FakeTerminalInput { processId: string; text: string; } +interface FakeFileChange { + threadId: string; + turnId: string; + callId: string; + path: string; + diff: string; +} interface FakeLegacyPatch { threadId: string; callId: string; @@ -88,6 +95,7 @@ export interface FakeCodexAppServer { completesSubAgentActivity(params: FakeSubAgentActivity): void; completesCompaction(params: { threadId: string; itemId: string }): void; runsLegacyCommand(params: FakeLegacyCommand): void; + startsFileChange(params: FakeFileChange): void; appliesLegacyPatch(params: FakeLegacyPatch): void; completesCommand(params: FakeLegacyCommand): void; completesSilentCommand(params: FakeSilentCommand): void; @@ -467,6 +475,20 @@ export function createFakeCodexAppServer( success: true, }); }, + startsFileChange(params) { + writeNotification("item/started", { + threadId: params.threadId, + turnId: params.turnId, + item: { + type: "fileChange", + id: params.callId, + status: "inProgress", + changes: [ + { path: params.path, kind: { type: "update", move_path: null }, diff: params.diff }, + ], + }, + }); + }, appliesLegacyPatch(params) { const changes = [ {