Preserve interactive ACP permission choices (#2762)

* fix(acp): preserve permission choices ACP question bridges encode answers as repeated permission options. Preserve each option through the UI and return the exact selected ID, while keeping auto-accept from choosing on the user's behalf. * fix(acp): validate selected permission behavior Reject inconsistent action IDs instead of allowing the action ID to override the response behavior. Move the misplaced browser spec so the current base CI contract can run. * fix(acp): preserve invalid permission requests Validate selected ACP actions before consuming the pending request so inconsistent client responses can be corrected and retried. * test(acp): verify real permission questions * fix(acp): reject empty permission action ids * test(e2e): scope Kimi config to ACP spec
2026-08-01 20:56:19 +02:00
parent 74e4a9f90c
commit 048b82f2a7
9 changed files with 362 additions and 16 deletions
@@ -12,6 +12,8 @@ The only built-in ACP provider today is `copilot` (`copilot-acp-agent.ts`). `Gen

 Copilot custom agents are exposed through ACP session config, not the slash-command list. When custom agents are available, Copilot returns a select config option with `id: "agent"` and `category: "_agent"`; Paseo maps that to the `agent` provider feature. Copilot uses the agent display name as the option value, and the blank value means the default Copilot agent.

+ACP permission options are rendered as ordered actions and Paseo returns the selected option's exact `optionId`. Agents can therefore encode a single-choice question as multiple options of the same allow kind. Auto-accept does not resolve those chooser requests; they always wait for the user.
+
 ### Direct

 Implement the `AgentClient` and `AgentSession` interfaces from `agent-sdk-types.ts` yourself. This gives full control but requires you to handle process management, streaming, permissions, and session persistence from scratch.
@@ -0,0 +1,70 @@
+import { mkdtempSync, realpathSync } from "node:fs";
+import { tmpdir } from "node:os";
+import path from "node:path";
+import { expect, test, type Page } from "../support/fixtures";
+import { submitMessage } from "../support/helpers/composer";
+import { choosePermissionAction, expectPermissionActions } from "../support/helpers/permissions";
+import {
+  cleanupRewindFlow,
+  launchAgent,
+  type AgentHandle,
+  type RewindFlowProvider,
+} from "../support/helpers/rewind-flow";
+
+const QUESTION = "Which option should I choose?";
+const OPTIONS = ["Alpha", "Beta", "Gamma"];
+
+async function askAgentToOfferChoices(handle: AgentHandle): Promise<void> {
+  await submitMessage(
+    handle.page,
+    `Use your built-in question tool now. Ask exactly "${QUESTION}" with exactly three choices named Alpha, Beta, and Gamma. Do not answer the question yourself. After I choose, reply exactly SELECTED_<CHOICE>, replacing <CHOICE> with my selection.`,
+  );
+}
+
+async function expectAgentToResumeWithSelection(handle: AgentHandle, selection: string) {
+  const finish = await handle.client.waitForFinish(handle.agentId, 240_000);
+  expect(finish.status).toBe("idle");
+  expect(finish.final?.lastError).toBeFalsy();
+  await expect(
+    handle.page
+      .getByTestId("assistant-message")
+      .filter({ hasText: `SELECTED_${selection}` })
+      .last(),
+  ).toBeVisible({ timeout: 30_000 });
+}
+
+async function capturePermissionChoices(page: Page, outputPath: string) {
+  await page.screenshot({ path: outputPath });
+  await expect(page.getByText(QUESTION, { exact: true })).toBeVisible();
+  await expect(page.getByRole("button", { name: "Beta" })).toBeVisible();
+}
+
+test.use({ e2eForkProviders: ["kimi"] });
+
+test.describe("real ACP permission choices", () => {
+  test.setTimeout(600_000);
+
+  test("Kimi renders its question choices and resumes with the selected option", async ({
+    page,
+  }, testInfo) => {
+    const provider = "kimi" satisfies RewindFlowProvider;
+    const cwd = realpathSync(mkdtempSync(path.join(tmpdir(), "paseo-acp-question-kimi-")));
+    let handle: AgentHandle | undefined;
+
+    try {
+      handle = await launchAgent({ page, provider, cwd, mode: "full-access" });
+      await askAgentToOfferChoices(handle);
+      await expectPermissionActions(page, OPTIONS);
+      const screenshotPath = testInfo.outputPath("kimi-permission-choices.png");
+      await capturePermissionChoices(page, screenshotPath);
+      await testInfo.attach("Kimi permission choices", {
+        path: screenshotPath,
+        contentType: "image/png",
+      });
+      await choosePermissionAction(page, "Beta");
+      await expectAgentToResumeWithSelection(handle, "BETA");
+    } finally {
+      await cleanupRewindFlow({ handle, cwd });
+    }
+  });
+});
@@ -42,11 +42,14 @@ const test = metroTest.extend<
    projectPickerFixture: TrackedProjectPickerFixture;
    withWorkspace: WithWorkspace;
  },
-  { e2eWorker: void; e2eWorkerClient: SeedDaemonClient }
+  { e2eForkProviders: string[]; e2eWorker: void; e2eWorkerClient: SeedDaemonClient }
 >({
+  e2eForkProviders: [[], { scope: "worker", option: true }],
  e2eWorker: [
-    async ({}, provide, workerInfo) => {
-      const worker = await startE2EWorker(workerInfo.workerIndex);
+    async ({ e2eForkProviders }, provide, workerInfo) => {
+      const worker = await startE2EWorker(workerInfo.workerIndex, {
+        forkProviders: e2eForkProviders,
+      });
      try {
        await provide();
      } finally {
@@ -1,5 +1,5 @@
 import { execSync } from "node:child_process";
-import { chmod, mkdtemp, rm, writeFile } from "node:fs/promises";
+import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import path from "node:path";
 import { forkPaseoHomeMetadata, resolvePaseoHomePath } from "./paseo-home-fork";
@@ -109,7 +109,7 @@ process.exit(result.status ?? 1);
  return binDir;
 }

-async function applyMetadataFork(targetHome: string): Promise<void> {
+async function applyMetadataFork(targetHome: string, providerIds: string[]): Promise<void> {
  const sourceHome = resolveOptionalHome(process.env.E2E_FORK_PASEO_HOME_FROM);
  if (!sourceHome) return;
  const result = await forkPaseoHomeMetadata({ sourceHome, targetHome });
@@ -117,9 +117,32 @@ async function applyMetadataFork(targetHome: string): Promise<void> {
  process.env.E2E_FORK_TARGET_PASEO_HOME = result.targetHome;
  process.env.E2E_FORK_COPIED_FILES = String(result.copiedFiles);
  process.env.E2E_FORK_COPIED_BYTES = String(result.copiedBytes);
+
+  if (providerIds.length === 0) return;
+
+  const sourceConfig = JSON.parse(
+    await readFile(path.join(result.sourceHome, "config.json"), "utf8"),
+  );
+  const sourceProviders = sourceConfig.agents?.providers ?? {};
+  const providers = Object.fromEntries(
+    providerIds.map((providerId: string) => {
+      const provider = sourceProviders[providerId];
+      if (!provider) {
+        throw new Error(`E2E provider '${providerId}' is not configured in ${result.sourceHome}`);
+      }
+      return [providerId, provider];
+    }),
+  );
+  await writeFile(
+    path.join(targetHome, "config.json"),
+    `${JSON.stringify({ version: 1, agents: { providers } }, null, 2)}\n`,
+  );
 }

-export async function startE2EWorker(workerIndex: number): Promise<E2EWorker> {
+export async function startE2EWorker(
+  workerIndex: number,
+  options: { forkProviders?: string[] } = {},
+): Promise<E2EWorker> {
  const requestedRoot = resolveOptionalHome(process.env.E2E_PASEO_HOME);
  const paseoHome = requestedRoot
    ? path.join(requestedRoot, `worker-${workerIndex}`)
@@ -130,7 +153,7 @@ export async function startE2EWorker(workerIndex: number): Promise<E2EWorker> {
  const serverId = `srv_e2e_worker_${workerIndex}`;

  try {
-    await applyMetadataFork(paseoHome);
+    await applyMetadataFork(paseoHome, options.forkProviders ?? []);
    const daemon = await startIsolatedHostDaemon(serverId, {
      paseoHome,
      preserveHome,
@@ -15,3 +15,16 @@ export async function denyPermission(page: Page): Promise<void> {
  await expect(denyButton).toBeVisible({ timeout: 5_000 });
  await denyButton.click();
 }
+
+export async function expectPermissionActions(page: Page, labels: string[]): Promise<void> {
+  await waitForPermissionPrompt(page, 120_000);
+  const card = page.getByTestId("permission-request-question").first().locator("..");
+  for (const label of labels) {
+    await expect(card.getByRole("button", { name: label })).toBeVisible();
+  }
+}
+
+export async function choosePermissionAction(page: Page, label: string): Promise<void> {
+  const card = page.getByTestId("permission-request-question").first().locator("..");
+  await card.getByRole("button", { name: label }).click();
+}
@@ -7,7 +7,7 @@ import { expectComposerEditable, submitMessage } from "./composer";
 import { connectSeedClient, type SeedDaemonClient } from "./seed-client";
 import { getServerId } from "./server-id";

-export type RewindFlowProvider = "claude" | "codex" | "opencode" | "pi";
+export type RewindFlowProvider = "claude" | "codex" | "kimi" | "opencode" | "pi";
 export type RewindFlowMode = "conversation" | "files" | "both";

 export interface AgentHandle {
@@ -54,6 +54,8 @@ function fullAccessConfig(provider: RewindFlowProvider): ProviderLaunchConfig {
        modeId: "build",
        featureValues: { auto_accept: true },
      };
+    case "kimi":
+      return { provider, modeId: "default", featureValues: { auto_accept: true } };
    case "pi":
      return {
        provider,
@@ -156,6 +158,8 @@ export async function launchAgent(input: {
  mode: "full-access";
  providerConfig?: {
    model?: string;
+    modeId?: string;
+    featureValues?: Record<string, unknown>;
    extra?: { codex?: { features?: { multi_agent_v2?: boolean } } };
  };
 }): Promise<AgentHandle> {
@@ -1166,7 +1166,13 @@ function PermissionActionButton({
    : permissionStyles.optionText;
  const colorMapping = isPrimary ? primaryColorMapping : mutedColorMapping;
  return (
-    <Pressable testID={testID} style={pressableStyle} onPress={handlePress} disabled={isResponding}>
+    <Pressable
+      accessibilityRole="button"
+      testID={testID}
+      style={pressableStyle}
+      onPress={handlePress}
+      disabled={isResponding}
+    >
      {isRespondingAction ? (
        <ThemedLoadingSpinner size="small" uniProps={colorMapping} />
      ) : (
@@ -1517,6 +1523,7 @@ const permissionStyles = StyleSheet.create((theme) => ({
  },
  optionsContainerDesktop: {
    flexDirection: "row",
+    flexWrap: "wrap",
    justifyContent: "flex-start",
    alignItems: "center",
    width: "100%",
@@ -1125,7 +1125,7 @@ describe("ACPAgentSession Zed parity", () => {
      provider: "cursor-acp",
      modeId: "https://agentclientprotocol.com/protocol/session-modes#agent",
    });
-    const events: Array<{ type: string; request?: { id: string } }> = [];
+    const events: AgentStreamEvent[] = [];
    const permissionOptions: PermissionOption[] = [
      { optionId: "allow-once", name: "Allow", kind: "allow_once" },
      { optionId: "reject-once", name: "Reject", kind: "reject_once" },
@@ -1133,7 +1133,7 @@ describe("ACPAgentSession Zed parity", () => {

    asInternals<ACPSessionInternals>(session).sessionId = "session-1";
    session.subscribe((event) => {
-      events.push(event as { type: string; request?: { id: string } });
+      events.push(event);
    });

    const permission = session.requestPermission({
@@ -1150,14 +1150,143 @@ describe("ACPAgentSession Zed parity", () => {
    await Promise.resolve();

    const requested = events.find((event) => event.type === "permission_requested");
-    expect(requested?.request?.id).toEqual(expect.any(String));
+    expect(requested).toMatchObject({
+      type: "permission_requested",
+      request: {
+        actions: [
+          { id: "allow-once", label: "Allow", behavior: "allow" },
+          { id: "reject-once", label: "Reject", behavior: "deny" },
+        ],
+      },
+    });
+    if (requested?.type !== "permission_requested") {
+      throw new Error("Expected permission request");
+    }

-    await session.respondToPermission(requested!.request!.id, { behavior: "allow" });
+    await session.respondToPermission(requested.request.id, { behavior: "allow" });
    await expect(permission).resolves.toEqual({
      outcome: { outcome: "selected", optionId: "allow-once" },
    });
  });

+  test("preserves ACP chooser actions and returns the selected option", async () => {
+    const session = createSessionWithConfig({
+      provider: "kimi-acp",
+      modeId: "https://agentclientprotocol.com/protocol/session-modes#agent",
+    });
+    const events: AgentStreamEvent[] = [];
+
+    asInternals<ACPSessionInternals>(session).sessionId = "session-1";
+    session.subscribe((event) => events.push(event));
+
+    const permission = session.requestPermission({
+      sessionId: "session-1",
+      toolCall: {
+        toolCallId: "question-1",
+        title: "AskUserQuestion",
+        status: "pending",
+        content: [
+          {
+            type: "content",
+            content: {
+              type: "text",
+              text: "Which path should Paseo take?",
+            },
+          },
+        ],
+      },
+      options: [
+        { optionId: "q0_opt_0", name: "Narrow fix", kind: "allow_once" },
+        { optionId: "q0_opt_1", name: "Protocol fix", kind: "allow_once" },
+        { optionId: "q0_skip", name: "Skip", kind: "reject_once" },
+      ],
+    } satisfies RequestPermissionRequest);
+
+    await Promise.resolve();
+
+    const requested = events.find((event) => event.type === "permission_requested");
+    expect(requested).toMatchObject({
+      type: "permission_requested",
+      request: {
+        detail: {
+          type: "plain_text",
+          label: "AskUserQuestion",
+          text: "Which path should Paseo take?",
+        },
+        actions: [
+          { id: "q0_opt_0", label: "Narrow fix", behavior: "allow" },
+          { id: "q0_opt_1", label: "Protocol fix", behavior: "allow" },
+          { id: "q0_skip", label: "Skip", behavior: "deny" },
+        ],
+      },
+    });
+    if (requested?.type !== "permission_requested") {
+      throw new Error("Expected permission request");
+    }
+
+    await session.respondToPermission(requested.request.id, {
+      behavior: "allow",
+      selectedActionId: "q0_opt_1",
+    });
+
+    await expect(permission).resolves.toEqual({
+      outcome: { outcome: "selected", optionId: "q0_opt_1" },
+    });
+  });
+
+  test("preserves ACP permission requests after invalid selected actions", async () => {
+    const session = createSessionWithConfig({ provider: "generic-acp" });
+    const events: AgentStreamEvent[] = [];
+
+    asInternals<ACPSessionInternals>(session).sessionId = "session-1";
+    session.subscribe((event) => events.push(event));
+
+    const permission = session.requestPermission({
+      sessionId: "session-1",
+      toolCall: {
+        toolCallId: "tool-1",
+        title: "Edit file",
+        kind: "edit",
+        status: "pending",
+      },
+      options: [
+        { optionId: "allow-once", name: "Allow", kind: "allow_once" },
+        { optionId: "reject-once", name: "Reject", kind: "reject_once" },
+      ],
+    } satisfies RequestPermissionRequest);
+
+    await Promise.resolve();
+
+    const requested = events.find((event) => event.type === "permission_requested");
+    if (requested?.type !== "permission_requested") {
+      throw new Error("Expected permission request");
+    }
+
+    await expect(
+      session.respondToPermission(requested.request.id, {
+        behavior: "allow",
+        selectedActionId: "",
+      }),
+    ).rejects.toThrow("does not exist");
+    expect(session.getPendingPermissions()).toHaveLength(1);
+
+    await expect(
+      session.respondToPermission(requested.request.id, {
+        behavior: "deny",
+        selectedActionId: "allow-once",
+      }),
+    ).rejects.toThrow("does not match 'deny' behavior");
+    expect(session.getPendingPermissions()).toHaveLength(1);
+
+    await session.respondToPermission(requested.request.id, {
+      behavior: "deny",
+      selectedActionId: "reject-once",
+    });
+    await expect(permission).resolves.toEqual({
+      outcome: { outcome: "selected", optionId: "reject-once" },
+    });
+  });
+
  test("auto-accepts ACP permission requests when the shared feature is enabled", async () => {
    const session = createSessionWithConfig({
      provider: "cursor-acp",
@@ -1179,6 +1308,7 @@ describe("ACPAgentSession Zed parity", () => {
        },
        options: [
          { optionId: "allow-once", name: "Allow", kind: "allow_once" },
+          { optionId: "allow-always", name: "Always allow", kind: "allow_always" },
          { optionId: "reject-once", name: "Reject", kind: "reject_once" },
        ],
      } satisfies RequestPermissionRequest),
@@ -1189,6 +1319,56 @@ describe("ACPAgentSession Zed parity", () => {
    expect(session.getPendingPermissions()).toEqual([]);
  });

+  test("does not auto-accept ACP chooser requests", async () => {
+    const session = createSessionWithConfig({
+      provider: "kimi-acp",
+      featureValues: { auto_accept: true },
+    });
+    const events: AgentStreamEvent[] = [];
+
+    asInternals<ACPSessionInternals>(session).sessionId = "session-1";
+    session.subscribe((event) => events.push(event));
+
+    const permission = session.requestPermission({
+      sessionId: "session-1",
+      toolCall: {
+        toolCallId: "question-1",
+        title: "AskUserQuestion",
+        status: "pending",
+      },
+      options: [
+        { optionId: "q0_opt_0", name: "Narrow fix", kind: "allow_once" },
+        { optionId: "q0_opt_1", name: "Protocol fix", kind: "allow_once" },
+        { optionId: "q0_skip", name: "Skip", kind: "reject_once" },
+      ],
+    } satisfies RequestPermissionRequest);
+
+    await Promise.resolve();
+
+    const requested = events.find((event) => event.type === "permission_requested");
+    expect(requested).toMatchObject({
+      type: "permission_requested",
+      request: {
+        actions: [
+          { id: "q0_opt_0", label: "Narrow fix", behavior: "allow" },
+          { id: "q0_opt_1", label: "Protocol fix", behavior: "allow" },
+          { id: "q0_skip", label: "Skip", behavior: "deny" },
+        ],
+      },
+    });
+    if (requested?.type !== "permission_requested") {
+      throw new Error("Expected permission request");
+    }
+
+    await session.respondToPermission(requested.request.id, {
+      behavior: "allow",
+      selectedActionId: "q0_opt_0",
+    });
+    await expect(permission).resolves.toEqual({
+      outcome: { outcome: "selected", optionId: "q0_opt_0" },
+    });
+  });
+
  test("starts auto-accepting permissions when the shared feature is toggled on", async () => {
    const session = createSessionWithConfig({ provider: "generic-acp" });
    const internals = asInternals<ACPSessionInternals>(session);
@@ -2053,8 +2053,14 @@ export class ACPAgentSession implements AgentSession, ACPClient {
      throw new Error(`No pending permission request with id '${requestId}'`);
    }

-    this.pendingPermissions.delete(requestId);
    const selectedOption = selectPermissionOption(pending.options, response);
+    if (response.selectedActionId !== undefined && !selectedOption) {
+      throw new Error(
+        `ACP permission action '${response.selectedActionId}' does not exist or does not match '${response.behavior}' behavior`,
+      );
+    }
+
+    this.pendingPermissions.delete(requestId);
    pending.resolve(
      selectedOption
        ? {
@@ -2159,7 +2165,9 @@ export class ACPAgentSession implements AgentSession, ACPClient {
  }

  async requestPermission(params: RequestPermissionRequest): Promise<RequestPermissionResponse> {
-    if (isACPAutoAcceptEnabled(this.config)) {
+    const canAutoAccept =
+      isACPAutoAcceptEnabled(this.config) && !isACPChooserRequest(params.options);
+    if (canAutoAccept) {
      const allowOption = selectPermissionOption(params.options, { behavior: "allow" });
      if (allowOption) {
        this.logger.info(
@@ -3441,13 +3449,28 @@ function mapPermissionRequest(
  snapshot: ACPToolSnapshot,
 ): AgentPermissionRequest {
  const kind: AgentPermissionRequestKind = snapshot.kind === "switch_mode" ? "mode" : "tool";
+  const chooserText = isACPChooserRequest(params.options)
+    ? extractToolText(params.toolCall.content)
+    : undefined;
  return {
    id: requestId,
    provider,
    name: snapshot.kind ?? snapshot.title,
    kind,
    title: params.toolCall.title ?? snapshot.title,
-    detail: mapToolDetail(snapshot, new Map()),
+    detail: chooserText
+      ? {
+          type: "plain_text",
+          label: params.toolCall.title ?? snapshot.title,
+          text: chooserText,
+          icon: "wrench",
+        }
+      : mapToolDetail(snapshot, new Map()),
+    actions: params.options.map((option) => ({
+      id: option.optionId,
+      label: option.name,
+      behavior: option.kind.startsWith("allow") ? "allow" : "deny",
+    })),
    metadata: {
      toolCallId: params.toolCall.toolCallId,
      rawRequest: params,
@@ -3460,6 +3483,13 @@ function selectPermissionOption(
  options: PermissionOption[],
  response: AgentPermissionResponse,
 ): PermissionOption | null {
+  if (response.selectedActionId !== undefined) {
+    const selectedOption = options.find((option) => option.optionId === response.selectedActionId);
+    if (!selectedOption) return null;
+    const selectedBehavior = selectedOption.kind.startsWith("allow") ? "allow" : "deny";
+    return selectedBehavior === response.behavior ? selectedOption : null;
+  }
+
  const order =
    response.behavior === "allow"
      ? ["allow_once", "allow_always"]
@@ -3473,6 +3503,20 @@ function selectPermissionOption(
  return null;
 }

+function isACPChooserRequest(options: PermissionOption[]): boolean {
+  const allowKinds = new Set<PermissionOption["kind"]>();
+  for (const option of options) {
+    if (!option.kind.startsWith("allow")) {
+      continue;
+    }
+    if (allowKinds.has(option.kind)) {
+      return true;
+    }
+    allowKinds.add(option.kind);
+  }
+  return false;
+}
+
 function appendTerminalOutput(entry: TerminalEntry, chunk: string): void {
  entry.output += chunk;
  const limit = entry.outputByteLimit;