diff --git a/packages/server/src/server/agent/agent-manager.test.ts b/packages/server/src/server/agent/agent-manager.test.ts index ffa36de19f..09a01e2c32 100644 --- a/packages/server/src/server/agent/agent-manager.test.ts +++ b/packages/server/src/server/agent/agent-manager.test.ts @@ -12996,6 +12996,62 @@ describe("agent options", () => { }); }); + test("a rejected update that saw provider events leaves the record, snapshot and stored copy as they were", async () => { + class NoisySession extends OptionsSession { + override async applyOptions(): Promise { + // The provider reports a change and a usage update, then refuses the request. + this.pushEvent({ + type: "options_changed", + provider: this.provider, + patch: { voice: "soft" }, + }); + this.pushEvent({ + type: "usage_updated", + provider: this.provider, + usage: { inputTokens: 1 }, + }); + throw new Error("refused"); + } + } + class NoisyClient extends OptionsClient { + override async createSession(config: AgentSessionConfig): Promise { + this.session = new NoisySession(config); + return this.session; + } + } + const workdir = mkdtempSync(join(tmpdir(), "agent-manager-options-rejected-")); + const storage = new AgentStorage(join(workdir, "agents"), logger); + const manager = new AgentManager({ + clients: { codex: new NoisyClient() }, + registry: storage, + logger, + }); + const created = await manager.createAgent( + { provider: "codex", cwd: workdir, providerOptions: { voice: "direct" } }, + undefined, + { workspaceId: undefined }, + ); + const published: Array = []; + manager.subscribe( + (event) => { + if (event.type === "agent_state" && event.agent.id === created.id) { + published.push(event.agent.effectiveOptions); + } + }, + { replayState: false }, + ); + + await expect(manager.setAgentOptions(created.id, { voice: "soft" })).rejects.toThrow("refused"); + await manager.flush(); + + expect(manager.getAgent(created.id)?.config.providerOptions).toEqual({ voice: "direct" }); + expect(toAgentPayload(manager.getAgent(created.id)!).options).toMatchObject({ + voice: "direct", + }); + expect(published.every((options) => options?.voice === "direct")).toBe(true); + expect((await storage.get(created.id))?.config?.providerOptions).toEqual({ voice: "direct" }); + }); + test("a metadata-only update of a stored agent reports its saved options and config defaults", async () => { class PlanSession extends OptionsSession { get controls(): AgentOptionControl[] { diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index bf1438ea40..aa1e5a5f80 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -781,6 +781,8 @@ export class AgentManager { private readonly runtimeCleanupIdentities = new WeakMap(); private readonly collectingRuntimes = new Set(); private readonly lifecycleMutationTails = new Map>(); + /** Options a `setAgentOptions` call is applying, until the provider accepts or rejects them. */ + private readonly pendingOptionCandidates = new Map(); private readonly agentStreamCoalescer: AgentStreamCoalescer; private mcpBaseUrl: string | null; private readonly mcpAuthToken: string | null; @@ -2280,26 +2282,23 @@ export class AgentManager { // overlapping patches to different keys would otherwise overwrite one another. await this.runLifecycleMutation(agentId, async () => { const agent = this.requireAgent(agentId); - const previous = agent.config.providerOptions; - const ownOptions = applyProviderOptionsPatch(previous, patch); - const commit = (options: ProviderOptions) => { - agent.config.providerOptions = Object.keys(options).length > 0 ? options : undefined; - }; + let candidate = applyProviderOptionsPatch(agent.config.providerOptions, patch); if (agent.session.applyOptions) { - // Values the provider reports while applying (`options_changed`) land on top of the - // requested ones, so the provider's report wins. A rejection restores the record. - commit(ownOptions); + // The candidate stays out of the agent's record until the provider accepts it. Values + // the provider reports meanwhile (`options_changed`) are folded into the candidate, so + // a rejection leaves the record, its snapshots and its stored copy as they were. + this.pendingOptionCandidates.set(agentId, candidate); try { - await agent.session.applyOptions(this.effectiveOptionsFor(agent.provider, ownOptions)); + await agent.session.applyOptions(this.effectiveOptionsFor(agent.provider, candidate)); + } finally { + // Events the provider queued before answering must reach the candidate, not the record. await this.drainSessionEvents(agentId); - } catch (error) { - agent.config.providerOptions = previous; - throw error; + candidate = this.pendingOptionCandidates.get(agentId) ?? candidate; + this.pendingOptionCandidates.delete(agentId); } - } else { - commit(ownOptions); } + agent.config.providerOptions = Object.keys(candidate).length > 0 ? candidate : undefined; this.touchUpdatedAt(agent); this.emitState(agent); }); @@ -4707,9 +4706,14 @@ export class AgentManager { flags: StreamEventFlags; }): void { const { agent, patch, flags } = params; + flags.shouldDispatchEvent = false; + const candidate = this.pendingOptionCandidates.get(agent.id); + if (candidate) { + this.pendingOptionCandidates.set(agent.id, applyProviderOptionsPatch(candidate, patch)); + return; + } const ownOptions = applyProviderOptionsPatch(agent.config.providerOptions, patch); agent.config.providerOptions = Object.keys(ownOptions).length > 0 ? ownOptions : undefined; - flags.shouldDispatchEvent = false; this.emitState(agent); } diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 6e10224ac4..f13061a207 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -2407,6 +2407,27 @@ export class ACPAgentSession implements AgentSession, ACPClient { } } + /** + * When the provider reports a different value for a select the agent holds an explicit option + * for, the stored option follows the report. Selections with no stored option are shown through + * the control's default instead. + */ + private reportSelectionChanges(): AgentStreamEvent[] { + const patch: Record = {}; + for (const featureOption of this.configFeatureOptions) { + const stored = this.config.providerOptions?.[featureOption.id]; + if (typeof stored !== "string") continue; + const current = findSelectConfigFeatureOption( + this.configOptions, + featureOption, + )?.currentValue; + if (current != null && current !== stored) patch[featureOption.id] = current; + } + if (Object.keys(patch).length === 0) return []; + this.config.providerOptions = { ...this.config.providerOptions, ...patch }; + return [{ type: "options_changed", provider: this.provider, patch }]; + } + async applyOptions(options: ProviderOptions): Promise { for (const featureOption of this.configFeatureOptions) { const explicit = Object.prototype.hasOwnProperty.call(options, featureOption.id); @@ -2476,6 +2497,14 @@ export class ACPAgentSession implements AgentSession, ACPClient { label: featureOption.label, }); this.config.providerOptions = { ...this.config.providerOptions, [featureId]: currentValue }; + if (currentValue !== requestedValue) { + // The provider kept a different selection than the one asked for. + this.pushEvent({ + type: "options_changed", + provider: this.provider, + patch: { [featureId]: currentValue }, + }); + } } private applyConfigOptionResponse({ @@ -3317,6 +3346,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { thinkingOptionId: this.thinkingOptionId, }); } + events.push(...this.reportSelectionChanges()); return events; } diff --git a/packages/server/src/server/agent/providers/cursor-acp-agent.test.ts b/packages/server/src/server/agent/providers/cursor-acp-agent.test.ts index c6c982b4b4..d149960394 100644 --- a/packages/server/src/server/agent/providers/cursor-acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/cursor-acp-agent.test.ts @@ -6,7 +6,7 @@ import { describe, expect, test, vi } from "vitest"; import { ACPAgentSession } from "./acp-agent.js"; import type { SpawnedACPProcess, SessionStateResponse } from "./acp-agent.js"; -import type { AgentSessionConfig } from "../agent-sdk-types.js"; +import type { AgentSessionConfig, AgentStreamEvent } from "../agent-sdk-types.js"; import { CURSOR_FAST_FEATURE_OPTION, CursorACPAgentClient } from "./cursor-acp-agent.js"; import { GenericACPAgentClient } from "./generic-acp-agent.js"; import { createTestLogger } from "../../../test-utils/test-logger.js"; @@ -373,6 +373,30 @@ describe("CursorACPAgentClient session start", () => { await vi.waitFor(() => expect(session.getPendingPermissions()).toHaveLength(1)); }); + test("a selection the provider changes on its own replaces the stored option", async () => { + const session = createCursorSession( + { model: "composer-2.5", providerOptions: { [CURSOR_FAST_FEATURE_OPTION.id]: "true" } }, + { + currentModelId: "composer-2.5", + configOptions: [fastConfigOption("true")], + }, + ); + await session.initializeNewSession(); + const events: AgentStreamEvent[] = []; + session.subscribe((event) => events.push(event)); + + await session.sessionUpdate({ + sessionId: "session-1", + update: { sessionUpdate: "config_option_update", configOptions: [fastConfigOption("false")] }, + }); + + expect(events).toContainEqual({ + type: "options_changed", + provider: "acp", + patch: { [CURSOR_FAST_FEATURE_OPTION.id]: "false" }, + }); + }); + test("removing the Fast option returns the session to the value it started with", async () => { const setSessionConfigOption = vi.fn(async (request: { value: string }) => ({ configOptions: [fastConfigOption(request.value === "true" ? "true" : "false")],