Enrich file change payloads with paths and diff fallbacks

Author: i1bro

src/CodexApprovalHandler.ts

@@ -6,7 +6,6 @@ import type {
     CommandExecutionRequestApprovalResponse,
     FileChangeRequestApprovalParams,
     FileChangeRequestApprovalResponse,
-    FileUpdateChange,
     McpServerElicitationRequestParams,
     McpServerElicitationRequestResponse
 } from "./app-server/v2";
@@ -15,7 +14,12 @@ import type {ToolCallContent} from "@agentclientprotocol/sdk/dist/schema/types.g
 import {logger} from "./Logger";
 import {stripShellPrefix} from "./CodexEventHandler";
 import type {ApprovalContextStore} from "./CodexApprovalContext";
-import {createFileChangeContents} from "./CodexToolCallMapper";
+import {
+    createFileChangeContents,
+    createFileChangeLocations,
+    createRawFileChangeInput,
+    parseUnifiedDiffChanges,
+} from "./CodexToolCallMapper";
 
 const APPROVAL_OPTIONS: acp.PermissionOption[] = [
     { optionId: "allow_once", name: "Allow Once", kind: "allow_once" },
@@ -119,47 +123,39 @@ export class CodexApprovalHandler implements ApprovalHandler {
         }
     }
 
