Skip to content

Commit 5a0a2c2

Browse files
benbrandtbenbrandt
authored
Merge pull request #192
* Add ACP session delete support
1 parent 1d76251 commit 5a0a2c2

8 files changed

Lines changed: 255 additions & 0 deletions

‎src/CodexAcpClient.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,6 +270,10 @@ export class CodexAcpClient {
270270
}
271271
}
272272

273+
async deleteSession(sessionId: string): Promise<void> {
274+
await this.codexClient.threadArchive({threadId: sessionId});
275+
}
276+
273277
async awaitMcpServerStartup(serverNames: Array<string>, afterVersion: number): Promise<McpStartupResult> {
274278
return await this.codexClient.awaitMcpServerStartup(serverNames, afterVersion);
275279
}

‎src/CodexAcpServer.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ export class CodexAcpServer implements acp.Agent {
160160
resume: { },
161161
list: { },
162162
close: { },
163+
delete: { },
163164
},
164165
mcpCapabilities: {
165166
acp: false,
@@ -455,6 +456,41 @@ export class CodexAcpServer implements acp.Agent {
455456
return {};
456457
}
457458

459+
async unstable_deleteSession(params: acp.DeleteSessionRequest): Promise<acp.DeleteSessionResponse> {
460+
logger.log("Deleting session...", {sessionId: params.sessionId});
461+
const sessionId = params.sessionId;
462+
const shouldCloseLocalSession = this.hasLocalSession(sessionId);
463+
464+
this.beginSessionCloseFence(sessionId);
465+
try {
466+
if (shouldCloseLocalSession) {
467+
await this.closeSession({sessionId});
468+
} else {
469+
this.bumpSessionGeneration(sessionId);
470+
}
471+
472+
await this.runWithProcessCheck(() => this.codexAcpClient.deleteSession(sessionId));
473+
logger.log("Session deleted", {sessionId});
474+
} finally {
475+
this.endSessionCloseFence(sessionId);
476+
}
477+
478+
return {};
479+
}
480+
481+
private hasLocalSession(sessionId: string): boolean {
482+
return this.sessions.has(sessionId)
483+
|| this.pendingMcpStartupSessions.has(sessionId)
484+
|| this.pendingTurnStarts.has(sessionId)
485+
|| this.activePrompts.has(sessionId)
486+
|| this.hasPendingSessionOpen(sessionId)
487+
|| this.sessionIsClosing(sessionId);
488+
}
489+
490+
private hasPendingSessionOpen(sessionId: string): boolean {
491+
return this.sessionOpenGenerations.get(sessionId) === this.getSessionGeneration(sessionId);
492+
}
493+
458494
async newSession(
459495
params: acp.NewSessionRequest,
460496
): Promise<acp.NewSessionResponse> {

‎src/CodexAppServerClient.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,8 @@ import type {
2424
SkillsExtraRootsSetParams,
2525
SkillsListParams,
2626
SkillsListResponse,
27+
ThreadArchiveParams,
28+
ThreadArchiveResponse,
2729
ThreadLoadedListParams,
2830
ThreadLoadedListResponse,
2931
ThreadListParams,
@@ -241,6 +243,10 @@ export class CodexAppServerClient {
241243
return await this.sendRequest({ method: "thread/read", params: params });
242244
}
243245

246+
async threadArchive(params: ThreadArchiveParams): Promise<ThreadArchiveResponse> {
247+
return await this.sendRequest({ method: "thread/archive", params: params });
248+
}
249+
244250
async threadUnsubscribe(params: ThreadUnsubscribeParams): Promise<ThreadUnsubscribeResponse> {
245251
return await this.sendRequest({ method: "thread/unsubscribe", params: params });
246252
}

‎src/__tests__/CodexACPAgent/data/session-delete-active-turn.json‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
{
2+
"eventType": "request",
3+
"method": "turn/interrupt",
4+
"params": {
5+
"threadId": "session-id",
6+
"turnId": "turn-id"
7+
}
8+
}
9+
{
10+
"eventType": "response"
11+
}
12+
{
13+
"eventType": "request",
14+
"method": "thread/unsubscribe",
15+
"params": {
16+
"threadId": "session-id"
17+
}
18+
}
19+
{
20+
"eventType": "response"
21+
}
22+
{
23+
"eventType": "request",
24+
"method": "thread/archive",
25+
"params": {
26+
"threadId": "session-id"
27+
}
28+
}
29+
{
30+
"eventType": "response"
31+
}

‎src/__tests__/CodexACPAgent/data/session-delete-idle.json‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
{
2+
"eventType": "request",
3+
"method": "thread/unsubscribe",
4+
"params": {
5+
"threadId": "session-id"
6+
}
7+
}
8+
{
9+
"eventType": "response"
10+
}
11+
{
12+
"eventType": "request",
13+
"method": "thread/archive",
14+
"params": {
15+
"threadId": "session-id"
16+
}
17+
}
18+
{
19+
"eventType": "response"
20+
}

‎src/__tests__/CodexACPAgent/data/session-delete-unknown-local.json‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
{
2+
"eventType": "request",
3+
"method": "thread/archive",
4+
"params": {
5+
"threadId": "session-id"
6+
}
7+
}
8+
{
9+
"eventType": "response"
10+
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ describe('CodexACPAgent - initialize', () => {
5050
resume: {},
5151
list: {},
5252
close: {},
53+
delete: {},
5354
},
5455
mcpCapabilities: {
5556
acp: false,

‎src/__tests__/CodexACPAgent/session-delete.test.ts‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
import {describe, expect, it, vi} from "vitest";
2+
import {
3+
createCodexMockTestFixture,
4+
createTestModel,
5+
type CodexMockTestFixture,
6+
} from "../acp-test-utils";
7+
import type {CodexAcpServer} from "../../CodexAcpServer";
8+
import type {CodexAcpClient} from "../../CodexAcpClient";
9+
10+
const sessionId = "session-id";
11+
12+
describe("ACP session delete", () => {
13+
it("advertises session delete support", async () => {
14+
const fixture = createCodexMockTestFixture();
15+
16+
const response = await fixture.getCodexAcpAgent().initialize({protocolVersion: 1});
17+
18+
expect(response.agentCapabilities?.sessionCapabilities?.delete).toEqual({});
19+
});
20+
21+
it("archives sessions that are not active locally", async () => {
22+
const fixture = createCodexMockTestFixture();
23+
24+
await expect(fixture.getCodexAcpAgent().unstable_deleteSession({sessionId})).resolves.toEqual({});
25+
26+
await expect(fixture.getCodexConnectionDump([])).toMatchFileSnapshot(
27+
"data/session-delete-unknown-local.json"
28+
);
29+
});
30+
31+
it("closes local session resources before archiving", async () => {
32+
const {fixture, codexAcpAgent} = await createSession();
33+
34+
await expect(codexAcpAgent.unstable_deleteSession({sessionId})).resolves.toEqual({});
35+
36+
await expect(fixture.getCodexConnectionDump([])).toMatchFileSnapshot(
37+
"data/session-delete-idle.json"
38+
);
39+
expect(() => codexAcpAgent.getSessionState(sessionId)).toThrow(`Session ${sessionId} not found`);
40+
});
41+
42+
it("does not close again when deleting a previously closed session", async () => {
43+
const {fixture, codexAcpAgent} = await createSession();
44+
45+
await codexAcpAgent.closeSession({sessionId});
46+
fixture.clearCodexConnectionDump();
47+
48+
await expect(codexAcpAgent.unstable_deleteSession({sessionId})).resolves.toEqual({});
49+
50+
await expect(fixture.getCodexConnectionDump([])).toMatchFileSnapshot(
51+
"data/session-delete-unknown-local.json"
52+
);
53+
});
54+
55+
it("interrupts active turns before archiving", async () => {
56+
const {fixture, codexAcpAgent} = await createSession();
57+
codexAcpAgent.getSessionState(sessionId).currentTurnId = "turn-id";
58+
59+
await expect(codexAcpAgent.unstable_deleteSession({sessionId})).resolves.toEqual({});
60+
61+
await expect(fixture.getCodexConnectionDump([])).toMatchFileSnapshot(
62+
"data/session-delete-active-turn.json"
63+
);
64+
expect(() => codexAcpAgent.getSessionState(sessionId)).toThrow(`Session ${sessionId} not found`);
65+
});
66+
67+
it("rejects resume while unknown-local delete archive is in flight", async () => {
68+
const fixture = createCodexMockTestFixture();
69+
const codexAcpAgent = fixture.getCodexAcpAgent();
70+
const codexAcpClient = fixture.getCodexAcpClient();
71+
const archive = deferred<void>();
72+
vi.spyOn(codexAcpClient, "authRequired").mockResolvedValue(false);
73+
vi.spyOn(codexAcpClient, "deleteSession").mockReturnValue(archive.promise);
74+
const resumeSessionSpy = vi.spyOn(codexAcpClient, "resumeSession");
75+
76+
const deletePromise = codexAcpAgent.unstable_deleteSession({sessionId});
77+
await vi.waitFor(() => {
78+
expect(codexAcpClient.deleteSession).toHaveBeenCalledWith(sessionId);
79+
});
80+
81+
await expect(codexAcpAgent.resumeSession({
82+
sessionId,
83+
cwd: "/test/cwd",
84+
mcpServers: [],
85+
})).rejects.toThrow("Invalid request");
86+
expect(resumeSessionSpy).not.toHaveBeenCalled();
87+
88+
archive.resolve(undefined);
89+
await expect(deletePromise).resolves.toEqual({});
90+
});
91+
92+
it("rejects load after local close completes while delete archive is in flight", async () => {
93+
const {codexAcpAgent, codexAcpClient} = await createSession();
94+
const archive = deferred<void>();
95+
vi.spyOn(codexAcpClient, "deleteSession").mockReturnValue(archive.promise);
96+
const loadSessionSpy = vi.spyOn(codexAcpClient, "loadSession");
97+
98+
const deletePromise = codexAcpAgent.unstable_deleteSession({sessionId});
99+
await vi.waitFor(() => {
100+
expect(codexAcpClient.deleteSession).toHaveBeenCalledWith(sessionId);
101+
});
102+
103+
await expect(codexAcpAgent.loadSession({
104+
sessionId,
105+
cwd: "/test/cwd",
106+
mcpServers: [],
107+
})).rejects.toThrow("Invalid request");
108+
expect(loadSessionSpy).not.toHaveBeenCalled();
109+
110+
archive.resolve(undefined);
111+
await expect(deletePromise).resolves.toEqual({});
112+
});
113+
});
114+
115+
async function createSession(): Promise<{
116+
fixture: CodexMockTestFixture,
117+
codexAcpAgent: CodexAcpServer,
118+
codexAcpClient: CodexAcpClient,
119+
}> {
120+
const fixture = createCodexMockTestFixture();
121+
const codexAcpAgent = fixture.getCodexAcpAgent();
122+
const codexAcpClient = fixture.getCodexAcpClient();
123+
const model = createTestModel();
124+
125+
vi.spyOn(codexAcpClient, "authRequired").mockResolvedValue(false);
126+
vi.spyOn(codexAcpClient, "getAccount").mockResolvedValue({account: null, requiresOpenaiAuth: false});
127+
vi.spyOn(codexAcpClient, "newSession").mockResolvedValue({
128+
sessionId,
129+
currentModelId: "model-id[medium]",
130+
models: [model],
131+
currentServiceTier: null,
132+
});
133+
134+
await codexAcpAgent.newSession({cwd: "/test/cwd", mcpServers: []});
135+
fixture.clearCodexConnectionDump();
136+
fixture.clearAcpConnectionDump();
137+
138+
return {fixture, codexAcpAgent, codexAcpClient};
139+
}
140+
141+
function deferred<T>(): {promise: Promise<T>, resolve: (value: T) => void} {
142+
let resolve: (value: T) => void = () => {};
143+
const promise = new Promise<T>((innerResolve) => {
144+
resolve = innerResolve;
145+
});
146+
return {promise, resolve};
147+
}

0 commit comments

Comments
 (0)