diff --git a/packages/server/src/server/agent/providers/acp-agent.ts b/packages/server/src/server/agent/providers/acp-agent.ts index 339c491e46..0f7411d8f7 100644 --- a/packages/server/src/server/agent/providers/acp-agent.ts +++ b/packages/server/src/server/agent/providers/acp-agent.ts @@ -643,6 +643,7 @@ interface PendingPermission { resolve: (response: RequestPermissionResponse) => void; reject: (error: Error) => void; turnId: string | null; + toolCallId: string; } interface PendingUserMessage { @@ -2644,6 +2645,7 @@ export class ACPAgentSession implements AgentSession, ACPClient { resolve, reject, turnId: this.activeForegroundTurnId, + toolCallId: params.toolCall.toolCallId, }); }); @@ -3207,6 +3209,9 @@ export class ACPAgentSession implements AgentSession, ACPClient { snapshot = this.toolSnapshotTransformer(snapshot); } this.toolCalls.set(toolCallId, snapshot); + if (update.status === "completed" || update.status === "failed") { + this.cancelPendingPermissions((pending) => pending.toolCallId === toolCallId); + } return [this.wrapTimeline(mapToolSnapshotToTimeline(snapshot, this.terminalEntries))]; } @@ -3414,11 +3419,11 @@ export class ACPAgentSession implements AgentSession, ACPClient { event: Extract, ): void { this.deliverTranslatedEvents(this.flushPendingUserMessage()); - // A turn that ends without completing leaves no way to answer its open - // permission requests, so tell the agent they were cancelled. - if (event.type !== "turn_completed") { - this.cancelPendingPermissions(); - } + // Failed and canceled turns clear every request in the manager. Answer the + // same set here so an outside-turn request cannot keep the provider blocked. + this.cancelPendingPermissions( + (pending) => event.type !== "turn_completed" || pending.turnId === event.turnId, + ); this.activeForegroundTurnId = null; this.fallbackAssistantMessageId = null; if (this.submittedUserMessageTurnId === event.turnId) { @@ -3427,11 +3432,21 @@ export class ACPAgentSession implements AgentSession, ACPClient { this.pushEvent(event); } - private cancelPendingPermissions(): void { - for (const pending of this.pendingPermissions.values()) { + private cancelPendingPermissions( + matches: (pending: PendingPermission) => boolean = () => true, + ): void { + for (const [requestId, pending] of this.pendingPermissions) { + if (!matches(pending)) continue; + this.pendingPermissions.delete(requestId); pending.resolve({ outcome: { outcome: "cancelled" } }); + this.pushEvent({ + type: "permission_resolved", + provider: this.provider, + requestId, + resolution: { behavior: "deny", message: "Permission request canceled" }, + turnId: pending.turnId ?? undefined, + }); } - this.pendingPermissions.clear(); } private emitBootstrapThreadEvent(): void { diff --git a/packages/server/src/server/agent/providers/generic-acp-agent.permission-turn-failure.test.ts b/packages/server/src/server/agent/providers/generic-acp-agent.permission-turn-failure.test.ts index aaa6a5110e..f64c719c6d 100644 --- a/packages/server/src/server/agent/providers/generic-acp-agent.permission-turn-failure.test.ts +++ b/packages/server/src/server/agent/providers/generic-acp-agent.permission-turn-failure.test.ts @@ -5,6 +5,7 @@ import path from "node:path"; import { describe, expect, test } from "vitest"; import { createTestLogger } from "../../../test-utils/test-logger.js"; +import { AgentManager } from "../agent-manager.js"; import type { AgentSession, AgentStreamEvent } from "../agent-sdk-types.js"; import { GenericACPAgentClient } from "./generic-acp-agent.js"; @@ -12,6 +13,7 @@ describe("GenericACPAgentClient permission requests when a turn ends early", () test.each([ { ending: "fail", terminalEvent: "turn_failed" }, { ending: "cancel", terminalEvent: "turn_canceled" }, + { ending: "end_turn", terminalEvent: "turn_completed" }, ] as const)( "answers a permission request left open by a $terminalEvent turn with cancelled", async ({ ending, terminalEvent }) => { @@ -25,6 +27,16 @@ describe("GenericACPAgentClient permission requests when a turn ends early", () await waitForEvent(events, terminalEvent); expect(hasEvent(events, "permission_requested")).toBe(true); expect(session.getPendingPermissions()).toEqual([]); + const requested = permissionRequests(events)[0]!; + expect(resolvedEvents(events)).toEqual([ + { + type: "permission_resolved", + provider: "acp", + requestId: requested.request.id, + resolution: { behavior: "deny", message: "Permission request canceled" }, + turnId: requested.turnId, + }, + ]); const replyStart = events.length; await session.startTurn("turn two"); @@ -39,6 +51,84 @@ describe("GenericACPAgentClient permission requests when a turn ends early", () }); }, ); + + test.each(["completed", "failed"] as const)( + "cancels only the matching permission when its tool becomes %s", + async (status) => { + await withFakeACPAgent(status, async (command, cwd) => { + const client = new GenericACPAgentClient({ logger: createTestLogger(), command }); + const session = await client.createSession({ provider: "acp", cwd }); + try { + const events = collectEvents(session); + await session.startTurn("turn one"); + await waitForToolUpdate(events); + expect(pendingToolIds(session)).toEqual(["tool-2"]); + expect(resolvedEvents(events)).toEqual([ + expect.objectContaining({ requestId: permissionRequests(events)[0]!.request.id }), + ]); + } finally { + await session.close(); + } + }); + }, + ); + + test("keeps a permission raised outside a turn answerable after another turn completes", async () => { + await withFakeACPAgent("outside_turn", async (command, cwd) => { + const client = new GenericACPAgentClient({ logger: createTestLogger(), command }); + const session = await client.createSession({ provider: "acp", cwd }); + try { + const outside = session.getPendingPermissions(); + expect(pendingToolIds(session)).toEqual(["tool-outside"]); + const events = collectEvents(session); + await session.startTurn("turn one"); + await waitForEvent(events, "turn_completed"); + expect(session.getPendingPermissions()).toEqual(outside); + await session.respondToPermission(outside[0]!.id, { behavior: "allow" }); + expect(session.getPendingPermissions()).toEqual([]); + } finally { + await session.close(); + } + }); + }); + + test.each(["fail", "cancel"] as const)( + "settles an outside-turn permission that the manager clears when a turn ends with %s", + async (ending) => { + await withFakeACPAgent(`outside_${ending}`, async (command, cwd) => { + const logger = createTestLogger(); + const client = new GenericACPAgentClient({ + logger, + command, + waitForInitialCommands: false, + }); + const manager = new AgentManager({ clients: { acp: client }, logger }); + const agent = await manager.createAgent({ provider: "acp", cwd }, undefined, { + workspaceId: undefined, + }); + try { + await waitForManagedPermission(manager, agent.id); + if (ending === "fail") { + await expect(manager.runAgent(agent.id, "turn one")).rejects.toThrow( + "turn failed with a permission open", + ); + } else { + await expect(manager.runAgent(agent.id, "turn one")).resolves.toMatchObject({ + canceled: true, + }); + } + expect(manager.getPendingPermissions(agent.id)).toEqual([]); + const result = await manager.runAgent(agent.id, "turn two"); + expect(result.finalText).toBe( + 'perm-outside answered: {"outcome":{"outcome":"cancelled"}}', + ); + } finally { + await manager.closeAgent(agent.id); + await manager.flush(); + } + }); + }, + ); }); function collectEvents(session: AgentSession): AgentStreamEvent[] { @@ -47,6 +137,10 @@ function collectEvents(session: AgentSession): AgentStreamEvent[] { return events; } +async function waitForManagedPermission(manager: AgentManager, agentId: string): Promise { + await expect.poll(() => manager.getPendingPermissions(agentId).length).toBe(1); +} + function hasEvent( events: AgentStreamEvent[], type: AgentStreamEvent["type"], @@ -77,8 +171,32 @@ function assistantText(events: AgentStreamEvent[]): string { .join(""); } +function permissionRequests(events: AgentStreamEvent[]) { + return events.filter((event) => event.type === "permission_requested"); +} + +function resolvedEvents(events: AgentStreamEvent[]) { + return events.filter((event) => event.type === "permission_resolved"); +} + +function pendingToolIds(session: AgentSession) { + return session.getPendingPermissions().map((request) => request.metadata?.toolCallId); +} + +async function waitForToolUpdate(events: AgentStreamEvent[]): Promise { + await expect.poll(() => assistantText(events)).toBe("tool terminal"); +} + async function withFakeACPAgent( - ending: "fail" | "cancel", + ending: + | "fail" + | "cancel" + | "end_turn" + | "completed" + | "failed" + | "outside_turn" + | "outside_fail" + | "outside_cancel", run: (command: [string, ...string[]], cwd: string) => Promise, ): Promise { const testDir = await mkdtemp(path.join(tmpdir(), "paseo-acp-permission-turn-failure-")); @@ -91,8 +209,7 @@ async function withFakeACPAgent( } } -// Turn one asks for permission, then fails or is cancelled while the request is -// still open. +// Turn one ends while its permission request is still open. // Later turns report whether that request was ever answered. const fakeACPAgentScript = ` const readline = require("node:readline"); @@ -101,15 +218,34 @@ const ending = process.argv[2]; const rl = readline.createInterface({ input: process.stdin }); let promptCount = 0; let permissionAnswer = null; +let outsidePermissionAnswer = null; function write(message) { process.stdout.write(JSON.stringify({ jsonrpc: "2.0", ...message }) + "\\n"); } +function ask(sessionId, requestId, toolCallId) { + const toolCall = { toolCallId, title: "echo hello", kind: "execute", status: "pending" }; + write({ method: "session/update", params: { sessionId, update: { sessionUpdate: "tool_call", ...toolCall } } }); + write({ + id: requestId, + method: "session/request_permission", + params: { + sessionId, + toolCall, + options: [ + { optionId: "allow", name: "Allow once", kind: "allow_once" }, + { optionId: "reject", name: "Reject", kind: "reject_once" }, + ], + }, + }); +} + rl.on("line", (line) => { const message = JSON.parse(line); - if (message.method === undefined && message.id === "perm-1") { - permissionAnswer = message.result ?? { error: message.error }; + if (message.method === undefined) { + if (message.id === "perm-1") permissionAnswer = message.result ?? { error: message.error }; + if (message.id === "perm-outside") outsidePermissionAnswer = message.result ?? { error: message.error }; return; } if (message.method === "initialize") { @@ -117,6 +253,7 @@ rl.on("line", (line) => { return; } if (message.method === "session/new") { + if (ending.startsWith("outside_")) ask("session-1", "perm-outside", "tool-outside"); write({ id: message.id, result: { sessionId: "session-1" } }); return; } @@ -124,35 +261,32 @@ rl.on("line", (line) => { const sessionId = message.params.sessionId; promptCount += 1; if (promptCount === 1) { - const toolCall = { toolCallId: "tool-1", title: "echo hello", kind: "execute", status: "pending" }; - write({ method: "session/update", params: { sessionId, update: { sessionUpdate: "tool_call", ...toolCall } } }); - write({ - id: "perm-1", - method: "session/request_permission", - params: { - sessionId, - toolCall, - options: [ - { optionId: "allow", name: "Allow once", kind: "allow_once" }, - { optionId: "reject", name: "Reject", kind: "reject_once" }, - ], - }, - }); + ask(sessionId, "perm-1", "tool-1"); + if (ending === "completed" || ending === "failed") { + ask(sessionId, "perm-2", "tool-2"); + write({ method: "session/update", params: { sessionId, update: { sessionUpdate: "tool_call_update", toolCallId: "tool-1", status: ending } } }); + write({ method: "session/update", params: { sessionId, update: { sessionUpdate: "agent_message_chunk", content: { type: "text", text: "tool terminal" } } } }); + return; + } setTimeout(() => { - if (ending === "cancel") { + if (ending === "cancel" || ending === "outside_cancel") { write({ id: message.id, result: { stopReason: "cancelled" } }); + } else if (ending === "end_turn" || ending === "outside_turn") { + write({ id: message.id, result: { stopReason: "end_turn" } }); } else { write({ id: message.id, error: { code: -32603, message: "turn failed with a permission open" } }); } }, 50); return; } - const state = permissionAnswer === null ? "unanswered" : "answered: " + JSON.stringify(permissionAnswer); + const answer = ending.startsWith("outside_") ? outsidePermissionAnswer : permissionAnswer; + const requestId = ending.startsWith("outside_") ? "perm-outside" : "perm-1"; + const state = answer === null ? "unanswered" : "answered: " + JSON.stringify(answer); write({ method: "session/update", params: { sessionId, - update: { sessionUpdate: "agent_message_chunk", content: { type: "text", text: "perm-1 " + state } }, + update: { sessionUpdate: "agent_message_chunk", content: { type: "text", text: requestId + " " + state } }, }, }); write({ id: message.id, result: { stopReason: "end_turn" } });