-    private createTextContents(...texts: Array<string | null | undefined>): Array<ToolCallContent> | null {
-        const contents = texts
-            .map(text => this.createTextContent(text ?? null))
-            .filter((content): content is ToolCallContent => content !== null)
-        return contents.length > 0 ? contents : null
-    }
-
     private async buildFileChangePermissionRequest(
         sessionId: string,
         params: FileChangeRequestApprovalParams
     ): Promise<acp.RequestPermissionRequest> {
         const reasonContent = this.createTextContent(params.reason ?? null);
         const fileChange = this.approvalContext.fileChangesByItemId.get(params.itemId);
-        const diffContent = fileChange ? await createFileChangeContents(fileChange.changes) : [];
+        const content: ToolCallContent[] = reasonContent ? [reasonContent] : [];
         const toolCall: acp.ToolCallUpdate = {
             toolCallId: params.itemId,
             kind: "edit",
             status: "pending",
         };
-        const content = [
-            ...(reasonContent ? [reasonContent] : []),
-            ...diffContent,
-        ];
-        if (content.length > 0) {
-            toolCall.content = content;
-        }
         if (fileChange) {
-            toolCall.locations = dedupePaths(fileChange.changes).map(path => ({ path }));
-            toolCall.rawInput = {
-                changes: fileChange.changes.map(change => ({
-                    path: change.path,
-                    kind: change.kind.type,
-                    diff: change.diff,
-                })),
-            };
+            content.push(...await createFileChangeContents(fileChange.changes));
+            toolCall.locations = createFileChangeLocations(fileChange.changes);
+            toolCall.rawInput = createRawFileChangeInput(fileChange.changes);
         } else {
             const turnDiff = this.approvalContext.turnDiffsByTurnId.get(params.turnId);
             if (turnDiff) {
-                toolCall.rawInput = { unifiedDiff: turnDiff };
+                const parsedChanges = parseUnifiedDiffChanges(turnDiff);
+                content.push(...await createFileChangeContents(parsedChanges));
+                const locations = createFileChangeLocations(parsedChanges);
+                if (locations.length > 0) {
+                    toolCall.locations = locations;
+                }
+                toolCall.rawInput = parsedChanges.length > 0
+                    ? { unifiedDiff: turnDiff, ...createRawFileChangeInput(parsedChanges) }
+                    : { unifiedDiff: turnDiff };
             }
         }
+        if (content.length > 0) {
+            toolCall.content = content;
+        }
         return {
             sessionId,
             toolCall,
@@ -179,6 +175,7 @@ export class CodexApprovalHandler implements ApprovalHandler {
         const toolDescription = typeof meta?.["tool_description"] === "string"
             ? meta["tool_description"]
             : null;
+        const toolDescriptionContent = this.createTextContent(toolDescription);
         const rawInput = this.tryToJsonValue(meta?.["tool_params"]) ?? null;
 
         return {
@@ -188,7 +185,7 @@ export class CodexApprovalHandler implements ApprovalHandler {
                 title: params.message !== "" ? params.message : "MCP permission request",
                 kind: "other",
                 status: "pending",
-                content: this.createTextContents(toolDescription),
+                content: toolDescriptionContent ? [toolDescriptionContent] : null,
                 rawInput,
             },
             options: this.buildMcpServerElicitationOptions(persistOptions),
@@ -350,7 +347,3 @@ export class CodexApprovalHandler implements ApprovalHandler {
         return { action: "cancel", content: null, _meta: null }
     }
 }
-
-function dedupePaths(changes: Array<FileUpdateChange>): Array<string> {
-    return Array.from(new Set(changes.map(change => change.path)));
-}

src/CodexToolCallMapper.ts

@@ -40,13 +40,26 @@ export async function createFileChangeUpdate(
     item: ThreadItem & { type: "fileChange" }
 ): Promise<UpdateSessionEvent> {
     const patches = await createFileChangeContents(item.changes);
+    const details = createFileChangeDetails(item.changes);
     return {
         sessionUpdate: "tool_call",
         toolCallId: item.id,
-        title: "Editing files",
+        title: details.title,
         kind: "edit",
         status: toAcpStatus(item.status),
         content: patches,
+        locations: details.locations,
+        rawInput: details.rawInput,
+    };
+}
+
+export function createFileChangeCompletionUpdate(
+    item: ThreadItem & { type: "fileChange" }
+): UpdateSessionEvent {
+    return {
+        sessionUpdate: "tool_call_update",
+        toolCallId: item.id,
+        status: toAcpStatus(item.status),
     };
 }
 
@@ -60,6 +73,67 @@ export async function createFileChangeContents(changes: Array<FileUpdateChange>)
     return patches;
 }
 
+export function createFileChangeLocations(changes: Array<FileUpdateChange>): Array<{ path: string }> {
+    return Array.from(new Set(changes.map(change => change.path))).map(path => ({ path }));
+}
+
+export function createRawFileChangeInput(changes: Array<FileUpdateChange>): {
+    changes: Array<{
+        path: string;
+        kind: FileUpdateChange["kind"];
+        diff: string;
+    }>;
+} {
+    return {
+        changes: changes.map(change => ({
+            path: change.path,
+            kind: change.kind,
+            diff: change.diff,
+        })),
+    };
+}
+
+export function parseUnifiedDiffChanges(unifiedDiff: string): Array<FileUpdateChange> {
+    try {
+        return parsePatch(unifiedDiff)
+            .map(patch => {
+                const oldFileName = normalizeDiffPath(patch.oldFileName);
+                const newFileName = normalizeDiffPath(patch.newFileName);
+                const path = newFileName === "/dev/null" ? oldFileName : newFileName;
+                if (!path) {
+                    return null;
+                }
+                return {
+                    path,
+                    kind: toPatchChangeKind(oldFileName, newFileName),
+                    diff: formatParsedPatch(patch, oldFileName, newFileName),
+                } satisfies FileUpdateChange;
+            })
+            .filter((change): change is FileUpdateChange => change !== null);
+    } catch {
+        return [];
+    }
+}
+
+function createFileChangeDetails(changes: Array<FileUpdateChange>): {
+    title: string;
+    locations: Array<{ path: string }>;
+    rawInput: {
+        changes: Array<{
+            path: string;
+            kind: FileUpdateChange["kind"];
+            diff: string;
+        }>;
+    };
+} {
+    const uniquePaths = createFileChangeLocations(changes);
+    return {
+        title: uniquePaths.length > 0 ? uniquePaths.map(location => location.path).join(", ") : "File change",
+        locations: uniquePaths,
+        rawInput: createRawFileChangeInput(changes),
+    };
+}
+
 export async function createCommandExecutionUpdate(
     item: ThreadItem & { type: "commandExecution" }
 ): Promise<UpdateSessionEvent> {
@@ -313,6 +387,52 @@ function isUnifiedDiff(content: string): boolean {
     return content.startsWith("--- ") || content.includes("\n--- ");
 }
 
+function normalizeDiffPath(fileName: string | undefined): string | undefined {
+    if (!fileName || fileName === "/dev/null") {
+        return fileName;
+    }
+    return fileName.replace(/^[ab]\//, "");
+}
+
+function toPatchChangeKind(oldFileName: string | undefined, newFileName: string | undefined): FileUpdateChange["kind"] {
+    if (oldFileName === "/dev/null") {
+        return { type: "add" };
+    }
+    if (newFileName === "/dev/null") {
+        return { type: "delete" };
+    }
+    return {
+        type: "update",
+        move_path: oldFileName && newFileName && oldFileName !== newFileName ? oldFileName : null,
+    };
+}
+
+function formatParsedPatch(
+    patch: ReturnType<typeof parsePatch>[number],
+    oldFileName: string | undefined,
+    newFileName: string | undefined,
+): string {
+    const lines = [
+        `--- ${oldFileName ?? "/dev/null"}`,
+        `+++ ${newFileName ?? "/dev/null"}`,
+    ];
+    for (const hunk of patch.hunks) {
+        lines.push(`@@ -${formatHunkRange(hunk.oldStart, hunk.oldLines)} +${formatHunkRange(hunk.newStart, hunk.newLines)} @@`);
+        lines.push(...hunk.lines);
+    }
+    return lines.join("\n");
+}
+
+function formatHunkRange(start: number, lineCount: number): string {
+    if (lineCount === 0) {
+        return `${start - 1},0`;
+    }
+    if (lineCount === 1) {
+        return `${start}`;
+    }
+    return `${start},${lineCount}`;
+}
+
 /**
  * Recreates the content of a deleted file from the unified diff.
  * @param unifiedDiff The unified diff of the file deletion patch

src/__tests__/CodexACPAgent/approval-events.test.ts

@@ -37,7 +37,7 @@ describe('Approval Events', () => {
     function setupSessionWithPendingPrompt() {
         const codexAcpAgent = fixture.getCodexAcpAgent();
 
-        let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void;
+        let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void = () => {};
         const turnCompletedPromise = new Promise<{ threadId: string; turn: { id: string; items: never[]; status: string; error: null } }>((resolve) => {
             resolveTurnCompleted = resolve;
         });
@@ -61,7 +61,7 @@ describe('Approval Events', () => {
 
         return {
             promptPromise,
-            completeTurn: () => resolveTurnCompleted!({
+            completeTurn: () => resolveTurnCompleted({
                 threadId: sessionId,
                 turn: { id: "turn-id", items: [], status: "completed", error: null }
             })
@@ -400,5 +400,51 @@ describe('Approval Events', () => {
             completeTurn();
             await promptPromise;
         });
+
+        it('should include diff content from turn diff when file change item is unavailable', async () => {
+            const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
+            fixture.setPermissionResponse({
+                outcome: { outcome: 'selected', optionId: 'allow_once' }
+            });
+            mockFileContent('/test/project/config.json', '{"feature":false}');
+
+            const notification: ServerNotification = {
+                method: 'turn/diff/updated',
+                params: {
+                    threadId: sessionId,
+                    turnId: 'turn-1',
+                    diff: `diff --git a/test/project/config.json b/test/project/config.json
+index 0000000..1111111 100644
+--- a/test/project/config.json
++++ b/test/project/config.json
+@@ -1 +1 @@
+-{"feature":false}
++{"feature":true}`,
+                },
+            };
+
+            fixture.sendServerNotification(notification);
+            await Promise.resolve();
+
+            const params: FileChangeRequestApprovalParams = {
+                threadId: sessionId,
+                turnId: 'turn-1',
+                itemId: 'file-change-from-turn-diff',
+                reason: 'Updating config file',
+                grantRoot: null,
+            };
+
+            await fixture.sendServerRequest(
+                'item/fileChange/requestApproval',
+                params
+            );
+
+            await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
+                'data/approval-file-change-from-turn-diff.json'
+            );
+
+            completeTurn();
+            await promptPromise;
+        });
     });
 });

src/__tests__/CodexACPAgent/data/approval-file-change-from-turn-diff.json

@@ -0,0 +1,57 @@
+{
+  "method": "requestPermission",
+  "args": [
+    {
+      "sessionId": "test-session-id",
+      "toolCall": {
+        "toolCallId": "file-change-from-turn-diff",
+        "kind": "edit",
+        "status": "pending",
+        "locations": [
+          {
+            "path": "test/project/config.json"
+          }
+        ],
+        "rawInput": {
+          "unifiedDiff": "diff --git a/test/project/config.json b/test/project/config.json\nindex 0000000..1111111 100644\n--- a/test/project/config.json\n+++ b/test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}",
+          "changes": [
+            {
+              "path": "test/project/config.json",
+              "kind": {
+                "type": "update",
+                "move_path": null
+              },
+              "diff": "--- test/project/config.json\n+++ test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}"
+            }
+          ]
+        },
+        "content": [
+          {
+            "type": "content",
+            "content": {
+              "type": "text",
+              "text": "Updating config file"
+            }
+          }
+        ]
+      },
+      "options": [
+        {
+          "optionId": "allow_once",
+          "name": "Allow Once",
+          "kind": "allow_once"
+        },
+        {
+          "optionId": "allow_always",
+          "name": "Allow for Session",
+          "kind": "allow_always"
+        },
+        {
+          "optionId": "reject_once",
+          "name": "Reject",
+          "kind": "reject_once"
+        }
+      ]
+    }
+  ]
+}