Skip to content

Commit 3554f52

Browse files
ishulginishulgin
committed
Add e2e for files/commands approval
1 parent 1f62c24 commit 3554f52

6 files changed

Lines changed: 215 additions & 21 deletions

File tree

‎src/ApprovalOptionId.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
export const ApprovalOptionId = {
2+
AllowOnce: "allow_once",
3+
AllowAlways: "allow_always",
4+
RejectOnce: "reject_once",
5+
} as const;
6+
7+
export type ApprovalOptionId = typeof ApprovalOptionId[keyof typeof ApprovalOptionId];

‎src/CodexApprovalHandler.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,12 @@ import type {
1010
import type {ToolCallContent} from "@agentclientprotocol/sdk/dist/schema/types.gen";
1111
import {logger} from "./Logger";
1212
import {stripShellPrefix} from "./CodexEventHandler";
13+
import {ApprovalOptionId} from "./ApprovalOptionId";
1314

1415
const APPROVAL_OPTIONS: acp.PermissionOption[] = [
15-
{ optionId: "allow_once", name: "Allow Once", kind: "allow_once" },
16-
{ optionId: "allow_always", name: "Allow for Session", kind: "allow_always" },
17-
{ optionId: "reject_once", name: "Reject", kind: "reject_once" },
16+
{ optionId: ApprovalOptionId.AllowOnce, name: "Allow Once", kind: "allow_once" },
17+
{ optionId: ApprovalOptionId.AllowAlways, name: "Allow for Session", kind: "allow_always" },
18+
{ optionId: ApprovalOptionId.RejectOnce, name: "Reject", kind: "reject_once" },
1819
];
1920

2021
export class CodexApprovalHandler implements ApprovalHandler {
@@ -113,9 +114,9 @@ export class CodexApprovalHandler implements ApprovalHandler {
113114
}
114115

115116
const optionId = response.outcome.optionId;
116-
if (optionId === "allow_once") {
117+
if (optionId === ApprovalOptionId.AllowOnce) {
117118
return { decision: "accept" };
118-
} else if (optionId === "allow_always") {
119+
} else if (optionId === ApprovalOptionId.AllowAlways) {
119120
return { decision: "acceptForSession" };
120121
} else {
121122
return { decision: "decline" };
@@ -130,9 +131,9 @@ export class CodexApprovalHandler implements ApprovalHandler {
130131
}
131132

132133
const optionId = response.outcome.optionId;
133-
if (optionId === "allow_once") {
134+
if (optionId === ApprovalOptionId.AllowOnce) {
134135
return { decision: "accept" };
135-
} else if (optionId === "allow_always") {
136+
} else if (optionId === ApprovalOptionId.AllowAlways) {
136137
return { decision: "acceptForSession" };
137138
} else {
138139
return { decision: "cancel" };

‎src/CodexElicitationHandler.ts‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,16 @@ import type {
99
McpServerElicitationRequestResponse,
1010
} from "./app-server/v2";
1111
import { logger } from "./Logger";
12+
import { ApprovalOptionId } from "./ApprovalOptionId";
1213

1314
// Standard elicitation options (non-tool-call approval).
1415
const ELICITATION_OPTIONS: acp.PermissionOption[] = [
1516
{ optionId: "accept", name: "Accept", kind: "allow_once" },
1617
{ optionId: "decline", name: "Decline", kind: "reject_once" },
1718
];
1819

19-
// Option IDs used for MCP tool call approval persist choices.
20-
const OPTION_ALLOW_ONCE = "allow_once";
20+
// Option ID unique to elicitation persist choices — not part of the shared ApprovalOptionId set.
2121
const OPTION_ALLOW_SESSION = "allow_session";
22-
const OPTION_ALLOW_ALWAYS = "allow_always";
2322

2423
type PersistValue = "session" | "always";
2524

@@ -57,13 +56,13 @@ function isMcpToolCallApproval(meta: unknown): boolean {
5756
*/
5857
function buildToolApprovalOptions(persistOptions: Set<PersistValue>): acp.PermissionOption[] {
5958
const options: acp.PermissionOption[] = [
60-
{ optionId: OPTION_ALLOW_ONCE, name: "Allow", kind: "allow_once" },
59+
{ optionId: ApprovalOptionId.AllowOnce, name: "Allow", kind: "allow_once" },
6160
];
6261
if (persistOptions.has("session")) {
6362
options.push({ optionId: OPTION_ALLOW_SESSION, name: "Allow for This Session", kind: "allow_always" });
6463
}
6564
if (persistOptions.has("always")) {
66-
options.push({ optionId: OPTION_ALLOW_ALWAYS, name: "Allow and Don't Ask Again", kind: "allow_always" });
65+
options.push({ optionId: ApprovalOptionId.AllowAlways, name: "Allow and Don't Ask Again", kind: "allow_always" });
6766
}
6867
options.push({ optionId: "decline", name: "Decline", kind: "reject_once" });
6968
return options;
@@ -215,10 +214,10 @@ export class CodexElicitationHandler implements ElicitationHandler {
215214
if (optionId === OPTION_ALLOW_SESSION) {
216215
return { action: "accept", content: null, _meta: { persist: "session" } };
217216
}
218-
if (optionId === OPTION_ALLOW_ALWAYS) {
217+
if (optionId === ApprovalOptionId.AllowAlways) {
219218
return { action: "accept", content: null, _meta: { persist: "always" } };
220219
}
221-
if (optionId === OPTION_ALLOW_ONCE || optionId === "accept") {
220+
if (optionId === ApprovalOptionId.AllowOnce || optionId === "accept") {
222221
return { action: "accept", content: null, _meta: null };
223222
}
224223
return { action: "decline", content: null, _meta: null };

‎src/__tests__/CodexACPAgent/e2e/acp-e2e-file-approval.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
import fs from "node:fs";
2+
import path from "node:path";
3+
import {afterEach, beforeEach, expect, it} from "vitest";
4+
import {ApprovalOptionId} from "../../../ApprovalOptionId";
5+
import {
6+
createPermissionResponder,
7+
createReadOnlyFixture,
8+
describeE2E,
9+
type SpawnedAgentFixture,
10+
type SpawnedSessionFixture,
11+
} from "./acp-e2e-test-utils";
12+
13+
const FILE_NAME = "approval-file.txt";
14+
const FILE_CONTENT = "file approval e2e";
15+
16+
describeE2E("E2E file approval tests", () => {
17+
let fixture: SpawnedAgentFixture;
18+
let session: SpawnedSessionFixture;
19+
20+
beforeEach(async () => {
21+
fixture = await createReadOnlyFixture();
22+
session = await fixture.createSession();
23+
});
24+
25+
afterEach(async () => {
26+
await fixture.dispose();
27+
});
28+
29+
async function expectFileApproval(
30+
optionId: ApprovalOptionId,
31+
expectedStopReason: "end_turn" | "cancelled",
32+
) {
33+
fixture.setPermissionResponder(createPermissionResponder("edit", optionId));
34+
const response = await fixture.connection.prompt({
35+
sessionId: session.response.sessionId,
36+
prompt: [{
37+
type: "text",
38+
text: `Create ${FILE_NAME} by editing files directly. Content must be exactly: ${FILE_CONTENT}. Do not use shell commands, and stop if the edit is rejected.`,
39+
}],
40+
});
41+
expect(response.stopReason).toBe(expectedStopReason);
42+
expect(session.readPermissionRequests("edit").length).toBe(1);
43+
expect(session.readPermissionRequests("execute").length).toBe(0);
44+
}
45+
46+
it("applies approved file edits", async () => {
47+
await expectFileApproval(ApprovalOptionId.AllowOnce, "end_turn");
48+
const filePath = path.join(fixture.workspaceDir, FILE_NAME);
49+
expect(fs.existsSync(filePath)).toBe(true);
50+
expect(fs.readFileSync(filePath, "utf8").trim()).toBe(FILE_CONTENT);
51+
});
52+
53+
it("does not apply rejected file edits", async () => {
54+
await expectFileApproval(ApprovalOptionId.RejectOnce, "cancelled");
55+
expect(fs.existsSync(path.join(fixture.workspaceDir, FILE_NAME))).toBe(false);
56+
});
57+
});

‎src/__tests__/CodexACPAgent/e2e/acp-e2e-shell-approval.test.ts‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
import fs from "node:fs";
2+
import path from "node:path";
3+
import {afterEach, beforeEach, expect, it} from "vitest";
4+
import {ApprovalOptionId} from "../../../ApprovalOptionId";
5+
import {
6+
createPermissionResponse,
7+
createPermissionResponder,
8+
createReadOnlyFixture,
9+
describeE2E,
10+
expectEndTurn,
11+
type SpawnedAgentFixture,
12+
type SpawnedSessionFixture,
13+
} from "./acp-e2e-test-utils";
14+
15+
const FIRST_FILE_NAME = "approval-first.txt";
16+
const SECOND_FILE_NAME = "approval-second.txt";
17+
const COMMAND = `if [ -e ${FIRST_FILE_NAME} ]; then touch ${SECOND_FILE_NAME}; else touch ${FIRST_FILE_NAME}; fi`;
18+
19+
describeE2E("E2E shell approval tests", () => {
20+
let fixture: SpawnedAgentFixture;
21+
let session: SpawnedSessionFixture;
22+
23+
beforeEach(async () => {
24+
fixture = await createReadOnlyFixture();
25+
session = await fixture.createSession();
26+
});
27+
28+
afterEach(async () => {
29+
await fixture.dispose();
30+
});
31+
32+
async function promptShellCommandTwice() {
33+
for (const text of [
34+
`Use your shell tool to run exactly \`${COMMAND}\`.`,
35+
`Use your shell tool to run exactly the same command again: \`${COMMAND}\`.`,
36+
]) {
37+
expectEndTurn(await fixture.connection.prompt({
38+
sessionId: session.response.sessionId,
39+
prompt: [{type: "text", text}],
40+
}));
41+
}
42+
}
43+
44+
it("prompts for every command when allow_once is selected", async () => {
45+
const responses = [ApprovalOptionId.AllowOnce, ApprovalOptionId.RejectOnce];
46+
fixture.setPermissionResponder((request) => createPermissionResponse(
47+
request.toolCall.kind === "execute"
48+
? responses.shift() ?? ApprovalOptionId.RejectOnce
49+
: null
50+
));
51+
await promptShellCommandTwice();
52+
expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(true);
53+
expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(false);
54+
expect(session.readPermissionRequests("execute").length).toBe(2);
55+
});
56+
57+
it("skips subsequent approvals when allow_always is selected", async () => {
58+
fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.AllowAlways));
59+
await promptShellCommandTwice();
60+
expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(true);
61+
expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(true);
62+
expect(session.readPermissionRequests("execute").length).toBe(1);
63+
});
64+
65+
it("prompts for every command when reject_once is selected", async () => {
66+
fixture.setPermissionResponder(createPermissionResponder("execute", ApprovalOptionId.RejectOnce));
67+
await promptShellCommandTwice();
68+
expect(fs.existsSync(path.join(fixture.workspaceDir, FIRST_FILE_NAME))).toBe(false);
69+
expect(fs.existsSync(path.join(fixture.workspaceDir, SECOND_FILE_NAME))).toBe(false);
70+
expect(session.readPermissionRequests("execute").length).toBe(2);
71+
});
72+
});

‎src/__tests__/CodexACPAgent/e2e/acp-e2e-test-utils.ts‎

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import os from "node:os";
55
import path from "node:path";
66
import {Readable, Writable} from "node:stream";
77
import {describe, expect, vi} from "vitest";
8+
import {AgentMode} from "../../../AgentMode";
9+
import {ApprovalOptionId} from "../../../ApprovalOptionId";
810
import {removeDirectoryWithRetry, writeCodexHomeConfig} from "../../acp-test-utils";
911

1012
export const RUN_E2E_TESTS = process.env["RUN_E2E_TESTS"] === "true";
@@ -13,11 +15,38 @@ const DEFAULT_E2E_SUITE_TIMEOUT_MS = 60_000;
1315
export interface SpawnedSessionFixture {
1416
readonly response: acp.NewSessionResponse;
1517
expectPromptText(promptText: string, assertText: (text: string) => void, timeoutMs?: number): Promise<void>;
18+
readPermissionRequests(toolCallKind: acp.ToolKind): acp.RequestPermissionRequest[];
19+
}
20+
21+
export function expectEndTurn(response: acp.PromptResponse): void {
22+
expect(response.stopReason).toBe("end_turn");
23+
}
24+
25+
export type PermissionResponder = (
26+
params: acp.RequestPermissionRequest,
27+
) => acp.RequestPermissionResponse;
28+
29+
export function createPermissionResponder(
30+
expectedToolCallKind: acp.ToolKind,
31+
optionId: ApprovalOptionId,
32+
): PermissionResponder {
33+
return (request) => createPermissionResponse(
34+
request.toolCall.kind === expectedToolCallKind ? optionId : null
35+
);
36+
}
37+
38+
export function createPermissionResponse(optionId: ApprovalOptionId | null): acp.RequestPermissionResponse {
39+
if (optionId === null) {
40+
return {outcome: {outcome: "cancelled"}};
41+
}
42+
return {outcome: {outcome: "selected", optionId}};
1643
}
1744

1845
export interface SpawnedAgentFixture {
1946
readonly connection: acp.ClientSideConnection;
47+
readonly workspaceDir: string;
2048
createSession(): Promise<SpawnedSessionFixture>;
49+
setPermissionResponder(responder: PermissionResponder): void;
2150
dispose(): Promise<void>;
2251
}
2352

@@ -51,11 +80,23 @@ interface RuntimePaths {
5180

5281
class RecordingClient implements acp.Client {
5382
private readonly textBySessionId = new Map<string, string>();
83+
private readonly permissionRequestsBySessionId = new Map<string, acp.RequestPermissionRequest[]>();
84+
private permissionResponder: PermissionResponder = () => ({
85+
outcome: {outcome: "cancelled"},
86+
});
5487

55-
async requestPermission(_params: acp.RequestPermissionRequest): Promise<acp.RequestPermissionResponse> {
56-
return {
57-
outcome: {outcome: "cancelled"},
58-
};
88+
setPermissionResponder(responder: PermissionResponder): void {
89+
this.permissionResponder = responder;
90+
}
91+
92+
async requestPermission(params: acp.RequestPermissionRequest): Promise<acp.RequestPermissionResponse> {
93+
let requests = this.permissionRequestsBySessionId.get(params.sessionId);
94+
if (!requests) {
95+
requests = [];
96+
this.permissionRequestsBySessionId.set(params.sessionId, requests);
97+
}
98+
requests.push(params);
99+
return this.permissionResponder(params);
59100
}
60101

61102
async sessionUpdate(params: acp.SessionNotification): Promise<void> {
@@ -70,6 +111,14 @@ class RecordingClient implements acp.Client {
70111
readText(sessionId: string): string {
71112
return this.textBySessionId.get(sessionId) ?? "";
72113
}
114+
115+
readPermissionRequests(
116+
sessionId: string,
117+
toolCallKind: acp.ToolKind,
118+
): acp.RequestPermissionRequest[] {
119+
const requests = this.permissionRequestsBySessionId.get(sessionId) ?? [];
120+
return requests.filter((request) => request.toolCall.kind === toolCallKind);
121+
}
73122
}
74123

75124
export async function createFixtureWithSkill(skill: TestSkill): Promise<SpawnedAgentFixture> {
@@ -104,6 +153,10 @@ export async function createAuthenticatedFixture(
104153
}, runtimePaths, extraEnv);
105154
}
106155

156+
export async function createReadOnlyFixture(): Promise<SpawnedAgentFixture> {
157+
return await createAuthenticatedFixture(undefined, {INITIAL_AGENT_MODE: AgentMode.ReadOnly.id});
158+
}
159+
107160
export interface GatewayFixtureOptions {
108161
readonly baseUrl: string;
109162
readonly headers?: Record<string, string>;
@@ -186,12 +239,19 @@ async function createSpawnedFixture(
186239
async expectPromptText(promptText: string, assertText: (text: string) => void, timeoutMs = 30_000): Promise<void> {
187240
await expectPromptTextForSession(connection, client, newSessionResponse.sessionId, promptText, assertText, timeoutMs);
188241
},
242+
readPermissionRequests(toolCallKind: acp.ToolKind): acp.RequestPermissionRequest[] {
243+
return client.readPermissionRequests(newSessionResponse.sessionId, toolCallKind);
244+
},
189245
};
190246
};
191247

192248
return {
193249
connection,
250+
workspaceDir: runtimePaths.workspaceDir,
194251
createSession,
252+
setPermissionResponder(responder: PermissionResponder): void {
253+
client.setPermissionResponder(responder);
254+
},
195255
async dispose(): Promise<void> {
196256
if (!agentProcess.stdin.destroyed && !agentProcess.stdin.writableEnded) {
197257
agentProcess.stdin.end();
@@ -281,9 +341,7 @@ async function expectPromptTextForSession(
281341
}],
282342
});
283343

284-
if (promptResponse.stopReason !== "end_turn") {
285-
throw new Error(`Unexpected stop reason: ${promptResponse.stopReason}`);
286-
}
344+
expectEndTurn(promptResponse);
287345

288346
await vi.waitFor(() => {
289347
const sessionText = client.readText(sessionId);

0 commit comments

Comments
 (0)