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>
2026-11-06 16:34:36 +00:00 · 2026-11-06 15:10:43 +00:00
parent 616f5e155c
commit 14845d3ff3
3 changed files with 352 additions and 13 deletions
@@ -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<SessionStateResponse, "sessionId">;
+    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<ModeCall[]> {
+      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);
+  });
+});
@@ -140,6 +140,7 @@ interface ACPModelSelectionInternals {
 }

 interface ACPConfiguredOverrideInternals {
+  applySessionState(response: SessionStateResponse): void;
  sessionId: string | null;
  connection: {
    setSessionMode: (input: { sessionId: string; modeId: string }) => Promise<void>;
@@ -444,6 +445,24 @@ function prepareConfiguredOverrideSession(
    configOptions: options.configOptions ?? [],
  }));
  const internals = asInternals<ACPConfiguredOverrideInternals>(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,
@@ -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<void> {
    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");