Skip to content

Commit ea754af

Browse files
i1broi1bro
committed
Track file change context for approval prompts
1 parent c7b6656 commit ea754af

7 files changed

Lines changed: 227 additions & 20 deletions

File tree

‎src/CodexAcpServer.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
createFileChangeUpdate,
3636
createMcpToolCallUpdate,
3737
} from "./CodexToolCallMapper";
38+
import { createApprovalContextStore } from "./CodexApprovalContext";
3839

3940
export interface SessionState {
4041
sessionId: string,
@@ -705,8 +706,9 @@ export class CodexAcpServer implements acp.Agent {
705706
sessionState.lastTokenUsage = null;
706707

707708
try {
708-
const eventHandler = new CodexEventHandler(this.connection, sessionState);
709-
const approvalHandler = new CodexApprovalHandler(this.connection, sessionState);
709+
const approvalContext = createApprovalContextStore();
710+
const eventHandler = new CodexEventHandler(this.connection, sessionState, approvalContext);
711+
const approvalHandler = new CodexApprovalHandler(this.connection, sessionState, approvalContext);
710712
await this.codexAcpClient.subscribeToSessionEvents(params.sessionId,
711713
(event) => eventHandler.handleNotification(event),
712714
approvalHandler);

‎src/CodexApprovalContext.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import type { ThreadItem } from "./app-server/v2";
2+
3+
export interface ApprovalContextStore {
4+
readonly fileChangesByItemId: Map<string, ThreadItem & { type: "fileChange" }>;
5+
readonly turnDiffsByTurnId: Map<string, string>;
6+
}
7+
8+
export function createApprovalContextStore(): ApprovalContextStore {
9+
return {
10+
fileChangesByItemId: new Map(),
11+
turnDiffsByTurnId: new Map(),
12+
};
13+
}

‎src/CodexApprovalHandler.ts‎

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,16 @@ import type {
66
CommandExecutionRequestApprovalResponse,
77
FileChangeRequestApprovalParams,
88
FileChangeRequestApprovalResponse,
9+
FileUpdateChange,
910
McpServerElicitationRequestParams,
1011
McpServerElicitationRequestResponse
1112
} from "./app-server/v2";
1213
import type {JsonValue} from "./app-server/serde_json/JsonValue";
1314
import type {ToolCallContent} from "@agentclientprotocol/sdk/dist/schema/types.gen";
1415
import {logger} from "./Logger";
1516
import {stripShellPrefix} from "./CodexEventHandler";
17+
import type {ApprovalContextStore} from "./CodexApprovalContext";
18+
import {createFileChangeContents} from "./CodexToolCallMapper";
1619

1720
const APPROVAL_OPTIONS: acp.PermissionOption[] = [
1821
{ optionId: "allow_once", name: "Allow Once", kind: "allow_once" },
@@ -31,13 +34,16 @@ type JsonObject = { [key: string]: JsonValue };
3134
export class CodexApprovalHandler implements ApprovalHandler {
3235
private readonly connection: acp.AgentSideConnection;
3336
private readonly sessionState: SessionState;
37+
private readonly approvalContext: ApprovalContextStore;
3438

3539
constructor(
3640
connection: acp.AgentSideConnection,
37-
sessionState: SessionState
41+
sessionState: SessionState,
42+
approvalContext: ApprovalContextStore,
3843
) {
3944
this.connection = connection;
4045
this.sessionState = sessionState;
46+
this.approvalContext = approvalContext;
4147
}
4248

4349
async handleCommandExecution(
@@ -59,7 +65,7 @@ export class CodexApprovalHandler implements ApprovalHandler {
5965
): Promise<FileChangeRequestApprovalResponse> {
6066
try {
6167
const sessionId = this.sessionState.sessionId;
62-
const acpRequest = this.buildFileChangePermissionRequest(sessionId, params);
68+
const acpRequest = await this.buildFileChangePermissionRequest(sessionId, params);
6369
const response = await this.connection.requestPermission(acpRequest);
6470
return this.convertFileChangeResponse(response);
6571
} catch (error) {
@@ -120,19 +126,43 @@ export class CodexApprovalHandler implements ApprovalHandler {
120126
return contents.length > 0 ? contents : null
121127
}
122128

123-
private buildFileChangePermissionRequest(
129+
private async buildFileChangePermissionRequest(
124130
sessionId: string,
125131
params: FileChangeRequestApprovalParams
126-
): acp.RequestPermissionRequest {
132+
): Promise<acp.RequestPermissionRequest> {
127133
const reasonContent = this.createTextContent(params.reason ?? null);
134+
const fileChange = this.approvalContext.fileChangesByItemId.get(params.itemId);
135+
const diffContent = fileChange ? await createFileChangeContents(fileChange.changes) : [];
136+
const toolCall: acp.ToolCallUpdate = {
137+
toolCallId: params.itemId,
138+
kind: "edit",
139+
status: "pending",
140+
};
141+
const content = [
142+
...(reasonContent ? [reasonContent] : []),
143+
...diffContent,
144+
];
145+
if (content.length > 0) {
146+
toolCall.content = content;
147+
}
148+
if (fileChange) {
149+
toolCall.locations = dedupePaths(fileChange.changes).map(path => ({ path }));
150+
toolCall.rawInput = {
151+
changes: fileChange.changes.map(change => ({
152+
path: change.path,
153+
kind: change.kind.type,
154+
diff: change.diff,
155+
})),
156+
};
157+
} else {
158+
const turnDiff = this.approvalContext.turnDiffsByTurnId.get(params.turnId);
159+
if (turnDiff) {
160+
toolCall.rawInput = { unifiedDiff: turnDiff };
161+
}
162+
}
128163
return {
129164
sessionId,
130-
toolCall: {
131-
toolCallId: params.itemId,
132-
kind: "edit",
133-
status: "pending",
134-
content: reasonContent ? [reasonContent] : null,
135-
},
165+
toolCall,
136166
options: APPROVAL_OPTIONS,
137167
};
138168
}
@@ -320,3 +350,7 @@ export class CodexApprovalHandler implements ApprovalHandler {
320350
return { action: "cancel", content: null, _meta: null }
321351
}
322352
}
353+
354+
function dedupePaths(changes: Array<FileUpdateChange>): Array<string> {
355+
return Array.from(new Set(changes.map(change => change.path)));
356+
}

‎src/CodexEventHandler.ts‎

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,19 +34,22 @@ import {
3434
fuzzyFileSearchToolCallId,
3535
} from "./CodexToolCallMapper";
3636
import { stripShellPrefix } from "./CommandUtils";
37+
import type { ApprovalContextStore } from "./CodexApprovalContext";
3738

3839
export { stripShellPrefix };
3940

4041
export class CodexEventHandler {
4142

4243
private readonly connection: acp.AgentSideConnection;
4344
private readonly sessionState: SessionState;
45+
private readonly approvalContext: ApprovalContextStore;
4446
private failure: RequestError | null = null;
4547
private readonly activeFuzzyFileSearchSessions = new Set<string>();
4648

47-
constructor(connection: acp.AgentSideConnection, sessionState: SessionState) {
49+
constructor(connection: acp.AgentSideConnection, sessionState: SessionState, approvalContext: ApprovalContextStore) {
4850
this.connection = connection;
4951
this.sessionState = sessionState;
52+
this.approvalContext = approvalContext;
5053
}
5154

5255
getFailure(): RequestError | null {
@@ -85,6 +88,8 @@ export class CodexEventHandler {
8588
return null;
8689
case "turn/completed":
8790
this.sessionState.currentTurnId = null;
91+
this.approvalContext.fileChangesByItemId.clear();
92+
this.approvalContext.turnDiffsByTurnId.clear();
8893
return null;
8994
case "thread/tokenUsage/updated":
9095
return this.createUsageUpdate(notification.params);
@@ -100,14 +105,16 @@ export class CodexEventHandler {
100105
case "item/reasoning/summaryPartAdded":
101106
//skipped events
102107
case "item/reasoning/textDelta": //for raw output
103-
case "turn/diff/updated":
104108
case "item/commandExecution/terminalInteraction":
105109
case "item/fileChange/outputDelta":
106110
case "serverRequest/resolved":
107111
case "account/updated":
108112
case "fs/changed":
109113
case "mcpServer/startupStatus/updated":
110114
return null;
115+
case "turn/diff/updated":
116+
this.approvalContext.turnDiffsByTurnId.set(notification.params.turnId, notification.params.diff);
117+
return null;
111118
case "item/mcpToolCall/progress":
112119
return this.createMcpToolProgressEvent(notification.params);
113120
case "account/rateLimits/updated":
@@ -189,6 +196,7 @@ export class CodexEventHandler {
189196
private async createItemEvent(event: ItemStartedNotification): Promise<UpdateSessionEvent | null> {
190197
switch (event.item.type) {
191198
case "fileChange":
199+
this.approvalContext.fileChangesByItemId.set(event.item.id, event.item);
192200
return await createFileChangeUpdate(event.item);
193201
case "commandExecution":
194202
return await createCommandExecutionUpdate(event.item);
@@ -215,6 +223,12 @@ export class CodexEventHandler {
215223
private async completeItemEvent(event: ItemCompletedNotification): Promise<UpdateSessionEvent | null> {
216224
switch (event.item.type) {
217225
case "fileChange":
226+
this.approvalContext.fileChangesByItemId.set(event.item.id, event.item);
227+
return {
228+
sessionUpdate: "tool_call_update",
229+
toolCallId: event.item.id,
230+
status: event.item.status === "completed" ? "completed" : "failed",
231+
}
218232
case "dynamicToolCall":
219233
return {
220234
sessionUpdate: "tool_call_update",

‎src/CodexToolCallMapper.ts‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,7 @@ function toAcpStatus(status: CodexItemStatus): AcpToolCallStatus {
3939
export async function createFileChangeUpdate(
4040
item: ThreadItem & { type: "fileChange" }
4141
): Promise<UpdateSessionEvent> {
42-
const patches: ToolCallContent[] = [];
43-
for (const change of item.changes) {
44-
const content = await createPatchContent(change);
45-
if (content) patches.push(content);
46-
// ignore unparseable diffs
47-
}
42+
const patches = await createFileChangeContents(item.changes);
4843
return {
4944
sessionUpdate: "tool_call",
5045
toolCallId: item.id,
@@ -55,6 +50,16 @@ export async function createFileChangeUpdate(
5550
};
5651
}
5752

53+
export async function createFileChangeContents(changes: Array<FileUpdateChange>): Promise<ToolCallContent[]> {
54+
const patches: ToolCallContent[] = [];
55+
for (const change of changes) {
56+
const content = await createPatchContent(change);
57+
if (content) patches.push(content);
58+
// ignore unparseable diffs
59+
}
60+
return patches;
61+
}
62+
5863
export async function createCommandExecutionUpdate(
5964
item: ThreadItem & { type: "commandExecution" }
6065
): Promise<UpdateSessionEvent> {

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,34 @@ import type { CommandExecutionRequestApprovalParams, FileChangeRequestApprovalPa
33
import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils';
44
import type { SessionState } from '../../CodexAcpServer';
55
import {AgentMode} from "../../AgentMode";
6+
import type { ServerNotification } from '../../app-server';
7+
8+
const { mockFiles, mockFileContent, clearMockFiles } = vi.hoisted(() => {
9+
const files = new Map<string, string>();
10+
return {
11+
mockFiles: files,
12+
mockFileContent: (path: string, content: string) => files.set(path, content),
13+
clearMockFiles: () => files.clear(),
14+
};
15+
});
16+
17+
vi.mock('node:fs/promises', () => ({
18+
readFile: (path: string) => {
19+
const content = mockFiles.get(path);
20+
if (content !== undefined) {
21+
return Promise.resolve(content);
22+
}
23+
return Promise.reject(new Error(`ENOENT: no such file or directory, open '${path}'`));
24+
},
25+
}));
626

727
describe('Approval Events', () => {
828
let fixture: CodexMockTestFixture;
929
const sessionId = 'test-session-id';
1030

1131
beforeEach(() => {
1232
fixture = createCodexMockTestFixture();
33+
clearMockFiles();
1334
vi.clearAllMocks();
1435
});
1536

@@ -321,5 +342,63 @@ describe('Approval Events', () => {
321342
completeTurn();
322343
await promptPromise;
323344
});
345+
346+
it('should include diff content for file change approval when file change item is available', async () => {
347+
const { promptPromise, completeTurn } = setupSessionWithPendingPrompt();
348+
fixture.setPermissionResponse({
349+
outcome: { outcome: 'selected', optionId: 'allow_once' }
350+
});
351+
mockFileContent('/test/project/config.json', '{"feature":false}');
352+
353+
const notification = {
354+
method: 'item/started',
355+
params: {
356+
threadId: sessionId,
357+
turnId: 'turn-1',
358+
item: {
359+
type: 'fileChange',
360+
id: 'file-change-with-diff',
361+
changes: [
362+
{
363+
path: '/test/project/config.json',
364+
kind: { type: 'update' },
365+
diff: `--- /test/project/config.json
366+
+++ /test/project/config.json
367+
@@ -1 +1 @@
368+
-{"feature":false}
369+
+{"feature":true}`,
370+
},
371+
],
372+
status: 'inProgress',
373+
},
374+
},
375+
} as ServerNotification;
376+
377+
fixture.sendServerNotification(notification);
378+
await vi.waitFor(() => {
379+
expect(fixture.getAcpConnectionDump([])).toContain('file-change-with-diff');
380+
});
381+
fixture.clearAcpConnectionDump();
382+
383+
const params: FileChangeRequestApprovalParams = {
384+
threadId: sessionId,
385+
turnId: 'turn-1',
386+
itemId: 'file-change-with-diff',
387+
reason: 'Updating config file',
388+
grantRoot: null,
389+
};
390+
391+
await fixture.sendServerRequest(
392+
'item/fileChange/requestApproval',
393+
params
394+
);
395+
396+
await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
397+
'data/approval-file-change-with-diff.json'
398+
);
399+
400+
completeTurn();
401+
await promptPromise;
402+
});
324403
});
325404
});

0 commit comments

Comments
 (0)