diff --git a/packages/server/src/server/agent/providers/acp-agent.test.ts b/packages/server/src/server/agent/providers/acp-agent.test.ts index 3118f6db90..cd0f4f9b0b 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -4,7 +4,7 @@ import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; import { createRequire } from "node:module"; import { tmpdir } from "node:os"; import path from "node:path"; -import { pathToFileURL } from "node:url"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; import { AgentSideConnection, @@ -301,6 +301,7 @@ function selectConfigOption( function createCopilotSessionWithConfig( modeId?: string | null, providerOptions?: Record, + defaultCommand: string[] = ["copilot", "--acp"], ): ACPAgentSession { return new ACPAgentSession( { @@ -312,7 +313,7 @@ function createCopilotSessionWithConfig( { provider: "copilot", logger: createTestLogger(), - defaultCommand: ["copilot", "--acp"], + defaultCommand, defaultModes: COPILOT_MODES, sessionResponseTransformer: transformCopilotSessionResponse, configOptionsTransformer: transformCopilotConfigOptions, @@ -1802,73 +1803,6 @@ describe("ACPAgentSession Zed parity", () => { ]); }); - test("a select the provider opened differently from the options is the session's open patch, not an event", async () => { - const setSessionConfigOption = vi.fn(async () => ({ - configOptions: [copilotAgentConfigOption("")], - })); - const session = createCopilotSessionWithConfig(null, { agent: "Probe Agent" }); - const { internals } = prepareConfiguredOverrideSession(session, { - configOptions: [copilotAgentConfigOption("")], - connection: { setSessionConfigOption }, - }); - const emitted: AgentStreamEvent[] = []; - session.subscribe((event) => emitted.push(event)); - - await internals.applyConfiguredOverrides(); - - expect(session.openOptionsPatch).toEqual({ agent: "" }); - expect(emitted.filter((event) => event.type === "options_changed")).toEqual([]); - }); - - test("applying a patch returns the selects that ended up different and emits nothing", async () => { - const setSessionConfigOption = vi.fn(async () => ({ - configOptions: [copilotAgentConfigOption("")], - })); - const session = createCopilotSessionWithConfig(); - prepareConfiguredOverrideSession(session, { - configOptions: [copilotAgentConfigOption("")], - connection: { setSessionConfigOption }, - }); - const emitted: AgentStreamEvent[] = []; - session.subscribe((event) => emitted.push(event)); - - await expect(session.applyOptions({ agent: "Probe Agent" })).resolves.toEqual({ agent: "" }); - expect(emitted.filter((event) => event.type === "options_changed")).toEqual([]); - }); - - test("a removal writes the select's empty choice and returns nothing when that took", async () => { - const setSessionConfigOption = vi.fn(async () => ({ - configOptions: [copilotAgentConfigOption("")], - })); - const session = createCopilotSessionWithConfig(); - prepareConfiguredOverrideSession(session, { - configOptions: [copilotAgentConfigOption("Probe Agent")], - connection: { setSessionConfigOption }, - }); - - await expect(session.applyOptions({ agent: null })).resolves.toEqual({}); - expect(setSessionConfigOption).toHaveBeenCalledWith(expect.objectContaining({ value: "" })); - }); - - test("forwards a select the provider changes on its own", async () => { - const session = createCopilotSessionWithConfig(); - prepareConfiguredOverrideSession(session, { - configOptions: [copilotAgentConfigOption("Probe Agent")], - connection: { setSessionConfigOption: vi.fn() }, - }); - const emitted: AgentStreamEvent[] = []; - session.subscribe((event) => emitted.push(event)); - - asInternals(session).translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: [copilotAgentConfigOption("")], - }); - - expect(emitted.filter((event) => event.type === "options_changed")).toEqual([ - { type: "options_changed", provider: "copilot", patch: { agent: "" } }, - ]); - }); - test("sets Copilot custom agent through ACP config options", async () => { const setSessionConfigOption = vi.fn(async () => ({ configOptions: [copilotAgentConfigOption("Probe Agent")], @@ -1893,6 +1827,130 @@ describe("ACPAgentSession Zed parity", () => { }); }); +describe("ACPAgentSession options against an ACP agent", () => { + const fakeAgentPath = fileURLToPath(new URL("./test-utils/fake-acp-agent.mjs", import.meta.url)); + const sessions: ACPAgentSession[] = []; + + afterEach(async () => { + await Promise.all(sessions.splice(0).map((session) => session.close())); + }); + + function thoughtOption(currentValue: string): SessionConfigOption { + return { + id: "thought", + name: "Thinking", + category: "thought_level", + type: "select", + currentValue, + options: [ + { value: "low", name: "Low" }, + { value: "high", name: "High" }, + ], + }; + } + + async function openSession(input: { + agent?: string; + scenario?: Record; + providerOptions?: Record; + }): Promise<{ session: ACPAgentSession; optionEvents: AgentStreamEvent[] }> { + const scenario = { + options: [copilotAgentConfigOption(input.agent ?? ""), thoughtOption("low")], + ...input.scenario, + }; + await mkdir("/tmp/paseo-acp-test", { recursive: true }); + const session = createCopilotSessionWithConfig(null, input.providerOptions, [ + process.execPath, + fakeAgentPath, + JSON.stringify(scenario), + ]); + sessions.push(session); + const optionEvents: AgentStreamEvent[] = []; + session.subscribe((event) => { + if (event.type === "options_changed") optionEvents.push(event); + }); + await session.initializeNewSession(); + return { session, optionEvents }; + } + + test("a select the agent opened differently from the options is the open patch, not an event", async () => { + const { session, optionEvents } = await openSession({ + providerOptions: { agent: "Probe Agent" }, + scenario: { answers: { agent: "" } }, + }); + + expect(session.openOptionsPatch).toEqual({ agent: "" }); + expect(optionEvents).toEqual([]); + }); + + test("a notification that arrives before the open write is answered is not an event", async () => { + const { session, optionEvents } = await openSession({ + providerOptions: { agent: "Probe Agent" }, + scenario: { onWrite: "notify-first" }, + }); + + expect(session.openOptionsPatch).toEqual({}); + expect(optionEvents).toEqual([]); + }); + + test("applying a patch returns the selects that ended up different and emits nothing", async () => { + const { session, optionEvents } = await openSession({ scenario: { answers: { agent: "" } } }); + + await expect(session.applyOptions({ agent: "Probe Agent" })).resolves.toEqual({ agent: "" }); + expect(optionEvents).toEqual([]); + }); + + test("a notification that arrives before the apply write is answered is not an event", async () => { + const { session, optionEvents } = await openSession({ scenario: { onWrite: "notify-first" } }); + + await expect(session.applyOptions({ agent: "Probe Agent" })).resolves.toEqual({}); + expect(optionEvents).toEqual([]); + }); + + test("a rejected apply reports what the agent moved before refusing, since nothing is stored", async () => { + const { session, optionEvents } = await openSession({ + scenario: { onWrite: "notify-then-reject" }, + }); + + await expect(session.applyOptions({ agent: "Probe Agent" })).rejects.toThrow(/write refused/); + expect(optionEvents).toEqual([ + { type: "options_changed", provider: "copilot", patch: { agent: "Probe Agent" } }, + ]); + }); + + test("a removal writes the select's empty choice and returns nothing when that took", async () => { + const { session, optionEvents } = await openSession({ agent: "Probe Agent" }); + + await expect(session.applyOptions({ agent: null })).resolves.toEqual({}); + expect(optionEvents).toEqual([]); + }); + + test("rejects a live auto_accept the creation schema would reject, and writes nothing", async () => { + const { session, optionEvents } = await openSession({ + scenario: { onWrite: "notify-then-reject" }, + }); + + await expect( + session.applyOptions({ auto_accept: "yes", agent: "Probe Agent" }), + ).rejects.toThrow(/auto_accept/); + expect(optionEvents).toEqual([]); + }); + + test("forwards a select the agent changes on its own", async () => { + const { session, optionEvents } = await openSession({ + scenario: { moves: { thought: { agent: "Probe Agent" } } }, + }); + + await session.setThinkingOption("high"); + + await vi.waitFor(() => + expect(optionEvents).toEqual([ + { type: "options_changed", provider: "copilot", patch: { agent: "Probe Agent" } }, + ]), + ); + }); +}); + describe("deriveModelDefinitionsFromACP", () => { test("attaches shared thinking options to ACP model state", () => { const result = deriveModelDefinitionsFromACP( diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index db0afc2b7c..5c2e83fc8f 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -1,5 +1,6 @@ import type { ProviderOptions } from "@getpaseo/protocol/agent-types"; import { ACPProviderOptionsSchema } from "./acp-options.js"; +import { applyProviderOptionsPatch } from "../option-controls.js"; import { type ChildProcess, type ChildProcessWithoutNullStreams } from "node:child_process"; import { randomUUID } from "node:crypto"; import fs from "node:fs/promises"; @@ -1757,6 +1758,8 @@ export class ACPAgentSession implements AgentSession, ACPClient { private configOptions: SessionConfigOption[] = []; /** How the opened session differs from the options it was given. */ openOptionsPatch: Record = {}; + /** Selects the provider moves while Paseo is opening or applying options belong to that result, not to events. */ + private optionsTransactionDepth = 0; private cachedCommands: AgentSlashCommand[] = []; /** When the last command batch arrived; null while nothing has been published. */ private commandsUpdatedAt: number | null = null; @@ -2397,19 +2400,31 @@ export class ACPAgentSession implements AgentSession, ACPClient { }); } - /** Applies only the options the patch sets; returns the selects that ended up different. */ + /** + * Applies only the options the patch sets; returns the selects that ended up different. + * Provider notifications that arrive meanwhile are part of that result. When a write is + * rejected, what the provider moved is reported as an event, since nothing is stored. + */ async applyOptions(options: ProviderOptions): Promise> { + ACPProviderOptionsSchema.parse(applyProviderOptionsPatch(this.config.providerOptions, options)); const before = this.featureValues(); const requested: Record = {}; - for (const [id, value] of Object.entries(options)) { - const featureOption = this.configFeatureOptions.find((candidate) => candidate.id === id); - if (!featureOption) continue; - // A removal asks for the select's empty choice; a provider without one rejects it. - requested[id] = value ?? ""; - const option = findSelectConfigFeatureOption(this.configOptions, featureOption); - if (!option || requested[id] !== option.currentValue) { - await this.setConfigFeature(id, value); - } + try { + await this.inOptionsTransaction(async () => { + for (const [id, value] of Object.entries(options)) { + const featureOption = this.configFeatureOptions.find((candidate) => candidate.id === id); + if (!featureOption) continue; + // A removal asks for the select's empty choice; a provider without one rejects it. + requested[id] = value ?? ""; + const option = findSelectConfigFeatureOption(this.configOptions, featureOption); + if (!option || requested[id] !== option.currentValue) { + await this.setConfigFeature(id, value); + } + } + }); + } catch (error) { + this.reportFeatureChanges(before); + throw error; } // Auto Accept is Paseo's own setting, so it changes only once every write has been accepted. if (ACP_AUTO_ACCEPT_FEATURE_ID in options) { @@ -2421,6 +2436,15 @@ export class ACPAgentSession implements AgentSession, ACPClient { return this.featureDifferences({ ...before, ...requested }); } + private async inOptionsTransaction(run: () => Promise): Promise { + this.optionsTransactionDepth += 1; + try { + return await run(); + } finally { + this.optionsTransactionDepth -= 1; + } + } + /** The selects as the provider reports them now. */ private featureValues(): Record { const values: Record = {}; @@ -2445,6 +2469,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { /** Reports the selects the provider moved on its own, compared with their values `before`. */ private reportFeatureChanges(before: Record): void { + if (this.optionsTransactionDepth > 0) return; const patch = this.featureDifferences(before); if (Object.keys(patch).length > 0) { this.pushEvent({ type: "options_changed", provider: this.provider, patch }); @@ -3041,7 +3066,11 @@ export class ACPAgentSession implements AgentSession, ACPClient { return this.modeIdTransformer ? this.modeIdTransformer(modeId) : modeId; } - private async applyConfiguredOverrides(): Promise { + private applyConfiguredOverrides(): Promise { + return this.inOptionsTransaction(() => this.writeConfiguredOverrides()); + } + + private async writeConfiguredOverrides(): Promise { const configuredModeId = this.config.modeId; if (configuredModeId && configuredModeId !== this.currentMode) { const selection = resolveACPModeSelection({ diff --git a/packages/server/src/server/agent/providers/acp-options.ts b/packages/server/src/server/agent/providers/acp-options.ts index 4f75456e38..4d3a5c56d7 100644 --- a/packages/server/src/server/agent/providers/acp-options.ts +++ b/packages/server/src/server/agent/providers/acp-options.ts @@ -2,6 +2,8 @@ import { z } from "zod"; export const ACPProviderOptionsSchema = z .object({ + // Composer control: approves permission prompts without asking. + auto_accept: z.boolean().optional(), supportsMcpServers: z.boolean().optional(), clientCapabilities: z .object({ diff --git a/packages/server/src/server/agent/providers/test-utils/fake-acp-agent.mjs b/packages/server/src/server/agent/providers/test-utils/fake-acp-agent.mjs new file mode 100644 index 0000000000..1ea77896f2 --- /dev/null +++ b/packages/server/src/server/agent/providers/test-utils/fake-acp-agent.mjs @@ -0,0 +1,57 @@ +// A stdio ACP agent for tests. argv[2] is a JSON scenario: +// options: the session's config options +// answers: { [configId]: value } the agent ends up with instead of the requested value +// onWrite: "notify-first" sends config_option_update before answering the write; +// "notify-then-reject" does the same and then fails the write +// moves: { [configId]: { [id]: value } } writes that also move another select, announced +// by a config_option_update after the write is answered +import { Readable, Writable } from "node:stream"; +import { AgentSideConnection, ndJsonStream } from "@agentclientprotocol/sdk"; + +const scenario = JSON.parse(process.argv[2] ?? "{}"); +let options = scenario.options ?? []; + +function set(id, value) { + options = options.map((option) => + option.id === id ? { ...option, currentValue: value } : option, + ); +} + +const stream = ndJsonStream(Writable.toWeb(process.stdout), Readable.toWeb(process.stdin)); +export const connection = new AgentSideConnection( + (agentConnection) => ({ + async initialize() { + return { protocolVersion: 1, agentCapabilities: {} }; + }, + async newSession() { + return { sessionId: "session-1", modes: null, models: null, configOptions: options }; + }, + async setSessionConfigOption({ sessionId, configId, value }) { + set(configId, scenario.answers?.[configId] ?? value); + const announce = () => + agentConnection.sessionUpdate({ + sessionId, + update: { sessionUpdate: "config_option_update", configOptions: options }, + }); + if (scenario.onWrite === "notify-first" || scenario.onWrite === "notify-then-reject") { + await announce(); + if (scenario.onWrite === "notify-then-reject") throw new Error("write refused"); + return { configOptions: options }; + } + const moves = scenario.moves?.[configId]; + if (moves) { + for (const [id, moved] of Object.entries(moves)) set(id, moved); + setTimeout(() => void announce(), 20); + } + return { configOptions: options }; + }, + async authenticate() { + return {}; + }, + async prompt() { + return { stopReason: "end_turn" }; + }, + async cancel() {}, + }), + stream, +);