From 14845d3ff3482fe785bbfb15be7cf9abdfb90fb3 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Fri, 6 Nov 2026 15:10:43 +0000 Subject: [PATCH] Keep ACP mode selections valid after config updates (#8912) * fix(acp): preserve session mode identity across config updates * test(acp): verify mode identity through public sessions Exercise initialization, queued notifications, mode selection, and rejection over stdio instead of replacing private connection state. Co-authored-by: a-sh <846704+a-sh@users.noreply.github.com> --------- Co-authored-by: a-sh <846704+a-sh@users.noreply.github.com> Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> --- .../acp-agent.mode-namespace.test.ts | 298 ++++++++++++++++++ .../server/agent/providers/acp-agent.test.ts | 19 ++ .../src/server/agent/providers/acp-agent.ts | 48 ++- 3 files changed, 352 insertions(+), 13 deletions(-) create mode 100644 packages/server/src/server/agent/providers/acp-agent.mode-namespace.test.ts diff --git a/packages/server/src/server/agent/providers/acp-agent.mode-namespace.test.ts b/packages/server/src/server/agent/providers/acp-agent.mode-namespace.test.ts new file mode 100644 index 0000000000..8128604684 --- /dev/null +++ b/packages/server/src/server/agent/providers/acp-agent.mode-namespace.test.ts @@ -0,0 +1,298 @@ +import { afterEach, describe, expect, test } from "vitest"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import path from "node:path"; +import type { SessionConfigOption, SessionUpdate } from "@agentclientprotocol/sdk"; +import { ACPAgentClient, type SessionStateResponse } from "./acp-agent.js"; +import { CopilotACPAgentClient, COPILOT_ALLOW_ALL_MODE_ID } from "./copilot-acp-agent.js"; +import type { AgentSession } from "../agent-sdk-types.js"; +import { createTestLogger } from "../../../test-utils/test-logger.js"; + +const modes = { + currentModeId: "accept_edits", + availableModes: [ + { id: "default", name: "Default" }, + { id: "accept_edits", name: "Accept Edits" }, + { id: "dont_ask", name: "Don't Ask" }, + ], +}; + +function editOptions(currentValue = "workspace_session"): SessionConfigOption[] { + return [ + { + id: "edit_approval_policy", + name: "Edits", + category: "mode", + type: "select", + currentValue, + options: [ + { value: "ask", name: "Default" }, + { value: "workspace_session", name: "Accept Edits" }, + { value: "session", name: "Don't Ask" }, + ], + }, + ]; +} + +// A controlled ACP process, speaking the same stdio protocol as a provider. +// Notifications precede session/new's response to exercise queued initialization updates. +const modePeerScript = String.raw` +import { createInterface } from "node:readline"; +import { readFileSync, appendFileSync } from "node:fs"; +const scenario = JSON.parse(readFileSync(process.argv[2], "utf8")); +let configOptions = scenario.state.configOptions; +const send = (message) => process.stdout.write(JSON.stringify(message) + "\n"); +createInterface({ input: process.stdin }).on("line", (line) => { + const request = JSON.parse(line); + const reply = (result) => send({jsonrpc: "2.0", id: request.id, result}); + const reject = (message) => send({jsonrpc: "2.0", id: request.id, error: {code: -32602, message}}); + switch (request.method) { + case "initialize": + reply({protocolVersion: 1, agentCapabilities: {}}); + break; + case "session/new": + for (const update of scenario.updates) { + send({jsonrpc: "2.0", method: "session/update", params: {sessionId: "mode-session", update}}); + } + reply({sessionId: "mode-session", ...scenario.state}); + break; + case "session/set_mode": + appendFileSync(process.argv[3], JSON.stringify(request) + "\n"); + if (request.params.modeId === scenario.rejectedMode) { + reject("mode denied"); + } else if (!scenario.state.modes?.availableModes.some(mode => mode.id === request.params.modeId)) { + reject("invalid_params"); + } else { + reply({}); + } + break; + case "session/set_config_option": + appendFileSync(process.argv[3], JSON.stringify(request) + "\n"); + configOptions = configOptions.map(option => option.id === request.params.configId + ? {...option, currentValue: request.params.value} : option); + reply({configOptions}); + break; + default: + reject("Unexpected request: " + request.method); + } +}); +`; + +interface ModeCall { + method: string; + params: { sessionId: string; modeId?: string; configId?: string; value?: string }; +} + +let session: AgentSession | undefined; +let directory: string | undefined; + +afterEach(async () => { + try { + await session?.close(); + } finally { + session = undefined; + if (directory) await rm(directory, { recursive: true, force: true }); + directory = undefined; + } +}); + +async function fixture( + options: { + state?: Omit; + updates?: SessionUpdate[]; + rejectedMode?: string; + startupMode?: string; + copilot?: boolean; + } = {}, +) { + const artifacts = path.resolve(".artifacts"); + await mkdir(artifacts, { recursive: true }); + directory = await mkdtemp(path.join(artifacts, "acp-mode-")); + const script = path.join(directory, "peer.mjs"); + const scenario = path.join(directory, "scenario.json"); + const calls = path.join(directory, "calls.jsonl"); + await writeFile(script, modePeerScript); + await writeFile(calls, ""); + await writeFile( + scenario, + JSON.stringify({ + state: options.state ?? { modes, configOptions: editOptions() }, + updates: options.updates ?? [ + { sessionUpdate: "config_option_update", configOptions: editOptions() }, + ], + rejectedMode: options.rejectedMode, + }), + ); + const command: [string, ...string[]] = [process.execPath, script, scenario, calls]; + const client = options.copilot + ? new CopilotACPAgentClient({ + logger: createTestLogger(), + runtimeSettings: { command: { mode: "replace", argv: command } }, + }) + : new ACPAgentClient({ + provider: "mode-peer", + logger: createTestLogger(), + defaultCommand: command, + }); + session = await client.createSession({ + provider: options.copilot ? "copilot" : "mode-peer", + cwd: directory, + modeId: options.startupMode, + }); + return { + session, + async calls(): Promise { + const lines = (await readFile(calls, "utf8")).trim(); + return lines + ? lines.split("\n").map((line) => { + const { method, params } = JSON.parse(line) as ModeCall; + return { method, params }; + }) + : []; + }, + }; +} + +const modeCall = (modeId: string) => ({ + method: "session/set_mode", + params: { sessionId: "mode-session", modeId }, +}); +const configCall = (configId: string, value: string) => ({ + method: "session/set_config_option", + params: { sessionId: "mode-session", configId, value }, +}); + +describe("ACP mode and config-value namespaces over stdio", () => { + test("selects Don't Ask from the advertised catalogue after a queued config update", async () => { + const peer = await fixture(); + const available = await peer.session.getAvailableModes(); + const dontAsk = available.find((mode) => mode.label === "Don't Ask"); + expect(dontAsk).toBeDefined(); + await peer.session.setMode(dontAsk!.id); + expect(await peer.calls()).toEqual([modeCall("dont_ask")]); + expect(await peer.session.getCurrentMode()).toBe("dont_ask"); + expect(available.map((mode) => mode.id)).toEqual(["default", "accept_edits", "dont_ask"]); + }); + + test("keeps a pushed current mode through separate-policy and empty config updates", async () => { + const peer = await fixture({ + updates: [ + { sessionUpdate: "current_mode_update", currentModeId: "dont_ask" }, + { sessionUpdate: "config_option_update", configOptions: editOptions("session") }, + { sessionUpdate: "config_option_update", configOptions: [] }, + ], + }); + expect(await peer.session.getCurrentMode()).toBe("dont_ask"); + expect((await peer.session.getAvailableModes()).map((mode) => mode.id)).toEqual([ + "default", + "accept_edits", + "dont_ask", + ]); + expect(await peer.calls()).toEqual([]); + }); + + test("ignores policy values outside the advertised session modes without an RPC", async () => { + const peer = await fixture(); + await peer.session.setMode("session"); + expect(await peer.calls()).toEqual([]); + expect(await peer.session.getCurrentMode()).toBe("accept_edits"); + }); + + test("retains confirmed state after rejection without falling back to a config write", async () => { + const peer = await fixture({ rejectedMode: "dont_ask", updates: [] }); + await expect(peer.session.setMode("dont_ask")).rejects.toThrow("mode denied"); + expect(await peer.session.getCurrentMode()).toBe("accept_edits"); + expect(await peer.calls()).toEqual([modeCall("dont_ask")]); + }); + + test("selects a config-only mode through its owning config option", async () => { + const peer = await fixture({ state: { configOptions: editOptions() } }); + await peer.session.setMode("session"); + expect(await peer.calls()).toEqual([configCall("edit_approval_policy", "session")]); + expect(await peer.session.getCurrentMode()).toBe("session"); + }); + + test.each([true, false])("routes startup overrides with session modes: %s", async (withModes) => { + const peer = await fixture({ + state: { modes: withModes ? modes : undefined, configOptions: editOptions() }, + startupMode: withModes ? "dont_ask" : "session", + }); + expect(await peer.calls()).toEqual([ + withModes ? modeCall("dont_ask") : configCall("edit_approval_policy", "session"), + ]); + expect(await peer.session.getCurrentMode()).toBe(withModes ? "dont_ask" : "session"); + }); +}); + +const agentMode = "https://agentclientprotocol.com/protocol/session-modes#agent"; +const planMode = "https://agentclientprotocol.com/protocol/session-modes#plan"; +function copilotConfig(allowAll: string): SessionConfigOption[] { + return [ + { + id: "mode", + name: "Mode", + category: "mode", + type: "select", + currentValue: agentMode, + options: [ + { value: agentMode, name: "Agent" }, + { value: planMode, name: "Plan" }, + ], + }, + { + id: "allow_all", + name: "Allow All", + type: "select", + currentValue: allowAll, + options: [ + { value: "on", name: "On" }, + { value: "off", name: "Off" }, + ], + }, + ]; +} +function copilotFixture(initial: string, pushed: string) { + return fixture({ + copilot: true, + state: { + modes: { + currentModeId: agentMode, + availableModes: [ + { id: agentMode, name: "Agent" }, + { id: planMode, name: "Plan" }, + ], + }, + configOptions: copilotConfig(initial), + }, + updates: [{ sessionUpdate: "config_option_update", configOptions: copilotConfig(pushed) }], + }); +} + +describe("Copilot Allow All through the public client", () => { + test("adopts an Allow All notification and disables it before switching modes", async () => { + const peer = await copilotFixture("off", "on"); + expect(await peer.session.getCurrentMode()).toBe(COPILOT_ALLOW_ALL_MODE_ID); + await peer.session.setMode(planMode); + expect(await peer.calls()).toEqual([configCall("allow_all", "off"), modeCall(planMode)]); + expect(await peer.session.getCurrentMode()).toBe(planMode); + }); + + test("adopts an Allow All off notification after initializing with it enabled", async () => { + const peer = await copilotFixture("on", "off"); + expect(await peer.session.getCurrentMode()).toBe(agentMode); + expect(await peer.calls()).toEqual([]); + }); + + test("selects Allow All and switches away in provider RPC order", async () => { + const peer = await copilotFixture("off", "off"); + await peer.session.setMode(COPILOT_ALLOW_ALL_MODE_ID); + expect(await peer.session.getCurrentMode()).toBe(COPILOT_ALLOW_ALL_MODE_ID); + await peer.session.setMode(planMode); + expect(await peer.calls()).toEqual([ + modeCall(agentMode), + configCall("allow_all", "on"), + configCall("allow_all", "off"), + modeCall(planMode), + ]); + expect(await peer.session.getCurrentMode()).toBe(planMode); + }); +}); 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 ea0c1702e9..ae80dc74f7 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -140,6 +140,7 @@ interface ACPModelSelectionInternals { } interface ACPConfiguredOverrideInternals { + applySessionState(response: SessionStateResponse): void; sessionId: string | null; connection: { setSessionMode: (input: { sessionId: string; modeId: string }) => Promise; @@ -444,6 +445,24 @@ function prepareConfiguredOverrideSession( configOptions: options.configOptions ?? [], })); const internals = asInternals(session); + const availableModes = options.availableModes ?? []; + const modes = + availableModes.length > 0 + ? { + currentModeId: options.currentMode ?? availableModes[0]!.id, + availableModes: availableModes.map((mode) => ({ + id: mode.id, + label: mode.label, + description: mode.description, + name: mode.label, + })), + } + : undefined; + internals.applySessionState({ + sessionId: "session-1", + modes, + configOptions: options.configOptions ?? [], + }); internals.sessionId = "session-1"; internals.connection = { setSessionMode, diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 09662aaa1c..4b67d827dd 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -1769,6 +1769,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { private readonly earlySessionUpdates: SessionNotification[] = []; private currentMode: string | null = null; private availableModes: AgentMode[]; + private sessionModeState: SessionStateResponse["modes"] = null; private currentModel: string | null = null; private availableModels: AvailableACPModel[] | null = null; private thinkingOptionId: string | null = null; @@ -2148,11 +2149,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { throw new Error("ACP session not initialized"); } - const selection = resolveACPModeSelection({ - modeId, - availableModes: this.availableModes, - configOptions: this.configOptions, - }); + const selection = this.resolveModeSelection(modeId); await this.setModeWithSelection({ modeId, selection }); } @@ -2178,7 +2175,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { if (providerResult.configOptions) { this.configOptions = this.transformConfigOptions(providerResult.configOptions); } - this.availableModes = deriveModesFromACP(this.defaultModes, null, this.configOptions).modes; + this.availableModes = this.modesFromConfig().modes; this.pushEvent({ type: "mode_changed", provider: this.provider, @@ -2257,7 +2254,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { requestedValue: modeId, label: "mode", }); - this.availableModes = deriveModesFromACP(this.defaultModes, null, this.configOptions).modes; + this.availableModes = this.modesFromConfig().modes; this.pushEvent({ type: "mode_changed", provider: this.provider, @@ -3010,6 +3007,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { : response; this.configOptions = this.transformConfigOptions(transformed.configOptions ?? []); + this.sessionModeState = transformed.modes ?? null; const modeInfo = deriveModesFromACP(this.defaultModes, transformed.modes, this.configOptions); this.availableModes = modeInfo.modes; @@ -3035,11 +3033,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { private async applyConfiguredOverrides(): Promise { const configuredModeId = this.config.modeId; if (configuredModeId && configuredModeId !== this.currentMode) { - const selection = resolveACPModeSelection({ - modeId: configuredModeId, - availableModes: this.availableModes, - configOptions: this.configOptions, - }); + const selection = this.resolveModeSelection(configuredModeId); await this.setModeWithSelection({ modeId: configuredModeId, selection }); } const configuredModelId = this.config.model; @@ -3296,9 +3290,37 @@ export class ACPAgentSession implements AgentSession, ACPClient { this.currentMode = this.transformModeId(update.currentModeId); } + private modesFromConfig() { + let modes = this.sessionModeState; + if (modes) { + let currentModeId = this.currentMode ?? modes.currentModeId; + const configMode = findSelectConfigOption({ + configOptions: this.configOptions, + category: "mode", + }); + // Copilot config updates name real modes; Hermes config values are a separate policy namespace. + if (configMode && modes.availableModes.some((mode) => mode.id === configMode.currentValue)) { + currentModeId = configMode.currentValue; + } + modes = { ...modes, currentModeId }; + } + return deriveModesFromACP(this.defaultModes, modes, this.configOptions); + } + + private resolveModeSelection(modeId: string): ACPModeSelection { + const hasSessionModes = Boolean(this.sessionModeState?.availableModes.length); + const modeConfig = findSelectConfigOption({ + configOptions: this.configOptions, + category: "mode", + }); + const configOnly = !hasSessionModes && modeConfig !== null; + const availableModes = configOnly ? [] : this.availableModes; + return resolveACPModeSelection({ modeId, availableModes, configOptions: this.configOptions }); + } + private handleConfigOptionUpdate(update: ConfigOptionUpdate): AgentStreamEvent[] { this.configOptions = this.transformConfigOptions(update.configOptions); - const modeInfo = deriveModesFromACP(this.defaultModes, null, this.configOptions); + const modeInfo = this.modesFromConfig(); const nextMode = modeInfo.currentModeId; const nextModel = deriveCurrentConfigValue(this.configOptions, "model"); const nextThinkingOptionId = deriveCurrentConfigValue(this.configOptions, "thought_level");