diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d779d1..97227a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ Repo: https://github.com/openclaw/acpx ### Changes +- Runtime/embedding: add an optional `onPermissionRequest` callback to `AcpRuntimeOptions` and `AcpClientOptions` so embedders can intercept ACP per-call permission requests with their own UI. Returning a decision short-circuits the mode-based resolver; returning `undefined` falls through to it, leaving CLI behavior unchanged. + ### Breaking ### Fixes diff --git a/src/acp/client.ts b/src/acp/client.ts index 74599e1..abd16c5 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -42,7 +42,12 @@ import { PermissionPromptUnavailableError, } from "../errors.js"; import { FileSystemHandlers } from "../filesystem.js"; -import { classifyPermissionDecision, resolvePermissionRequest } from "../permissions.js"; +import { + classifyPermissionDecision, + decisionToResponse, + inferToolKind, + resolvePermissionRequest, +} from "../permissions.js"; import { textPrompt } from "../prompt-content.js"; import { extractRuntimeSessionId } from "../session/runtime-session-id.js"; import { buildSpawnCommandOptions } from "../spawn-command-options.js"; @@ -267,6 +272,7 @@ export class AcpClient { promise: Promise; }; private readonly cancellingSessionIds = new Set(); + private readonly permissionAbortControllers = new Map(); private closing = false; private agentStartedAt?: string; private lastAgentExit?: AgentExitInfo; @@ -769,6 +775,7 @@ export class AcpClient { this.activePrompt = undefined; } this.cancellingSessionIds.delete(sessionId); + this.abortAndDropPermissionSignal(sessionId); this.promptPermissionFailures.delete(sessionId); } } @@ -848,6 +855,7 @@ export class AcpClient { async cancel(sessionId: string): Promise { const connection = this.getConnection(); this.cancellingSessionIds.add(sessionId); + this.abortAndDropPermissionSignal(sessionId); await this.runConnectionRequest(() => connection.cancel({ sessionId, @@ -946,6 +954,10 @@ export class AcpClient { this.suppressReplaySessionUpdateMessages = false; this.activePrompt = undefined; this.cancellingSessionIds.clear(); + for (const controller of this.permissionAbortControllers.values()) { + controller.abort(); + } + this.permissionAbortControllers.clear(); this.promptPermissionFailures.clear(); this.loadedSessionId = undefined; this.initResult = undefined; @@ -1204,6 +1216,33 @@ export class AcpClient { }; } + if (this.options.onPermissionRequest) { + const signal = this.cancellationSignalForSession(params.sessionId); + try { + const decision = await this.options.onPermissionRequest( + { + sessionId: params.sessionId, + raw: params, + inferredKind: inferToolKind(params), + }, + { signal }, + ); + if (decision) { + const response = decisionToResponse(params, decision); + this.recordPermissionDecision(classifyPermissionDecision(params, response)); + return response; + } + } catch (error) { + // Fall through to the mode-based resolver so a host UI error + // doesn't take down the turn. + this.log( + `onPermissionRequest threw, falling through to mode-based resolver: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + } + let response: RequestPermissionResponse; try { response = await resolvePermissionRequest( @@ -1377,6 +1416,23 @@ export class AcpClient { return await this.terminalManager.releaseTerminal(params); } + private cancellationSignalForSession(sessionId: string): AbortSignal { + let controller = this.permissionAbortControllers.get(sessionId); + if (!controller) { + controller = new AbortController(); + this.permissionAbortControllers.set(sessionId, controller); + } + return controller.signal; + } + + private abortAndDropPermissionSignal(sessionId: string): void { + const controller = this.permissionAbortControllers.get(sessionId); + if (controller) { + controller.abort(); + this.permissionAbortControllers.delete(sessionId); + } + } + private recordPermissionDecision(decision: "approved" | "denied" | "cancelled"): void { this.permissionStats.requested += 1; if (decision === "approved") { diff --git a/src/permissions.ts b/src/permissions.ts index 79c0b74..0db9613 100644 --- a/src/permissions.ts +++ b/src/permissions.ts @@ -6,7 +6,11 @@ import { } from "@agentclientprotocol/sdk"; import { PermissionPromptUnavailableError } from "./errors.js"; import { promptForPermission } from "./permission-prompt.js"; -import type { NonInteractivePermissionPolicy, PermissionMode } from "./types.js"; +import type { + AcpPermissionDecision, + NonInteractivePermissionPolicy, + PermissionMode, +} from "./types.js"; type PermissionDecision = "approved" | "denied" | "cancelled"; const PERMISSION_MODE_RANK: Record = { @@ -36,7 +40,7 @@ function pickOption( return undefined; } -function inferToolKind(params: RequestPermissionRequest): ToolKind | undefined { +export function inferToolKind(params: RequestPermissionRequest): ToolKind | undefined { if (params.toolCall.kind) { return params.toolCall.kind; } @@ -151,6 +155,27 @@ export async function resolvePermissionRequest( return cancelled(); } +const DECISION_FALLBACK_ORDER: Record< + Exclude, + PermissionOption["kind"][] +> = { + allow_once: ["allow_once", "allow_always"], + allow_always: ["allow_always", "allow_once"], + reject_once: ["reject_once", "reject_always"], + reject_always: ["reject_always", "reject_once"], +}; + +export function decisionToResponse( + params: RequestPermissionRequest, + decision: AcpPermissionDecision, +): RequestPermissionResponse { + if (decision.outcome === "cancel") { + return cancelled(); + } + const matched = pickOption(params.options ?? [], DECISION_FALLBACK_ORDER[decision.outcome]); + return matched ? selected(matched.optionId) : cancelled(); +} + export function classifyPermissionDecision( params: RequestPermissionRequest, response: RequestPermissionResponse, diff --git a/src/runtime.ts b/src/runtime.ts index fe7acdd..2c5a26e 100644 --- a/src/runtime.ts +++ b/src/runtime.ts @@ -29,6 +29,8 @@ export { export type { AcpAgentRegistry, AcpFileSessionStoreOptions, + AcpPermissionDecision, + AcpPermissionRequest, AcpRuntime, AcpRuntimeCapabilities, AcpRuntimeDoctorReport, diff --git a/src/runtime/engine/connected-session.ts b/src/runtime/engine/connected-session.ts index a0f10ab..238f53a 100644 --- a/src/runtime/engine/connected-session.ts +++ b/src/runtime/engine/connected-session.ts @@ -3,6 +3,8 @@ import { AcpClient } from "../../acp/client.js"; import { withInterrupt } from "../../async-control.js"; import { absolutePath, isoNow } from "../../session/persistence.js"; import type { + AcpPermissionDecision, + AcpPermissionRequest, AuthPolicy, McpServer, NonInteractivePermissionPolicy, @@ -39,6 +41,10 @@ export type WithConnectedSessionOptions = { mcpServers?: McpServer[]; permissionMode?: PermissionMode; nonInteractivePermissions?: NonInteractivePermissionPolicy; + onPermissionRequest?: ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, + ) => Promise; authCredentials?: Record; authPolicy?: AuthPolicy; terminal?: boolean; @@ -90,6 +96,7 @@ export async function withConnectedSession( mcpServers: options.mcpServers, permissionMode: options.permissionMode ?? "approve-reads", nonInteractivePermissions: options.nonInteractivePermissions, + onPermissionRequest: options.onPermissionRequest, authCredentials: options.authCredentials, authPolicy: options.authPolicy, terminal: options.terminal, @@ -102,6 +109,7 @@ export async function withConnectedSession( mcpServers: options.mcpServers, permissionMode: options.permissionMode ?? "approve-reads", nonInteractivePermissions: options.nonInteractivePermissions, + onPermissionRequest: options.onPermissionRequest, authCredentials: options.authCredentials, authPolicy: options.authPolicy, terminal: options.terminal, diff --git a/src/runtime/engine/manager.ts b/src/runtime/engine/manager.ts index 73ecf05..5c2345c 100644 --- a/src/runtime/engine/manager.ts +++ b/src/runtime/engine/manager.ts @@ -343,6 +343,7 @@ export class AcpRuntimeManager { mcpServers: [...(this.options.mcpServers ?? [])], permissionMode: this.options.permissionMode, nonInteractivePermissions: this.options.nonInteractivePermissions, + onPermissionRequest: this.options.onPermissionRequest, verbose: this.options.verbose, timeoutMs: this.options.timeoutMs, resumePolicy: resumePolicyForSessionMode(sessionMode), @@ -385,6 +386,7 @@ export class AcpRuntimeManager { mcpServers: [...(this.options.mcpServers ?? [])], permissionMode: this.options.permissionMode, nonInteractivePermissions: this.options.nonInteractivePermissions, + onPermissionRequest: this.options.onPermissionRequest, verbose: this.options.verbose, }); let keepClientOpen = false; @@ -532,6 +534,7 @@ export class AcpRuntimeManager { mcpServers: [...(this.options.mcpServers ?? [])], permissionMode: this.options.permissionMode, nonInteractivePermissions: this.options.nonInteractivePermissions, + onPermissionRequest: this.options.onPermissionRequest, verbose: this.options.verbose, }); const runtimeClient = client; @@ -897,6 +900,7 @@ export class AcpRuntimeManager { mcpServers: [...(this.options.mcpServers ?? [])], permissionMode: this.options.permissionMode, nonInteractivePermissions: this.options.nonInteractivePermissions, + onPermissionRequest: this.options.onPermissionRequest, verbose: this.options.verbose, }); diff --git a/src/runtime/public/contract.ts b/src/runtime/public/contract.ts index 4ce602a..3579ad3 100644 --- a/src/runtime/public/contract.ts +++ b/src/runtime/public/contract.ts @@ -1,10 +1,14 @@ import type { + AcpPermissionDecision, + AcpPermissionRequest, McpServer, NonInteractivePermissionPolicy, PermissionMode, SessionRecord, } from "../../types.js"; +export type { AcpPermissionDecision, AcpPermissionRequest } from "../../types.js"; + export type AcpRuntimePromptMode = "prompt" | "steer"; export type AcpRuntimeSessionMode = "persistent" | "oneshot"; @@ -195,6 +199,10 @@ export type AcpRuntimeOptions = { timeoutMs?: number; probeAgent?: string; verbose?: boolean; + onPermissionRequest?: ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, + ) => Promise; }; export type AcpFileSessionStoreOptions = { diff --git a/src/types.ts b/src/types.ts index b41cdad..2d1a70c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,14 +2,29 @@ import type { AgentCapabilities, AnyMessage, McpServer, + RequestPermissionRequest, SessionNotification, SessionConfigOption, SetSessionConfigOptionResponse, StopReason, + ToolKind, } from "@agentclientprotocol/sdk"; export type { McpServer, SessionNotification } from "@agentclientprotocol/sdk"; import type { PromptInput } from "./prompt-content.js"; +export type AcpPermissionRequest = { + sessionId: string; + raw: RequestPermissionRequest; + inferredKind: ToolKind | undefined; +}; + +export type AcpPermissionDecision = + | { outcome: "allow_once" } + | { outcome: "allow_always" } + | { outcome: "reject_once" } + | { outcome: "reject_always" } + | { outcome: "cancel" }; + export const EXIT_CODES = { SUCCESS: 0, ERROR: 1, @@ -180,6 +195,10 @@ export type AcpClientOptions = { onAcpOutputMessage?: (direction: AcpMessageDirection, message: AcpJsonRpcMessage) => void; onSessionUpdate?: (notification: SessionNotification) => void; onClientOperation?: (operation: ClientOperation) => void; + onPermissionRequest?: ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, + ) => Promise; }; export const SESSION_RECORD_SCHEMA = "acpx.session.v1" as const; diff --git a/test/client.test.ts b/test/client.test.ts index 34ae5cf..849e40c 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -357,6 +357,114 @@ test("AcpClient handlePermissionRequest records approved decisions", async () => }); }); +test("AcpClient onPermissionRequest decision short-circuits the mode-based resolver", async () => { + let callbackInvocations = 0; + const client = makeClient({ + permissionMode: "approve-reads", + nonInteractivePermissions: "deny", + onPermissionRequest: async (req) => { + callbackInvocations += 1; + assert.equal(req.sessionId, "session-cb-1"); + assert.equal(req.inferredKind, "edit"); + return { outcome: "allow_once" }; + }, + }); + + const response = await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-1", "edit"), + ); + + assert.deepEqual(response, { + outcome: { + outcome: "selected", + optionId: "allow", + }, + }); + assert.equal(callbackInvocations, 1); + assert.deepEqual(client.getPermissionStats(), { + requested: 1, + approved: 1, + denied: 0, + cancelled: 0, + }); +}); + +test("AcpClient onPermissionRequest returning undefined falls through to mode-based resolver", async () => { + let callbackInvocations = 0; + const client = makeClient({ + permissionMode: "approve-all", + onPermissionRequest: async () => { + callbackInvocations += 1; + return undefined; + }, + }); + + const response = await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-2", "edit"), + ); + + assert.deepEqual(response, { + outcome: { + outcome: "selected", + optionId: "allow", + }, + }); + assert.equal(callbackInvocations, 1); + assert.deepEqual(client.getPermissionStats(), { + requested: 1, + approved: 1, + denied: 0, + cancelled: 0, + }); +}); + +test("AcpClient onPermissionRequest throws fall through to mode-based resolver", async () => { + let callbackInvocations = 0; + const client = makeClient({ + permissionMode: "approve-all", + onPermissionRequest: async () => { + callbackInvocations += 1; + throw new Error("UI exploded"); + }, + }); + + const response = await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-3", "edit"), + ); + + assert.deepEqual(response, { + outcome: { + outcome: "selected", + optionId: "allow", + }, + }); + assert.equal(callbackInvocations, 1); +}); + +test("AcpClient onPermissionRequest receives an AbortSignal that fires on session cancel", async () => { + let observedSignal: AbortSignal | undefined; + const client = makeClient({ + permissionMode: "approve-all", + onPermissionRequest: async (_req, ctx) => { + observedSignal = ctx.signal; + return { outcome: "allow_once" }; + }, + }); + + await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-4", "edit"), + ); + + assert(observedSignal instanceof AbortSignal); + assert.equal(observedSignal?.aborted, false); + + const internals = asInternals(client) as unknown as { + abortAndDropPermissionSignal: (sessionId: string) => void; + }; + internals.abortAndDropPermissionSignal("session-cb-4"); + assert.equal(observedSignal?.aborted, true); +}); + test("AcpClient client-method permission errors update permission stats", async () => { const client = makeClient(); const internals = asInternals(client); diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 71e3f08..077e758 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -2,7 +2,12 @@ import assert from "node:assert/strict"; import test from "node:test"; import type { RequestPermissionRequest } from "@agentclientprotocol/sdk"; import { PermissionPromptUnavailableError } from "../src/errors.js"; -import { classifyPermissionDecision, resolvePermissionRequest } from "../src/permissions.js"; +import { + classifyPermissionDecision, + decisionToResponse, + inferToolKind, + resolvePermissionRequest, +} from "../src/permissions.js"; import { withMockedReadline, withTtyState } from "./tty-test-helpers.js"; const BASE_OPTIONS = [ @@ -192,3 +197,66 @@ test("classifyPermissionDecision maps selected outcomes to approved, denied, or "cancelled", ); }); + +test("decisionToResponse allow_once prefers allow_once over allow_always", () => { + const request = makeRequestWithTitle("tool", "edit", [ + { optionId: "always", kind: "allow_always" }, + { optionId: "once", kind: "allow_once" }, + { optionId: "reject", kind: "reject_once" }, + ]); + const response = decisionToResponse(request, { outcome: "allow_once" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "once" } }); +}); + +test("decisionToResponse allow_always prefers allow_always over allow_once", () => { + const request = makeRequestWithTitle("tool", "edit", [ + { optionId: "once", kind: "allow_once" }, + { optionId: "always", kind: "allow_always" }, + { optionId: "reject", kind: "reject_once" }, + ]); + const response = decisionToResponse(request, { outcome: "allow_always" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); +}); + +test("decisionToResponse allow_once falls back to allow_always when allow_once is missing", () => { + const request = makeRequestWithTitle("tool", "edit", [ + { optionId: "always", kind: "allow_always" }, + { optionId: "reject", kind: "reject_once" }, + ]); + const response = decisionToResponse(request, { outcome: "allow_once" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); +}); + +test("decisionToResponse reject_once falls back to reject_always", () => { + const onlyAlways = makeRequestWithTitle("tool", "edit", [ + { optionId: "allow", kind: "allow_once" }, + { optionId: "reject-always", kind: "reject_always" }, + ]); + assert.deepEqual(decisionToResponse(onlyAlways, { outcome: "reject_once" }), { + outcome: { outcome: "selected", optionId: "reject-always" }, + }); +}); + +test("decisionToResponse cancels when no matching option exists", () => { + const request = makeRequestWithTitle("tool", "edit", [{ optionId: "allow", kind: "allow_once" }]); + assert.deepEqual(decisionToResponse(request, { outcome: "reject_once" }), { + outcome: { outcome: "cancelled" }, + }); +}); + +test("decisionToResponse cancel always returns cancelled", () => { + const request = makeRequestWithTitle("tool", "edit", [ + { optionId: "allow", kind: "allow_once" }, + { optionId: "reject", kind: "reject_once" }, + ]); + assert.deepEqual(decisionToResponse(request, { outcome: "cancel" }), { + outcome: { outcome: "cancelled" }, + }); +}); + +test("inferToolKind classifies titles when toolCall.kind is missing", () => { + assert.equal(inferToolKind(makeRequest("edit")), "edit"); + assert.equal(inferToolKind(makeRequestWithTitle("patch: foo.ts", undefined)), "edit"); + assert.equal(inferToolKind(makeRequestWithTitle("cat README", undefined)), "read"); + assert.equal(inferToolKind(makeRequestWithTitle("totally unknown", undefined)), "other"); +});