diff --git a/readme-dev.md b/readme-dev.md index f26698b78..12b0fa8f9 100644 --- a/readme-dev.md +++ b/readme-dev.md @@ -66,6 +66,29 @@ Run from binaries } ``` +### Native conversation renaming + +New sessions expose the eager `set_thread_title({title: "A short title"})` dynamic tool. +It takes exactly one non-empty string, normalizes whitespace, and applies the existing +256-code-unit display limit without splitting surrogate pairs. The app-server supplies +the current thread and turn; the tool cannot select another thread. It needs no AIR +capability or MCP service. Successful calls persist through `thread/name/set` and send +an ACP `session_info_update` for both protocol versions. + +Registration uses the experimental `thread/start.dynamicTools` field, verified with bundled +Codex 0.165.0. Threads keep the tool after resume. Threads created without it do not gain it +when loaded; start a new thread or use `/rename`. + +Explicit renames share the automatic-title write queue. A rejected write lets automatic +titles continue. An uncertain or saved-but-not-delivered write keeps them blocked, and the +tool result says so; check the saved title before retrying. + +Check it with: + +```bash +npm run codex-test -- -p 'Call set_thread_title with title "Native rename verification". Do not use any other tool.' +``` + ### Build binaries Building standalone binaries requires [bun](https://bun.com/docs/installation). diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index ed7e6120f..533d702a7 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -32,6 +32,7 @@ import {isDeepStrictEqual} from "node:util"; import {logger} from "./Logger"; import {isAccountReadAuthFailureError, isAccountReadUnavailableError} from "./CodexThreadErrors"; import {listedSessionTitle, normalizeSessionTitle} from "./SessionTitle"; +import {SESSION_TITLE_TOOL} from "./SessionTitleTool"; import {type AcpMcpServer, type WithAcpMcpServers, normalizeMcpServers, toCodexMcpServerConfig} from "./McpServerConfig"; import type { AccountUpdatedNotification, @@ -867,6 +868,7 @@ export class CodexAcpClient { await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []); const response = await this.codexClient.threadStart({ config, + dynamicTools: [SESSION_TITLE_TOOL], modelProvider: this.getModelProvider(), cwd: request.cwd, ...(developerInstructions !== undefined && {developerInstructions}), diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 87a438872..7d6305c1d 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -85,6 +85,8 @@ import type { import type {RateLimitsMap} from "./RateLimitsMap"; import {ModelId} from "./ModelId"; import {normalizeSessionTitle} from "./SessionTitle"; +import {sessionTitleArgument, sessionTitleToolResult} from "./SessionTitleTool"; +import {SessionTitleWriteError} from "./SessionIndexTitles"; import {AgentMode, MODE_CONFIG_ID} from "./AgentMode"; import { COLLABORATION_MODE_CONFIG_ID, @@ -648,6 +650,49 @@ export class CodexAcpServer { this.currentAuthStatus = null; this.availableCommands = this.createAvailableCommands(codexAcpClient); this.recovery = this.createRecovery(); + this.bindSessionTitleTool(codexAcpClient); + } + + private setCodexClient(client: CodexAcpClient): void { + this.codexAcpClient = client; + this.bindSessionTitleTool(client); + } + + private bindSessionTitleTool(client: CodexAcpClient): void { + client.appServerClient.onDynamicToolCall(async (params) => { + if (params.tool !== "set_thread_title" || params.namespace != null) { + return sessionTitleToolResult(false, "Unsupported dynamic tool."); + } + const title = sessionTitleArgument(params.arguments); + if (title === null) return sessionTitleToolResult(false, "Expected exactly one non-empty string property: title."); + const state = this.sessions.get(params.threadId); + const generation = this.getSessionGeneration(params.threadId); + const assertCurrent = () => { + if (!state || this.sessions.get(params.threadId) !== state + || this.codexAcpClient !== client || this.sessionIsClosing(params.threadId) + || this.getSessionGeneration(params.threadId) !== generation + || !client.appServerClient.isCurrentToolTurn(params.threadId, params.turnId)) { + throw new Error("The session or turn is no longer current."); + } + }; + try { + assertCurrent(); + const saved = await this.sessionIndex.renameTitle(params.threadId, title, {client, assertCurrent}); + return sessionTitleToolResult(true, `Session title saved as ${JSON.stringify(saved)}.`); + } catch (err) { + if (err instanceof SessionTitleWriteError) { + switch (err.outcome) { + case "saved": + return sessionTitleToolResult(false, `Session title saved as ${JSON.stringify(err.title)}, but the session changed or the client update failed. Do not retry the rename.`); + case "uncertain": + return sessionTitleToolResult(false, "The rename outcome is uncertain; it may have been saved. Automatic title writes are blocked. Check the saved title before retrying."); + case "rejected": + return sessionTitleToolResult(false, `Session title was not changed: ${err.message}`); + } + } + return sessionTitleToolResult(false, `Session title was not changed: ${err instanceof Error ? err.message : String(err)}`); + } + }); } private createAvailableCommands(client: CodexAcpClient): CodexCommands { @@ -2374,7 +2419,7 @@ export class CodexAcpServer { throw new Error("Cannot restart Codex app-server before ACP initialization"); } await replacement.initialize(this.initializeRequest); - this.codexAcpClient = replacement; + this.setCodexClient(replacement); this.sessionIndex.observe(replacement); this.availableCommands = this.createAvailableCommands(replacement); @@ -2605,7 +2650,7 @@ export class CodexAcpServer { private installCodexClient(client: CodexAcpClient): void { // A `providers/set` can have changed the routing of the old client while the restart ran. client.adoptRoutingFrom(this.codexAcpClient); - this.codexAcpClient = client; + this.setCodexClient(client); // The session index watches the thread notifications of the app-server that runs now. this.sessionIndex.observe(client); this.availableCommands = this.createAvailableCommands(client); diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index fa5b2f53c..23466ffca 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -12,6 +12,9 @@ import type { } from "./app-server"; import type { ActivePermissionProfile, + DynamicToolSpec, + DynamicToolCallParams, + DynamicToolCallResponse, CancelLoginAccountParams, CancelLoginAccountResponse, ConfigReadParams, @@ -157,6 +160,8 @@ const ToolRequestUserInputRequest = new RequestType< const GOAL_RUNTIME_EFFECTS_GRACE_MS = 1_000; +const DynamicToolCallRequest = new RequestType('item/tool/call'); + /** The number of turns in one page of a full history read. */ const HISTORY_PAGE_TURNS = 5; @@ -177,6 +182,7 @@ export class CodexAppServerClient { readonly connection: MessageConnection; private approvalHandlers = new Map(); private elicitationHandlers = new Map(); + private dynamicToolHandler: ((params: DynamicToolCallParams) => Promise) | undefined; readonly mcpStartup = new McpStartupTracker(); readonly mcpOauthCompletions = new McpOauthCompletions(); private readonly pendingTurnCompletionResolvers = new Map void>>(); @@ -302,6 +308,22 @@ export class CodexAppServerClient { } return await handler.handleUserInput(params); }); + + this.connection.onRequest(DynamicToolCallRequest, async (params) => { + if (!this.isCurrentToolTurn(params.threadId, params.turnId) || !this.dynamicToolHandler) { + return {success: false, contentItems: [{type: "inputText", text: "The tool is unavailable for this session or turn."}]}; + } + return await this.dynamicToolHandler(params); + }); + } + + onDynamicToolCall(handler: (params: DynamicToolCallParams) => Promise): void { + this.dynamicToolHandler = handler; + } + + isCurrentToolTurn(threadId: string, turnId: string): boolean { + return !this.connectionLoss.lost && !this.isStaleTurn(threadId, turnId) + && this.runningTurnIds.get(threadId) === turnId; } onApprovalRequest(threadId: string, handler: ApprovalHandler): void { @@ -676,7 +698,7 @@ export class CodexAppServerClient { this.staleTurnIds.set(threadId, threadStaleTurns); } - async threadStart(params: ThreadStartParams): Promise { + async threadStart(params: ThreadStartParams & {dynamicTools?: DynamicToolSpec[]}): Promise { return await this.sendRequest({ method: "thread/start", params: params }); } @@ -1297,6 +1319,7 @@ export class CodexAppServerClient { } private disposeMcpWaits(): void { + this.dynamicToolHandler = undefined; this.mcpStartup.dispose(); this.mcpOauthCompletions.dispose(); } diff --git a/src/SessionIndexService.ts b/src/SessionIndexService.ts index 2df9289e8..df6e2ad26 100644 --- a/src/SessionIndexService.ts +++ b/src/SessionIndexService.ts @@ -1,7 +1,7 @@ /** * The AIR `sessionIndex` extension as `CodexAcpServer` serves it: the session list, `_session/rename`, * `_session/archive`, `_session/unarchive`, `session/delete` and `_session/list/subscribe`. - * `session/set_title` shares its title writer without requiring AIR capabilities. + * `session/set_title` and the native rename tool share its title writer without requiring AIR capabilities. * See `SessionIndex.ts` and `docs/air-extensions.md`. */ @@ -43,7 +43,7 @@ import { type SessionRenameRequest, } from "./SessionIndex"; import {deleteThread, sessionIndexRequestError, setThreadArchived} from "./SessionIndexMutations"; -import {SessionIndexTitles} from "./SessionIndexTitles"; +import {SessionIndexTitles, SessionTitleWriteError, type SessionTitleWriteContext} from "./SessionIndexTitles"; import {canonicalCwds, CodexWorktreeMarks, isUncPath, linkedWorktreeCwds} from "./SessionIndexWorktrees"; import {SessionForkOrigins} from "./SessionForkOrigins"; import {SessionListSubscriptions, type SessionListSubscriptionTimings, type ThreadEntry} from "./SessionListSubscriptions"; @@ -244,7 +244,16 @@ export class SessionIndexService { /** Standard title requests share persistence, normalization and automatic-title suppression with AIR. */ async setTitle({sessionId, title}: acp.SetSessionTitleRequest): Promise { await this.host.ensureAppServer(); - return {title: await this.titles.rename(sessionId, title)}; + try { + return {title: await this.titles.rename(sessionId, title)}; + } catch (err) { + throw err instanceof SessionTitleWriteError ? err.cause : err; + } + } + + /** Internal tool write; the caller fences its live session without AIR negotiation. */ + renameTitle(sessionId: string, title: string, context: SessionTitleWriteContext): Promise { + return this.titles.rename(sessionId, title, context); } /** diff --git a/src/SessionIndexTitles.ts b/src/SessionIndexTitles.ts index e235b31ec..cf6c15e5b 100644 --- a/src/SessionIndexTitles.ts +++ b/src/SessionIndexTitles.ts @@ -1,6 +1,6 @@ /** * Client-set session titles and how they and the automatic title of `TitleGenerator` - * keep out of each other's way. Shared by `session/set_title` and AIR's `_session/rename`. + * keep out of each other's way. Shared by `session/set_title`, AIR's `_session/rename` and the native rename tool. */ import {RequestError} from "@agentclientprotocol/sdk/unstable"; @@ -9,24 +9,38 @@ import type {SessionIndexHost} from "./SessionIndexService"; import {renameRequestError} from "./SessionIndexMutations"; import {normalizeSessionTitle} from "./SessionTitle"; import type {SessionWriteQueue} from "./SessionWriteQueue"; +import type {CodexAcpClient} from "./CodexAcpClient"; +import type {SessionState} from "./CodexAcpServer"; +import {ErrorCodes, ResponseError} from "vscode-jsonrpc/node"; + +export interface SessionTitleWriteContext { + client: CodexAcpClient; + assertCurrent(): void; +} + +export class SessionTitleWriteError extends Error { + constructor(readonly outcome: "rejected" | "uncertain" | "saved", readonly title: string, override readonly cause: unknown) { + super(cause instanceof Error ? cause.message : String(cause)); + } +} /** The title fields that `SessionState` carries for the session index. */ export interface SessionIndexTitleState { /** - * The title that `_session/rename` set while the session was loaded here, until its + * The title that an explicit writer set while the session was loaded here, until its * `thread/name/updated` echo. `/rename` clears it. */ sessionIndexExplicitTitle?: string; /** * The automatic title that the title generation wrote, until its `thread/name/updated` echo. When that - * echo comes after a `_session/rename` with another title, it is stale and is not shown. + * echo comes after an explicit rename with another title, it is stale and is not shown. */ automaticTitleEcho?: string; } /** * Takes a `thread/name/updated` of a loaded session into account, and tells whether it is a late echo of an - * automatic title that Codex wrote before `_session/rename`, which the client must not see. + * automatic title that Codex wrote before an explicit rename, which the client must not see. */ export function isStaleAutomaticTitleEcho(state: SessionIndexTitleState, name: string | null): boolean { const explicitTitle = state.sessionIndexExplicitTitle; @@ -44,22 +58,26 @@ export function isStaleAutomaticTitleEcho(state: SessionIndexTitleState, name: s export class SessionIndexTitles { /** - * The sessions whose title `_session/rename` has set or is setting, by session id. It outlives the + * The sessions with an explicit title write, by session id. It outlives the * `SessionState`, so an automatic title of a session that was closed or loaded again since its generation * started still sees the rename, see {@link writeAutomaticTitle}. */ - private readonly explicitTitles = new Map(); + private readonly explicitTitles = new Map(); constructor(private readonly host: SessionIndexHost, private readonly writes: SessionWriteQueue) {} /** - * `_session/rename`: sets the explicit title of a thread, loaded or not. + * Sets an explicit title. A tool call also fences the originating session and app-server. * * The rename is recorded before its write waits for its turn in the write queue, so an automatic title * that has not been written yet is skipped, and one that Codex is writing now completes before this write - * starts. Automatic titles stop for good only once the rename succeeded. + * starts. An uncertain write stays protected because it may have succeeded. */ - async rename(sessionId: string, requestedTitle: string): Promise { + async rename(sessionId: string, requestedTitle: string, context?: SessionTitleWriteContext): Promise { const title = normalizeSessionTitle(requestedTitle); if (title === null) { throw RequestError.invalidParams({sessionId}, "title must be a non-empty string"); @@ -69,24 +87,39 @@ export class SessionIndexTitles { if (sessionState) { sessionState.sessionTitleSource = "explicit"; } - const explicitTitle = this.explicitTitles.get(sessionId) ?? {confirmed: false, pending: 0}; + const explicitTitle = this.explicitTitles.get(sessionId) ?? {protected: false, pending: 0, previousSource: previousTitleSource}; this.explicitTitles.set(sessionId, explicitTitle); explicitTitle.pending++; + let outcome: "rejected" | "uncertain" = "rejected"; try { await this.writes.run(sessionId, async () => { // The outcome is recorded before the next title write of the queue starts, so an automatic // title that waits behind this rename sees whether it failed. try { - await this.host.runWithProcessCheck(() => this.host.client().renameSession(sessionId, title)); + context?.assertCurrent(); + const client = context?.client ?? this.host.client(); + outcome = "uncertain"; + try { + await this.host.runWithProcessCheck(() => client.renameSession(sessionId, title)); + } catch (err) { + if (err instanceof ResponseError && !client.appServerClient.connectionLoss.lost + && err.code !== ErrorCodes.PendingResponseRejected + && err.code !== ErrorCodes.ConnectionInactive + && err.code !== ErrorCodes.MessageWriteError && err.code !== ErrorCodes.MessageReadError) { + outcome = "rejected"; + } + throw err; + } } catch (err) { explicitTitle.pending--; - if (!explicitTitle.confirmed && explicitTitle.pending === 0 + if (outcome === "uncertain") explicitTitle.protected = true; + if (!explicitTitle.protected && explicitTitle.pending === 0 && this.explicitTitles.get(sessionId) === explicitTitle) { this.explicitTitles.delete(sessionId); } - if (sessionState && previousTitleSource !== undefined + if (!this.explicitTitles.has(sessionId) && sessionState && explicitTitle.previousSource !== undefined && sessionState.sessionTitleSource === "explicit") { - sessionState.sessionTitleSource = previousTitleSource; + sessionState.sessionTitleSource = explicitTitle.previousSource; } if (!this.explicitTitles.has(sessionId)) { // An automatic title that was dropped for this rename comes after the next turn. @@ -96,11 +129,17 @@ export class SessionIndexTitles { throw err; } explicitTitle.pending--; - explicitTitle.confirmed = true; + explicitTitle.protected = true; }); } catch (err) { - const client = this.host.client(); - throw await renameRequestError(client.appServerClient, sessionId, err, client.getHomePath()); + const client = context?.client ?? this.host.client(); + const cause = await renameRequestError(client.appServerClient, sessionId, err, client.getHomePath()); + throw new SessionTitleWriteError(outcome, title, cause); + } + try { + context?.assertCurrent(); + } catch (err) { + throw new SessionTitleWriteError("saved", title, err); } sessionState?.titleGen?.markExistingTitle(); const current = this.host.session(sessionId); @@ -111,23 +150,26 @@ export class SessionIndexTitles { current.sessionIndexExplicitTitle = title; // Sent here, not left to the `thread/name/updated` echo: a session gets the Codex notifications // of its thread only after its first prompt on this connection. - await new ACPSessionConnection(this.host.connection(), sessionId).update({ - sessionUpdate: "session_info_update", - title, - }); + try { + await new ACPSessionConnection(this.host.connection(), sessionId).update({ + sessionUpdate: "session_info_update", + title, + }); + } catch (err) { + throw new SessionTitleWriteError("saved", title, err); + } } return title; } /** - * Writes an automatic title in the write queue of the session, unless `_session/rename` has set or is - * setting the title. The check runs when the write gets its turn, whatever `SessionState` the generator - * belongs to. Both standard and AIR title requests suppress automatic title writes. + * Writes an automatic title unless an explicit write has set or might have set it. + * The check runs when the write gets its turn, even if its session was loaded again. */ writeAutomaticTitle(sessionId: string, title: string, write: () => Promise): Promise { return this.writes.run(sessionId, async () => { if (this.explicitTitles.has(sessionId)) return false; - // Its echo can come after a later `_session/rename`, see `isStaleAutomaticTitleEcho`. + // Its echo can come after a later explicit rename, see `isStaleAutomaticTitleEcho`. const echo = normalizeSessionTitle(title) ?? undefined; const current = this.host.session(sessionId); if (current && echo !== undefined) current.automaticTitleEcho = echo; diff --git a/src/SessionTitleTool.ts b/src/SessionTitleTool.ts new file mode 100644 index 000000000..440a60414 --- /dev/null +++ b/src/SessionTitleTool.ts @@ -0,0 +1,27 @@ +import type {DynamicToolCallResponse, DynamicToolSpec} from "./app-server/v2"; +import {normalizeSessionTitle} from "./SessionTitle"; + +export const SESSION_TITLE_TOOL: DynamicToolSpec = { + type: "function", + name: "set_thread_title", + description: "Set the current conversation's title when the user requests a rename.", + inputSchema: { + type: "object", + properties: {title: {type: "string"}}, + required: ["title"], + additionalProperties: false, + }, + deferLoading: false, +}; + +export function sessionTitleArgument(value: unknown): string | null { + if (value === null || typeof value !== "object" || Array.isArray(value)) return null; + const keys = Object.keys(value); + if (keys.length !== 1 || keys[0] !== "title") return null; + const title = (value as {title: unknown}).title; + return typeof title === "string" ? normalizeSessionTitle(title) : null; +} + +export function sessionTitleToolResult(success: boolean, text: string): DynamicToolCallResponse { + return {success, contentItems: [{type: "inputText", text}]}; +} diff --git a/src/SessionWriteQueue.ts b/src/SessionWriteQueue.ts index 5d130924e..1daee8807 100644 --- a/src/SessionWriteQueue.ts +++ b/src/SessionWriteQueue.ts @@ -1,6 +1,6 @@ /** * Runs the writes of each session one after another, each through its completion: the automatic title of - * `TitleGenerator`, and for a `sessionIndex` client `_session/rename`, `_session/archive`, + * `TitleGenerator`, explicit title writes, and for a `sessionIndex` client `_session/archive`, * `_session/unarchive` and `session/delete`. Codex may apply two requests that are in flight together in * either order, so a write starts only after the previous one has finished. */ diff --git a/src/__tests__/CodexACPAgent/data/auth-v2-login-then-new-session.json b/src/__tests__/CodexACPAgent/data/auth-v2-login-then-new-session.json index 72e5b2c21..a65511fb8 100644 --- a/src/__tests__/CodexACPAgent/data/auth-v2-login-then-new-session.json +++ b/src/__tests__/CodexACPAgent/data/auth-v2-login-then-new-session.json @@ -33,6 +33,26 @@ } } }, + "dynamicTools": [ + { + "type": "function", + "name": "set_thread_title", + "description": "Set the current conversation's title when the user requests a rename.", + "inputSchema": { + "type": "object", + "properties": { + "title": { + "type": "string" + } + }, + "required": [ + "title" + ], + "additionalProperties": false + }, + "deferLoading": false + } + ], "modelProvider": null, "cwd": "/workspace" } diff --git a/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new-mcp-servers.json b/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new-mcp-servers.json index c5a3d8134..a0828ad5f 100644 --- a/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new-mcp-servers.json +++ b/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new-mcp-servers.json @@ -34,6 +34,26 @@ } } }, + "dynamicTools": [ + { + "type": "function", + "name": "set_thread_title", + "description": "Set the current conversation's title when the user requests a rename.", + "inputSchema": { + "type": "object", + "properties": { + "title": { + "type": "string" + } + }, + "required": [ + "title" + ], + "additionalProperties": false + }, + "deferLoading": false + } + ], "modelProvider": null, "cwd": "/workspace" } diff --git a/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new.json b/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new.json index 1cab90d1d..733f49071 100644 --- a/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new.json +++ b/src/__tests__/CodexACPAgent/data/session-lifecycle-v2-new.json @@ -191,6 +191,26 @@ } } }, + "dynamicTools": [ + { + "type": "function", + "name": "set_thread_title", + "description": "Set the current conversation's title when the user requests a rename.", + "inputSchema": { + "type": "object", + "properties": { + "title": { + "type": "string" + } + }, + "required": [ + "title" + ], + "additionalProperties": false + }, + "deferLoading": false + } + ], "modelProvider": null, "cwd": "/workspace" } diff --git a/src/__tests__/CodexACPAgent/data/session-title-tool-v1.json b/src/__tests__/CodexACPAgent/data/session-title-tool-v1.json new file mode 100644 index 000000000..7d7f1070a --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/session-title-tool-v1.json @@ -0,0 +1,20 @@ +{ + "result": { + "success": true, + "contentItems": [ + { + "type": "inputText", + "text": "Session title saved as \"Native rename\"." + } + ] + }, + "updates": [ + { + "sessionId": "thread-1", + "update": { + "title": "Native rename", + "sessionUpdate": "session_info_update" + } + } + ] +} diff --git a/src/__tests__/CodexACPAgent/data/session-title-tool-v2.json b/src/__tests__/CodexACPAgent/data/session-title-tool-v2.json new file mode 100644 index 000000000..7d7f1070a --- /dev/null +++ b/src/__tests__/CodexACPAgent/data/session-title-tool-v2.json @@ -0,0 +1,20 @@ +{ + "result": { + "success": true, + "contentItems": [ + { + "type": "inputText", + "text": "Session title saved as \"Native rename\"." + } + ] + }, + "updates": [ + { + "sessionId": "thread-1", + "update": { + "title": "Native rename", + "sessionUpdate": "session_info_update" + } + } + ] +} diff --git a/src/__tests__/CodexACPAgent/provider-restart-turn-tracking.test.ts b/src/__tests__/CodexACPAgent/provider-restart-turn-tracking.test.ts index 67d35ec80..0c9b629ad 100644 --- a/src/__tests__/CodexACPAgent/provider-restart-turn-tracking.test.ts +++ b/src/__tests__/CodexACPAgent/provider-restart-turn-tracking.test.ts @@ -72,6 +72,34 @@ describe('provider restart reinstalls the baseline turn tracker', () => { expectConformingV2SessionUpdates(); }); + it.each([1, 2] as const)('v%i: rebinds title calls and rejects a queued call from the old provider', async version => { + const client = await connectSession(version); + closeClient = () => client.connection.close(); + const replacement = createReplacementCodexAcpClient(); + vi.spyOn(client.agent as any, "restartCodexClient").mockResolvedValue(replacement.codexAcpClient); + client.emit(turnStarted()); + let release!: (written: boolean) => void; + const automatic = client.agent.sessionIndex.titleWriter(sessionId)("Automatic", () => new Promise(resolve => { release = resolve; })); + await settle(); + const params = {threadId: sessionId, turnId: "turn-1", callId: "rename", namespace: null, + tool: "set_thread_title", arguments: {title: "New provider title"}}; + const oldWrite = vi.spyOn(client.appServer, "threadSetName"); + const queued = client.triggerApproval("item/tool/call", params); + await settle(); + await client.request(version === 1 ? acp.methods.agent.providers.set : acpV2.methods.agent.providers.set, setProviderParams()); + release(true); + await automatic; + await expect(queued).resolves.toMatchObject({success: false}); + client.emit(turnStarted()); + await expect(client.triggerApproval("item/tool/call", params)).resolves.toMatchObject({success: false}); + expect(oldWrite).not.toHaveBeenCalled(); + replacement.emit(turnStarted()); + const write = vi.spyOn(replacement.codexAcpClient.appServerClient, "threadSetName").mockResolvedValue({}); + await expect(replacement.triggerApproval("item/tool/call", params)).resolves.toMatchObject({success: true}); + expect(write).toHaveBeenCalledExactlyOnceWith({threadId: sessionId, name: "New provider title"}); + await settle(); + }); + it('v2: renders a Codex-started turn after providers/set, for a session never prompted', async () => { const client = await connectSession(2); closeClient = () => client.connection.close(); diff --git a/src/__tests__/CodexACPAgent/session-index.test.ts b/src/__tests__/CodexACPAgent/session-index.test.ts index b30b1e362..5233294f5 100644 --- a/src/__tests__/CodexACPAgent/session-index.test.ts +++ b/src/__tests__/CodexACPAgent/session-index.test.ts @@ -7,6 +7,7 @@ import {createCodexMockTestFixture, createTestModel, deferred, type CodexMockTes import type {Thread, ThreadListParams, ThreadListResponse} from "../../app-server/v2"; import {SESSION_LIST_CHANGES_METHOD} from "../../SessionIndex"; import {MAX_SESSION_TITLE_LENGTH} from "../../SessionTitle"; +import {ResponseError} from "vscode-jsonrpc/node"; const threadId = "01a0637c-5b99-7242-9064-04545d605fdb"; const otherThreadId = "01a0637c-5b99-7242-9064-04545d605fdc"; @@ -1035,7 +1036,7 @@ describe("_session/rename", () => { turn: {items: [{type: "agentMessage", text: JSON.stringify({title: "Automatic"})}]}, } as never); const threadSetName = vi.spyOn(appServer, "threadSetName") - .mockRejectedValueOnce(new Error("app-server busy")) + .mockRejectedValueOnce(new ResponseError(-32600, "app-server busy")) .mockResolvedValue({}); await expect(agent.sessionIndex.rename({sessionId: threadId, title: "Explicit"})).rejects.toThrow("app-server busy"); @@ -1061,7 +1062,7 @@ describe("_session/rename", () => { const rename = agent.sessionIndex.rename({sessionId: threadId, title: "Explicit"}); turn.resolve(titleTurn); await titleGen.waitForIdle(1_000); - renameWrite.reject(new Error("app-server busy")); + renameWrite.reject(new ResponseError(-32600, "app-server busy")); await expect(rename).rejects.toThrow("app-server busy"); titleGen.onTurnCompleted("Fix the build"); diff --git a/src/__tests__/CodexACPAgent/session-title-tool.test.ts b/src/__tests__/CodexACPAgent/session-title-tool.test.ts new file mode 100644 index 000000000..4a6cd9ae5 --- /dev/null +++ b/src/__tests__/CodexACPAgent/session-title-tool.test.ts @@ -0,0 +1,152 @@ +import {afterEach, describe, expect, it, vi} from "vitest"; +import {ErrorCodes, ResponseError} from "vscode-jsonrpc/node"; +import {connectSession, sessionId, turnId, turnStarted, turnCompleted, settle, type PromptSession} from "./v2-prompt-harness"; +import {deferred} from "../acp-test-utils"; +import {sessionTitleArgument, SESSION_TITLE_TOOL} from "../../SessionTitleTool"; +import type {DynamicToolCallParams, DynamicToolCallResponse} from "../../app-server/v2"; +import {CodexAppServerClient} from "../../CodexAppServerClient"; +import {FakeConnection} from "../app-server-recovery/fake-app-server"; + +const params = (overrides: Partial = {}): DynamicToolCallParams => ({ + threadId: sessionId, turnId, callId: "rename-1", namespace: null, tool: "set_thread_title", + arguments: {title: " Native\n rename "}, ...overrides, +}); + +describe("title tool arguments", () => { + it.each([null, [], {}, {title: null}, {title: 1}, {title: " \n "}, {title: "Name", threadId: "other"}])("rejects %j", value => { + expect(sessionTitleArgument(value)).toBeNull(); + }); + + it("normalizes whitespace and truncates without splitting a surrogate pair", () => { + expect(sessionTitleArgument({title: " A\n title "})).toBe("A title"); + const title = sessionTitleArgument({title: "a".repeat(254) + "😀" + "b".repeat(5)})!; + expect(title).toBe("a".repeat(254) + "…"); + }); + + it("completes calls without a registered adapter handler", async () => { + const rpc = new FakeConnection(); + new CodexAppServerClient(rpc.connection); + rpc.notify(turnStarted()); + await expect(rpc.requestHandlers.get("item/tool/call")!(params())).resolves.toMatchObject({success: false}); + }); +}); + +for (const version of [1, 2] as const) { + describe(`native title tool ACP v${version}`, () => { + let client: PromptSession; + afterEach(async () => { await settle(); client?.connection.close(); }); + const open = async () => { + client = await connectSession(version); + client.emit(turnStarted()); + return client; + }; + const call = (overrides: Partial = {}) => + client.triggerApproval("item/tool/call", params(overrides)) as Promise; + + it("registers one eager tool and persists and publishes the normalized title without AIR", async () => { + const registrations: unknown[] = []; + const {codexResponse} = await import("./v2-prompt-harness"); + client = await connectSession(version, {codexResponses: { + "thread/start": async p => { registrations.push(p); return codexResponse("thread/start"); }, + }}); + client.emit(turnStarted()); + const write = vi.spyOn(client.appServer, "threadSetName").mockResolvedValue({}); + const result = await call(); + expect(registrations).toEqual([expect.objectContaining({dynamicTools: [SESSION_TITLE_TOOL]})]); + expect(write).toHaveBeenCalledExactlyOnceWith({threadId: sessionId, name: "Native rename"}); + const updates = client.scopedUpdates.filter(x => x.update.sessionUpdate === "session_info_update" && "title" in x.update); + await expect(JSON.stringify({result, updates}, null, 2) + "\n").toMatchFileSnapshot(`data/session-title-tool-v${version}.json`); + const automatic = vi.fn(async () => true); + expect(await client.agent.sessionIndex.titleWriter(sessionId)("Automatic", automatic)).toBe(false); + expect(automatic).not.toHaveBeenCalled(); + }); + + it.each([ + {tool: "unknown"}, {namespace: "other"}, {arguments: {title: "x", threadId: "other"}}, + {threadId: "unloaded"}, {turnId: "old-turn"}, + ])("rejects invalid or unrelated requests %j", async overrides => { + await open(); + const write = vi.spyOn(client.appServer, "threadSetName"); + expect((await call(overrides)).success).toBe(false); + expect(write).not.toHaveBeenCalled(); + }); + + it("rejects a completed turn", async () => { + await open(); + client.emit(turnCompleted()); + expect((await call()).success).toBe(false); + }); + + it("rechecks the turn after waiting for an automatic write", async () => { + await open(); + const held = deferred(); + const automatic = client.agent.sessionIndex.titleWriter(sessionId)("Automatic", () => held.promise); + const write = vi.spyOn(client.appServer, "threadSetName"); + const rename = call(); + client.emit(turnCompleted()); + held.resolve(true); + await automatic; + expect((await rename).success).toBe(false); + expect(write).not.toHaveBeenCalled(); + }); + + it("rechecks session close after waiting for another write", async () => { + await open(); + const held = deferred(); + const automatic = client.agent.sessionIndex.titleWriter(sessionId)("Automatic", () => held.promise); + const write = vi.spyOn(client.appServer, "threadSetName"); + const rename = call(); + const close = client.agent.closeSession({sessionId}); + held.resolve(true); + await automatic; + expect((await rename).success).toBe(false); + await close; + expect(write).not.toHaveBeenCalled(); + }); + + it("releases automatic writes after a confirmed rejection", async () => { + await open(); + vi.spyOn(client.appServer, "threadSetName").mockRejectedValue(new ResponseError(-32600, "rejected")); + expect((await call()).contentItems).toEqual([{type: "inputText", text: "Session title was not changed: rejected"}]); + const automatic = vi.fn(async () => true); + expect(await client.agent.sessionIndex.titleWriter(sessionId)("Automatic", automatic)).toBe(true); + expect(automatic).toHaveBeenCalledOnce(); + }); + + it("restores the original title source when two queued renames are rejected", async () => { + await open(); + const source = client.agent.getSessionState(sessionId).sessionTitleSource; + vi.spyOn(client.appServer, "threadSetName").mockRejectedValue(new ResponseError(-32600, "rejected")); + const results = await Promise.all([call(), call({callId: "rename-2"})]); + expect(results.map(result => result.success)).toEqual([false, false]); + expect(client.agent.getSessionState(sessionId).sessionTitleSource).toBe(source); + }); + + it("blocks a queued automatic write when the rename saved but its acknowledgement was lost", async () => { + await open(); + let saved = "Initial"; + const ack = deferred>(); + vi.spyOn(client.appServer, "threadSetName").mockImplementation(async ({name}) => { saved = name; return ack.promise; }); + const rename = call(); + await vi.waitFor(() => expect(saved).toBe("Native rename")); + const automatic = client.agent.sessionIndex.titleWriter(sessionId)("Automatic", async () => { saved = "Automatic"; return true; }); + ack.reject(new ResponseError(ErrorCodes.PendingResponseRejected, "acknowledgement lost")); + expect((await rename).contentItems).toEqual([{type: "inputText", text: "The rename outcome is uncertain; it may have been saved. Automatic title writes are blocked. Check the saved title before retrying."}]); + expect(await automatic).toBe(false); + expect(saved).toBe("Native rename"); + expect(client.agent.getSessionState(sessionId).sessionTitleSource).toBe("explicit"); + }); + + it("reports saved persistence when the client notification fails", async () => { + await open(); + const write = vi.spyOn(client.appServer, "threadSetName").mockResolvedValue({}); + await client.waitForNotifications(); + const connection = (client.agent as unknown as {connection: {notify: (...args: unknown[]) => unknown}}).connection; + vi.spyOn(connection, "notify").mockRejectedValue(new Error("client disconnected")); + const result = await call(); + expect(write).toHaveBeenCalledOnce(); + expect(result).toEqual({success: false, contentItems: [{type: "inputText", text: 'Session title saved as "Native rename", but the session changed or the client update failed. Do not retry the rename.'}]}); + expect(await client.agent.sessionIndex.titleWriter(sessionId)("Automatic", async () => true)).toBe(false); + }); + }); +} diff --git a/src/__tests__/CodexACPAgent/v2-prompt-harness.ts b/src/__tests__/CodexACPAgent/v2-prompt-harness.ts index fa740b2ae..2f0126571 100644 --- a/src/__tests__/CodexACPAgent/v2-prompt-harness.ts +++ b/src/__tests__/CodexACPAgent/v2-prompt-harness.ts @@ -395,6 +395,7 @@ export async function connectSession(protocolVersion: 1 | 2 = 2, options: { export function createReplacementCodexAcpClient(): { codexAcpClient: CodexAcpClient; appServer: CodexAppServerClient; + triggerApproval(method: string, params: unknown): Promise; emit(notification: ServerNotification): void; turnStartParams: Array>; /** Overrides the canned `turn/start` response, e.g. to run a prompt through this replacement. */ @@ -420,6 +421,11 @@ export function createReplacementCodexAcpClient(): { return { codexAcpClient, appServer, + triggerApproval: async (method, params) => { + const handler = mocks.getRequestHandler(method); + if (!handler) throw new Error(`No handler registered for '${method}'`); + return await handler(params); + }, emit: (notification) => mocks.getUnhandledNotificationHandler()!(notification), turnStartParams, setTurnStart: (handler) => { diff --git a/src/__tests__/app-server-recovery/recovery.test.ts b/src/__tests__/app-server-recovery/recovery.test.ts index 5ba4bc6fc..ce1cae324 100644 --- a/src/__tests__/app-server-recovery/recovery.test.ts +++ b/src/__tests__/app-server-recovery/recovery.test.ts @@ -50,6 +50,25 @@ function completeTurn(fixture: RecoveryFixture, sessionId: string, turnId = "tur stubGatewayModelListing(["gpt-test"]); describe("app-server recovery", () => { + it("rebinds native title calls after recovery and rejects the retired server", async () => { + const fixture = createRecoveryFixture(); + const sessionId = await openSession(fixture); + const old = fixture.current(); + const call = {threadId: sessionId, turnId: "rename-turn", callId: "rename", namespace: null, + tool: "set_thread_title", arguments: {title: "Recovered title"}}; + const started = {method: "turn/started", params: {threadId: sessionId, + turn: {id: call.turnId, items: [], status: "inProgress", error: null}}}; + await fixture.kill(); + await fixture.agent.listSessions({}); + await internals(fixture).ensureSessionReady(fixture.agent.getSessionState(sessionId)); + fixture.current().rpc.notify(started); + await expect(fixture.current().rpc.requestHandlers.get("item/tool/call")!(call)).resolves.toMatchObject({success: true}); + old.rpc.notify(started); + await expect(old.rpc.requestHandlers.get("item/tool/call")!(call)).resolves.toMatchObject({success: false}); + expect(requestsOf(old, "thread/name/set")).toEqual([]); + expect(requestsOf(fixture.current(), "thread/name/set")).toEqual([{threadId: sessionId, name: "Recovered title"}]); + }); + it("starts the app-server again on the next request after an idle crash, with the same handshake", async () => { const fixture = createRecoveryFixture(); await openSession(fixture);