Skip to content

Commit ca9de27

Browse files
authored
fix: Do not send reason as content on permission request (#171)
According to the specification, the permissionRequest contains a `ToolCallUpdate`, which - again according to the spec - "Replace[s] the content collection." https://agentclientprotocol.com/protocol/v1/schema#param-content-13 This means that if the agent sends a tool call with a diff for file edits and then the permission request, a typical client would replace the the diff with just the reason text. I'd say that either ACP should add a new field `reason` to `RequestPermissionRequest`, or we use the existing `_meta` field to include the reason for clients that want to handle it specially. This PR moves the reason into _meta.
1 parent 509ed1a commit ca9de27

5 files changed

Lines changed: 44 additions & 52 deletions

File tree

src/CodexApprovalHandler.ts

Lines changed: 2 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import type {
77
FileChangeRequestApprovalParams,
88
FileChangeRequestApprovalResponse
99
} from "./app-server/v2";
10-
import type {ToolCallContent} from "@agentclientprotocol/sdk/dist/schema/types.gen";
1110
import {logger} from "./Logger";
1211
import {stripShellPrefix} from "./CodexEventHandler";
1312
import {ApprovalOptionId} from "./ApprovalOptionId";
@@ -62,47 +61,32 @@ export class CodexApprovalHandler implements ApprovalHandler {
6261
sessionId: string,
6362
params: CommandExecutionRequestApprovalParams
6463
): acp.RequestPermissionRequest {
65-
const reasonContent = this.createContentFromReason(params.reason ?? null);
6664
return {
6765
sessionId,
6866
toolCall: {
6967
toolCallId: params.itemId,
7068
kind: "execute",
7169
status: "pending",
72-
content: reasonContent ? [reasonContent] : null,
7370
rawInput: params.command ? { command: stripShellPrefix(params.command), cwd: params.cwd } : null,
7471
},
7572
options: APPROVAL_OPTIONS,
73+
_meta: { codex: { params } }
7674
};
7775
}
7876

79-
private createContentFromReason(reason: string | null): ToolCallContent | null {
80-
if (reason === null || reason === "") {
81-
return null;
82-
}
83-
return {
84-
type: "content",
85-
content: {
86-
type: "text",
87-
text: reason
88-
}
89-
}
90-
}
91-
9277
private buildFileChangePermissionRequest(
9378
sessionId: string,
9479
params: FileChangeRequestApprovalParams
9580
): acp.RequestPermissionRequest {
96-
const reasonContent = this.createContentFromReason(params.reason ?? null);
9781
return {
9882
sessionId,
9983
toolCall: {
10084
toolCallId: params.itemId,
10185
kind: "edit",
10286
status: "pending",
103-
content: reasonContent ? [reasonContent] : null,
10487
},
10588
options: APPROVAL_OPTIONS,
89+
_meta: { codex: { params } }
10690
};
10791
}
10892

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ describe('Approval Events', () => {
143143
params
144144
);
145145

146-
await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
146+
await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot(
147147
'data/approval-command-allow-once.json'
148148
);
149149

@@ -172,7 +172,7 @@ describe('Approval Events', () => {
172172
params
173173
);
174174

175-
await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
175+
await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot(
176176
'data/approval-command-with-rawInput.json'
177177
);
178178

@@ -314,7 +314,7 @@ describe('Approval Events', () => {
314314
params
315315
);
316316

317-
await expect(fixture.getAcpConnectionDump(['_meta'])).toMatchFileSnapshot(
317+
await expect(fixture.getAcpConnectionDump([])).toMatchFileSnapshot(
318318
'data/approval-file-change.json'
319319
);
320320

src/__tests__/CodexACPAgent/data/approval-command-allow-once.json

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,6 @@
77
"toolCallId": "item-snapshot",
88
"kind": "execute",
99
"status": "pending",
10-
"content": [
11-
{
12-
"type": "content",
13-
"content": {
14-
"type": "text",
15-
"text": "Running npm install"
16-
}
17-
}
18-
],
1910
"rawInput": null
2011
},
2112
"options": [
@@ -34,7 +25,18 @@
3425
"name": "Reject",
3526
"kind": "reject_once"
3627
}
37-
]
28+
],
29+
"_meta": {
30+
"codex": {
31+
"params": {
32+
"threadId": "test-session-id",
33+
"turnId": "turn-1",
34+
"itemId": "item-snapshot",
35+
"reason": "Running npm install",
36+
"proposedExecpolicyAmendment": null
37+
}
38+
}
39+
}
3840
}
3941
]
4042
}

src/__tests__/CodexACPAgent/data/approval-command-with-rawInput.json

Lines changed: 14 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,15 +7,6 @@
77
"toolCallId": "item-with-command",
88
"kind": "execute",
99
"status": "pending",
10-
"content": [
11-
{
12-
"type": "content",
13-
"content": {
14-
"type": "text",
15-
"text": "Installing dependencies"
16-
}
17-
}
18-
],
1910
"rawInput": {
2011
"command": "npm install",
2112
"cwd": "/home/user/project"
@@ -37,7 +28,20 @@
3728
"name": "Reject",
3829
"kind": "reject_once"
3930
}
40-
]
31+
],
32+
"_meta": {
33+
"codex": {
34+
"params": {
35+
"threadId": "test-session-id",
36+
"turnId": "turn-1",
37+
"itemId": "item-with-command",
38+
"reason": "Installing dependencies",
39+
"command": "npm install",
40+
"cwd": "/home/user/project",
41+
"proposedExecpolicyAmendment": null
42+
}
43+
}
44+
}
4145
}
4246
]
4347
}

src/__tests__/CodexACPAgent/data/approval-file-change.json

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,7 @@
66
"toolCall": {
77
"toolCallId": "file-change-snapshot",
88
"kind": "edit",
9-
"status": "pending",
10-
"content": [
11-
{
12-
"type": "content",
13-
"content": {
14-
"type": "text",
15-
"text": "Modifying config file"
16-
}
17-
}
18-
]
9+
"status": "pending"
1910
},
2011
"options": [
2112
{
@@ -33,7 +24,18 @@
3324
"name": "Reject",
3425
"kind": "reject_once"
3526
}
36-
]
27+
],
28+
"_meta": {
29+
"codex": {
30+
"params": {
31+
"threadId": "test-session-id",
32+
"turnId": "turn-1",
33+
"itemId": "file-change-snapshot",
34+
"reason": "Modifying config file",
35+
"grantRoot": null
36+
}
37+
}
38+
}
3739
}
3840
]
3941
}

0 commit comments

Comments
 (0)