Skip to content

Commit 4785f42

Browse files
committed
LLM-25262 Special handling of file deletion when processing patches
Codex can delete the file after the patch has been created. To avoid accessing non-existing files, file update changes with the kind 'delete' are processed as follows. - If the file still exists, its old content is read directly from the file. The new content is always set to an empty string (it's consistent with the IntelliJ Idea ACP client handling of file deletion). - If the file can't be read, the old content is restored from the patch itself.
1 parent ada7daf commit 4785f42

3 files changed

Lines changed: 129 additions & 4 deletions

File tree

src/CodexToolCallMapper.ts

Lines changed: 61 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import type { ToolCallContent } from "@agentclientprotocol/sdk";
2-
import { applyPatch } from "diff";
2+
import { applyPatch, parsePatch } from "diff";
33
import { readFile } from "node:fs/promises";
44
import path from "node:path";
55
import type { UpdateSessionEvent } from "./ACPSessionConnection";
@@ -86,6 +86,7 @@ export async function createMcpToolCallUpdate(
8686
): Promise<UpdateSessionEvent> {
8787
return createExecuteToolCallUpdate(item, `mcp.${item.server}.${item.tool}`);
8888
}
89+
8990
export async function createDynamicToolCallUpdate(
9091
item: ThreadItem & { type: "dynamicToolCall" }
9192
): Promise<UpdateSessionEvent> {
@@ -235,7 +236,28 @@ async function createPatchContent(change: FileUpdateChange): Promise<ToolCallCon
235236
};
236237
}
237238

238-
const oldContent = change.kind.type === "add" ? "" : await readFile(change.path, { encoding: "utf8" });
239+
if (change.kind.type === "delete") {
240+
// If the patch deletes a file, the old content may be only available from the diff.
241+
const oldContent = await readFile(change.path, { encoding: "utf8"} ).catch(() =>
242+
isUnifiedDiff(change.diff) ? patchToDeletedContent(change.diff) : change.diff
243+
);
244+
245+
return {
246+
type: "diff",
247+
oldText: oldContent,
248+
newText: "",
249+
path: change.path,
250+
_meta: {
251+
kind: "delete",
252+
}
253+
}
254+
}
255+
256+
const oldContent = change.kind.type === "add" ? "" : await readFile(change.path, { encoding: "utf8" }).catch(() => null);
257+
if (oldContent === null) {
258+
return null;
259+
}
260+
239261
const newContent = applyPatch(oldContent, change.diff);
240262
if (newContent === false) {
241263
return null;
@@ -254,3 +276,40 @@ async function createPatchContent(change: FileUpdateChange): Promise<ToolCallCon
254276
function isUnifiedDiff(content: string): boolean {
255277
return content.startsWith("--- ") || content.includes("\n--- ");
256278
}
279+
280+
/**
281+
* Recreates the content of a deleted file from the unified diff.
282+
* @param unifiedDiff The unified diff of the file deletion patch
283+
*/
284+
function patchToDeletedContent(unifiedDiff: string): string | null {
285+
try {
286+
const [patch] = parsePatch(unifiedDiff);
287+
if (!patch || patch.hunks.length === 0) {
288+
return null;
289+
}
290+
291+
const oldLines: string[] = [];
292+
let hasNoTrailingNewlineMarker = false;
293+
294+
for (const hunk of patch.hunks) {
295+
for (const line of hunk.lines) {
296+
if (line === "\\ No newline at end of file") {
297+
hasNoTrailingNewlineMarker = true;
298+
continue;
299+
}
300+
if (line.startsWith("-") || line.startsWith(" ")) {
301+
oldLines.push(line.slice(1));
302+
}
303+
}
304+
}
305+
306+
if (oldLines.length === 0) {
307+
return "";
308+
}
309+
310+
const oldText = oldLines.join("\n");
311+
return hasNoTrailingNewlineMarker || !unifiedDiff.endsWith("\n") ? oldText : `${oldText}\n`;
312+
} catch {
313+
return null;
314+
}
315+
}

src/__tests__/CodexACPAgent/data/file-change-delete-raw-content.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
{
1414
"type": "diff",
1515
"oldText": "fun main() {\n println(\"Hello, World!\")\n}\n",
16-
"newText": "fun main() {\n println(\"Hello, World!\")\n}\n",
16+
"newText": "",
1717
"path": "/test/project/RawDeleteFile.kt",
1818
"_meta": {
1919
"kind": "delete"

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

Lines changed: 67 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@ import type { ServerNotification } from '../../app-server';
44
import { createCodexMockTestFixture, createTestSessionState, setupPromptAndSendNotifications, type CodexMockTestFixture } from '../acp-test-utils';
55
import {AgentMode} from "../../AgentMode";
66

7-
const { mockFiles, mockFileContent, clearMockFiles } = vi.hoisted(() => {
7+
const { mockFiles, mockFileContent, removeMockFile, clearMockFiles } = vi.hoisted(() => {
88
const files = new Map<string, string>();
99
return {
1010
mockFiles: files,
1111
mockFileContent: (path: string, content: string) => files.set(path, content),
12+
removeMockFile: (path: string) => files.delete(path),
1213
clearMockFiles: () => files.clear(),
1314
};
1415
});
@@ -205,4 +206,69 @@ describe('CodexEventHandler - file change events', () => {
205206
'data/file-change-delete-raw-content.json'
206207
);
207208
});
209+
210+
it('should handle file deletion when old file is already missing', async () => {
211+
removeMockFile('/test/project/OldFile.kt');
212+
213+
const deleteFileNotification: ServerNotification = {
214+
method: 'item/started',
215+
params: {
216+
threadId: 'thread-1',
217+
turnId: 'turn-1',
218+
item: {
219+
type: 'fileChange',
220+
id: 'file-change-3',
221+
changes: [
222+
{
223+
path: '/test/project/OldFile.kt',
224+
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 {}`,
231+
},
232+
],
233+
status: 'completed',
234+
},
235+
},
236+
};
237+
238+
await setupPromptAndSendNotifications(mockFixture, sessionId, sessionState, [deleteFileNotification]);
239+
240+
await expect(mockFixture.getAcpConnectionDump(['id'])).toMatchFileSnapshot(
241+
'data/file-change-delete-file.json'
242+
);
243+
});
244+
245+
it('should handle file deletion with raw content when old file is already missing', async () => {
246+
removeMockFile('/test/project/RawDeleteFile.kt');
247+
248+
const deletedFileNotification: ServerNotification = {
249+
method: 'item/started',
250+
params: {
251+
threadId: 'thread-1',
252+
turnId: 'turn-1',
253+
item: {
254+
type: 'fileChange',
255+
id: 'file-delete-raw',
256+
changes: [
257+
{
258+
path: '/test/project/RawDeleteFile.kt',
259+
kind: { type: 'delete' },
260+
diff: 'fun main() {\n println("Hello, World!")\n}\n',
261+
},
262+
],
263+
status: 'completed',
264+
},
265+
},
266+
};
267+
268+
await setupPromptAndSendNotifications(mockFixture, sessionId, sessionState, [deletedFileNotification]);
269+
270+
await expect(mockFixture.getAcpConnectionDump(['id'])).toMatchFileSnapshot(
271+
'data/file-change-delete-raw-content.json'
272+
);
273+
});
208274
});

0 commit comments

Comments
 (0)