Skip to content

Commit 78d8bd2

Browse files
NikolaiSviridovNikolaiSviridov
committed
feat: rebase, add test
1 parent f78faee commit 78d8bd2

10 files changed

Lines changed: 165 additions & 32 deletions

‎src/CodexAcpClient.ts‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -348,8 +348,6 @@ export class CodexAcpClient {
348348
private createMcpSeverConfig(mcpServer: McpServer): JsonObject {
349349
if ("type" in mcpServer) {
350350
switch (mcpServer.type) {
351-
case "acp":
352-
throw RequestError.invalidRequest("Codex doesn't support MCP ACP transport protocol")
353351
case "sse":
354352
throw RequestError.invalidRequest("Codex doesn't support MCP SSE transport protocol")
355353
case "http":

‎src/CodexAcpServer.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,6 @@ export class CodexAcpServer implements acp.Agent {
121121
list: { }
122122
},
123123
mcpCapabilities: {
124-
acp: false,
125124
http: true,
126125
sse: false
127126
}

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describeE2E("E2E file approval tests", () => {
1919
let fixture: SpawnedAgentFixture;
2020

2121
beforeEach(async () => {
22-
fixture = await createAuthenticatedFixture(AgentMode.ReadOnly);
22+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.ReadOnly});
2323
});
2424

2525
afterEach(async () => {
@@ -43,7 +43,7 @@ describeE2E("E2E Agent mode file permission tests", () => {
4343
let fixture: SpawnedAgentFixture;
4444

4545
beforeEach(async () => {
46-
fixture = await createAuthenticatedFixture(AgentMode.Agent);
46+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent});
4747
});
4848

4949
afterEach(async () => {
@@ -65,7 +65,7 @@ describeE2E("E2E Agent with full access file permission tests", () => {
6565
let fixture: SpawnedAgentFixture;
6666

6767
beforeEach(async () => {
68-
fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess);
68+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess});
6969
});
7070

7171
afterEach(async () => {

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

Lines changed: 102 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type * as acp from "@agentclientprotocol/sdk";
22
import path from "node:path";
3-
import {afterEach, beforeEach, expect, it} from "vitest";
3+
import {afterEach, beforeEach, describe, expect, it} from "vitest";
44
import {ApprovalOptionId} from "../../../ApprovalOptionId";
55
import {
66
createAuthenticatedFixture,
@@ -13,6 +13,7 @@ import {
1313

1414
const MCP_SERVER_NAME = "integration-mcp";
1515
const MCP_ECHO_MESSAGE = "mcp approval e2e";
16+
const MCP_ECHO_PROMPT = `Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Reply with exactly the tool result and no extra text.`;
1617

1718
function createMcpServer(): acp.McpServerStdio {
1819
return {
@@ -31,6 +32,12 @@ function createMcpPermissionResponder(optionId: ApprovalOptionId): PermissionRes
3132
return (request) => createPermissionResponse(isMcpPermissionRequest(request) ? optionId : null);
3233
}
3334

35+
function failingPermissionResponder(label: string): PermissionResponder {
36+
return (request) => {
37+
throw new Error(`${label}: unexpected permission request (kind=${request.toolCall.kind})`);
38+
};
39+
}
40+
3441
describeE2E("E2E MCP approval tests", () => {
3542
let fixture: SpawnedAgentFixture;
3643
let sessionId: string;
@@ -55,7 +62,7 @@ describeE2E("E2E MCP approval tests", () => {
5562

5663
await fixture.expectPromptText(
5764
sessionId,
58-
`Use the ${MCP_SERVER_NAME} MCP echo tool with message "${MCP_ECHO_MESSAGE}". Reply with exactly the tool result and no extra text.`,
65+
MCP_ECHO_PROMPT,
5966
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
6067
);
6168
expectMcpToolPermissionRequest();
@@ -73,4 +80,97 @@ describeE2E("E2E MCP approval tests", () => {
7380
}));
7481
expectMcpToolPermissionRequest();
7582
});
83+
84+
describe("persisted approvals", () => {
85+
let beforeRestartFixture: SpawnedAgentFixture | null = null;
86+
let afterRestartFixture: SpawnedAgentFixture | null = null;
87+
88+
beforeEach(async () => {
89+
// The outer beforeEach already created `fixture` without a config-backed MCP server.
90+
// Persistence tests need the server in config.toml so Codex offers "Always allow",
91+
// so dispose that fixture and replace it with a config-backed one.
92+
await fixture.dispose();
93+
beforeRestartFixture = await createAuthenticatedFixture({
94+
configBackedMcpServers: [createMcpServer()],
95+
});
96+
fixture = beforeRestartFixture;
97+
sessionId = (await fixture.createSession()).sessionId;
98+
});
99+
100+
afterEach(async () => {
101+
await afterRestartFixture?.dispose();
102+
afterRestartFixture = null;
103+
beforeRestartFixture = null;
104+
});
105+
106+
it("does not re-prompt across agent restart when user picks Always allow", async () => {
107+
fixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowPersist));
108+
109+
await fixture.expectPromptText(
110+
sessionId,
111+
MCP_ECHO_PROMPT,
112+
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
113+
);
114+
115+
const requests = fixture.readPermissionRequests(sessionId, "execute");
116+
expect(requests.length).toBe(1);
117+
expect(isMcpPermissionRequest(requests[0]!)).toBe(true);
118+
const optionIds = requests[0]!.options.map((option) => option.optionId);
119+
expect(optionIds).toContain(ApprovalOptionId.AllowPersist);
120+
121+
afterRestartFixture = await fixture.restart();
122+
// `fixture` is now stopped; route all subsequent calls through afterRestartFixture.
123+
fixture = afterRestartFixture;
124+
afterRestartFixture.setPermissionResponder(failingPermissionResponder("after restart"));
125+
const resumedSessionId = (await afterRestartFixture.createSession()).sessionId;
126+
127+
await afterRestartFixture.expectPromptText(
128+
resumedSessionId,
129+
MCP_ECHO_PROMPT,
130+
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
131+
);
132+
expect(afterRestartFixture.readPermissionRequests(resumedSessionId, "execute").length).toBe(0);
133+
});
134+
135+
it("does not re-prompt within a session when user picks Allow for session, but re-prompts after restart", async () => {
136+
let approvalsGranted = 0;
137+
fixture.setPermissionResponder((request) => {
138+
if (!isMcpPermissionRequest(request)) {
139+
return createPermissionResponse(null);
140+
}
141+
approvalsGranted += 1;
142+
if (approvalsGranted > 1) {
143+
throw new Error("Allow-for-session approval should be reused within the same session");
144+
}
145+
return createPermissionResponse(ApprovalOptionId.AllowForSession);
146+
});
147+
148+
await fixture.expectPromptText(
149+
sessionId,
150+
MCP_ECHO_PROMPT,
151+
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
152+
);
153+
expect(fixture.readPermissionRequests(sessionId, "execute").length).toBe(1);
154+
155+
await fixture.expectPromptText(
156+
sessionId,
157+
MCP_ECHO_PROMPT,
158+
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
159+
);
160+
// Still just the one approval recorded - the second call reused the session-scoped grant.
161+
expect(fixture.readPermissionRequests(sessionId, "execute").length).toBe(1);
162+
163+
afterRestartFixture = await fixture.restart();
164+
fixture = afterRestartFixture;
165+
afterRestartFixture.setPermissionResponder(createMcpPermissionResponder(ApprovalOptionId.AllowOnce));
166+
const newSessionId = (await afterRestartFixture.createSession()).sessionId;
167+
168+
await afterRestartFixture.expectPromptText(
169+
newSessionId,
170+
MCP_ECHO_PROMPT,
171+
(text) => expect(text).toContain(`You said: ${MCP_ECHO_MESSAGE}`),
172+
);
173+
expect(afterRestartFixture.readPermissionRequests(newSessionId, "execute").length).toBe(1);
174+
});
175+
});
76176
});

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ describeE2E("E2E shell approval tests", () => {
2424
let sessionId: string;
2525

2626
beforeEach(async () => {
27-
fixture = await createAuthenticatedFixture(AgentMode.ReadOnly);
27+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.ReadOnly});
2828
sessionId = (await fixture.createSession()).sessionId;
2929
});
3030

@@ -78,7 +78,7 @@ describeE2E("E2E Agent mode shell permission tests", () => {
7878
let fixture: SpawnedAgentFixture;
7979

8080
beforeEach(async () => {
81-
fixture = await createAuthenticatedFixture(AgentMode.Agent);
81+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.Agent});
8282
});
8383

8484
afterEach(async () => {
@@ -103,7 +103,7 @@ describeE2E("E2E Agent with full access shell permission tests", () => {
103103
let fixture: SpawnedAgentFixture;
104104

105105
beforeEach(async () => {
106-
fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess);
106+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess});
107107
});
108108

109109
afterEach(async () => {
@@ -135,7 +135,7 @@ describeE2E("E2E shell cancellation tests", () => {
135135
}
136136

137137
it("cancels a running shell command", async () => {
138-
fixture = await createAuthenticatedFixture(AgentMode.AgentFullAccess);
138+
fixture = await createAuthenticatedFixture({initialMode: AgentMode.AgentFullAccess});
139139
const sessionId = (await fixture.createSession()).sessionId;
140140
const pidFilePath = path.join(fixture.workspaceDir, "cancel-command.pid");
141141
const command = `/bin/sh -c 'echo $$ > "${pidFilePath}"; exec sleep 100'`;

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,9 +46,14 @@ export function expectNoPermissionRequests(fixture: SpawnedAgentFixture, session
4646
expectPermissionRequests(fixture, sessionId, { edit: 0, execute: 0 });
4747
}
4848

49-
export async function createAuthenticatedFixture(initialMode?: AgentMode): Promise<SpawnedAgentFixture> {
49+
export interface AuthenticatedFixtureOptions {
50+
initialMode?: AgentMode;
51+
configBackedMcpServers?: acp.McpServerStdio[];
52+
}
53+
54+
export async function createAuthenticatedFixture(options: AuthenticatedFixtureOptions = {}): Promise<SpawnedAgentFixture> {
5055
const apiKey = requireLiveApiKey();
51-
const extraEnv = initialMode ? {INITIAL_AGENT_MODE: initialMode.id} : undefined;
56+
const extraEnv = options.initialMode ? {INITIAL_AGENT_MODE: options.initialMode.id} : undefined;
5257
return await createSpawnedFixture(async (connection, authMethods) => {
5358
if (!authMethods.some((method) => method.id === "api-key")) {
5459
throw new Error("API key authentication is not available.");
@@ -67,7 +72,7 @@ export async function createAuthenticatedFixture(initialMode?: AgentMode): Promi
6772
if (authenticationStatus["type"] !== "api-key") {
6873
throw new Error(`Unexpected authentication status: ${JSON.stringify(authenticationStatus)}`);
6974
}
70-
}, extraEnv);
75+
}, extraEnv, options.configBackedMcpServers);
7176
}
7277

7378
export async function createGatewayFixture(
@@ -119,6 +124,7 @@ type Authenticator = (connection: acp.ClientSideConnection, authMethods: acp.Aut
119124
async function createSpawnedFixture(
120125
authenticate: Authenticator,
121126
extraEnv?: NodeJS.ProcessEnv,
127+
configBackedMcpServers?: acp.McpServerStdio[],
122128
): Promise<SpawnedAgentFixture> {
123129
return await createSpawnedAgentFixture(async (connection) => {
124130
const initializeResponse = await connection.initialize({
@@ -135,7 +141,7 @@ async function createSpawnedFixture(
135141
}
136142

137143
await authenticate(connection, initializeResponse.authMethods ?? []);
138-
}, extraEnv);
144+
}, extraEnv, undefined, configBackedMcpServers);
139145
}
140146

141147
export function requireLiveApiKey(): string {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ describeE2E("E2E tests", () => {
7979

8080
it("respects INITIAL_AGENT_MODE when seeding the initial session mode", async () => {
8181
const initialMode = AgentMode.ReadOnly;
82-
fixture = await createAuthenticatedFixture(initialMode);
82+
fixture = await createAuthenticatedFixture({initialMode});
8383
const session = await fixture.createSession();
8484

8585
expect(session.modes?.currentModeId).toBe(initialMode.id);

‎src/__tests__/CodexACPAgent/e2e/spawned-agent-fixture.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,14 +43,16 @@ type ConnectionInitializer = (connection: acp.ClientSideConnection) => Promise<v
4343
export async function createSpawnedAgentFixture(
4444
initializeConnection: ConnectionInitializer,
4545
extraEnv?: NodeJS.ProcessEnv,
46-
paths = RuntimePaths.createTemporary(),
46+
paths?: RuntimePaths,
47+
configBackedMcpServers: acp.McpServerStdio[] = [],
4748
): Promise<SpawnedAgentFixture> {
49+
const resolvedPaths = paths ?? RuntimePaths.createTemporary(configBackedMcpServers);
4850
const agentProcess = spawn("npm", ["run", "--silent", "start"], {
4951
cwd: process.cwd(),
5052
env: {
5153
...process.env,
52-
CODEX_HOME: paths.codexHome,
53-
APP_SERVER_LOGS: paths.appServerLogsDir,
54+
CODEX_HOME: resolvedPaths.codexHome,
55+
APP_SERVER_LOGS: resolvedPaths.appServerLogsDir,
5456
...extraEnv,
5557
},
5658
stdio: ["pipe", "pipe", "pipe"],
@@ -59,9 +61,10 @@ export async function createSpawnedAgentFixture(
5961
const fixture = new SpawnedAgentFixtureImpl(
6062
new RecordingClient(),
6163
agentProcess,
62-
paths,
64+
resolvedPaths,
6365
initializeConnection,
6466
extraEnv,
67+
configBackedMcpServers,
6568
);
6669
await initializeConnection(fixture.connection);
6770
return fixture;
@@ -78,7 +81,7 @@ class RuntimePaths {
7881
this.appServerLogsDir = path.join(rootDir, "logs");
7982
}
8083

81-
static createTemporary(): RuntimePaths {
84+
static createTemporary(mcpServers: acp.McpServerStdio[] = []): RuntimePaths {
8285
const rootDir = path.join(process.cwd(), "tmp", crypto.randomUUID());
8386
const paths = new RuntimePaths(rootDir);
8487
for (const dir of [paths.rootDir, paths.codexHome, paths.workspaceDir, paths.appServerLogsDir]) {
@@ -88,7 +91,7 @@ class RuntimePaths {
8891
model: DEFAULT_TEST_MODEL_ID.model,
8992
model_reasoning_effort: DEFAULT_TEST_MODEL_ID.effort,
9093
web_search: "disabled",
91-
});
94+
}, mcpServers);
9295
return paths;
9396
}
9497
}
@@ -145,7 +148,8 @@ class SpawnedAgentFixtureImpl implements SpawnedAgentFixture {
145148
private readonly agentProcess: ChildProcessWithoutNullStreams,
146149
private readonly paths: RuntimePaths,
147150
private readonly initializeConnection: ConnectionInitializer,
148-
private readonly extraEnv?: NodeJS.ProcessEnv,
151+
private readonly extraEnv: NodeJS.ProcessEnv | undefined,
152+
private readonly configBackedMcpServers: acp.McpServerStdio[],
149153
) {
150154
const output = Readable.toWeb(agentProcess.stdout) as ReadableStream<Uint8Array>;
151155
this.connection = new acp.ClientSideConnection(
@@ -167,7 +171,12 @@ class SpawnedAgentFixtureImpl implements SpawnedAgentFixture {
167171

168172
async restart(): Promise<SpawnedAgentFixture> {
169173
await this.stopProcess(false);
170-
return await createSpawnedAgentFixture(this.initializeConnection, this.extraEnv, this.paths);
174+
return await createSpawnedAgentFixture(
175+
this.initializeConnection,
176+
this.extraEnv,
177+
this.paths,
178+
this.configBackedMcpServers,
179+
);
171180
}
172181

173182
writeSkill(skill: TestSkill): void {

‎src/__tests__/CodexACPAgent/initialize.test.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,6 @@ describe('CodexACPAgent - initialize', () => {
5151
list: {},
5252
},
5353
mcpCapabilities: {
54-
acp: false,
5554
http: true,
5655
sse: false,
5756
},

0 commit comments

Comments
 (0)