diff --git a/src/CodexToolCallMapper.ts b/src/CodexToolCallMapper.ts index 88654a3e..80f2a8b9 100644 --- a/src/CodexToolCallMapper.ts +++ b/src/CodexToolCallMapper.ts @@ -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,56 +258,84 @@ function createSearchTitle(query: string | null, path: string | null): string { } async function createPatchContent(change: FileUpdateChange): Promise { - 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 { + let newText; + if (isUnifiedDiff(change.diff)) { + newText = applyPatch("", change.diff) + if (!newText) return null; + } else { + newText = change.diff; } return { type: "diff", - oldText: change.kind.type === "add" ? null : oldContent, + oldText: null, + newText: newText, + path: change.path, + _meta: { + kind: "add", + }, + }; +} + +async function createUpdateFileContent(change: FileUpdateChange): Promise { + const oldContent = await readFile(change.path, { encoding: "utf8" }).catch(() => null); + if (oldContent === null) return null; + + const unifiedDiff = recoverCorruptedDiff(change.diff); + const newContent = applyPatch(oldContent, unifiedDiff); + if (!newContent) return null; + + return { + type: "diff", + oldText: oldContent, newText: newContent, path: change.path, _meta: { - kind: change.kind.type, + kind: "update", }, }; } +async function createDeleteFileContent(change: FileUpdateChange): Promise { + // 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", + } + } +} + +/** + * Fix unified diff content corrupted by codex agent. + * Removes synthetic "Moved to" from the end. + */ +function recoverCorruptedDiff(diff: string): string { + return diff.replace(/\nMoved to: .*$/, ""); +} + function isUnifiedDiff(content: string): boolean { return content.startsWith("--- ") || content.includes("\n--- "); } diff --git a/src/__tests__/CodexACPAgent/file-change-events.test.ts b/src/__tests__/CodexACPAgent/file-change-events.test.ts index ce00135a..dafd3034 100644 --- a/src/__tests__/CodexACPAgent/file-change-events.test.ts +++ b/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"; @@ -271,4 +273,65 @@ describe('CodexEventHandler - file change events', () => { 'data/file-change-delete-raw-content.json' ); }); + + it('should ignore broken unified diffs in file changes', async () => { + const fileChange: ThreadItem = { + type: 'fileChange', + id: 'file-change-broken-diff', + changes: [ + { + path: '/test/project/BrokenFile.kt', + kind: { type: 'add' }, + diff: +`--- /dev/null ++++ /test/project/BrokenFile.kt +@@ broken @@ ++class BrokenFile +`, + }, + ], + 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', + }, + ], + }); + }); });