mirror of
https://github.com/getpaseo/paseo.git
synced 2026-11-02 22:52:58 +00:00
fix(acp): clear abandoned approvals without leaving providers blocked (#8548)
* fix(acp): clear permission requests a finished turn abandoned An ACP agent can give up on a permission request itself and end its turn normally. Paseo kept that request pending, so the approval card stayed up with nothing listening, and the next prompt reported the agent as waiting. Cancel a turn's open requests whenever that turn ends, cancel a request when its tool reaches a terminal status, and publish the resolved event in both cases. Requests raised outside a turn stay answerable. Closes #7664 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * fix(acp): keep permission cleanup consistent after failed turns Failed and canceled turns clear every permission in the agent manager. Answer that same set in the ACP session so a request raised outside the foreground turn cannot leave the provider waiting after its approval disappears. Co-authored-by: 3ae3ae <ogyshjsm@naver.com> --------- Co-authored-by: 3ae3ae <ogyshjsm@naver.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
3ae3ae
parent
27caf913f6
commit
555818f5f7
@@ -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<AgentStreamEvent, { type: "turn_completed" | "turn_failed" | "turn_canceled" }>,
|
||||
): 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 {
|
||||
|
||||
+156
-22
@@ -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<void> {
|
||||
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<void> {
|
||||
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<void>,
|
||||
): Promise<void> {
|
||||
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" } });
|
||||
|
||||
Reference in New Issue
Block a user