diff --git a/src/AcpExtensions.ts b/src/AcpExtensions.ts index a469d515..3eeec4f0 100644 --- a/src/AcpExtensions.ts +++ b/src/AcpExtensions.ts @@ -50,6 +50,7 @@ export type LegacyNewSessionResponse = NewSessionResponse & { } export type LegacyLoadSessionResponse = LoadSessionResponse & { + sessionId: SessionId; models?: LegacySessionModelState | null; } diff --git a/src/CodexAcpClient.ts b/src/CodexAcpClient.ts index 8d78cae0..866e5871 100644 --- a/src/CodexAcpClient.ts +++ b/src/CodexAcpClient.ts @@ -477,7 +477,7 @@ export class CodexAcpClient { const response = await this.codexClient.threadResume({ config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), cwd: request.cwd, - modelProvider: await this.getResumeModelProvider(), + ...(await this.getResumeModelProviderParams()), threadId: request.sessionId, }); onSubscribed?.(); @@ -501,7 +501,7 @@ export class CodexAcpClient { refreshSkills: (cwd, directories) => this.refreshSkills(cwd, directories), createSessionConfig: (cwd, directories, mcpServers) => this.createSessionConfig(cwd, directories, mcpServers), - getResumeModelProvider: () => this.getResumeModelProvider(), + getResumeModelProviderParams: () => this.getResumeModelProviderParams(), fetchAvailableModels: () => this.fetchAvailableModels(), createCurrentModelId: (models, model, reasoningEffort) => this.createModelId(models, model, reasoningEffort).toString(), @@ -516,7 +516,7 @@ export class CodexAcpClient { const response = await this.codexClient.threadResume({ config: await this.createSessionConfig(request.cwd, additionalDirectories, request.mcpServers ?? []), cwd: request.cwd, - modelProvider: await this.getResumeModelProvider(), + ...(await this.getResumeModelProviderParams()), threadId: request.sessionId, }); onSubscribed?.(); @@ -735,10 +735,17 @@ export class CodexAcpClient { return this.gatewayConfig?.modelProvider ?? this.modelProvider; } - private async getResumeModelProvider(): Promise { - // Prefer an explicit/gateway provider, then the provider persisted in Codex config. - // Keep OpenAI as the final fallback for ChatGPT-authenticated sessions without a configured provider. - return (await this.getCurrentModelProvider()) ?? "openai"; + /** + * Resume-time provider override, as `thread/resume` params. + * + * Prefer an explicit/gateway provider, then the provider persisted in Codex config. + * When neither is configured the field is omitted entirely: supplying one makes the + * app-server re-resolve the thread's model and reasoning effort from config, which + * discards the picks stored on the thread itself. + */ + async getResumeModelProviderParams(): Promise<{modelProvider?: string}> { + const modelProvider = await this.getCurrentModelProvider(); + return modelProvider ? {modelProvider} : {}; } private async refreshSkills( @@ -1020,6 +1027,15 @@ export class CodexAcpClient { }); } + async setModelAndEffort(sessionId: string, currentModelId: string): Promise { + const modelId = ModelId.fromString(currentModelId); + await this.codexClient.threadSettingsUpdate({ + threadId: sessionId, + model: modelId.model, + effort: modelId.effort as ReasoningEffort, + }); + } + private getCollaborationMode(sessionId: string): ModeKind { return this.codexClient.getThreadSettings(sessionId)?.collaborationMode.mode ?? "default"; } diff --git a/src/CodexAcpServer.ts b/src/CodexAcpServer.ts index 3828e7c4..a29db9ee 100644 --- a/src/CodexAcpServer.ts +++ b/src/CodexAcpServer.ts @@ -729,6 +729,7 @@ export class CodexAcpServer { availableModelCount: modelState.availableModels.length }); return { + sessionId, models: modelState, modes: modeState, ...this.createSessionConfigOptionsResponse(this.getSessionState(sessionId)), @@ -1103,10 +1104,10 @@ export class CodexAcpServer { await this.applyCollaborationModeChange(sessionState, this.stringConfigValue(params)); break; case MODEL_CONFIG_ID: - this.applyModelChange(sessionState, this.stringConfigValue(params)); + await this.applyModelChange(sessionState, this.stringConfigValue(params)); break; case REASONING_EFFORT_CONFIG_ID: - this.applyReasoningEffortChange(sessionState, this.stringConfigValue(params)); + await this.applyReasoningEffortChange(sessionState, this.stringConfigValue(params)); break; default: throw RequestError.invalidParams(); @@ -1149,7 +1150,7 @@ export class CodexAcpServer { sessionState.collaborationMode = mode; } - private applyModelChange(sessionState: SessionState, value: string): void { + private async applyModelChange(sessionState: SessionState, value: string): Promise { const model = sessionState.availableModels.find(m => m.id === value); if (!model) { const currentModel = ModelId.fromString(sessionState.currentModelId).model; @@ -1161,16 +1162,22 @@ export class CodexAcpServer { const currentEffort = ModelId.fromString(sessionState.currentModelId).effort; const effort = findSupportedEffort(model.supportedReasoningEfforts, currentEffort) ?? model.defaultReasoningEffort; + await this.codexAcpClient.setModelAndEffort( + sessionState.sessionId, + ModelId.fromComponents(model, effort).toString(), + ); this.applyModelAndEffort(sessionState, model, effort); } - private applyReasoningEffortChange(sessionState: SessionState, value: string): void { + private async applyReasoningEffortChange(sessionState: SessionState, value: string): Promise { const effort = findSupportedEffort(sessionState.supportedReasoningEfforts, value); if (!effort) { throw RequestError.invalidParams(); } const {model} = ModelId.fromString(sessionState.currentModelId); - sessionState.currentModelId = ModelId.create(model, effort).toString(); + const currentModelId = ModelId.create(model, effort).toString(); + await this.codexAcpClient.setModelAndEffort(sessionState.sessionId, currentModelId); + sessionState.currentModelId = currentModelId; } private applyModelAndEffort(sessionState: SessionState, model: Model, effort: ReasoningEffort): void { @@ -1206,6 +1213,10 @@ export class CodexAcpServer { } sessionState.availableModels = models; + await this.codexAcpClient.setModelAndEffort( + sessionState.sessionId, + ModelId.fromComponents(model, reasoningEffort).toString(), + ); this.applyModelAndEffort(sessionState, model, reasoningEffort); return {}; diff --git a/src/CodexAppServerClient.ts b/src/CodexAppServerClient.ts index dac0bfc5..4f8a9cf9 100644 --- a/src/CodexAppServerClient.ts +++ b/src/CodexAppServerClient.ts @@ -3,6 +3,7 @@ import type { ClientRequest, InitializeParams, InitializeResponse, + ReasoningEffort, ServerNotification } from "./app-server"; import type { @@ -546,7 +547,7 @@ export class CodexAppServerClient { return this.threadSettings.get(threadId); } - async threadSettingsUpdate(params: ExperimentalThreadSettingsUpdateParams): Promise { + async threadSettingsUpdate(params: ThreadSettingsUpdateParams): Promise { await this.connection.sendRequest("thread/settings/update", params); } @@ -992,9 +993,11 @@ type DistributiveOmit = T extends any ? Omit : never; -export interface ExperimentalThreadSettingsUpdateParams { +export interface ThreadSettingsUpdateParams { threadId: string; - collaborationMode: { + model?: string; + effort?: ReasoningEffort; + collaborationMode?: { mode: "default" | "plan"; settings: { model: string; diff --git a/src/SessionFork.ts b/src/SessionFork.ts index b52b7cae..e4b810ee 100644 --- a/src/SessionFork.ts +++ b/src/SessionFork.ts @@ -15,7 +15,7 @@ export type SessionForkDependencies = { additionalDirectories: string[], mcpServers: acp.McpServer[], ): Promise>; - getResumeModelProvider(): Promise; + getResumeModelProviderParams(): Promise<{modelProvider?: string}>; fetchAvailableModels(): Promise; createCurrentModelId(models: Model[], model: string, reasoningEffort: string | null): string; getCollaborationMode(sessionId: string): ModeKind; @@ -36,7 +36,7 @@ export async function forkSession( ), cwd: request.cwd, ...(lastTurnId !== undefined && {lastTurnId}), - modelProvider: await dependencies.getResumeModelProvider(), + ...(await dependencies.getResumeModelProviderParams()), threadId: request.sessionId, }); await dependencies.codexClient.threadUnsubscribe({threadId: response.thread.id}); diff --git a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts index ced1752a..67d6232f 100644 --- a/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts +++ b/src/__tests__/CodexACPAgent/CodexAcpClient.test.ts @@ -594,6 +594,7 @@ describe('ACP server test', { timeout: 40_000 }, () => { expect(forked.sessionId).toBe("fork-id"); expect(forked.additionalDirectories).toEqual(["/workspace/extra"]); + expect(threadForkSpy.mock.calls[0]![0]).not.toHaveProperty("modelProvider"); expect(threadForkSpy).toHaveBeenCalledWith(expect.objectContaining({ threadId: "source-id", cwd: "/workspace", @@ -791,6 +792,47 @@ describe('ACP server test', { timeout: 40_000 }, () => { expect(threadResumeSpy.mock.calls[1]![0].modelProvider).toBe("azure"); }); + it('omits the model provider when none is configured so the thread keeps its model and effort', async () => { + const mockFixture = createCodexMockTestFixture(); + const codexAcpClient = mockFixture.getCodexAcpClient(); + const codexAppServerClient = mockFixture.getCodexAppServerClient(); + + vi.spyOn(codexAcpClient, "getModelProvider").mockReturnValue(null); + vi.spyOn(codexAppServerClient, "skillsExtraRootsSet").mockResolvedValue(undefined); + vi.spyOn(codexAppServerClient, "listSkills").mockResolvedValue({data: []}); + vi.spyOn(codexAppServerClient, "configRead").mockResolvedValue({config: {}} as any); + const threadResumeSpy = vi.spyOn(codexAppServerClient, "threadResume").mockResolvedValue({ + thread: {id: "thread-id"} as any, + model: "gpt-5", + reasoningEffort: "high", + serviceTier: null, + } as any); + vi.spyOn(codexAppServerClient, "threadRead").mockResolvedValue({ + thread: {id: "thread-id"} as any, + }); + vi.spyOn(codexAppServerClient, "listModels").mockResolvedValue({ + data: [createTestModel({id: "gpt-5", defaultReasoningEffort: "medium"})], + nextCursor: null, + }); + + const resumed = await codexAcpClient.resumeSession({ + sessionId: "resume-id", + cwd: "/workspace", + }); + const loaded = await codexAcpClient.loadSession({ + sessionId: "load-id", + cwd: "/workspace", + mcpServers: [], + }); + + // Supplying a provider makes the app-server re-resolve model/effort from config, + // discarding the picks stored on the thread (issue #343). + expect(threadResumeSpy.mock.calls[0]![0]).not.toHaveProperty("modelProvider"); + expect(threadResumeSpy.mock.calls[1]![0]).not.toHaveProperty("modelProvider"); + expect(resumed.currentModelId).toBe("gpt-5[high]"); + expect(loaded.currentModelId).toBe("gpt-5[high]"); + }); + it('tracks configured model provider auth state for resumed and loaded sessions', async () => { const mockFixture = createCodexMockTestFixture(); const codexAcpAgent = mockFixture.getCodexAcpAgent(); diff --git a/src/__tests__/CodexACPAgent/e2e/acp-e2e-session-persistence.test.ts b/src/__tests__/CodexACPAgent/e2e/acp-e2e-session-persistence.test.ts index 95ada735..c29e6b7c 100644 --- a/src/__tests__/CodexACPAgent/e2e/acp-e2e-session-persistence.test.ts +++ b/src/__tests__/CodexACPAgent/e2e/acp-e2e-session-persistence.test.ts @@ -18,6 +18,30 @@ describeE2E("E2E session persistence tests", () => { beforeRestartFixture = null; }); + it("persists the selected model and effort across an ACP process restart", async () => { + beforeRestartFixture = await createAuthenticatedFixture(); + const sessionId = (await beforeRestartFixture.createSession()).sessionId; + + await beforeRestartFixture.expectPromptText( + sessionId, + "Reply with exactly materialized-ok and nothing else.", + (text) => expect(text.toLowerCase()).toContain("materialized-ok"), + ); + await legacySetSessionModel(beforeRestartFixture.connection, { + sessionId, + modelId: OTHER_TEST_MODEL_ID.toString(), + }); + + afterRestartFixture = await beforeRestartFixture.restart(); + const loadSessionResponse = await afterRestartFixture.connection.loadSession({ + sessionId, + cwd: afterRestartFixture.workspaceDir, + mcpServers: [], + }) as LegacyLoadSessionResponse; + + expect(loadSessionResponse.models?.currentModelId).toBe(OTHER_TEST_MODEL_ID.toString()); + }); + // Temporarily disabled as flaky it.skip("persists a session across ACP process restart", async () => { beforeRestartFixture = await createAuthenticatedFixture(); diff --git a/src/__tests__/CodexACPAgent/fast-mode-config.test.ts b/src/__tests__/CodexACPAgent/fast-mode-config.test.ts index 6007d529..11fd8ced 100644 --- a/src/__tests__/CodexACPAgent/fast-mode-config.test.ts +++ b/src/__tests__/CodexACPAgent/fast-mode-config.test.ts @@ -47,6 +47,7 @@ describe("Fast mode session config", () => { currentServiceTier, additionalDirectories: [], }); + vi.spyOn((codexAcpClient as any).codexClient, "threadSettingsUpdate").mockResolvedValue(undefined); await codexAcpAgent.initialize({ protocolVersion: acp.PROTOCOL_VERSION, diff --git a/src/__tests__/CodexACPAgent/load-session.test.ts b/src/__tests__/CodexACPAgent/load-session.test.ts index 04730017..c4335b62 100644 --- a/src/__tests__/CodexACPAgent/load-session.test.ts +++ b/src/__tests__/CodexACPAgent/load-session.test.ts @@ -370,8 +370,9 @@ describe("CodexACPAgent - loadSession", () => { cwd: "/test/project", mcpServers: [], }; - await codexAcpAgent.loadSession(loadParams); + const response = await codexAcpAgent.loadSession(loadParams); + expect(response.sessionId).toBe(thread.id); expect(codexAppServerClient.threadRead).toHaveBeenCalledWith({ threadId: thread.id, includeTurns: true, diff --git a/src/__tests__/CodexACPAgent/session-config-options.test.ts b/src/__tests__/CodexACPAgent/session-config-options.test.ts index e895d282..a2e393e2 100644 --- a/src/__tests__/CodexACPAgent/session-config-options.test.ts +++ b/src/__tests__/CodexACPAgent/session-config-options.test.ts @@ -1,5 +1,8 @@ import {describe, expect, it, vi} from "vitest"; -import {createCodexMockTestFixture, createTestModel} from "../acp-test-utils"; +import { + createCodexMockTestFixture, + createTestModel, +} from "../acp-test-utils"; import {AgentMode, MODE_CONFIG_ID} from "../../AgentMode"; import { MODEL_CONFIG_ID, @@ -49,9 +52,11 @@ async function createSession(currentModelId: string, availableModels: Array { @@ -173,8 +178,7 @@ describe("Session config options", () => { it("changes collaboration mode without starting a model turn", async () => { const {fast} = buildModels(); - const {codexAcpAgent, codexAcpClient} = await createSession("fast-model[medium]", [fast]); - const update = vi.spyOn((codexAcpClient as any).codexClient, "threadSettingsUpdate").mockResolvedValue(undefined); + const {codexAcpAgent, update} = await createSession("fast-model[medium]", [fast]); const result = await codexAcpAgent.setSessionConfigOption({ sessionId: "session-id", @@ -192,8 +196,7 @@ describe("Session config options", () => { it("toggles collaboration mode with /plan without starting a model turn", async () => { const {fast} = buildModels(); - const {fixture, codexAcpAgent, codexAcpClient} = await createSession("fast-model[medium]", [fast]); - const update = vi.spyOn((codexAcpClient as any).codexClient, "threadSettingsUpdate").mockResolvedValue(undefined); + const {fixture, codexAcpAgent, update} = await createSession("fast-model[medium]", [fast]); const turnStart = vi.spyOn(fixture.getCodexAppServerClient(), "turnStart"); const enabledResponse = await codexAcpAgent.prompt({ @@ -247,7 +250,7 @@ describe("Session config options", () => { it("changes the model and keeps the current reasoning effort when supported", async () => { const {fast, slow} = buildModels(); - const {codexAcpAgent} = await createSession("fast-model[medium]", [fast, slow]); + const {codexAcpAgent, update} = await createSession("fast-model[medium]", [fast, slow]); await codexAcpAgent.setSessionConfigOption({ sessionId: "session-id", @@ -256,6 +259,11 @@ describe("Session config options", () => { }); expect(codexAcpAgent.getSessionState("session-id").currentModelId).toBe("slow-model[medium]"); + expect(update).toHaveBeenCalledWith({ + threadId: "session-id", + model: "slow-model", + effort: "medium", + }); }); it("falls back to the new model's default effort when the current effort is unsupported", async () => { @@ -273,7 +281,7 @@ describe("Session config options", () => { it("changes only the reasoning effort", async () => { const {fast} = buildModels(); - const {codexAcpAgent} = await createSession("fast-model[medium]", [fast]); + const {codexAcpAgent, update} = await createSession("fast-model[medium]", [fast]); await codexAcpAgent.setSessionConfigOption({ sessionId: "session-id", @@ -282,6 +290,11 @@ describe("Session config options", () => { }); expect(codexAcpAgent.getSessionState("session-id").currentModelId).toBe("fast-model[high]"); + expect(update).toHaveBeenCalledWith({ + threadId: "session-id", + model: "fast-model", + effort: "high", + }); }); it("refreshes the cached model list when unstable_setSessionModel picks a freshly fetched model", async () => { @@ -309,6 +322,7 @@ describe("Session config options", () => { defaultReasoningEffort: "medium", }); vi.spyOn(codexAcpClient, "fetchAvailableModels").mockResolvedValue([fast, extraModel]); + vi.spyOn((codexAcpClient as any).codexClient, "threadSettingsUpdate").mockResolvedValue(undefined); await codexAcpAgent.unstable_setSessionModel({ sessionId: "session-id",