Suppress diff exceptions

src/CodexToolCallMapper.ts

@@ -1,5 +1,5 @@
 import type { ToolCallContent } from "@agentclientprotocol/sdk";
-import { applyPatch, parsePatch } from "diff";
+import { applyPatch, parsePatch, reversePatch } from "diff";
 import { readFile } from "node:fs/promises";
 import path from "node:path";
 import type { UpdateSessionEvent } from "./ACPSessionConnection";
@@ -20,6 +20,7 @@ import type {
     ThreadItem,
 } from "./app-server/v2";
 import type { JsonValue } from "./app-server/serde_json/JsonValue";
+import {logger} from "./Logger";
 
 type CodexItemStatus = CommandExecutionStatus | PatchApplyStatus | McpToolCallStatus | DynamicToolCallStatus;
 type AcpToolCallStatus = "pending" | "in_progress" | "completed" | "failed";
@@ -257,93 +258,98 @@ function createSearchTitle(query: string | null, path: string | null): string {
 }
 
 async function createPatchContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
-    if (change.kind.type === "add" && !isUnifiedDiff(change.diff)) {
-        // For new files, diff may contain raw file content instead of a patch.
-        return {
-            type: "diff",
-            oldText: null,
-            newText: change.diff,
-            path: change.path,
-            _meta: {
-                kind: "add",
-            },
-        };
-    }
-
-    if (change.kind.type === "delete") {
-        // If the patch deletes a file, the old content may be only available from the diff.
-        const oldContent = await readFile(change.path, { encoding: "utf8"} ).catch(() =>
-            isUnifiedDiff(change.diff) ? patchToDeletedContent(change.diff) : change.diff
-        );
-
-        return {
-            type: "diff",
-            oldText: oldContent,
-            newText: "",
-            path: change.path,
-            _meta: {
-                kind: "delete",
-            }
+    try {
+        switch (change.kind.type) {
+            case "add":
+                return await createAddFileContent(change);
+            case "delete":
+                return await createDeleteFileContent(change);
+            case "update":
+                return await createUpdateFileContent(change);
         }
-    }
-
-    const oldContent = change.kind.type === "add" ? "" : await readFile(change.path, { encoding: "utf8" }).catch(() => null);
-    if (oldContent === null) {
+    } catch (error) {
+        logger.log(`Error processing file update change: ${error}`);
         return null;
     }
+}
 
-    const newContent = applyPatch(oldContent, change.diff);
-    if (newContent === false) {
-        return null;
-    }
+async function createAddFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
     return {
         type: "diff",
-        oldText: change.kind.type === "add" ? null : oldContent,
-        newText: newContent,
+        oldText: null,
+        newText: change.diff, // app-server always returns file content instead of diff
         path: change.path,
         _meta: {
-            kind: change.kind.type,
+            kind: "add",
         },
     };
 }
 
-function isUnifiedDiff(content: string): boolean {
-    return content.startsWith("--- ") || content.includes("\n--- ");
+async function createUpdateFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
+    if (change.kind.type !== "update") return null;
+
+    const unifiedDiff = recoverCorruptedDiff(change.diff);
+    const movePath = change.kind.move_path;
+
+    const oldContent = await readFileContent(change.path);
+    if (oldContent !== null) {
+        const patchedContent = applyPatch(oldContent, unifiedDiff);
+        if (patchedContent === false) return null;
+        return createUpdateDiffContent(movePath ?? change.path, oldContent, patchedContent);
+    }
+
+    if (!movePath) return null;
+    const newContent = await readFileContent(movePath);
+    if (newContent === null) return null;
+
+    const revertedPatch = revertPatch(unifiedDiff);
+    if (!revertedPatch) return null;
+
+    const revertedContent = applyPatch(newContent, revertedPatch);
+    if (revertedContent === false) return null;
+
+    return createUpdateDiffContent(movePath, revertedContent, newContent);
 }
 
