diff --git a/packages/plugin/src/server/acp-internal/connection.ts b/packages/plugin/src/server/acp-internal/connection.ts index cf7f1204e3..f853a6006a 100644 --- a/packages/plugin/src/server/acp-internal/connection.ts +++ b/packages/plugin/src/server/acp-internal/connection.ts @@ -695,15 +695,16 @@ class AcpRuntime { async configure( changes: Extract["changes"], ): Promise { - const ordered: AcpConfigChange[] = []; - if (changes.model !== undefined) ordered.push({ target: "model", value: changes.model }); - if (changes.mode !== undefined) ordered.push({ target: "mode", value: changes.mode }); + const requested: AcpConfigChange[] = []; + if (changes.model !== undefined) requested.push({ target: "model", value: changes.model }); + if (changes.mode !== undefined) requested.push({ target: "mode", value: changes.mode }); if (changes.thinkingOption !== undefined) { - ordered.push({ target: "thinking", value: changes.thinkingOption }); + requested.push({ target: "thinking", value: changes.thinkingOption }); } for (const [id, value] of Object.entries(changes.settings ?? {})) { - ordered.push({ target: "setting", id, value }); + requested.push({ target: "setting", id, value }); } + const ordered = this.inAgentOrder(requested); const previousModes = this.modes ? structuredClone(this.modes) : null; const previousOptions = structuredClone(this.configOptions); this.configTransaction = true; @@ -769,14 +770,36 @@ class AcpRuntime { } private async applyInitialConfig(config: ProviderSessionConfig): Promise { - if (config.model) await this.applyConfigChange({ target: "model", value: config.model }); - if (config.mode) await this.applyConfigChange({ target: "mode", value: config.mode }); + const requested: AcpConfigChange[] = []; + if (config.model) requested.push({ target: "model", value: config.model }); + if (config.mode) requested.push({ target: "mode", value: config.mode }); if (config.thinkingOption) { - await this.applyConfigChange({ target: "thinking", value: config.thinkingOption }); + requested.push({ target: "thinking", value: config.thinkingOption }); } for (const [id, value] of Object.entries(config.settings)) { - await this.applyConfigChange({ target: "setting", id, value }); + requested.push({ target: "setting", id, value }); } + for (const change of this.inAgentOrder(requested)) await this.applyConfigChange(change); + } + + // One option can reset another: choosing an fx provider also selects that provider's saved + // model, so a model applied first would be lost. Agents list the option that decides first. + private inAgentOrder(changes: readonly AcpConfigChange[]): AcpConfigChange[] { + const position = (change: AcpConfigChange): number => { + const configId = this.configIdFor(change); + const index = this.configOptions.findIndex((option) => option.id === configId); + return index === -1 ? this.configOptions.length : index; + }; + return changes.toSorted((left, right) => position(left) - position(right)); + } + + private configIdFor(change: AcpConfigChange): string | undefined { + if (change.target === "setting") return change.id; + if (change.target === "model") return modelOption(this.configOptions)?.id; + if (change.target === "thinking") { + return this.configOptions.find((option) => option.category === "thought_level")?.id; + } + return this.configOptions.find((option) => option.category === "mode")?.id; } private async applyConfigChange(change: AcpConfigChange): Promise { @@ -804,12 +827,7 @@ class AcpRuntime { this.modes = { ...this.modes, currentModeId: change.value }; return; } - let configId: string | undefined; - if (change.target === "setting") configId = change.id; - if (change.target === "model") configId = modelOption(this.configOptions)?.id; - if (change.target === "thinking") { - configId = this.configOptions.find((option) => option.category === "thought_level")?.id; - } + const configId = this.configIdFor(change); if (!configId) { throw new Error(`ACP session does not expose ${change.target} configuration`); } diff --git a/packages/plugin/src/server/acp.test.ts b/packages/plugin/src/server/acp.test.ts index 47bc4917ec..266d0b0109 100644 --- a/packages/plugin/src/server/acp.test.ts +++ b/packages/plugin/src/server/acp.test.ts @@ -1211,6 +1211,43 @@ lines.on("line", (line) => { await connection.close(); }); + it("applies opening configuration in the order the agent lists its options", async () => { + const executable = await fakeAcp(`const readline = require("node:readline"); +const lines = readline.createInterface({ input: process.stdin }); +const send = (message) => process.stdout.write(JSON.stringify(message) + "\\n"); +let provider = "first"; +let model = "first-saved"; +const saved = { first: "first-saved", second: "second-saved" }; +const select = (id, currentValue, values) => ({ id, name: id, category: "model", type: "select", currentValue, options: values.map((value) => ({ name: value, value })) }); +const options = () => [ + select("provider", provider, ["first", "second"]), + select("model", model, ["first-saved", "second-saved", "second-chosen"]), +]; +lines.on("line", (line) => { + const message = JSON.parse(line); + if (message.method === "initialize") send({ jsonrpc: "2.0", id: message.id, result: { protocolVersion: message.params.protocolVersion, agentCapabilities: {} } }); + else if (message.method === "session/new") send({ jsonrpc: "2.0", id: message.id, result: { sessionId: "native-1", configOptions: options() } }); + else if (message.method === "session/set_config_option") { + if (message.params.configId === "provider") { provider = message.params.value; model = saved[provider]; } + if (message.params.configId === "model") model = message.params.value; + send({ jsonrpc: "2.0", id: message.id, result: { configOptions: options() } }); + } +});`); + const { connection, events } = await connect(executable, [ + "prompt.message", + "session.configure", + ]); + const input = openInput({ provider: "second" }); + + await connection.send({ ...input, config: { ...input.config, model: "second-chosen" } }); + + await waitForEvent(events, (event) => event.type === "session.ready"); + expect(events.findLast((event) => event.type === "session.config")).toMatchObject({ + config: { model: "second-chosen" }, + }); + await connection.close(); + }); + it("gives discovery transformers the discovered session configuration facade", async () => { const executable = await fakeAcp(`const readline = require("node:readline"); const lines = readline.createInterface({ input: process.stdin }); diff --git a/packages/server/src/server/agent/agent-manager.test.ts b/packages/server/src/server/agent/agent-manager.test.ts index b876a35031..250c675739 100644 --- a/packages/server/src/server/agent/agent-manager.test.ts +++ b/packages/server/src/server/agent/agent-manager.test.ts @@ -13511,6 +13511,9 @@ describe("agent options", () => { /** What the session says it ended up with after the next apply. */ ended: Record = {}; openOptionsPatch: Record = {}; + runtimeModel: string | null = null; + /** The model the next apply moves the session to, as a provider's connection option does. */ + movesModelTo: string | null = null; async applyOptions(options: ProviderOptions): Promise> { if (this.rejectWith) throw this.rejectWith; @@ -13518,13 +13521,20 @@ describe("agent options", () => { this.applyGate = null; await gate; this.applied.push(options); + if (this.movesModelTo) this.runtimeModel = this.movesModelTo; return this.ended; } + + override async getRuntimeInfo() { + const info = await super.getRuntimeInfo(); + return this.runtimeModel ? { ...info, model: this.runtimeModel } : info; + } } class OptionsClient extends TestAgentClient { session: OptionsSession | null = null; openOptionsPatch: Record = {}; + runtimeModel: string | null = null; // Stands in for the `config.json` provider defaults the registry wrapper merges in. effectiveOptions(own: ProviderOptions | undefined): ProviderOptions { @@ -13534,6 +13544,7 @@ describe("agent options", () => { override async createSession(config: AgentSessionConfig): Promise { this.session = new OptionsSession(config); this.session.openOptionsPatch = this.openOptionsPatch; + this.session.runtimeModel = this.runtimeModel; return this.session; } } @@ -13541,14 +13552,21 @@ describe("agent options", () => { async function createOptionsAgent( providerOptions?: ProviderOptions, openOptionsPatch: Record = {}, + opened: { model?: string; runtimeModel?: string } = {}, ) { const workdir = mkdtempSync(join(tmpdir(), "agent-manager-options-")); const storage = new AgentStorage(join(workdir, "agents"), logger); const client = new OptionsClient(); client.openOptionsPatch = openOptionsPatch; + client.runtimeModel = opened.runtimeModel ?? null; const manager = new AgentManager({ clients: { codex: client }, registry: storage, logger }); const agent = await manager.createAgent( - { provider: "codex", cwd: workdir, ...(providerOptions ? { providerOptions } : {}) }, + { + provider: "codex", + cwd: workdir, + ...(opened.model ? { model: opened.model } : {}), + ...(providerOptions ? { providerOptions } : {}), + }, undefined, { workspaceId: undefined }, ); @@ -13637,6 +13655,33 @@ describe("agent options", () => { }); }); + test("an option that moves the agent to another model reopens on that model", async () => { + const { manager, client, storage, agentId } = await createOptionsAgent( + undefined, + {}, + { model: "first-connection-model" }, + ); + client.session!.movesModelTo = "second-connection-model"; + + await manager.setAgentOptions(agentId, { web_search: true }); + + expect(manager.getAgent(agentId)?.config.model).toBe("second-connection-model"); + await manager.flush(); + expect((await storage.get(agentId))?.config?.model).toBe("second-connection-model"); + }); + + test("an option that leaves the model alone keeps the model the user chose", async () => { + const { manager, agentId } = await createOptionsAgent( + undefined, + {}, + { model: "alias", runtimeModel: "canonical" }, + ); + + await manager.setAgentOptions(agentId, { web_search: true }); + + expect(manager.getAgent(agentId)?.config.model).toBe("alias"); + }); + test("a patch key is resolved against the configured defaults, merging nested objects", async () => { const workdir = mkdtempSync(join(tmpdir(), "agent-manager-options-")); const client = new OptionsClient(); diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index 880709f152..cce5966e40 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -2506,9 +2506,17 @@ export class AgentManager { const set = Object.fromEntries(Object.entries(patch).filter(([, value]) => value != null)); const resolved = this.effectiveOptionsFor(agent.provider, set); sent = Object.fromEntries(Object.keys(patch).map((key) => [key, resolved[key] ?? null])); + const modelBefore = agent.runtimeInfo?.model; ended = (await agent.session.applyOptions(sent)) ?? {}; // Changes the provider made on its own while applying are older than its final answer. await this.drainSessionEvents(agentId); + await this.refreshRuntimeInfo(agent, { emit: false }); + // An option can choose the model too, as an fx connection does. Reopening the agent on + // the earlier preference would send that model to the new connection. + const modelAfter = agent.runtimeInfo?.model; + if (agent.config.model && modelAfter && modelAfter !== modelBefore) { + agent.config.model = modelAfter; + } } this.storeOptionsPatch({ agent, patch: { ...patch, ...ended } }); this.updateEffectiveOptions({ agent, sent, reported: ended });