Skip to content

Commit 6ec28b8

Browse files
AlexandrSuhininAlexandrSuhinin
authored
Suppress diff exceptions (#167)
* clean: introduce exhaustive switch into `createPatchContent` function * fix: suppress exceptions caused by broken unified diffs from codex * fix: recover corrupted unified diff content * fix: recover moved fileUpdate diff from new file * clean: remove recovery from unified diff in add/delete file changes incorrect assumption that add/diff changes may have unified diff * fix: improve matching for corrupted diff
1 parent d3c7b3e commit 6ec28b8

3 files changed

Lines changed: 186 additions & 107 deletions

File tree

‎src/CodexToolCallMapper.ts‎

Lines changed: 78 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { ToolCallContent } from "@agentclientprotocol/sdk";
2-
import { applyPatch, parsePatch } from "diff";
2+
import { applyPatch, parsePatch, reversePatch } from "diff";
33
import { readFile } from "node:fs/promises";
44
import path from "node:path";
55
import type { UpdateSessionEvent } from "./ACPSessionConnection";
@@ -20,6 +20,7 @@ import type {
2020
ThreadItem,
2121
} from "./app-server/v2";
2222
import type { JsonValue } from "./app-server/serde_json/JsonValue";
23+
import {logger} from "./Logger";
2324

2425
type CodexItemStatus = CommandExecutionStatus | PatchApplyStatus | McpToolCallStatus | DynamicToolCallStatus;
2526
type AcpToolCallStatus = "pending" | "in_progress" | "completed" | "failed";
@@ -257,93 +258,98 @@ function createSearchTitle(query: string | null, path: string | null): string {
257258
}
258259

259260
async function createPatchContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
260-
if (change.kind.type === "add" && !isUnifiedDiff(change.diff)) {
261-
// For new files, diff may contain raw file content instead of a patch.
262-
return {
263-
type: "diff",
264-
oldText: null,
265-
newText: change.diff,
266-
path: change.path,
267-
_meta: {
268-
kind: "add",
269-
},
270-
};
271-
}
272-
273-
if (change.kind.type === "delete") {
274-
// If the patch deletes a file, the old content may be only available from the diff.
275-
const oldContent = await readFile(change.path, { encoding: "utf8"} ).catch(() =>
276-
isUnifiedDiff(change.diff) ? patchToDeletedContent(change.diff) : change.diff
277-
);
278-
279-
return {
280-
type: "diff",
281-
oldText: oldContent,
282-
newText: "",
283-
path: change.path,
284-
_meta: {
285-
kind: "delete",
286-
}
261+
try {
262+
switch (change.kind.type) {
263+
case "add":
264+
return await createAddFileContent(change);
265+
case "delete":
266+
return await createDeleteFileContent(change);
267+
case "update":
268+
return await createUpdateFileContent(change);
287269
}
288-
}
289-
290-
const oldContent = change.kind.type === "add" ? "" : await readFile(change.path, { encoding: "utf8" }).catch(() => null);
291-
if (oldContent === null) {
270+
} catch (error) {
271+
logger.log(`Error processing file update change: ${error}`);
292272
return null;
293273
}
274+
}
294275

295-
const newContent = applyPatch(oldContent, change.diff);
296-
if (newContent === false) {
297-
return null;
298-
}
276+
async function createAddFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
299277
return {
300278
type: "diff",
301-
oldText: change.kind.type === "add" ? null : oldContent,
302-
newText: newContent,
279+
oldText: null,
280+
newText: change.diff, // app-server always returns file content instead of diff
303281
path: change.path,
304282
_meta: {
305-
kind: change.kind.type,
283+
kind: "add",
306284
},
307285
};
308286
}
309287

310-
function isUnifiedDiff(content: string): boolean {
311-
return content.startsWith("--- ") || content.includes("\n--- ");
288+
async function createUpdateFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
289+
if (change.kind.type !== "update") return null;
290+
291+
const unifiedDiff = recoverCorruptedDiff(change.diff);
292+
const movePath = change.kind.move_path;
293+
294+
const oldContent = await readFileContent(change.path);
295+
if (oldContent !== null) {
296+
const patchedContent = applyPatch(oldContent, unifiedDiff);
297+
if (patchedContent === false) return null;
298+
return createUpdateDiffContent(movePath ?? change.path, oldContent, patchedContent);
299+
}
300+
301+
if (!movePath) return null;
302+
const newContent = await readFileContent(movePath);
303+
if (newContent === null) return null;
304+
305+
const revertedPatch = revertPatch(unifiedDiff);
306+
if (!revertedPatch) return null;
307+
308+
const revertedContent = applyPatch(newContent, revertedPatch);
309+
if (revertedContent === false) return null;
310+
311+
return createUpdateDiffContent(movePath, revertedContent, newContent);
312312
}
313313

314-
/**
315-
* Recreates the content of a deleted file from the unified diff.
316-
* @param unifiedDiff The unified diff of the file deletion patch
317-
*/
318-
function patchToDeletedContent(unifiedDiff: string): string | null {
319-
try {
320-
const [patch] = parsePatch(unifiedDiff);
321-
if (!patch || patch.hunks.length === 0) {
322-
return null;
323-
}
314+
function revertPatch(unifiedDiff: string) {
315+
const [patch] = parsePatch(unifiedDiff);
316+
if (!patch) return null;
324317

325-
const oldLines: string[] = [];
326-
let hasNoTrailingNewlineMarker = false;
327-
328-
for (const hunk of patch.hunks) {
329-
for (const line of hunk.lines) {
330-
if (line === "\\ No newline at end of file") {
331-
hasNoTrailingNewlineMarker = true;
332-
continue;
333-
}
334-
if (line.startsWith("-") || line.startsWith(" ")) {
335-
oldLines.push(line.slice(1));
336-
}
337-
}
338-
}
318+
return reversePatch(patch);
319+
}
339320

340-
if (oldLines.length === 0) {
341-
return "";
342-
}
321+
function createUpdateDiffContent(path: string, oldText: string, newText: string): ToolCallContent {
322+
return {
323+
type: "diff",
324+
oldText,
325+
newText,
326+
path,
327+
_meta: {
328+
kind: "update",
329+
},
330+
};
331+
}
343332

344-
const oldText = oldLines.join("\n");
345-
return hasNoTrailingNewlineMarker || !unifiedDiff.endsWith("\n") ? oldText : `${oldText}\n`;
346-
} catch {
347-
return null;
333+
async function createDeleteFileContent(change: FileUpdateChange): Promise<ToolCallContent> {
334+
return {
335+
type: "diff",
336+
oldText: change.diff, // app-server always returns file content instead of diff
337+
newText: "",
338+
path: change.path,
339+
_meta: {
340+
kind: "delete",
341+
}
348342
}
349343
}
344+
345+
async function readFileContent(filePath: string): Promise<string | null> {
346+
return await readFile(filePath, { encoding: "utf8" }).catch(() => null);
347+
}
348+
349+
/**
350+
* Fix unified diff content corrupted by codex agent.
351+
* Removes synthetic "Moved to" from the end.
352+
*/
353+
function recoverCorruptedDiff(diff: string): string {
354+
return diff.replace(/\n\nMoved to: .*$/, "");
355+
}

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

Lines changed: 106 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
import { describe, it, expect, vi, beforeEach } from 'vitest';
22
import type { SessionState } from '../../CodexAcpServer';
33
import type { ServerNotification } from '../../app-server';
4+
import { createFileChangeUpdate } from '../../CodexToolCallMapper';
5+
import type { ThreadItem } from '../../app-server/v2';
46
import { createCodexMockTestFixture, createTestSessionState, setupPromptAndSendNotifications, type CodexMockTestFixture } from '../acp-test-utils';
57
import {AgentMode} from "../../AgentMode";
68

@@ -53,14 +55,7 @@ describe('CodexEventHandler - file change events', () => {
5355
{
5456
path: '/test/project/NewFile.kt',
5557
kind: { type: 'add' },
56-
diff: `--- /dev/null
57-
+++ /test/project/NewFile.kt
58-
@@ -0,0 +1,5 @@
59-
+package test.project
60-
+
61-
+class NewFile {
62-
+ fun hello() = "Hello"
63-
+}`,
58+
diff: 'package test.project\n\nclass NewFile {\n fun hello() = "Hello"\n}\n',
6459
},
6560
],
6661
status: 'completed',
@@ -88,18 +83,12 @@ describe('CodexEventHandler - file change events', () => {
8883
{
8984
path: '/test/project/FileA.kt',
9085
kind: { type: 'add' },
91-
diff: `--- /dev/null
92-
+++ /test/project/FileA.kt
93-
@@ -0,0 +1 @@
94-
+class FileA`,
86+
diff: 'class FileA\n',
9587
},
9688
{
9789
path: '/test/project/FileB.kt',
9890
kind: { type: 'add' },
99-
diff: `--- /dev/null
100-
+++ /test/project/FileB.kt
101-
@@ -0,0 +1 @@
102-
+class FileB`,
91+
diff: 'class FileB\n',
10392
},
10493
],
10594
status: 'completed',
@@ -156,12 +145,7 @@ describe('CodexEventHandler - file change events', () => {
156145
{
157146
path: '/test/project/OldFile.kt',
158147
kind: { type: 'delete' },
159-
diff: `--- /test/project/OldFile.kt
160-
+++ /dev/null
161-
@@ -1,3 +0,0 @@
162-
-package test.project
163-
-
164-
-class OldFile {}`,
148+
diff: 'package test.project\n\nclass OldFile {}',
165149
},
166150
],
167151
status: 'completed',
@@ -222,12 +206,7 @@ describe('CodexEventHandler - file change events', () => {
222206
{
223207
path: '/test/project/OldFile.kt',
224208
kind: { type: 'delete' },
225-
diff: `--- /test/project/OldFile.kt
226-
+++ /dev/null
227-
@@ -1,3 +0,0 @@
228-
-package test.project
229-
-
230-
-class OldFile {}`,
209+
diff: 'package test.project\n\nclass OldFile {}',
231210
},
232211
],
233212
status: 'completed',
@@ -271,4 +250,103 @@ describe('CodexEventHandler - file change events', () => {
271250
'data/file-change-delete-raw-content.json'
272251
);
273252
});
253+
254+
it('should ignore broken unified diffs in update file changes', async () => {
255+
const fileChange: ThreadItem & { type: 'fileChange' } = {
256+
type: 'fileChange',
257+
id: 'file-change-broken-diff',
258+
changes: [
259+
{
260+
path: '/test/project/OldFile.kt',
261+
kind: { type: 'update', move_path: null },
262+
diff:
263+
`--- /test/project/OldFile.kt
264+
+++ /test/project/OldFile.kt
265+
@@ broken @@
266+
+class UpdatedFile
267+
`,
268+
},
269+
],
270+
status: 'completed',
271+
};
272+
273+
const updateEvent = await createFileChangeUpdate(fileChange);
274+
expect(updateEvent).toMatchObject({
275+
content: [],
276+
});
277+
});
278+
279+
it('should parse update diffs with move metadata appended', async () => {
280+
mockFileContent('/test/project/OriginalFile.kt', 'old code line\n');
281+
282+
const fileChange: ThreadItem = {
283+
type: 'fileChange',
284+
id: 'file-change-move-metadata',
285+
changes: [
286+
{
287+
path: '/test/project/OriginalFile.kt',
288+
kind: {
289+
type: 'update',
290+
move_path: '/test/project/NewFile.kt',
291+
},
292+
diff:
293+
`@@ -1 +1 @@
294+
-old code line
295+
+new code line
296+
297+
298+
Moved to: /test/project/NewFile.kt`,
299+
},
300+
],
301+
status: 'inProgress',
302+
};
303+
304+
const updateEvent = await createFileChangeUpdate(fileChange);
305+
expect(updateEvent).toMatchObject({
306+
content: [
307+
{
308+
oldText: 'old code line\n',
309+
newText: 'new code line\n',
310+
path: '/test/project/NewFile.kt',
311+
},
312+
],
313+
});
314+
});
315+
316+
it('should parse update diffs when the original file was moved already', async () => {
317+
mockFileContent('/test/project/NewFile.kt', 'new code line\n');
318+
319+
const fileChange: ThreadItem = {
320+
type: 'fileChange',
321+
id: 'file-change-moved-file-exists',
322+
changes: [
323+
{
324+
path: '/test/project/OriginalFile.kt',
325+
kind: {
326+
type: 'update',
327+
move_path: '/test/project/NewFile.kt',
328+
},
329+
diff:
330+
`@@ -1 +1 @@
331+
-old code line
332+
+new code line
333+
334+
335+
Moved to: /test/project/NewFile.kt`,
336+
},
337+
],
338+
status: 'inProgress',
339+
};
340+
341+
const updateEvent = await createFileChangeUpdate(fileChange);
342+
expect(updateEvent).toMatchObject({
343+
content: [
344+
{
345+
oldText: 'old code line\n',
346+
newText: 'new code line\n',
347+
path: '/test/project/NewFile.kt',
348+
},
349+
],
350+
});
351+
});
274352
});

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,8 @@ describe("CodexACPAgent - loadSession", () => {
108108
{
109109
path: "/test/project/Added.txt",
110110
kind: { type: "add" },
111-
diff: `--- /dev/null
112-
+++ /test/project/Added.txt
113-
@@ -0,0 +1,2 @@
114-
+Hello
115-
+World
116-
`,
117-
},
111+
diff: "Hello\nWorld\n",
112+
}
118113
],
119114
status: "completed",
120115
},

0 commit comments

Comments
 (0)