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;