fix(server): commit option updates only after the provider accepts them, and follow ACP selections

- setAgentOptions keeps the requested options as a candidate outside the agent's record. Provider
  reports (options_changed) made while applying are folded into the candidate, and the record is
  published and persisted once, after acceptance. A rejection leaves the record, snapshots and
  stored copy as they were.
- ACP reports a select the provider changed on its own (config_option_update, or a write that
  kept another value) as options_changed when the agent holds an explicit option for it, so the
  stored option follows the provider.
This commit is contained in:
Mohamed Boudra
2026-11-01 08:53:13 +01:00
parent fd4077e24d
commit 571ae26050
4 changed files with 130 additions and 16 deletions
@@ -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<void> {
// 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<AgentSession> {
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<ProviderOptions | undefined> = [];
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[] {
@@ -781,6 +781,8 @@ export class AgentManager {
private readonly runtimeCleanupIdentities = new WeakMap<AgentSession, object>();
private readonly collectingRuntimes = new Set<string>();
private readonly lifecycleMutationTails = new Map<string, Promise<void>>();
/** Options a `setAgentOptions` call is applying, until the provider accepts or rejects them. */
private readonly pendingOptionCandidates = new Map<string, ProviderOptions>();
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);
}
@@ -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<string, string> = {};
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<void> {
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;
}
@@ -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")],