-/**
- * Recreates the content of a deleted file from the unified diff.
- * @param unifiedDiff The unified diff of the file deletion patch
- */
-function patchToDeletedContent(unifiedDiff: string): string | null {
-    try {
-        const [patch] = parsePatch(unifiedDiff);
-        if (!patch || patch.hunks.length === 0) {
-            return null;
-        }
+function revertPatch(unifiedDiff: string) {
+    const [patch] = parsePatch(unifiedDiff);
+    if (!patch) return null;
 
-        const oldLines: string[] = [];
-        let hasNoTrailingNewlineMarker = false;
-
-        for (const hunk of patch.hunks) {
-            for (const line of hunk.lines) {
-                if (line === "\\ No newline at end of file") {
-                    hasNoTrailingNewlineMarker = true;
-                    continue;
-                }
-                if (line.startsWith("-") || line.startsWith(" ")) {
-                    oldLines.push(line.slice(1));
-                }
-            }
-        }
+    return reversePatch(patch);
+}
 
-        if (oldLines.length === 0) {
-            return "";
-        }
+function createUpdateDiffContent(path: string, oldText: string, newText: string): ToolCallContent {
+    return {
+        type: "diff",
+        oldText,
+        newText,
+        path,
+        _meta: {
+            kind: "update",
+        },
+    };
+}
 
-        const oldText = oldLines.join("\n");
-        return hasNoTrailingNewlineMarker || !unifiedDiff.endsWith("\n") ? oldText : `${oldText}\n`;
-    } catch {
-        return null;
+async function createDeleteFileContent(change: FileUpdateChange): Promise<ToolCallContent> {
+    return {
+        type: "diff",
+        oldText: change.diff, // app-server always returns file content instead of diff
+        newText: "",
+        path: change.path,
+        _meta: {
+            kind: "delete",
+        }
     }
 }
+
+async function readFileContent(filePath: string): Promise<string | null> {
+    return await readFile(filePath, { encoding: "utf8" }).catch(() => null);
+}
+
+/**
+ * Fix unified diff content corrupted by codex agent.
+ * Removes synthetic "Moved to" from the end.
+ */
+function recoverCorruptedDiff(diff: string): string {
+    return diff.replace(/\n\nMoved to: .*$/, "");
+}

src/__tests__/CodexACPAgent/file-change-events.test.ts

@@ -1,6 +1,8 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import type { SessionState } from '../../CodexAcpServer';
 import type { ServerNotification } from '../../app-server';
+import { createFileChangeUpdate } from '../../CodexToolCallMapper';
+import type { ThreadItem } from '../../app-server/v2';
 import { createCodexMockTestFixture, createTestSessionState, setupPromptAndSendNotifications, type CodexMockTestFixture } from '../acp-test-utils';
 import {AgentMode} from "../../AgentMode";
 
@@ -53,14 +55,7 @@ describe('CodexEventHandler - file change events', () => {
                         {
                             path: '/test/project/NewFile.kt',
                             kind: { type: 'add' },
-                            diff: `--- /dev/null
-+++ /test/project/NewFile.kt
-@@ -0,0 +1,5 @@
-+package test.project
-+
-+class NewFile {
-+    fun hello() = "Hello"
-+}`,
+                            diff: 'package test.project\n\nclass NewFile {\n    fun hello() = "Hello"\n}\n',
                         },
                     ],
                     status: 'completed',
@@ -88,18 +83,12 @@ describe('CodexEventHandler - file change events', () => {
                         {
                             path: '/test/project/FileA.kt',
                             kind: { type: 'add' },
-                            diff: `--- /dev/null
-+++ /test/project/FileA.kt
-@@ -0,0 +1 @@
-+class FileA`,
+                            diff: 'class FileA\n',
                         },
                         {
                             path: '/test/project/FileB.kt',
                             kind: { type: 'add' },
-                            diff: `--- /dev/null
-+++ /test/project/FileB.kt
-@@ -0,0 +1 @@
-+class FileB`,
+                            diff: 'class FileB\n',
                         },
                     ],
                     status: 'completed',
@@ -156,12 +145,7 @@ describe('CodexEventHandler - file change events', () => {
                         {
                             path: '/test/project/OldFile.kt',
                             kind: { type: 'delete' },
-                            diff: `--- /test/project/OldFile.kt
-+++ /dev/null
-@@ -1,3 +0,0 @@
--package test.project
--
--class OldFile {}`,
+                            diff: 'package test.project\n\nclass OldFile {}',
                         },
                     ],
                     status: 'completed',
@@ -222,12 +206,7 @@ describe('CodexEventHandler - file change events', () => {
                         {
                             path: '/test/project/OldFile.kt',
                             kind: { type: 'delete' },
-                            diff: `--- /test/project/OldFile.kt
-+++ /dev/null
-@@ -1,3 +0,0 @@
--package test.project
--
--class OldFile {}`,
+                            diff: 'package test.project\n\nclass OldFile {}',
                         },
                     ],
                     status: 'completed',
@@ -271,4 +250,103 @@ describe('CodexEventHandler - file change events', () => {
             'data/file-change-delete-raw-content.json'
         );
     });
+
+    it('should ignore broken unified diffs in update file changes', async () => {
+        const fileChange: ThreadItem & { type: 'fileChange' } = {
+            type: 'fileChange',
+            id: 'file-change-broken-diff',
+            changes: [
+                {
+                    path: '/test/project/OldFile.kt',
+                    kind: { type: 'update', move_path: null },
+                    diff:
+`--- /test/project/OldFile.kt
++++ /test/project/OldFile.kt
+@@ broken @@
++class UpdatedFile
+`,
+                },
+            ],
+            status: 'completed',
+        };
+
+        const updateEvent = await createFileChangeUpdate(fileChange);
+        expect(updateEvent).toMatchObject({
+            content: [],
+        });
+    });
+
+    it('should parse update diffs with move metadata appended', async () => {
+        mockFileContent('/test/project/OriginalFile.kt', 'old code line\n');
+
+        const fileChange: ThreadItem = {
+            type: 'fileChange',
+            id: 'file-change-move-metadata',
+            changes: [
+                {
+                    path: '/test/project/OriginalFile.kt',
+                    kind: {
+                        type: 'update',
+                        move_path: '/test/project/NewFile.kt',
+                    },
+                    diff:
+`@@ -1 +1 @@
+-old code line
++new code line
+
+
+Moved to: /test/project/NewFile.kt`,
+                },
+            ],
+            status: 'inProgress',
+        };
+
+        const updateEvent = await createFileChangeUpdate(fileChange);
+        expect(updateEvent).toMatchObject({
+            content: [
+                {
+                    oldText: 'old code line\n',
+                    newText: 'new code line\n',
+                    path: '/test/project/NewFile.kt',
+                },
+            ],
+        });
+    });
+
+    it('should parse update diffs when the original file was moved already', async () => {
+        mockFileContent('/test/project/NewFile.kt', 'new code line\n');
+
+        const fileChange: ThreadItem = {
+            type: 'fileChange',
+            id: 'file-change-moved-file-exists',
+            changes: [
+                {
+                    path: '/test/project/OriginalFile.kt',
+                    kind: {
+                        type: 'update',
+                        move_path: '/test/project/NewFile.kt',
+                    },
+                    diff:
+`@@ -1 +1 @@
+-old code line
++new code line
+
+
+Moved to: /test/project/NewFile.kt`,
+                },
+            ],
+            status: 'inProgress',
+        };
+
+        const updateEvent = await createFileChangeUpdate(fileChange);
+        expect(updateEvent).toMatchObject({
+            content: [
+                {
+                    oldText: 'old code line\n',
+                    newText: 'new code line\n',
+                    path: '/test/project/NewFile.kt',
+                },
+            ],
+        });
+    });
 });

src/__tests__/CodexACPAgent/load-session.test.ts

@@ -108,13 +108,8 @@ describe("CodexACPAgent - loadSession", () => {
                                 {
                                     path: "/test/project/Added.txt",
                                     kind: { type: "add" },
-                                    diff: `--- /dev/null
-+++ /test/project/Added.txt
-@@ -0,0 +1,2 @@
-+Hello
-+World
-`,
-                                },
+                                    diff: "Hello\nWorld\n",
+                                }
                             ],
                             status: "completed",
                         },