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")],