diff --git a/src/CodexToolCallMapper.ts b/src/CodexToolCallMapper.ts index 88654a3e..ad2542ef 100644 --- a/src/CodexToolCallMapper.ts +++ b/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 { - 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 { 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 { + 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 { + 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 { + 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: .*$/, ""); +} diff --git a/src/__tests__/CodexACPAgent/file-change-events.test.ts b/src/__tests__/CodexACPAgent/file-change-events.test.ts index ce00135a..c8956394 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"; @@ -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', + }, + ], + }); + }); }); diff --git a/src/__tests__/CodexACPAgent/load-session.test.ts b/src/__tests__/CodexACPAgent/load-session.test.ts index 43aaa09a..0b9263c3 100644 --- a/src/__tests__/CodexACPAgent/load-session.test.ts +++ b/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", },