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 index cfbf45d3f3..8128604684 100644 --- 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 @@ -1,21 +1,12 @@ -import { describe, expect, test, vi } from "vitest"; -import type { - ClientSideConnection, - SessionConfigOption, - SessionUpdate, -} from "@agentclientprotocol/sdk"; -import { ACPAgentSession, type SessionStateResponse } from "./acp-agent.js"; -import type { AgentStreamEvent } from "../agent-sdk-types.js"; -import { asInternals } from "../../test-utils/class-mocks.js"; +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"; -interface ModeSessionInternals { - sessionId: string | null; - connection: Pick; - applySessionState(response: SessionStateResponse): void; - translateSessionUpdate(update: SessionUpdate): AgentStreamEvent[]; -} - const modes = { currentModeId: "accept_edits", availableModes: [ @@ -24,6 +15,7 @@ const modes = { { id: "dont_ask", name: "Don't Ask" }, ], }; + function editOptions(currentValue = "workspace_session"): SessionConfigOption[] { return [ { @@ -40,113 +32,267 @@ function editOptions(currentValue = "workspace_session"): SessionConfigOption[] }, ]; } -function fixture(withSessionModes = true) { - const session = new ACPAgentSession( - { provider: "hermes", cwd: process.cwd() }, - { - provider: "hermes", - logger: createTestLogger(), - defaultCommand: ["not-spawned"], - defaultModes: [], - capabilities: { - supportsStreaming: true, - supportsSessionPersistence: true, - supportsDynamicModes: true, - supportsMcpServers: true, - supportsReasoningStream: true, - supportsToolInvocations: true, - }, - }, - ); - const setSessionMode = vi.fn(async ({ modeId }: { sessionId: string; modeId: string }) => { - if (!modes.availableModes.some((mode) => mode.id === modeId)) throw new Error("invalid_params"); - return {}; - }); - const setSessionConfigOption = vi.fn( - async ({ value }: { sessionId: string; configId: string; value: string }) => ({ - configOptions: editOptions(value), - }), - ); - const internals = asInternals(session); - internals.sessionId = "isolated-session"; - internals.connection = { setSessionMode, setSessionConfigOption }; - internals.applySessionState({ - sessionId: "isolated-session", - modes: withSessionModes ? modes : undefined, - configOptions: editOptions(), - }); - return { session, internals, setSessionMode, setSessionConfigOption }; + +// 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 }; } -describe("ACP mode and config-value namespaces", () => { - test("keeps a pushed current mode when config values describe a different namespace", async () => { - const { session, internals } = fixture(); - internals.translateSessionUpdate({ - sessionUpdate: "current_mode_update", - currentModeId: "dont_ask", +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: [] }, + ], }); - internals.translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: editOptions("session"), - }); - expect(await session.getCurrentMode()).toBe("dont_ask"); - internals.translateSessionUpdate({ sessionUpdate: "config_option_update", configOptions: [] }); - expect(await session.getCurrentMode()).toBe("dont_ask"); - expect((await session.getAvailableModes()).map((mode) => mode.id)).toEqual([ + 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("does not send a config policy value to the mode endpoint or silently fall back", async () => { - const { session, internals, setSessionMode, setSessionConfigOption } = fixture(); - internals.translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: editOptions(), - }); - await session.setMode("session"); - expect(setSessionMode).not.toHaveBeenCalled(); - expect(setSessionConfigOption).not.toHaveBeenCalled(); - expect(await session.getCurrentMode()).toBe("accept_edits"); + 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("keeps the confirmed mode when the mode request fails", async () => { - const { session, setSessionMode, setSessionConfigOption } = fixture(); - setSessionMode.mockRejectedValueOnce(new Error("mode denied")); - await expect(session.setMode("dont_ask")).rejects.toThrow("mode denied"); - expect(await session.getCurrentMode()).toBe("accept_edits"); - expect(setSessionConfigOption).not.toHaveBeenCalled(); + 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("uses the config endpoint when a provider only advertises config choices", async () => { - const { session, setSessionMode, setSessionConfigOption } = fixture(false); - await session.setMode("session"); - expect(setSessionMode).not.toHaveBeenCalled(); - expect(setSessionConfigOption).toHaveBeenCalledExactlyOnceWith({ - sessionId: "isolated-session", - configId: "edit_approval_policy", - value: "session", - }); - expect(await session.getCurrentMode()).toBe("session"); + 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("keeps real mode IDs after a config update and sends Don't Ask to the mode endpoint", async () => { - const { session, internals, setSessionMode, setSessionConfigOption } = fixture(); - internals.translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: editOptions(), + 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 session.getAvailableModes()).toEqual( - modes.availableModes.map(({ id, name }) => ({ id, label: name, description: undefined })), - ); - expect(await session.getCurrentMode()).toBe("accept_edits"); - await session.setMode("dont_ask"); - expect(setSessionMode).toHaveBeenCalledExactlyOnceWith({ - sessionId: "isolated-session", - modeId: "dont_ask", - }); - expect(setSessionConfigOption).not.toHaveBeenCalled(); - expect(await session.getCurrentMode()).toBe("dont_ask"); + 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 873e72b7aa..ae80dc74f7 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -1775,62 +1775,6 @@ describe("ACPAgentSession Zed parity", () => { }); }); - test("tracks initialized Copilot Allow All notifications and disables them before switching away", async () => { - const agentMode = "https://agentclientprotocol.com/protocol/session-modes#agent"; - const session = createCopilotSessionWithConfig(); - const prepared = prepareConfiguredOverrideSession(session, { - currentMode: agentMode, - availableModes: COPILOT_MODES, - configOptions: [copilotModeConfigOption(agentMode), copilotAllowAllConfigOption("off")], - }); - prepared.internals.applySessionState({ - sessionId: "session-1", - modes: { - currentModeId: agentMode, - availableModes: COPILOT_MODES.map((mode) => ({ id: mode.id, name: mode.label })), - }, - configOptions: [copilotModeConfigOption(agentMode), copilotAllowAllConfigOption("off")], - }); - asInternals(session).translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: [copilotModeConfigOption(agentMode), copilotAllowAllConfigOption("on")], - }); - await expect(session.getCurrentMode()).resolves.toBe(COPILOT_ALLOW_ALL_MODE_ID); - await session.setMode(agentMode); - expect(prepared.setSessionConfigOption).toHaveBeenCalledExactlyOnceWith({ - sessionId: "session-1", - configId: "allow_all", - value: "off", - }); - expect(prepared.setSessionMode).toHaveBeenCalledExactlyOnceWith({ - sessionId: "session-1", - modeId: agentMode, - }); - expect(prepared.setSessionConfigOption.mock.invocationCallOrder[0]).toBeLessThan( - prepared.setSessionMode.mock.invocationCallOrder[0]!, - ); - }); - - test("tracks initialized Copilot Allow All being turned off through config", async () => { - const agentMode = "https://agentclientprotocol.com/protocol/session-modes#agent"; - const session = createCopilotSessionWithConfig(); - const prepared = prepareConfiguredOverrideSession(session, { availableModes: COPILOT_MODES }); - prepared.internals.applySessionState({ - sessionId: "session-1", - modes: { - currentModeId: agentMode, - availableModes: COPILOT_MODES.map((mode) => ({ id: mode.id, name: mode.label })), - }, - configOptions: [copilotModeConfigOption(agentMode), copilotAllowAllConfigOption("on")], - }); - await expect(session.getCurrentMode()).resolves.toBe(COPILOT_ALLOW_ALL_MODE_ID); - asInternals(session).translateSessionUpdate({ - sessionUpdate: "config_option_update", - configOptions: [copilotModeConfigOption(agentMode), copilotAllowAllConfigOption("off")], - }); - await expect(session.getCurrentMode()).resolves.toBe(agentMode); - }); - test("trusts Copilot allow_all config updates as the current mode source", async () => { const session = createCopilotSessionWithConfig(); const internals = asInternals(session);