Skip to content

Commit 6c83fc4

Browse files
i1broi1bro
committed
Attach file paths and diffs to edit tool calls
1 parent d53d7b8 commit 6c83fc4

11 files changed

Lines changed: 383 additions & 52 deletions

‎src/CodexApprovalHandler.ts‎

Lines changed: 22 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,12 @@ import type {ToolCallContent} from "@agentclientprotocol/sdk/dist/schema/types.g
1212
import {logger} from "./Logger";
1313
import {stripShellPrefix} from "./CodexEventHandler";
1414
import type {ApprovalContextStore} from "./CodexApprovalContext";
15-
import {createFileChangeContents} from "./CodexToolCallMapper";
15+
import {
16+
createFileChangeContents,
17+
createFileChangeLocations,
18+
createRawFileChangeInput,
19+
parseUnifiedDiffChanges,
20+
} from "./CodexToolCallMapper";
1621

1722
const APPROVAL_OPTIONS: acp.PermissionOption[] = [
1823
{ optionId: "allow_once", name: "Allow Once", kind: "allow_once" },
@@ -100,34 +105,33 @@ export class CodexApprovalHandler implements ApprovalHandler {
100105
): Promise<acp.RequestPermissionRequest> {
101106
const reasonContent = this.createTextContent(params.reason ?? null);
102107
const fileChange = this.approvalContext.fileChangesByItemId.get(params.itemId);
103-
const diffContent = fileChange ? await createFileChangeContents(fileChange.changes) : [];
108+
const content: ToolCallContent[] = reasonContent ? [reasonContent] : [];
104109
const toolCall: acp.ToolCallUpdate = {
105110
toolCallId: params.itemId,
106111
kind: "edit",
107112
status: "pending",
108113
};
109-
const content = [
110-
...(reasonContent ? [reasonContent] : []),
111-
...diffContent,
112-
];
113-
if (content.length > 0) {
114-
toolCall.content = content;
115-
}
116114
if (fileChange) {
117-
toolCall.locations = dedupePaths(fileChange.changes).map(path => ({ path }));
118-
toolCall.rawInput = {
119-
changes: fileChange.changes.map(change => ({
120-
path: change.path,
121-
kind: change.kind.type,
122-
diff: change.diff,
123-
})),
124-
};
115+
content.push(...await createFileChangeContents(fileChange.changes));
116+
toolCall.locations = createFileChangeLocations(fileChange.changes);
117+
toolCall.rawInput = createRawFileChangeInput(fileChange.changes);
125118
} else {
126119
const turnDiff = this.approvalContext.turnDiffsByTurnId.get(params.turnId);
127120
if (turnDiff) {
128-
toolCall.rawInput = { unifiedDiff: turnDiff };
121+
const parsedChanges = parseUnifiedDiffChanges(turnDiff);
122+
content.push(...await createFileChangeContents(parsedChanges));
123+
const locations = createFileChangeLocations(parsedChanges);
124+
if (locations.length > 0) {
125+
toolCall.locations = locations;
126+
}
127+
toolCall.rawInput = parsedChanges.length > 0
128+
? { unifiedDiff: turnDiff, ...createRawFileChangeInput(parsedChanges) }
129+
: { unifiedDiff: turnDiff };
129130
}
130131
}
132+
if (content.length > 0) {
133+
toolCall.content = content;
134+
}
131135
return {
132136
sessionId,
133137
toolCall,
@@ -169,7 +173,3 @@ export class CodexApprovalHandler implements ApprovalHandler {
169173
}
170174
}
171175
}
172-
173-
function dedupePaths(changes: Array<FileUpdateChange>): Array<string> {
174-
return Array.from(new Set(changes.map(change => change.path)));
175-
}

‎src/CodexToolCallMapper.ts‎

Lines changed: 121 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,26 @@ export async function createFileChangeUpdate(
4040
item: ThreadItem & { type: "fileChange" }
4141
): Promise<UpdateSessionEvent> {
4242
const patches = await createFileChangeContents(item.changes);
43+
const details = createFileChangeDetails(item.changes);
4344
return {
4445
sessionUpdate: "tool_call",
4546
toolCallId: item.id,
46-
title: "Editing files",
47+
title: details.title,
4748
kind: "edit",
4849
status: toAcpStatus(item.status),
4950
content: patches,
51+
locations: details.locations,
52+
rawInput: details.rawInput,
53+
};
54+
}
55+
56+
export function createFileChangeCompletionUpdate(
57+
item: ThreadItem & { type: "fileChange" }
58+
): UpdateSessionEvent {
59+
return {
60+
sessionUpdate: "tool_call_update",
61+
toolCallId: item.id,
62+
status: toAcpStatus(item.status),
5063
};
5164
}
5265

@@ -60,6 +73,67 @@ export async function createFileChangeContents(changes: Array<FileUpdateChange>)
6073
return patches;
6174
}
6275

