diff --git a/agentclientprotocol-codex-acp-1.6.2.tgz b/agentclientprotocol-codex-acp-1.6.2.tgz new file mode 100644 index 00000000..58a2fa6b Binary files /dev/null and b/agentclientprotocol-codex-acp-1.6.2.tgz differ diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index bec75265..4e8fd265 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -554,7 +554,7 @@ export class CodexAcpClient { async runReview( sessionId: string, target: ReviewTarget, - onTurnStarted?: (turnId: string, threadId: string) => void, + onTurnStarted?: (turnId: string, threadId: string) => void | Promise, ): Promise { return await this.codexClient.runReview({ threadId: sessionId, @@ -575,7 +575,7 @@ export class CodexAcpClient { async setGoal( sessionId: string, objective: string, - onTurnStarted?: (turnId: string) => void, + onTurnStarted?: (turnId: string) => void | Promise, onGoalSet?: (goal: ThreadGoal) => void, ): Promise { const params = { @@ -605,7 +605,7 @@ export class CodexAcpClient { async resumeGoal( sessionId: string, - onTurnStarted?: (turnId: string) => void, + onTurnStarted?: (turnId: string) => void | Promise, onGoalSet?: (goal: ThreadGoal) => void, ): Promise { const params = { @@ -848,7 +848,7 @@ export class CodexAcpClient { disableSummary: boolean, cwd: string, additionalDirectories: string[], - onTurnStarted?: (turnId: string) => void, + onTurnStarted?: (turnId: string) => void | Promise, shouldCancel?: () => boolean, ): Promise { const input = buildPromptItems(request.prompt); diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index f3ebb373..efdf3b64 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -173,6 +173,15 @@ export interface SessionFailure { const CODEX_PROCESS_EXITED_ERROR_CODE = 1001; +/** + * The metadata key naming a backend's acceptance of a turn. + * + * Versioned, because a client reads it to decide whether a conversation now + * exists, and a changed meaning under an unchanged key would be read as the + * old one. + */ +const SESSION_MATERIALIZATION_META = "executablemd.session-materialization/v1"; + function clientSupportsAirCapability( capabilities: acp.ClientCapabilities | null, capability: string, @@ -2255,6 +2264,25 @@ export class CodexAcpServer { return turnId; } + /** + * Tell the client this thread's backend accepted a turn. + * + * A client that defers durable session state until a conversation really + * exists cannot learn that from what a turn produces: text, a stop reason + * and a terminal response each say the adapter is talking, not that the + * backend took the turn. This says exactly that and nothing else, on the + * session it happened on. + */ + private async publishSessionMaterialization(sessionId: string): Promise { + const session = new ACPSessionConnection(this.connection, sessionId); + await session.update({ + sessionUpdate: "session_info_update", + _meta: { + [SESSION_MATERIALIZATION_META]: {state: "accepted"}, + }, + }); + } + async prompt( params: acp.PromptRequest, signal?: AbortSignal, @@ -2273,6 +2301,12 @@ export class CodexAcpServer { : null; let agentFileChangeReportTurnId: string | null = null; let agentFileChangeReportUnavailableReason: AgentFileChangeReportUnavailableReason = "providerError"; + // The App Server turn that produced this prompt's terminal response, so a + // client can name the exact point this conversation reached. Request-local + // and assigned only where a turn actually completed: `currentTurnId` is + // session state that a concurrent prompt on the same session moves, and + // reading it here would report another prompt's turn as this one's. + let checkpointTurnId: string | null = null; let recoverableSessionFailure = sessionState.sessionFailure; sessionState.currentTurnId = null; sessionState.lastTokenUsage = null; @@ -2356,7 +2390,7 @@ export class CodexAcpServer { onTurnStartPending: () => { ensurePendingTurnStart(); }, - onTurnStarted: (turnId, threadId) => { + onTurnStarted: async (turnId, threadId) => { const turn = {threadId, turnId}; activePrompt.currentTurn = turn; if (this.promptShouldStop(params.sessionId, activePrompt)) { @@ -2365,6 +2399,10 @@ export class CodexAcpServer { } sessionState.currentTurnId = turnId; pendingTurnStart?.resolve(turnId); + // A command that starts a turn is a turn the backend + // accepted, exactly as an ordinary prompt is. A command that + // starts none never reaches here and publishes nothing. + await this.publishSessionMaterialization(params.sessionId); onTurnStarted?.(); }, setConfigOption: async (configId, value) => { @@ -2420,6 +2458,7 @@ export class CodexAcpServer { } if (commandResult.turnCompleted?.turn.status === "completed") { agentFileChangeReportTurnId = commandResult.turnCompleted.turn.id; + checkpointTurnId = commandResult.turnCompleted.turn.id; } else if (commandResult.turnCompleted === undefined) { agentFileChangeReportUnavailableReason = "notReported"; } @@ -2427,7 +2466,10 @@ export class CodexAcpServer { return { stopReason: "end_turn", usage: this.buildPromptUsage(sessionState.lastTokenUsage), - _meta: this.buildQuotaMeta(sessionState), + _meta: { + ...this.buildQuotaMeta(sessionState), + ...this.buildCheckpointMeta(checkpointTurnId), + }, }; } @@ -2469,7 +2511,7 @@ export class CodexAcpServer { disableSummary, sessionState.cwd, sessionState.additionalDirectories, - (turnId) => { + async (turnId) => { const turn = {threadId: params.sessionId, turnId}; activePrompt.currentTurn = turn; if (this.promptShouldStop(params.sessionId, activePrompt)) { @@ -2478,6 +2520,7 @@ export class CodexAcpServer { } sessionState.currentTurnId = turnId; pendingTurnStart?.resolve(turnId); + await this.publishSessionMaterialization(params.sessionId); onTurnStarted?.(); }, () => this.promptShouldStop(params.sessionId, activePrompt), @@ -2615,6 +2658,10 @@ export class CodexAcpServer { } if (turnCompleted.turn.status === "completed") { agentFileChangeReportTurnId = turnCompleted.turn.id; + // `turnCompleted` has already been reassigned if a plan + // implementation turn ran, so this is the final turn — the one + // that produced the response being returned. + checkpointTurnId = turnCompleted.turn.id; } await clearRecoveredSessionFailure(eventHandler); @@ -2627,7 +2674,10 @@ export class CodexAcpServer { return { stopReason: "end_turn", usage: this.buildPromptUsage(sessionState.lastTokenUsage), - _meta: this.buildQuotaMeta(sessionState), + _meta: { + ...this.buildQuotaMeta(sessionState), + ...this.buildCheckpointMeta(checkpointTurnId), + }, }; } catch (err) { logger.error(`Prompt for session ${params.sessionId} failed`, err); @@ -2742,6 +2792,21 @@ export class CodexAcpServer { }; } + /** + * Which App Server turn this response is, when one completed. + * + * A client that keeps this can later say exactly where a conversation had + * reached, rather than "wherever that session is now". The value is the App + * Server's own turn id, carried out unchanged. + * + * Empty for a cancelled prompt, for a failed one, and for a command this + * adapter answered itself without ever starting a provider turn — none of + * those is a point anything could resume from. + */ + private buildCheckpointMeta(turnId: string | null): { codex?: { turnId: string } } { + return turnId === null ? {} : { codex: { turnId } }; + } + private buildQuotaMeta(sessionState: SessionState): { quota: QuotaMeta } { const lastTokenUsage = sessionState.lastTokenUsage; diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index 0f802d68..cdb28a25 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -274,7 +274,7 @@ export class CodexAppServerClient { return await this.sendRequest({ method: "turn/start", params: params }); } - async runTurn(params: TurnStartParams, onTurnStarted?: (turnId: string) => void): Promise { + async runTurn(params: TurnStartParams, onTurnStarted?: (turnId: string) => void | Promise): Promise { const capturedCompletions: Array = []; const releaseCapture = this.captureTurnCompletions(params.threadId, (event) => { capturedCompletions.push(event); @@ -282,7 +282,9 @@ export class CodexAppServerClient { try { const turnStarted = await this.turnStart(params); - onTurnStarted?.(turnStarted.turn.id); + // Awaited: a caller that publishes the acceptance of this turn has to + // publish it before anything the turn produces reaches the client. + await onTurnStarted?.(turnStarted.turn.id); const earlyCompletion = capturedCompletions.find(event => event.turn.id === turnStarted.turn.id); releaseCapture(); if (earlyCompletion) { @@ -298,7 +300,7 @@ export class CodexAppServerClient { async runReview( params: ReviewStartParams, - onTurnStarted?: (turnId: string, threadId: string) => void, + onTurnStarted?: (turnId: string, threadId: string) => void | Promise, ): Promise { const capturedCompletions: Array = []; const releaseCapture = this.captureTurnCompletions(params.threadId, (event) => { @@ -307,7 +309,10 @@ export class CodexAppServerClient { try { const reviewStarted = await this.reviewStart(params); - onTurnStarted?.(reviewStarted.turn.id, reviewStarted.reviewThreadId); + // Awaited for the same reason `runTurn` awaits it: a caller that + // publishes this turn's acceptance has to publish it before + // anything the turn produces reaches the client. + await onTurnStarted?.(reviewStarted.turn.id, reviewStarted.reviewThreadId); const earlyCompletion = capturedCompletions.find(event => event.turn.id === reviewStarted.turn.id); releaseCapture(); if (earlyCompletion) { @@ -321,7 +326,7 @@ export class CodexAppServerClient { async runGoalSet( params: ThreadGoalSetParams, - onTurnStarted?: (turnId: string) => void, + onTurnStarted?: (turnId: string) => void | Promise, runtimeEffectsGraceMs = GOAL_RUNTIME_EFFECTS_GRACE_MS, onGoalSet?: (goal: ThreadGoal) => void, ): Promise { @@ -349,12 +354,17 @@ export class CodexAppServerClient { let expectedGoal: ThreadGoal | null = null; const noGoalTurnStarted = this.createNoGoalTurnStartedPromise(runtimeEffectsGraceMs); const capturedGoalUpdates: Array = []; + // A goal turn is routed to us on a notification, so the callback runs + // where nothing can be awaited. What it starts is kept instead, and + // waited for below — before this returns, and therefore before the + // prompt it belongs to answers. + let turnStartedPublication: Promise | undefined; const releaseRoutingCapture = this.captureTurnRoutings(params.threadId, (turnId) => { if (!goalUpdateHandled || goalTurnId !== null) { return; } goalTurnId = turnId; - onTurnStarted?.(turnId); + turnStartedPublication = Promise.resolve(onTurnStarted?.(turnId)).then(() => {}); resolveGoalTurnStarted(turnId); }); const releaseGoalUpdateCapture = this.captureThreadGoalUpdates(params.threadId, (event) => { @@ -386,6 +396,7 @@ export class CodexAppServerClient { return null; } const turnId = goalTurnId ?? await Promise.race([goalTurnStarted, noGoalTurnStarted.promise]); + await turnStartedPublication; noGoalTurnStarted.release(); releaseRoutingCapture(); releaseStatusCapture(); diff --git a/src/CodexCommands.ts b/src/CodexCommands.ts index e4cd16d4..68874222 100644 --- a/src/CodexCommands.ts +++ b/src/CodexCommands.ts @@ -30,7 +30,7 @@ export const GOAL_CONTINUATION_PROMPT: acp.ContentBlock[] = [{ export type CommandHandleOptions = { onTurnStartPending?: () => void; - onTurnStarted?: (turnId: string, threadId: string) => void; + onTurnStarted?: (turnId: string, threadId: string) => void | Promise; setConfigOption?: (configId: string, value: string) => Promise; }; @@ -315,8 +315,8 @@ export class CodexCommands { return await this.runWithProcessCheck(() => this.codexAcpClient.runReview( sessionState.sessionId, target, - (turnId, threadId) => { - this.handleCommandTurnStarted(sessionState, options, turnId, threadId); + async (turnId, threadId) => { + await this.handleCommandTurnStarted(sessionState, options, turnId, threadId); }, )); } @@ -341,8 +341,8 @@ export class CodexCommands { options.onTurnStartPending?.(); return this.createGoalCommandResult(await this.runWithProcessCheck(() => this.codexAcpClient.resumeGoal( sessionId, - (turnId) => { - this.handleCommandTurnStarted(sessionState, options, turnId, sessionId); + async (turnId) => { + await this.handleCommandTurnStarted(sessionState, options, turnId, sessionId); }, ))); case "clear": @@ -360,20 +360,24 @@ export class CodexCommands { return this.createGoalCommandResult(await this.runWithProcessCheck(() => this.codexAcpClient.setGoal( sessionId, argument, - (turnId) => { - this.handleCommandTurnStarted(sessionState, options, turnId, sessionId); + async (turnId) => { + await this.handleCommandTurnStarted(sessionState, options, turnId, sessionId); }, ))); } - private handleCommandTurnStarted( + private async handleCommandTurnStarted( sessionState: SessionState, options: CommandHandleOptions, turnId: string, threadId: string, - ): void { + ): Promise { if (options.onTurnStarted) { - options.onTurnStarted(turnId, threadId); + // Awaited, so a caller that publishes this turn's acceptance + // publishes it before the command goes on — every path that starts + // a turn reports it the same way, and a command that starts none + // never arrives here. + await options.onTurnStarted(turnId, threadId); } else { sessionState.currentTurnId = turnId; } diff --git a/src/__tests__/CodexACPAgent/checkpoint-metadata.test.ts b/src/__tests__/CodexACPAgent/checkpoint-metadata.test.ts new file mode 100644 index 00000000..631c37ee --- /dev/null +++ b/src/__tests__/CodexACPAgent/checkpoint-metadata.test.ts @@ -0,0 +1,124 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { createCodexMockTestFixture, createTestSessionState, type CodexMockTestFixture } from '../acp-test-utils'; +import type { Turn } from '../../app-server/v2'; + +/** + * The App Server turn a prompt response names. + * + * A client that keeps this can say exactly where a conversation had reached, + * rather than "wherever that session is now". Everything here compares the + * identity the App Server emitted with the identity the response returned, + * byte for byte — the point is that they are the same string, not that one is + * present. + */ + +function turn(id: string, status: string): Turn { + return { + id, + items: [], + itemsView: 'notLoaded', + status, + error: null, + startedAt: null, + completedAt: null, + durationMs: null, + } as unknown as Turn; +} + +/** + * Drive one prompt whose App Server turn completes with `emitted` as its id. + * + * `statuses` lets a case end the turn as something other than a completion, so + * an interrupted turn can be asked the same question. + */ +function promptWith( + fixture: CodexMockTestFixture, + sessionId: string, + emitted: string, + status = 'completed', +) { + const codexAcpAgent = fixture.getCodexAcpAgent(); + const client = fixture.getCodexAppServerClient(); + + client.turnStart = vi.fn().mockResolvedValue({ turn: turn(emitted, 'inProgress') }); + client.awaitTurnCompleted = vi.fn().mockResolvedValue({ + threadId: sessionId, + turn: turn(emitted, status), + }); + vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue(createTestSessionState({ sessionId })); + + return codexAcpAgent.prompt({ + sessionId, + prompt: [{ type: 'text', text: 'test prompt' }], + }); +} + +describe('Prompt checkpoint metadata', () => { + let mockFixture: CodexMockTestFixture; + const sessionId = 'test-session-id'; + + beforeEach(() => { + mockFixture = createCodexMockTestFixture(); + vi.clearAllMocks(); + }); + + it('returns the exact turn id the App Server emitted', async () => { + const emitted = 'turn-01J9ZQ8N4K5X7YB2M3P6R8T0V1'; + + const response = await promptWith(mockFixture, sessionId, emitted); + + expect(response.stopReason).toBe('end_turn'); + // The same string, not a derived or shortened one: a client compares it + // against what it kept, and any normalisation here would break that. + expect(response._meta?.['codex']).toEqual({ turnId: emitted }); + }); + + it('keeps the quota metadata it already reported', async () => { + const response = await promptWith(mockFixture, sessionId, 'turn-quota'); + + // The checkpoint is added beside what a client already reads, never in + // place of it. + expect(response._meta).toHaveProperty('quota'); + expect(response._meta?.['codex']).toEqual({ turnId: 'turn-quota' }); + }); + + it('names no turn when the turn was interrupted', async () => { + const response = await promptWith(mockFixture, sessionId, 'turn-interrupted', 'interrupted'); + + expect(response.stopReason).toBe('cancelled'); + // A cancelled prompt reached no point anything could resume from, + // whatever turn happened to be in flight when it was cancelled. + expect(response._meta?.['codex']).toBeUndefined(); + }); + + it('names no turn when the prompt failed', async () => { + const codexAcpAgent = mockFixture.getCodexAcpAgent(); + const client = mockFixture.getCodexAppServerClient(); + client.turnStart = vi.fn().mockResolvedValue({ turn: turn('turn-failed', 'inProgress') }); + client.awaitTurnCompleted = vi.fn().mockRejectedValue(new Error('the provider failed')); + vi.spyOn(codexAcpAgent, 'getSessionState').mockReturnValue( + createTestSessionState({ sessionId }), + ); + + await expect( + codexAcpAgent.prompt({ sessionId, prompt: [{ type: 'text', text: 'test prompt' }] }), + ).rejects.toThrow(); + }); + + it('gives interleaved sessions their own turn ids', async () => { + // Two prompts in flight at once on two sessions. A response built from + // session-global state — the adapter's own `currentTurnId`, say — would + // hand at least one of them the other's turn, and the whole point of a + // checkpoint is that it names this exact turn. + const first = createCodexMockTestFixture(); + const second = createCodexMockTestFixture(); + + const [alpha, beta] = await Promise.all([ + promptWith(first, 'session-alpha', 'turn-alpha'), + promptWith(second, 'session-beta', 'turn-beta'), + ]); + + expect(alpha._meta?.['codex']).toEqual({ turnId: 'turn-alpha' }); + expect(beta._meta?.['codex']).toEqual({ turnId: 'turn-beta' }); + }); +}); diff --git a/src/__tests__/CodexACPAgent/data/token-usage-end-turn.json b/src/__tests__/CodexACPAgent/data/token-usage-end-turn.json index 60ba2e2e..70621e29 100644 --- a/src/__tests__/CodexACPAgent/data/token-usage-end-turn.json +++ b/src/__tests__/CodexACPAgent/data/token-usage-end-turn.json @@ -28,6 +28,9 @@ } } ] + }, + "codex": { + "turnId": "turn-id" } } } diff --git a/src/__tests__/CodexACPAgent/data/token-usage-multiple-updates.json b/src/__tests__/CodexACPAgent/data/token-usage-multiple-updates.json index aebc1783..3289a1d9 100644 --- a/src/__tests__/CodexACPAgent/data/token-usage-multiple-updates.json +++ b/src/__tests__/CodexACPAgent/data/token-usage-multiple-updates.json @@ -28,6 +28,9 @@ } } ] + }, + "codex": { + "turnId": "turn-id" } } } diff --git a/src/__tests__/CodexACPAgent/data/token-usage-null.json b/src/__tests__/CodexACPAgent/data/token-usage-null.json index 65196087..d833d248 100644 --- a/src/__tests__/CodexACPAgent/data/token-usage-null.json +++ b/src/__tests__/CodexACPAgent/data/token-usage-null.json @@ -5,6 +5,9 @@ "quota": { "token_count": null, "model_usage": [] + }, + "codex": { + "turnId": "turn-id" } } } diff --git a/src/__tests__/CodexACPAgent/session-materialization-events.test.ts b/src/__tests__/CodexACPAgent/session-materialization-events.test.ts new file mode 100644 index 00000000..523fbe8a --- /dev/null +++ b/src/__tests__/CodexACPAgent/session-materialization-events.test.ts @@ -0,0 +1,167 @@ +/** + * The backend-acceptance marker (`executablemd.session-materialization/v1`). + * + * A client that defers creating durable session state until a conversation + * really exists needs one fact from the adapter: the App Server took the turn. + * Nothing a turn produces says that — text, a stop reason and a terminal + * response each say the adapter is talking — so the adapter says it itself, at + * the one boundary where it becomes true. + * + * The shared fixture keeps this marker out of the session updates every other + * test compares (see `isSessionMaterializationEvent`), so these read it from + * the accessor that keeps it. + */ +import { describe, expect, it, vi } from "vitest"; +import * as acp from "@agentclientprotocol/sdk"; +import { + createCodexMockTestFixture, + createTestSessionState, +} from "../acp-test-utils"; + +const SESSION_MATERIALIZATION_META = "executablemd.session-materialization/v1"; + +function createTurn(status: "inProgress" | "completed", id: string) { + return { + id, + items: [], + itemsView: "notLoaded" as const, + status, + error: null, + startedAt: null, + completedAt: null, + durationMs: null, + }; +} + +/** One prompt whose turn the App Server starts and completes. */ +async function runPrompt(sessionId: string, options: {startTurn?: boolean} = {}) { + const mockFixture = createCodexMockTestFixture(); + const codexAcpAgent = mockFixture.getCodexAcpAgent(); + const codexAppServerClient = mockFixture.getCodexAppServerClient(); + await codexAcpAgent.initialize({ + protocolVersion: acp.PROTOCOL_VERSION, + clientCapabilities: {fs: {readTextFile: false, writeTextFile: false}, terminal: false}, + }); + const sessionState = createTestSessionState({sessionId}); + vi.spyOn(codexAcpAgent, "getSessionState").mockReturnValue(sessionState); + if (options.startTurn === false) { + // `turn/start` never answers with a turn, so the backend accepted + // nothing — the boundary the marker names was never reached. + vi.spyOn(codexAppServerClient, "turnStart").mockRejectedValue(new Error("turn refused")); + } else { + vi.spyOn(codexAppServerClient, "turnStart").mockResolvedValue({ + turn: createTurn("inProgress", "turn-id"), + }); + vi.spyOn(codexAppServerClient, "awaitTurnCompleted").mockResolvedValue({ + threadId: sessionId, + turn: createTurn("completed", "turn-id"), + }); + } + try { + await codexAcpAgent.prompt({sessionId, prompt: [{type: "text", text: "hello"}]}); + } catch { + // A refused turn/start fails the prompt; what these ask about is the + // marker, and a prompt that raised published none. + } + return mockFixture; +} + +/** + * One prompt whose text is a slash command. + * + * `startsTurn` decides which kind: a review is a command that starts an App + * Server turn, and `/goal` with no objective is one that answers with a usage + * message and starts none. + */ +async function runCommand( + sessionId: string, + prompt: string, + options: {startsTurn: boolean}, +) { + const mockFixture = createCodexMockTestFixture(); + const codexAcpAgent = mockFixture.getCodexAcpAgent(); + const codexAcpClient = mockFixture.getCodexAcpClient(); + await codexAcpAgent.initialize({ + protocolVersion: acp.PROTOCOL_VERSION, + clientCapabilities: {fs: {readTextFile: false, writeTextFile: false}, terminal: false}, + }); + const sessionState = createTestSessionState({sessionId}); + vi.spyOn(codexAcpAgent, "getSessionState").mockReturnValue(sessionState); + if (options.startsTurn) { + vi.spyOn(codexAcpClient, "runReview").mockImplementation(async (_sessionId, _target, onTurnStarted) => { + await onTurnStarted?.("turn-review", sessionId); + return {threadId: sessionId, turn: createTurn("completed", "turn-review")}; + }); + } + await codexAcpAgent.prompt({sessionId, prompt: [{type: "text", text: prompt}]}); + return mockFixture; +} + +describe("session materialization marker", () => { + it("reports acceptance once, on the session whose turn started", async () => { + const mockFixture = await runPrompt("accepted-session"); + + const markers = mockFixture.getSessionMaterializationEvents(); + expect(markers).toHaveLength(1); + expect(markers[0]!.args[1]).toEqual({ + sessionId: "accepted-session", + update: { + sessionUpdate: "session_info_update", + _meta: {[SESSION_MATERIALIZATION_META]: {state: "accepted"}}, + }, + }); + }); + + it("carries no session information of its own", async () => { + const mockFixture = await runPrompt("control-only-session"); + + const update = mockFixture.getSessionMaterializationEvents()[0]!.args[1].update; + // Title and updatedAt are what a `session_info_update` otherwise + // carries. This one states acceptance and nothing else, so a client + // reading it as session info reads nothing. + expect(Object.keys(update).sort()).toEqual(["_meta", "sessionUpdate"]); + }); + + it("publishes nothing when the backend never started a turn", async () => { + const mockFixture = await runPrompt("refused-session", {startTurn: false}); + + expect(mockFixture.getSessionMaterializationEvents()).toEqual([]); + }); + + it("reports acceptance for a command that starts a turn", async () => { + // `/review` and `/goal` reach the App Server through their own path, and + // a turn it accepts is a turn like any other. A client waiting for + // acceptance must not be left waiting because the turn was asked for by + // a command. + const mockFixture = await runCommand("review-session", "/review", {startsTurn: true}); + + const markers = mockFixture.getSessionMaterializationEvents(); + expect(markers).toHaveLength(1); + expect(markers[0]!.args[1]).toEqual({ + sessionId: "review-session", + update: { + sessionUpdate: "session_info_update", + _meta: {[SESSION_MATERIALIZATION_META]: {state: "accepted"}}, + }, + }); + }); + + it("publishes nothing for a command that starts no turn", async () => { + // A usage message is not a conversation. Nothing was accepted, so + // nothing is reported — the marker says a backend took a turn, and no + // backend was asked. + const mockFixture = await runCommand("local-session", "/goal", {startsTurn: false}); + + expect(mockFixture.getSessionMaterializationEvents()).toEqual([]); + }); + + it("stays out of the session updates a prompt reports", async () => { + const mockFixture = await runPrompt("separated-session"); + + const updates = mockFixture.getAcpConnectionEvents([]) + .filter(event => event.method === "sessionUpdate") + .map(event => event.args[0].update); + expect(updates.filter((update: {_meta?: Record}) => + update._meta?.[SESSION_MATERIALIZATION_META] !== undefined)).toEqual([]); + }); +}); diff --git a/src/__tests__/acp-test-utils.ts b/src/__tests__/acp-test-utils.ts index 6b9e20c0..f48ea168 100644 --- a/src/__tests__/acp-test-utils.ts +++ b/src/__tests__/acp-test-utils.ts @@ -39,6 +39,25 @@ export function createSmartMock( }); } +/** + * Whether this notification is the backend-acceptance marker. + * + * It is control data: every accepted turn reports it, it carries no session + * information of its own, and a client that is not waiting for it ignores it. + * The shared collector drops it so that every test comparing the session + * updates a prompt produces keeps describing the conversation. The marker's own + * emission is proven in `session-materialization-events.test.ts`, against a + * connection mock that collects everything. + */ +export function isSessionMaterializationEvent(event: MethodCallEvent): boolean { + if (event.method !== "notify" || event.args[0] !== acp.methods.client.session.update) { + return false; + } + const update = (event.args[1] as {update?: {sessionUpdate?: string; _meta?: Record}})?.update; + return update?.sessionUpdate === "session_info_update" + && update._meta?.["executablemd.session-materialization/v1"] !== undefined; +} + function normalizeAcpConnectionEvent(event: MethodCallEvent): MethodCallEvent { if (event.method === "request" && event.args[0] === acp.methods.client.session.requestPermission) { return {method: "requestPermission", args: [event.args[1]]}; @@ -68,6 +87,8 @@ export interface TestFixture { onAcpConnectionEvent(handler: (event: MethodCallEvent) => void): void, getAcpConnectionEvents(ignoredFields: string[]): MethodCallEvent[], getAcpConnectionDump(ignoredFields: string[]): string, + /** The backend-acceptance markers this fixture kept out of the events above. */ + getSessionMaterializationEvents(): MethodCallEvent[], clearAcpConnectionDump(): void, } @@ -79,6 +100,7 @@ export interface AcpConnectionConfig { connection: AcpClientConnection; events: MethodCallEvent[]; eventHandlers: ((event: MethodCallEvent) => void)[]; + materializationEvents?: MethodCallEvent[]; } export interface ConnectionConfig { @@ -90,7 +112,12 @@ export interface ConnectionConfig { export function createBaseTestFixture(config: ConnectionConfig): TestFixture { const acpConnectionEvents = config.acpConnection?.events ?? []; const acpEventHandlers = config.acpConnection?.eventHandlers ?? []; + const acpMaterializationEvents = config.acpConnection?.materializationEvents ?? []; const acpConnection = config.acpConnection?.connection ?? createSmartMock((event) => { + if (isSessionMaterializationEvent(event)) { + acpMaterializationEvents.push(event); + return; + } const normalizedEvent = normalizeAcpConnectionEvent(event); acpConnectionEvents.push(normalizedEvent); acpEventHandlers.forEach(handler => handler(normalizedEvent)); @@ -165,6 +192,9 @@ export function createBaseTestFixture(config: ConnectionConfig): TestFixture { getAcpConnectionDump(ignoredFields: string[]): string { return createArrayDump(this.getAcpConnectionEvents(ignoredFields), []); }, + getSessionMaterializationEvents(): MethodCallEvent[] { + return [...acpMaterializationEvents]; + }, clearAcpConnectionDump() { acpConnectionEvents.splice(0, acpConnectionEvents.length); } @@ -302,7 +332,12 @@ export function createCodexMockTestFixture( }); returnValues.set('requestPermission', () => permissionState.response); + const acpMaterializationEvents: MethodCallEvent[] = []; const acpConnection = createSmartMock((event) => { + if (isSessionMaterializationEvent(event)) { + acpMaterializationEvents.push(event); + return; + } const normalizedEvent = normalizeAcpConnectionEvent(event); acpConnectionEvents.push(normalizedEvent); acpEventHandlers.forEach(handler => handler(normalizedEvent)); @@ -315,6 +350,7 @@ export function createCodexMockTestFixture( connection: acpConnection, events: acpConnectionEvents, eventHandlers: acpEventHandlers, + materializationEvents: acpMaterializationEvents, } }); if (restartCodexClient) {