From 048b82f2a7b3ecc9b62bc5b817729f4ca2741863 Mon Sep 17 00:00:00 2001 From: Mohamed Boudra Date: Sat, 1 Aug 2026 20:56:19 +0200 Subject: [PATCH] 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 --- docs/providers.md | 2 + .../acp-permission-choices.real.spec.ts | 70 +++++++ packages/app/e2e/support/fixtures.ts | 9 +- .../app/e2e/support/helpers/e2e-worker.ts | 31 ++- .../app/e2e/support/helpers/permissions.ts | 13 ++ .../app/e2e/support/helpers/rewind-flow.ts | 6 +- packages/app/src/agent-stream/view.tsx | 9 +- .../server/agent/providers/acp-agent.test.ts | 188 +++++++++++++++++- .../src/server/agent/providers/acp-agent.ts | 50 ++++- 9 files changed, 362 insertions(+), 16 deletions(-) create mode 100644 packages/app/e2e/browser/acp-permission-choices.real.spec.ts diff --git a/docs/providers.md b/docs/providers.md index 354764782..92311c463 100644 --- a/docs/providers.md +++ b/docs/providers.md @@ -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. diff --git a/packages/app/e2e/browser/acp-permission-choices.real.spec.ts b/packages/app/e2e/browser/acp-permission-choices.real.spec.ts new file mode 100644 index 000000000..364a23d99 --- /dev/null +++ b/packages/app/e2e/browser/acp-permission-choices.real.spec.ts @@ -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 { + 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_, replacing 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 }); + } + }); +}); diff --git a/packages/app/e2e/support/fixtures.ts b/packages/app/e2e/support/fixtures.ts index a4ee90049..e8352a32e 100644 --- a/packages/app/e2e/support/fixtures.ts +++ b/packages/app/e2e/support/fixtures.ts @@ -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 { diff --git a/packages/app/e2e/support/helpers/e2e-worker.ts b/packages/app/e2e/support/helpers/e2e-worker.ts index 25366a808..13c496d66 100644 --- a/packages/app/e2e/support/helpers/e2e-worker.ts +++ b/packages/app/e2e/support/helpers/e2e-worker.ts @@ -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 { +async function applyMetadataFork(targetHome: string, providerIds: string[]): Promise { 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 { 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 { +export async function startE2EWorker( + workerIndex: number, + options: { forkProviders?: string[] } = {}, +): Promise { 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 { const serverId = `srv_e2e_worker_${workerIndex}`; try { - await applyMetadataFork(paseoHome); + await applyMetadataFork(paseoHome, options.forkProviders ?? []); const daemon = await startIsolatedHostDaemon(serverId, { paseoHome, preserveHome, diff --git a/packages/app/e2e/support/helpers/permissions.ts b/packages/app/e2e/support/helpers/permissions.ts index 977e371bf..ee4211bb3 100644 --- a/packages/app/e2e/support/helpers/permissions.ts +++ b/packages/app/e2e/support/helpers/permissions.ts @@ -15,3 +15,16 @@ export async function denyPermission(page: Page): Promise { await expect(denyButton).toBeVisible({ timeout: 5_000 }); await denyButton.click(); } + +export async function expectPermissionActions(page: Page, labels: string[]): Promise { + 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 { + const card = page.getByTestId("permission-request-question").first().locator(".."); + await card.getByRole("button", { name: label }).click(); +} diff --git a/packages/app/e2e/support/helpers/rewind-flow.ts b/packages/app/e2e/support/helpers/rewind-flow.ts index a567e161d..f9e71fd19 100644 --- a/packages/app/e2e/support/helpers/rewind-flow.ts +++ b/packages/app/e2e/support/helpers/rewind-flow.ts @@ -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; extra?: { codex?: { features?: { multi_agent_v2?: boolean } } }; }; }): Promise { diff --git a/packages/app/src/agent-stream/view.tsx b/packages/app/src/agent-stream/view.tsx index c54555141..8454b1b93 100644 --- a/packages/app/src/agent-stream/view.tsx +++ b/packages/app/src/agent-stream/view.tsx @@ -1166,7 +1166,13 @@ function PermissionActionButton({ : permissionStyles.optionText; const colorMapping = isPrimary ? primaryColorMapping : mutedColorMapping; return ( - + {isRespondingAction ? ( ) : ( @@ -1517,6 +1523,7 @@ const permissionStyles = StyleSheet.create((theme) => ({ }, optionsContainerDesktop: { flexDirection: "row", + flexWrap: "wrap", justifyContent: "flex-start", alignItems: "center", width: "100%", 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 a5677b9f7..b82e8e215 100644 --- a/packages/server/src/server/agent/providers/acp-agent.test.ts +++ b/packages/server/src/server/agent/providers/acp-agent.test.ts @@ -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(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(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(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(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(session); diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index e687d5114..189218dac 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -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 { - 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(); + 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;