Skip to content

Commit 7916bdc

Browse files
committed
fix(tool-call-edit-diff): emit compact file-edit diffs and raw io
Replace whole-file diff expansion with hunk-local oldText/newText plus locations, normalized rawInput/rawOutput, and patchUpdated mapping for Codex file-change tool calls.
1 parent 3196231 commit 7916bdc

10 files changed

Lines changed: 395 additions & 145 deletions

src/CodexEventHandler.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,8 @@ import {
3939
createContextCompactionCompleteUpdate,
4040
createContextCompactionStartUpdate,
4141
createDynamicToolCallUpdate,
42+
createFileChangeCompleteUpdate,
43+
createFileChangePatchUpdate,
4244
createFileChangeUpdate,
4345
createGuardianApprovalReviewToolCall,
4446
createGuardianApprovalReviewToolCallUpdate,
@@ -188,6 +190,8 @@ export class CodexEventHandler {
188190
return this.createThreadGoalClearedEvent(notification.params);
189191
case "item/commandExecution/terminalInteraction":
190192
return this.createTerminalInteractionEvent(notification.params);
193+
case "item/fileChange/patchUpdated":
194+
return await createFileChangePatchUpdate(notification.params);
191195
// ignored events
192196
case "thread/deleted":
193197
case "command/exec/outputDelta":
@@ -196,7 +200,6 @@ export class CodexEventHandler {
196200
case "turn/diff/updated":
197201
case "turn/moderationMetadata":
198202
case "item/fileChange/outputDelta":
199-
case "item/fileChange/patchUpdated":
200203
case "account/updated":
201204
case "fs/changed":
202205
case "mcpServer/startupStatus/updated":
@@ -348,6 +351,7 @@ export class CodexEventHandler {
348351
private async completeItemEvent(event: ItemCompletedNotification): Promise<UpdateSessionEvent | null> {
349352
switch (event.item.type) {
350353
case "fileChange":
354+
return createFileChangeCompleteUpdate(event.item);
351355
case "dynamicToolCall":
352356
return {
353357
sessionUpdate: "tool_call_update",

src/CodexToolCallMapper.ts

Lines changed: 124 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
1-
import type { ContentBlock, ToolCallContent } from "@agentclientprotocol/sdk";
2-
import { applyPatch, parsePatch, reversePatch } from "diff";
3-
import { readFile } from "node:fs/promises";
1+
import type { ContentBlock, ToolCallContent, ToolCallLocation } from "@agentclientprotocol/sdk";
2+
import { parsePatch, type StructuredPatchHunk } from "diff";
43
import path from "node:path";
54
import type { UpdateSessionEvent } from "./ACPSessionConnection";
65
import { stripShellPrefix } from "./CommandUtils";
@@ -13,6 +12,7 @@ import type {
1312
CommandAction,
1413
CommandExecutionStatus,
1514
DynamicToolCallStatus,
15+
FileChangePatchUpdatedNotification,
1616
FileUpdateChange,
1717
GuardianApprovalReview,
1818
GuardianApprovalReviewAction,
@@ -42,6 +42,7 @@ type WebSearchItem = ThreadItem & { type: "webSearch" };
4242
type CollabAgentToolCallItem = ThreadItem & { type: "collabAgentToolCall" };
4343
type SubAgentActivityItem = ThreadItem & { type: "subAgentActivity" };
4444
type CommandExecutionItem = ThreadItem & { type: "commandExecution" };
45+
type FileChangeItem = ThreadItem & { type: "fileChange" };
4546
type ContextCompactionItem = ThreadItem & { type: "contextCompaction" };
4647
type AcpToolCallEvent = Extract<UpdateSessionEvent, { sessionUpdate: "tool_call" }>;
4748

@@ -60,21 +61,41 @@ function toAcpStatus(status: CodexItemStatus): AcpToolCallStatus {
6061
}
6162

6263
export async function createFileChangeUpdate(
63-
item: ThreadItem & { type: "fileChange" }
64+
item: FileChangeItem
6465
): Promise<UpdateSessionEvent> {
65-
const patches: ToolCallContent[] = [];
66-
for (const change of item.changes) {
67-
const content = await createPatchContent(change);
68-
if (content) patches.push(content);
69-
// ignore unparseable diffs
70-
}
66+
const rawOutput = createFileChangeRawOutput(item);
7167
return {
7268
sessionUpdate: "tool_call",
7369
toolCallId: item.id,
7470
title: "Editing files",
7571
kind: "edit",
7672
status: toAcpStatus(item.status),
77-
content: patches,
73+
content: await createFileChangeContent(item.changes),
74+
locations: createFileChangeLocations(item.changes),
75+
rawInput: createFileChangeRawInput(item.changes),
76+
...(rawOutput === undefined ? {} : { rawOutput }),
77+
};
78+
}
79+
80+
export async function createFileChangePatchUpdate(
81+
notification: FileChangePatchUpdatedNotification,
82+
): Promise<UpdateSessionEvent> {
83+
return {
84+
sessionUpdate: "tool_call_update",
85+
toolCallId: notification.itemId,
86+
status: "in_progress",
87+
content: await createFileChangeContent(notification.changes),
88+
locations: createFileChangeLocations(notification.changes),
89+
rawInput: createFileChangeRawInput(notification.changes),
90+
};
91+
}
92+
93+
export function createFileChangeCompleteUpdate(item: FileChangeItem): UpdateSessionEvent {
94+
return {
95+
sessionUpdate: "tool_call_update",
96+
toolCallId: item.id,
97+
status: toAcpStatus(item.status),
98+
rawOutput: createFileChangeRawOutput(item),
7899
};
79100
}
80101

@@ -801,23 +822,31 @@ function createContent(content: ContentBlock): ToolCallContent {
801822
};
802823
}
803824

804-
async function createPatchContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
825+
async function createFileChangeContent(changes: FileUpdateChange[]): Promise<ToolCallContent[]> {
826+
const content: ToolCallContent[] = [];
827+
for (const change of changes) {
828+
content.push(...await createPatchContent(change));
829+
}
830+
return content;
831+
}
832+
833+
async function createPatchContent(change: FileUpdateChange): Promise<ToolCallContent[]> {
805834
try {
806835
switch (change.kind.type) {
807836
case "add":
808-
return await createAddFileContent(change);
837+
return [createAddFileContent(change)];
809838
case "delete":
810-
return await createDeleteFileContent(change);
839+
return [createDeleteFileContent(change)];
811840
case "update":
812-
return await createUpdateFileContent(change);
841+
return createUpdateFileContent(change);
813842
}
814843
} catch (error) {
815844
logger.log(`Error processing file update change: ${error}`);
816-
return null;
845+
return [];
817846
}
818847
}
819848

820-
async function createAddFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
849+
function createAddFileContent(change: FileUpdateChange): ToolCallContent {
821850
return {
822851
type: "diff",
823852
oldText: null,
@@ -829,63 +858,46 @@ async function createAddFileContent(change: FileUpdateChange): Promise<ToolCallC
829858
};
830859
}
831860

832-
async function createUpdateFileContent(change: FileUpdateChange): Promise<ToolCallContent | null> {
833-
if (change.kind.type !== "update") return null;
834-
835-
const unifiedDiff = recoverCorruptedDiff(change.diff);
836-
const movePath = change.kind.move_path;
837-
838-
const oldContent = await readFileContent(change.path);
839-
if (oldContent !== null) {
840-
const patchedContent = applyPatch(oldContent, unifiedDiff);
841-
if (patchedContent === false) {
842-
// If Codex runs in full access mode, the file might already be patched.
843-
// we can verify this by checking if the reverted patch applies.
844-
const revertedPatch = revertPatch(unifiedDiff);
845-
if (revertedPatch) {
846-
const revertedContent = applyPatch(oldContent, revertedPatch);
847-
if (revertedContent !== false) {
848-
return createUpdateDiffContent(change.path, revertedContent, oldContent);
849-
}
850-
}
851-
return null;
852-
}
853-
return createUpdateDiffContent(movePath ?? change.path, oldContent, patchedContent);
854-
}
855-
856-
if (!movePath) return null;
857-
const newContent = await readFileContent(movePath);
858-
if (newContent === null) return null;
859-
860-
const revertedPatch = revertPatch(unifiedDiff);
861-
if (!revertedPatch) return null;
862-
863-
const revertedContent = applyPatch(newContent, revertedPatch);
864-
if (revertedContent === false) return null;
865-
866-
return createUpdateDiffContent(movePath, revertedContent, newContent);
867-
}
868-
869-
function revertPatch(unifiedDiff: string) {
870-
const [patch] = parsePatch(unifiedDiff);
871-
if (!patch) return null;
861+
function createUpdateFileContent(change: FileUpdateChange): ToolCallContent[] {
862+
if (change.kind.type !== "update") return [];
872863

873-
return reversePatch(patch);
864+
const patches = parsePatch(recoverCorruptedDiff(change.diff));
865+
const targetPath = change.kind.move_path ?? change.path;
866+
return patches.flatMap((patch) => patch.hunks.map((hunk) => createUpdateDiffContent(targetPath, hunk)));
874867
}
875868

876-
function createUpdateDiffContent(path: string, oldText: string, newText: string): ToolCallContent {
869+
function createUpdateDiffContent(path: string, hunk: StructuredPatchHunk): ToolCallContent {
877870
return {
878871
type: "diff",
879-
oldText,
880-
newText,
872+
oldText: createHunkText(hunk, "old"),
873+
newText: createHunkText(hunk, "new"),
881874
path,
882875
_meta: {
883876
kind: "update",
877+
old_start: hunk.oldStart,
878+
new_start: hunk.newStart,
884879
},
885880
};
886881
}
887882

888-
async function createDeleteFileContent(change: FileUpdateChange): Promise<ToolCallContent> {
883+
function createHunkText(hunk: StructuredPatchHunk, side: "old" | "new"): string {
884+
return hunk.lines.flatMap((line): string[] => {
885+
switch (line[0]) {
886+
case " ":
887+
return [line.slice(1)];
888+
case "-":
889+
return side === "old" ? [line.slice(1)] : [];
890+
case "+":
891+
return side === "new" ? [line.slice(1)] : [];
892+
case "\\":
893+
return [];
894+
default:
895+
return [];
896+
}
897+
}).join("\n");
898+
}
899+
900+
function createDeleteFileContent(change: FileUpdateChange): ToolCallContent {
889901
return {
890902
type: "diff",
891903
oldText: change.diff, // app-server always returns file content instead of diff
@@ -897,8 +909,56 @@ async function createDeleteFileContent(change: FileUpdateChange): Promise<ToolCa
897909
}
898910
}
899911

900-
async function readFileContent(filePath: string): Promise<string | null> {
901-
return await readFile(filePath, { encoding: "utf8" }).catch(() => null);
912+
function createFileChangeLocations(changes: FileUpdateChange[]): ToolCallLocation[] {
913+
const locations = new Map<string, ToolCallLocation>();
914+
const addLocation = (filePath: string, line?: number) => {
915+
const current = locations.get(filePath);
916+
if (current?.line !== undefined || (current && line === undefined)) {
917+
return;
918+
}
919+
locations.set(filePath, line === undefined ? { path: filePath } : { path: filePath, line });
920+
};
921+
922+
for (const change of changes) {
923+
switch (change.kind.type) {
924+
case "add":
925+
case "delete":
926+
addLocation(change.path);
927+
break;
928+
case "update": {
929+
const firstHunk = firstUpdateHunk(change);
930+
if (change.kind.move_path && change.kind.move_path !== change.path) {
931+
addLocation(change.path, firstHunk?.oldStart);
932+
}
933+
addLocation(change.kind.move_path ?? change.path, firstHunk?.newStart);
934+
break;
935+
}
936+
}
937+
}
938+
939+
return [...locations.values()];
940+
}
941+
942+
function firstUpdateHunk(change: FileUpdateChange): StructuredPatchHunk | undefined {
943+
try {
944+
return parsePatch(recoverCorruptedDiff(change.diff))[0]?.hunks[0];
945+
} catch {
946+
return undefined;
947+
}
948+
}
949+
950+
function createFileChangeRawInput(changes: FileUpdateChange[]) {
951+
return { changes };
952+
}
953+
954+
function createFileChangeRawOutput(item: FileChangeItem): Record<string, unknown> | undefined {
955+
if (item.status === "inProgress") {
956+
return undefined;
957+
}
958+
return {
959+
status: item.status,
960+
success: item.status === "completed",
961+
};
902962
}
903963

904964
/**

src/__tests__/CodexACPAgent/data/file-change-add-multiple-files.json

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,37 @@
2828
"kind": "add"
2929
}
3030
}
31-
]
31+
],
32+
"locations": [
33+
{
34+
"path": "/test/project/FileA.kt"
35+
},
36+
{
37+
"path": "/test/project/FileB.kt"
38+
}
39+
],
40+
"rawInput": {
41+
"changes": [
42+
{
43+
"path": "/test/project/FileA.kt",
44+
"kind": {
45+
"type": "add"
46+
},
47+
"diff": "class FileA\n"
48+
},
49+
{
50+
"path": "/test/project/FileB.kt",
51+
"kind": {
52+
"type": "add"
53+
},
54+
"diff": "class FileB\n"
55+
}
56+
]
57+
},
58+
"rawOutput": {
59+
"status": "completed",
60+
"success": true
61+
}
3262
}
3363
}
3464
]

src/__tests__/CodexACPAgent/data/file-change-add-new-file.json

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,27 @@
1919
"kind": "add"
2020
}
2121
}
22-
]
22+
],
23+
"locations": [
24+
{
25+
"path": "/test/project/NewFile.kt"
26+
}
27+
],
28+
"rawInput": {
29+
"changes": [
30+
{
31+
"path": "/test/project/NewFile.kt",
32+
"kind": {
33+
"type": "add"
34+
},
35+
"diff": "package test.project\n\nclass NewFile {\n fun hello() = \"Hello\"\n}\n"
36+
}
37+
]
38+
},
39+
"rawOutput": {
40+
"status": "completed",
41+
"success": true
42+
}
2343
}
2444
}
2545
]

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

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,27 @@
1919
"kind": "add"
2020
}
2121
}
22-
]
22+
],
23+
"locations": [
24+
{
25+
"path": "/test/project/RawFile.kt"
26+
}
27+
],
28+
"rawInput": {
29+
"changes": [
30+
{
31+
"path": "/test/project/RawFile.kt",
32+
"kind": {
33+
"type": "add"
34+
},
35+
"diff": "fun main() {\n println(\"Hello, World!\")\n}\n"
36+
}
37+
]
38+
},
39+
"rawOutput": {
40+
"status": "completed",
41+
"success": true
42+
}
2343
}
2444
}
2545
]

0 commit comments

Comments
 (0)