76+
export function createFileChangeLocations(changes: Array<FileUpdateChange>): Array<{ path: string }> {
77+
return Array.from(new Set(changes.map(change => change.path))).map(path => ({ path }));
78+
}
79+
80+
export function createRawFileChangeInput(changes: Array<FileUpdateChange>): {
81+
changes: Array<{
82+
path: string;
83+
kind: FileUpdateChange["kind"];
84+
diff: string;
85+
}>;
86+
} {
87+
return {
88+
changes: changes.map(change => ({
89+
path: change.path,
90+
kind: change.kind,
91+
diff: change.diff,
92+
})),
93+
};
94+
}
95+
96+
export function parseUnifiedDiffChanges(unifiedDiff: string): Array<FileUpdateChange> {
97+
try {
98+
return parsePatch(unifiedDiff)
99+
.map(patch => {
100+
const oldFileName = normalizeDiffPath(patch.oldFileName);
101+
const newFileName = normalizeDiffPath(patch.newFileName);
102+
const path = newFileName === "/dev/null" ? oldFileName : newFileName;
103+
if (!path) {
104+
return null;
105+
}
106+
return {
107+
path,
108+
kind: toPatchChangeKind(oldFileName, newFileName),
109+
diff: formatParsedPatch(patch, oldFileName, newFileName),
110+
} satisfies FileUpdateChange;
111+
})
112+
.filter((change): change is FileUpdateChange => change !== null);
113+
} catch {
114+
return [];
115+
}
116+
}
117+
118+
function createFileChangeDetails(changes: Array<FileUpdateChange>): {
119+
title: string;
120+
locations: Array<{ path: string }>;
121+
rawInput: {
122+
changes: Array<{
123+
path: string;
124+
kind: FileUpdateChange["kind"];
125+
diff: string;
126+
}>;
127+
};
128+
} {
129+
const uniquePaths = createFileChangeLocations(changes);
130+
return {
131+
title: uniquePaths.length > 0 ? uniquePaths.map(location => location.path).join(", ") : "File change",
132+
locations: uniquePaths,
133+
rawInput: createRawFileChangeInput(changes),
134+
};
135+
}
136+
63137
export async function createCommandExecutionUpdate(
64138
item: ThreadItem & { type: "commandExecution" }
65139
): Promise<UpdateSessionEvent> {
@@ -313,6 +387,52 @@ function isUnifiedDiff(content: string): boolean {
313387
return content.startsWith("--- ") || content.includes("\n--- ");
314388
}
315389

390+
function normalizeDiffPath(fileName: string | undefined): string | undefined {
391+
if (!fileName || fileName === "/dev/null") {
392+
return fileName;
393+
}
394+
return fileName.replace(/^[ab]\//, "");
395+
}
396+
397+
function toPatchChangeKind(oldFileName: string | undefined, newFileName: string | undefined): FileUpdateChange["kind"] {
398+
if (oldFileName === "/dev/null") {
399+
return { type: "add" };
400+
}
401+
if (newFileName === "/dev/null") {
402+
return { type: "delete" };
403+
}
404+
return {
405+
type: "update",
406+
move_path: oldFileName && newFileName && oldFileName !== newFileName ? oldFileName : null,
407+
};
408+
}
409+
410+
function formatParsedPatch(
411+
patch: ReturnType<typeof parsePatch>[number],
412+
oldFileName: string | undefined,
413+
newFileName: string | undefined,
414+
): string {
415+
const lines = [
416+
`--- ${oldFileName ?? "/dev/null"}`,
417+
`+++ ${newFileName ?? "/dev/null"}`,
418+
];
419+
for (const hunk of patch.hunks) {
420+
lines.push(`@@ -${formatHunkRange(hunk.oldStart, hunk.oldLines)} +${formatHunkRange(hunk.newStart, hunk.newLines)} @@`);
421+
lines.push(...hunk.lines);
422+
}
423+
return lines.join("\n");
424+
}
425+
426+
function formatHunkRange(start: number, lineCount: number): string {
427+
if (lineCount === 0) {
428+
return `${start - 1},0`;
429+
}
430+
if (lineCount === 1) {
431+
return `${start}`;
432+
}
433+
return `${start},${lineCount}`;
434+
}
435+
316436
/**
317437
* Recreates the content of a deleted file from the unified diff.
318438
* @param unifiedDiff The unified diff of the file deletion patch

‎src/__tests__/CodexACPAgent/approval-events.test.ts‎

Lines changed: 48 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ describe('Approval Events', () => {
3737
function setupSessionWithPendingPrompt() {
3838
const codexAcpAgent = fixture.getCodexAcpAgent();
3939

40-
let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void;
40+
let resolveTurnCompleted: (value: { threadId: string; turn: { id: string; items: never[]; status: string; error: null } }) => void = () => {};
4141
const turnCompletedPromise = new Promise<{ threadId: string; turn: { id: string; items: never[]; status: string; error: null } }>((resolve) => {
4242
resolveTurnCompleted = resolve;
4343
});
@@ -61,7 +61,7 @@ describe('Approval Events', () => {
6161

6262
return {
6363
promptPromise,
64-
completeTurn: () => resolveTurnCompleted!({
64+
completeTurn: () => resolveTurnCompleted({
6565
threadId: sessionId,
6666
turn: { id: "turn-id", items: [], status: "completed", error: null }
6767
})
@@ -400,5 +400,51 @@ describe('Approval Events', () => {
400400
completeTurn();
401401
await promptPromise;
402402
});
403+
404+
it('should include diff content from turn diff when file change item is unavailable', async () => {
405+
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
406+
fixture.setPermissionResponse({
407+
outcome: { outcome: 'selected', optionId: 'allow_once' }
408+
});
409+
mockFileContent('/test/project/config.json', '{"feature":false}');
410+
411+
const notification: ServerNotification = {
412+
method: 'turn/diff/updated',
413+
params: {
414+
threadId: sessionId,
415+
turnId: 'turn-1',
416+
diff: `diff --git a/test/project/config.json b/test/project/config.json
417+
index 0000000..1111111 100644
418+
--- a/test/project/config.json
419+
+++ b/test/project/config.json
420+
@@ -1 +1 @@
421+
-{"feature":false}
422+
+{"feature":true}`,
423+
},
424+
};
425+
426+
fixture.sendServerNotification(notification);
427+
await Promise.resolve();
428+
429+
const params: FileChangeRequestApprovalParams = {
430+
threadId: sessionId,
431+
turnId: 'turn-1',
432+
itemId: 'file-change-from-turn-diff',
433+
reason: 'Updating config file',
434+
grantRoot: null,
435+
};
436+
437+
await fixture.sendServerRequest(
438+
'item/fileChange/requestApproval',
439+
params
440+
);
441+
442+
await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
443+
'data/approval-file-change-from-turn-diff.json'
444+
);
445+
446+
completeTurn();
447+
await promptPromise;
448+
});
403449
});
404450
});

‎src/__tests__/CodexACPAgent/data/approval-file-change-from-turn-diff.json‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
{
2+
"method": "requestPermission",
3+
"args": [
4+
{
5+
"sessionId": "test-session-id",
6+
"toolCall": {
7+
"toolCallId": "file-change-from-turn-diff",
8+
"kind": "edit",
9+
"status": "pending",
10+
"locations": [
11+
{
12+
"path": "test/project/config.json"
13+
}
14+
],
15+
"rawInput": {
16+
"unifiedDiff": "diff --git a/test/project/config.json b/test/project/config.json\nindex 0000000..1111111 100644\n--- a/test/project/config.json\n+++ b/test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}",
17+
"changes": [
18+
{
19+
"path": "test/project/config.json",
20+
"kind": {
21+
"type": "update",
22+
"move_path": null
23+
},
24+
"diff": "--- test/project/config.json\n+++ test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}"
25+
}
26+
]
27+
},
28+
"content": [
29+
{
30+
"type": "content",
31+
"content": {
32+
"type": "text",
33+
"text": "Updating config file"
34+
}
35+
}
36+
]
37+
},
38+
"options": [
39+
{
40+
"optionId": "allow_once",
41+
"name": "Allow Once",
42+
"kind": "allow_once"
43+
},
44+
{
45+
"optionId": "allow_always",
46+
"name": "Allow for Session",
47+
"kind": "allow_always"
48+
},
49+
{
50+
"optionId": "reject_once",
51+
"name": "Reject",
52+
"kind": "reject_once"
53+
}
54+
]
55+
}
56+
]
57+
}

‎src/__tests__/CodexACPAgent/data/approval-file-change-with-diff.json‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,22 @@
77
"toolCallId": "file-change-with-diff",
88
"kind": "edit",
99
"status": "pending",
10+
"locations": [
11+
{
12+
"path": "/test/project/config.json"
13+
}
14+
],
15+
"rawInput": {
16+
"changes": [
17+
{
18+
"path": "/test/project/config.json",
19+
"kind": {
20+
"type": "update"
21+
},
22+
"diff": "--- /test/project/config.json\n+++ /test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}"
23+
}
24+
]
25+
},
1026
"content": [
1127
{
1228
"type": "content",
@@ -22,21 +38,7 @@
2238
"path": "/test/project/config.json",
2339
"_meta": "_meta"
2440
}
25-
],
26-
"locations": [
27-
{
28-
"path": "/test/project/config.json"
29-
}
30-
],
31-
"rawInput": {
32-
"changes": [
33-
{
34-
"path": "/test/project/config.json",
35-
"kind": "update",
36-
"diff": "--- /test/project/config.json\n+++ /test/project/config.json\n@@ -1 +1 @@\n-{\"feature\":false}\n+{\"feature\":true}"
37-
}
38-
]
39-
}
41+
]
4042
},
4143
"options": [
4244
{

0 commit comments

Comments
 (0)