From fd4077e24d2abfc0810f0ad946f4636408db5c2f Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sun, 1 Nov 2026 08:44:02 +0100 Subject: [PATCH] fix(server): serialize option patches, keep failed updates from changing state, report stored options - setAgentOptions runs in the agent's lifecycle lane, so overlapping patches to different keys no longer overwrite each other. Options the provider reports while applying land on top of the requested ones; a rejected update restores the previous record. - ACP applies auto_accept after the select writes, so a rejected bundle leaves permission handling as it was. - A plugin provider's own setting changes are reported as options_changed, so the stored option follows what the plugin reports. - Stored-only agent updates carry the effective options, so metadata-only updates keep reporting saved values and config defaults. --- .../src/server/agent/agent-manager.test.ts | 87 +++++++++++++++++++ .../server/src/server/agent/agent-manager.ts | 39 +++++++-- .../src/server/agent/plugin-provider.test.ts | 59 ++++++++++++- .../src/server/agent/plugin-provider.ts | 24 +++++ .../src/server/agent/providers/acp-agent.ts | 9 +- .../agent/providers/cursor-acp-agent.test.ts | 26 ++++++ 6 files changed, 230 insertions(+), 14 deletions(-) diff --git a/packages/server/src/server/agent/agent-manager.test.ts b/packages/server/src/server/agent/agent-manager.test.ts index c6442ca1d3..ffa36de19f 100644 --- a/packages/server/src/server/agent/agent-manager.test.ts +++ b/packages/server/src/server/agent/agent-manager.test.ts @@ -12951,4 +12951,91 @@ describe("agent options", () => { plan_mode: false, }); }); + + test("overlapping patches to different keys both reach the provider and the stored record", async () => { + let release: () => void = () => {}; + const firstApplyHeld = new Promise((resolve) => { + release = resolve; + }); + class HeldSession extends OptionsSession { + override async applyOptions(options: ProviderOptions): Promise { + const first = this.applied.length === 0; + this.applied.push(options); + if (first) await firstApplyHeld; + } + } + class HeldClient extends OptionsClient { + override async createSession(config: AgentSessionConfig): Promise { + const session = new HeldSession(config); + this.session = session; + return session; + } + } + const workdir = mkdtempSync(join(tmpdir(), "agent-manager-options-overlap-")); + const storage = new AgentStorage(join(workdir, "agents"), logger); + const client = new HeldClient(); + const manager = new AgentManager({ clients: { codex: client }, registry: storage, logger }); + const created = await manager.createAgent({ provider: "codex", cwd: workdir }, undefined, { + workspaceId: undefined, + }); + + const speed = manager.setAgentOptions(created.id, { service_tier: "fast" }); + const plan = manager.setAgentOptions(created.id, { plan_mode: true }); + release(); + await Promise.all([speed, plan]); + + expect(client.session?.applied.at(-1)).toEqual({ service_tier: "fast", plan_mode: true }); + expect(manager.getAgent(created.id)?.config.providerOptions).toEqual({ + service_tier: "fast", + plan_mode: true, + }); + await manager.flush(); + expect((await storage.get(created.id))?.config?.providerOptions).toEqual({ + service_tier: "fast", + plan_mode: true, + }); + }); + + test("a metadata-only update of a stored agent reports its saved options and config defaults", async () => { + class PlanSession extends OptionsSession { + get controls(): AgentOptionControl[] { + return [createControl({ key: "plan_mode", label: "Plan" })]; + } + } + class PlanClient extends OptionsClient { + override async createSession(config: AgentSessionConfig): Promise { + return new PlanSession(config); + } + } + const workdir = mkdtempSync(join(tmpdir(), "agent-manager-options-stored-")); + const storage = new AgentStorage(join(workdir, "agents"), logger); + const manager = new AgentManager({ + clients: { codex: new PlanClient() }, + registry: storage, + logger, + }); + const created = await manager.createAgent( + { provider: "codex", cwd: workdir, providerOptions: { plan_mode: true } }, + undefined, + { workspaceId: "workspace-1" }, + ); + await manager.closeAgent(created.id); + + const updates: ManagedAgent[] = []; + manager.subscribe( + (event) => { + if (event.type === "agent_state" && event.agent.id === created.id) + updates.push(event.agent); + }, + { replayState: false }, + ); + await manager.markAgentUnread(created.id); + + expect(updates).toHaveLength(1); + const payload = toAgentPayload(updates[0]!); + expect(payload.options).toEqual({ service_tier: "default", plan_mode: true }); + expect(payload.features?.find((feature) => feature.id === "plan_mode")).toMatchObject({ + value: true, + }); + }); }); diff --git a/packages/server/src/server/agent/agent-manager.ts b/packages/server/src/server/agent/agent-manager.ts index 48e8be0b65..bf1438ea40 100644 --- a/packages/server/src/server/agent/agent-manager.ts +++ b/packages/server/src/server/agent/agent-manager.ts @@ -2174,6 +2174,10 @@ export class AgentManager { updatedAt, availableModes: [], controls: record.controls, + effectiveOptions: this.effectiveOptionsFor( + record.provider, + record.config?.providerOptions ?? undefined, + ), currentModeId: record.lastModeId ?? null, pendingPermissions: new Map(), bufferedPermissionResolutions: new Map(), @@ -2272,16 +2276,33 @@ export class AgentManager { * value leaves the stored options unchanged. */ async setAgentOptions(agentId: string, patch: ProviderOptionsPatch): Promise { - const agent = this.requireAgent(agentId); - const ownOptions = applyProviderOptionsPatch(agent.config.providerOptions, patch); + // One patch at a time per agent: each derives its options from the committed record, so + // 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; + }; - if (agent.session.applyOptions) { - await agent.session.applyOptions(this.effectiveOptionsFor(agent.provider, ownOptions)); - await this.drainSessionEvents(agentId); - } - agent.config.providerOptions = Object.keys(ownOptions).length > 0 ? ownOptions : undefined; - this.touchUpdatedAt(agent); - this.emitState(agent); + 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); + try { + await agent.session.applyOptions(this.effectiveOptionsFor(agent.provider, ownOptions)); + await this.drainSessionEvents(agentId); + } catch (error) { + agent.config.providerOptions = previous; + throw error; + } + } else { + commit(ownOptions); + } + this.touchUpdatedAt(agent); + this.emitState(agent); + }); } private effectiveOptionsFor( diff --git a/packages/server/src/server/agent/plugin-provider.test.ts b/packages/server/src/server/agent/plugin-provider.test.ts index 1d474b513a..01efca69f4 100644 --- a/packages/server/src/server/agent/plugin-provider.test.ts +++ b/packages/server/src/server/agent/plugin-provider.test.ts @@ -17,7 +17,7 @@ import { setImmediate as nextTurn } from "node:timers/promises"; import { describe, expect, test } from "vitest"; import { createTestLogger } from "../../test-utils/test-logger.js"; import type { AgentClient, AgentStreamEvent } from "./agent-sdk-types.js"; -import { toStoredAgentRecord } from "./agent-projections.js"; +import { toAgentPayload, toStoredAgentRecord } from "./agent-projections.js"; import { AgentManager } from "./agent-manager.js"; import { buildProviderRegistry } from "./provider-registry.js"; import { ProviderOverrideSchema } from "@getpaseo/protocol/provider-config"; @@ -1489,6 +1489,63 @@ describe("PluginAgentClientRegistry", () => { } }); + test("a setting the plugin changes on its own replaces an option the user had selected", async () => { + const harness = createProviderHarness(); + const logger = createTestLogger(); + const registry = new PluginAgentClientRegistry(logger); + registry.replace([harness.registration]); + const manager = new AgentManager({ + logger, + clients: registry.clients(), + providerDefinitions: registry.definitions(), + }); + try { + const agent = await manager.createAgent( + { provider: harness.registration.id, cwd: tmpdir(), providerOptions: { voice: "direct" } }, + undefined, + { workspaceId: undefined }, + ); + const opened = harness.inputs.find((input) => input.type === "session.open"); + if (opened?.type !== "session.open") throw new Error("session was not opened"); + + // The plugin reports a new voice after, say, a mode change. + harness.emit({ + type: "session.config", + sessionId: opened.sessionId, + config: { + model: "plugin-model", + mode: "build", + models: [{ id: "plugin-model", label: "Plugin model" }], + modes: [{ id: "build", label: "Build" }], + thinkingOptions: [], + settings: [ + { + type: "select", + id: "voice", + label: "Voice", + value: "soft", + options: [ + { label: "Direct", value: "direct" }, + { label: "Soft", value: "soft" }, + ], + }, + ], + }, + }); + + await expect + .poll(() => manager.getAgent(agent.id)?.config.providerOptions) + .toEqual({ voice: "soft" }); + const payload = toAgentPayload(manager.getAgent(agent.id)!); + expect(payload.options).toEqual({ voice: "soft" }); + expect(payload.features?.find((feature) => feature.id === "voice")).toMatchObject({ + value: "soft", + }); + } finally { + await registry.shutdown(); + } + }); + test("adapts callback providers into the existing AgentClient and AgentSession path", async () => { const harness = createProviderHarness(); const registry = new PluginAgentClientRegistry(createTestLogger()); diff --git a/packages/server/src/server/agent/plugin-provider.ts b/packages/server/src/server/agent/plugin-provider.ts index a4db0f2ed6..0be667b78f 100644 --- a/packages/server/src/server/agent/plugin-provider.ts +++ b/packages/server/src/server/agent/plugin-provider.ts @@ -16,6 +16,7 @@ import { type ProviderCapability, type ProviderConfigChanges, type ProviderConfigState, + type ProviderSetting, type ProviderConnection, type ProviderError, type ProviderEvent, @@ -1211,6 +1212,8 @@ class PluginAgentSession implements AgentSession { closed: boolean; } | null = null; private replaying: Promise | null = null; + /** The setting values the session last reported, to tell which ones a config event changed. */ + private reportedSettings = new Map(); constructor( readonly provider: string, @@ -1223,6 +1226,7 @@ class PluginAgentSession implements AgentSession { private readonly onBind: (bridge: ProviderRuntimeSession) => void, ) { this.subagentIdsBySession.set(bridge.id, null); + this.reportedSettings = settingValues(bridge.config.settings); for (const event of bridge.history) this.accept(event, false); // Opening notices arrive before the manager subscribes to the session. // Commit them on registration and replay the same timestamps from history. @@ -1509,6 +1513,21 @@ class PluginAgentSession implements AgentSession { this.history.push(next); if (live) this.emit(next); } + if (event.type !== "session.config") return; + // A setting the plugin changed on its own is an option change, so the stored option + // follows what the plugin reports. + const patch = this.changedSettings(event.config.settings); + if (live && patch) this.emit({ type: "options_changed", provider: this.provider, patch }); + } + + private changedSettings(settings: readonly ProviderSetting[]): Record | null { + const next = settingValues(settings); + const changed: Record = {}; + for (const [id, value] of next) { + if (this.reportedSettings.get(id) !== value) changed[id] = value; + } + this.reportedSettings = next; + return Object.keys(changed).length > 0 ? changed : null; } private translate(event: ProviderEvent): AgentStreamEvent[] { @@ -1775,6 +1794,7 @@ class PluginAgentSession implements AgentSession { const bridge = await this.reopen(replay.persistence, replay.config); this.bridge = bridge; this.subagentIdsBySession.set(bridge.id, null); + this.reportedSettings = settingValues(bridge.config.settings); for (const event of bridge.history) this.accept(event, false); this.unsubscribe = bridge.onEvent((event) => this.accept(event, true)); this.onBind(bridge); @@ -2018,6 +2038,10 @@ function toJsonObject(value: unknown, label: string): Record return encoded as Record; } +function settingValues(settings: readonly ProviderSetting[]): Map { + return new Map(settings.map((setting) => [setting.id, setting.value])); +} + function toJsonValue(value: unknown, label: string): JsonValue { const encoded = JSON.stringify(value); if (encoded === undefined) throw new Error(`${label} must be JSON-serializable`); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 8e3cba69e2..6e10224ac4 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -2408,10 +2408,6 @@ export class ACPAgentSession implements AgentSession, ACPClient { } async applyOptions(options: ProviderOptions): Promise { - await this.setConfigFeature( - ACP_AUTO_ACCEPT_FEATURE_ID, - options[ACP_AUTO_ACCEPT_FEATURE_ID] === true, - ); for (const featureOption of this.configFeatureOptions) { const explicit = Object.prototype.hasOwnProperty.call(options, featureOption.id); const value = explicit @@ -2426,6 +2422,11 @@ export class ACPAgentSession implements AgentSession, ACPClient { } await this.setConfigFeature(featureOption.id, value); } + // Last, so a rejected selection leaves permission handling as it was. + await this.setConfigFeature( + ACP_AUTO_ACCEPT_FEATURE_ID, + options[ACP_AUTO_ACCEPT_FEATURE_ID] === true, + ); } private async setConfigFeature(featureId: string, value: unknown): Promise { 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 76ddd3dcb3..c6c982b4b4 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 @@ -347,6 +347,32 @@ describe("CursorACPAgentClient session start", () => { await vi.waitFor(() => expect(session.getPendingPermissions()).toHaveLength(1)); }); + test("a rejected options update leaves permission handling as it was", async () => { + const session = createCursorSession( + { model: "composer-2.5" }, + { + currentModelId: "composer-2.5", + configOptions: [fastConfigOption("false")], + setSessionConfigOption: vi.fn(async () => ({ configOptions: [fastConfigOption("false")] })), + }, + ); + await session.initializeNewSession(); + + await expect( + session.applyOptions({ + auto_accept: true, + [CURSOR_FAST_FEATURE_OPTION.id]: "no-such-choice", + }), + ).rejects.toThrow(); + + void session.requestPermission({ + sessionId: "session-1", + toolCall: { toolCallId: "tool-1", title: "Run command", kind: "execute", status: "pending" }, + options: [{ optionId: "allow-always", name: "Always allow", kind: "allow_always" }], + }); + await vi.waitFor(() => expect(session.getPendingPermissions()).toHaveLength(1)); + }); + 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")],