From 4c1d187294b2905c86884c1eb48a13f1475252e6 Mon Sep 17 00:00:00 2001 From: DaniAkash Date: Wed, 6 May 2026 19:31:39 +0530 Subject: [PATCH 1/4] feat: add onPermissionRequest callback for host-driven per-call gating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an optional `onPermissionRequest` hook to `AcpRuntimeOptions` and `AcpClientOptions`. When set, every per-call permission request the agent emits is routed through the host's callback first; returning a decision short-circuits the existing mode-based resolver, returning `undefined` (or throwing) falls through to it. Existing CLI users and programmatic consumers see zero behavior change — the option is undefined by default, and the mode-based path is preserved verbatim. Hosts that want to render approve/deny UI can now opt in without reaching into acpx internals. The wire shape: type AcpPermissionRequest = { sessionId: string; raw: RequestPermissionRequest; // full ACP request, untouched inferredKind: ToolKind | "other"; // best-effort classification } type AcpPermissionDecision = | { outcome: "approve_once" } | { outcome: "approve_always" } | { outcome: "deny" } | { outcome: "cancel" }; A new `decisionToResponse` helper in `src/permissions.ts` maps the decision to the right ACP `RequestPermissionResponse` shape based on the options the agent advertised — hosts never see `optionId` strings. The existing `inferToolKind` is now exported so hosts can render kind-aware UI without re-implementing the keyword classifier. The client gate in `handlePermissionRequest` runs the callback before the existing mode-based resolver, exposes a per-session `AbortSignal` the callback can honor, and falls through cleanly on `undefined` / thrown errors. The signal aborts when the session is cancelled so in-flight host UI can short-circuit. Throws are logged and don't take down the turn. Driving consumer: `acpx-ai-provider` needs this to allow approve / deny options on the Vercel AI SDK so it can be integrated with UI components built with the Vercel AI SDK (https://www.npmjs.com/package/acpx-ai-provider). Other tools can leverage it the same way. Tests: 5 new unit cases in `test/permissions.test.ts` cover `decisionToResponse` option-mapping; 4 new integration cases in `test/client.test.ts` cover the gate (decision honored / undefined fallthrough / thrown error fallthrough / abort signal propagation). `pnpm run check` passes locally — 613 tests, coverage 84.16% lines / 78.46% branches / 88.98% functions, all above thresholds. --- src/acp/client.ts | 71 ++++++++++++++- src/permissions.ts | 43 ++++++++- src/runtime.ts | 3 + src/runtime/engine/connected-session.ts | 4 + src/runtime/engine/manager.ts | 4 + src/runtime/public/contract.ts | 14 +++ src/types.ts | 54 +++++++++++ test/client.test.ts | 115 ++++++++++++++++++++++++ test/permissions.test.ts | 83 ++++++++++++++++- 9 files changed, 387 insertions(+), 4 deletions(-) diff --git a/src/acp/client.ts b/src/acp/client.ts index 74599e1d..26969259 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,12 @@ export class AcpClient { promise: Promise; }; private readonly cancellingSessionIds = new Set(); + /** + * Per-session AbortController for in-flight host-side + * permission-request callbacks. Lazily created on first call; + * aborted (and removed) when the session is cancelled or reset. + */ + private readonly permissionAbortControllers = new Map(); private closing = false; private agentStartedAt?: string; private lastAgentExit?: AgentExitInfo; @@ -769,6 +780,7 @@ export class AcpClient { this.activePrompt = undefined; } this.cancellingSessionIds.delete(sessionId); + this.abortAndDropPermissionSignal(sessionId); this.promptPermissionFailures.delete(sessionId); } } @@ -848,6 +860,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 +959,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 +1221,35 @@ export class AcpClient { }; } + // Host-controlled per-call gate. Returns a decision to bypass the + // mode-based resolver; returns undefined / throws to fall through. + 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) ?? "other", + }, + { signal }, + ); + if (decision) { + const response = decisionToResponse(params, decision); + this.recordPermissionDecision(classifyPermissionDecision(params, response)); + return response; + } + } catch (error) { + // Don't fail the whole turn just because the host's UI + // errored — log and fall through to mode-based logic. + 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 +1423,29 @@ export class AcpClient { return await this.terminalManager.releaseTerminal(params); } + /** + * Get-or-create an AbortSignal that fires when the session is + * cancelled or the client is reset. Hosts use this in their + * `onPermissionRequest` callback to short-circuit pending UI. + */ + private cancellationSignalForSession(sessionId: string): AbortSignal { + let controller = this.permissionAbortControllers.get(sessionId); + if (!controller) { + controller = new AbortController(); + this.permissionAbortControllers.set(sessionId, controller); + } + return controller.signal; + } + + /** Abort any in-flight permission callback for the given session. */ + 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 79c0b740..64220a74 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,41 @@ export async function resolvePermissionRequest( return cancelled(); } +/** + * Map a host-supplied `AcpPermissionDecision` to the ACP wire shape, + * picking the right `optionId` from whatever options the agent + * advertised. Pure function; no I/O. + * + * Pathological agent behavior (no allow option for an approve + * decision, no reject option for a deny decision) falls back to + * `cancelled` rather than guessing at semantics. + */ +export function decisionToResponse( + params: RequestPermissionRequest, + decision: AcpPermissionDecision, +): RequestPermissionResponse { + const options = params.options ?? []; + if (decision.outcome === "cancel") { + return cancelled(); + } + if (decision.outcome === "deny") { + const reject = pickOption(options, ["reject_once", "reject_always"]); + return reject ? selected(reject.optionId) : cancelled(); + } + // approve_once / approve_always: prefer the matching kind, then the + // other allow kind, then the first option (defensive — every real + // agent advertises at least one allow option for non-deny choices). + const preferOrder: PermissionOption["kind"][] = + decision.outcome === "approve_always" + ? ["allow_always", "allow_once"] + : ["allow_once", "allow_always"]; + const allow = pickOption(options, preferOrder); + if (allow) { + return selected(allow.optionId); + } + return options.length > 0 ? selected(options[0].optionId) : cancelled(); +} + export function classifyPermissionDecision( params: RequestPermissionRequest, response: RequestPermissionResponse, diff --git a/src/runtime.ts b/src/runtime.ts index fe7acddc..8edf589b 100644 --- a/src/runtime.ts +++ b/src/runtime.ts @@ -29,6 +29,9 @@ export { export type { AcpAgentRegistry, AcpFileSessionStoreOptions, + AcpOnPermissionRequest, + AcpPermissionDecision, + AcpPermissionRequest, AcpRuntime, AcpRuntimeCapabilities, AcpRuntimeDoctorReport, diff --git a/src/runtime/engine/connected-session.ts b/src/runtime/engine/connected-session.ts index a0f10ab9..109e4759 100644 --- a/src/runtime/engine/connected-session.ts +++ b/src/runtime/engine/connected-session.ts @@ -3,6 +3,7 @@ import { AcpClient } from "../../acp/client.js"; import { withInterrupt } from "../../async-control.js"; import { absolutePath, isoNow } from "../../session/persistence.js"; import type { + AcpOnPermissionRequest, AuthPolicy, McpServer, NonInteractivePermissionPolicy, @@ -39,6 +40,7 @@ export type WithConnectedSessionOptions = { mcpServers?: McpServer[]; permissionMode?: PermissionMode; nonInteractivePermissions?: NonInteractivePermissionPolicy; + onPermissionRequest?: AcpOnPermissionRequest; authCredentials?: Record; authPolicy?: AuthPolicy; terminal?: boolean; @@ -90,6 +92,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 +105,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 73ecf05e..5c2345cb 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 4ce602a0..56f25e4b 100644 --- a/src/runtime/public/contract.ts +++ b/src/runtime/public/contract.ts @@ -1,10 +1,17 @@ import type { + AcpOnPermissionRequest, McpServer, NonInteractivePermissionPolicy, PermissionMode, SessionRecord, } from "../../types.js"; +export type { + AcpOnPermissionRequest, + AcpPermissionDecision, + AcpPermissionRequest, +} from "../../types.js"; + export type AcpRuntimePromptMode = "prompt" | "steer"; export type AcpRuntimeSessionMode = "persistent" | "oneshot"; @@ -195,6 +202,13 @@ export type AcpRuntimeOptions = { timeoutMs?: number; probeAgent?: string; verbose?: boolean; + /** + * Optional async hook for host-driven per-call permission gating. + * Forwarded as-is to the underlying `AcpClient`. Returning a + * decision short-circuits the mode-based resolver; returning + * `undefined` (or throwing) falls through to it. + */ + onPermissionRequest?: AcpOnPermissionRequest; }; export type AcpFileSessionStoreOptions = { diff --git a/src/types.ts b/src/types.ts index b41cdad6..ee9cdea9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -2,14 +2,60 @@ 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"; +/** + * Structured permission-request handed to a host's + * `onPermissionRequest` callback. Wraps the raw ACP request with a + * best-effort kind classification so hosts can branch UI without + * re-parsing. + */ +export type AcpPermissionRequest = { + sessionId: string; + /** Full original RequestPermissionRequest from the ACP SDK. */ + raw: RequestPermissionRequest; + /** Inferred from `toolCall.kind` or `toolCall.title`; `"other"` when unknown. */ + inferredKind: ToolKind | "other"; +}; + +/** + * Decision a host returns from `onPermissionRequest`. acpx maps this + * to the right ACP `RequestPermissionResponse` shape based on the + * options the agent advertised — hosts don't need to know about + * `optionId` strings. + * + * approve_once → pick `allow_once` (fall back to `allow_always`, + * or first option, in that order) + * approve_always → pick `allow_always` (fall back to `allow_once`, + * or first option) + * deny → pick `reject_once` (fall back to `reject_always`, + * or `cancelled`) + * cancel → respond with `{ outcome: "cancelled" }` + */ +export type AcpPermissionDecision = + | { outcome: "approve_once" } + | { outcome: "approve_always" } + | { outcome: "deny" } + | { outcome: "cancel" }; + +/** + * Async hook invoked when the agent issues a permission request. + * Return a decision to bypass the mode-based resolver; return + * `undefined` (or throw) to fall through to it. + */ +export type AcpOnPermissionRequest = ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, +) => Promise; + export const EXIT_CODES = { SUCCESS: 0, ERROR: 1, @@ -180,6 +226,14 @@ export type AcpClientOptions = { onAcpOutputMessage?: (direction: AcpMessageDirection, message: AcpJsonRpcMessage) => void; onSessionUpdate?: (notification: SessionNotification) => void; onClientOperation?: (operation: ClientOperation) => void; + /** + * Optional async hook invoked when the agent issues a permission + * request. Return a decision to bypass the mode-based resolver; + * return `undefined` to fall through to it. Throwing also falls + * through (and is logged) so a buggy host UI can't take down the + * agent turn. + */ + onPermissionRequest?: AcpOnPermissionRequest; }; export const SESSION_RECORD_SCHEMA = "acpx.session.v1" as const; diff --git a/test/client.test.ts b/test/client.test.ts index 34ae5cf0..2bbdfe01 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -357,6 +357,121 @@ test("AcpClient handlePermissionRequest records approved decisions", async () => }); }); +test("AcpClient onPermissionRequest decision short-circuits the mode-based resolver", async () => { + let modeResolverCalls = 0; + const client = makeClient({ + // Mode is approve-reads which would deny edit; the callback's + // approve_once decision must override that. + permissionMode: "approve-reads", + nonInteractivePermissions: "deny", + onPermissionRequest: async (req) => { + modeResolverCalls += 1; + assert.equal(req.sessionId, "session-cb-1"); + assert.equal(req.inferredKind, "edit"); + return { outcome: "approve_once" }; + }, + }); + + const response = await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-1", "edit"), + ); + + assert.deepEqual(response, { + outcome: { + outcome: "selected", + optionId: "allow", + }, + }); + assert.equal(modeResolverCalls, 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"), + ); + + // approve-all path picks the allow option after the callback abstains. + 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 (turn does not fail)", 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"), + ); + + // Mode-based fallback runs regardless of the host's bug. + 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: "approve_once" }; + }, + }); + + await asInternals(client).handlePermissionRequest?.( + makePermissionRequest("session-cb-4", "edit"), + ); + + assert(observedSignal instanceof AbortSignal); + assert.equal(observedSignal?.aborted, false); + + // Fire the same cancellation path the runtime uses on session cancel. + // (The full `client.cancel()` path also aborts but requires a live + // connection; the private helper exercises just the abort wiring.) + 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 71e3f087..5c9d17b8 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,79 @@ test("classifyPermissionDecision maps selected outcomes to approved, denied, or "cancelled", ); }); + +// ── decisionToResponse ────────────────────────────────────────────── + +test("decisionToResponse approve_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: "approve_once" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "once" } }); +}); + +test("decisionToResponse approve_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: "approve_always" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); +}); + +test("decisionToResponse approve_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: "approve_once" }); + assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); +}); + +test("decisionToResponse deny prefers reject_once and falls back to reject_always", () => { + const oneOnly = makeRequestWithTitle("tool", "edit", [ + { optionId: "allow", kind: "allow_once" }, + { optionId: "reject-once", kind: "reject_once" }, + { optionId: "reject-always", kind: "reject_always" }, + ]); + assert.deepEqual(decisionToResponse(oneOnly, { outcome: "deny" }), { + outcome: { outcome: "selected", optionId: "reject-once" }, + }); + + const onlyAlways = makeRequestWithTitle("tool", "edit", [ + { optionId: "allow", kind: "allow_once" }, + { optionId: "reject-always", kind: "reject_always" }, + ]); + assert.deepEqual(decisionToResponse(onlyAlways, { outcome: "deny" }), { + outcome: { outcome: "selected", optionId: "reject-always" }, + }); +}); + +test("decisionToResponse deny cancels when no reject option exists", () => { + const request = makeRequestWithTitle("tool", "edit", [{ optionId: "allow", kind: "allow_once" }]); + assert.deepEqual(decisionToResponse(request, { outcome: "deny" }), { + 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" }, + }); +}); + +// ── inferToolKind export ───────────────────────────────────────────── + +test("inferToolKind exposes the title-based classifier for hosts", () => { + 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"); +}); From 49cc2b5a61d767c8b0f981e61d4150a8b8557768 Mon Sep 17 00:00:00 2001 From: DaniAkash Date: Wed, 6 May 2026 19:42:04 +0530 Subject: [PATCH 2/4] refactor: align onPermissionRequest with acpx code style - drop JSDoc blocks on new types and helpers to match the surrounding zero-JSDoc convention used by `permissionModeSatisfies`, `resolvePermissionRequest`, `classifyPermissionDecision`, etc. - inline the callback signature in `AcpClientOptions`, `AcpRuntimeOptions`, and `WithConnectedSessionOptions` to mirror the inline shape used by `onSessionUpdate`, `onClientOperation`, etc.; drop the `AcpOnPermissionRequest` type alias and its re-exports. - rename decision outcomes from `approve_once | approve_always | deny | cancel` to `allow_once | allow_always | reject_once | reject_always | cancel`, mapping 1:1 to the SDK's `PermissionOption["kind"]` strings. This removes the parallel vocabulary and lets `decisionToResponse` collapse to a single fallback-order table lookup. - expose `inferredKind` as `ToolKind | undefined` (matching `inferToolKind`'s own return convention) instead of substituting `"other"`. --- src/acp/client.ts | 19 ++------- src/permissions.ts | 38 ++++++------------ src/runtime.ts | 1 - src/runtime/engine/connected-session.ts | 8 +++- src/runtime/public/contract.ts | 20 ++++------ src/types.ts | 53 +++++-------------------- test/client.test.ts | 19 +++------ test/permissions.test.ts | 35 +++++----------- 8 files changed, 54 insertions(+), 139 deletions(-) diff --git a/src/acp/client.ts b/src/acp/client.ts index 26969259..abd16c5d 100644 --- a/src/acp/client.ts +++ b/src/acp/client.ts @@ -272,11 +272,6 @@ export class AcpClient { promise: Promise; }; private readonly cancellingSessionIds = new Set(); - /** - * Per-session AbortController for in-flight host-side - * permission-request callbacks. Lazily created on first call; - * aborted (and removed) when the session is cancelled or reset. - */ private readonly permissionAbortControllers = new Map(); private closing = false; private agentStartedAt?: string; @@ -1221,8 +1216,6 @@ export class AcpClient { }; } - // Host-controlled per-call gate. Returns a decision to bypass the - // mode-based resolver; returns undefined / throws to fall through. if (this.options.onPermissionRequest) { const signal = this.cancellationSignalForSession(params.sessionId); try { @@ -1230,7 +1223,7 @@ export class AcpClient { { sessionId: params.sessionId, raw: params, - inferredKind: inferToolKind(params) ?? "other", + inferredKind: inferToolKind(params), }, { signal }, ); @@ -1240,8 +1233,8 @@ export class AcpClient { return response; } } catch (error) { - // Don't fail the whole turn just because the host's UI - // errored — log and fall through to mode-based logic. + // 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) @@ -1423,11 +1416,6 @@ export class AcpClient { return await this.terminalManager.releaseTerminal(params); } - /** - * Get-or-create an AbortSignal that fires when the session is - * cancelled or the client is reset. Hosts use this in their - * `onPermissionRequest` callback to short-circuit pending UI. - */ private cancellationSignalForSession(sessionId: string): AbortSignal { let controller = this.permissionAbortControllers.get(sessionId); if (!controller) { @@ -1437,7 +1425,6 @@ export class AcpClient { return controller.signal; } - /** Abort any in-flight permission callback for the given session. */ private abortAndDropPermissionSignal(sessionId: string): void { const controller = this.permissionAbortControllers.get(sessionId); if (controller) { diff --git a/src/permissions.ts b/src/permissions.ts index 64220a74..0db9613f 100644 --- a/src/permissions.ts +++ b/src/permissions.ts @@ -155,39 +155,25 @@ export async function resolvePermissionRequest( return cancelled(); } -/** - * Map a host-supplied `AcpPermissionDecision` to the ACP wire shape, - * picking the right `optionId` from whatever options the agent - * advertised. Pure function; no I/O. - * - * Pathological agent behavior (no allow option for an approve - * decision, no reject option for a deny decision) falls back to - * `cancelled` rather than guessing at semantics. - */ +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 { - const options = params.options ?? []; if (decision.outcome === "cancel") { return cancelled(); } - if (decision.outcome === "deny") { - const reject = pickOption(options, ["reject_once", "reject_always"]); - return reject ? selected(reject.optionId) : cancelled(); - } - // approve_once / approve_always: prefer the matching kind, then the - // other allow kind, then the first option (defensive — every real - // agent advertises at least one allow option for non-deny choices). - const preferOrder: PermissionOption["kind"][] = - decision.outcome === "approve_always" - ? ["allow_always", "allow_once"] - : ["allow_once", "allow_always"]; - const allow = pickOption(options, preferOrder); - if (allow) { - return selected(allow.optionId); - } - return options.length > 0 ? selected(options[0].optionId) : cancelled(); + const matched = pickOption(params.options ?? [], DECISION_FALLBACK_ORDER[decision.outcome]); + return matched ? selected(matched.optionId) : cancelled(); } export function classifyPermissionDecision( diff --git a/src/runtime.ts b/src/runtime.ts index 8edf589b..2c5a26ed 100644 --- a/src/runtime.ts +++ b/src/runtime.ts @@ -29,7 +29,6 @@ export { export type { AcpAgentRegistry, AcpFileSessionStoreOptions, - AcpOnPermissionRequest, AcpPermissionDecision, AcpPermissionRequest, AcpRuntime, diff --git a/src/runtime/engine/connected-session.ts b/src/runtime/engine/connected-session.ts index 109e4759..238f53a4 100644 --- a/src/runtime/engine/connected-session.ts +++ b/src/runtime/engine/connected-session.ts @@ -3,7 +3,8 @@ import { AcpClient } from "../../acp/client.js"; import { withInterrupt } from "../../async-control.js"; import { absolutePath, isoNow } from "../../session/persistence.js"; import type { - AcpOnPermissionRequest, + AcpPermissionDecision, + AcpPermissionRequest, AuthPolicy, McpServer, NonInteractivePermissionPolicy, @@ -40,7 +41,10 @@ export type WithConnectedSessionOptions = { mcpServers?: McpServer[]; permissionMode?: PermissionMode; nonInteractivePermissions?: NonInteractivePermissionPolicy; - onPermissionRequest?: AcpOnPermissionRequest; + onPermissionRequest?: ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, + ) => Promise; authCredentials?: Record; authPolicy?: AuthPolicy; terminal?: boolean; diff --git a/src/runtime/public/contract.ts b/src/runtime/public/contract.ts index 56f25e4b..3579ad37 100644 --- a/src/runtime/public/contract.ts +++ b/src/runtime/public/contract.ts @@ -1,16 +1,13 @@ import type { - AcpOnPermissionRequest, + AcpPermissionDecision, + AcpPermissionRequest, McpServer, NonInteractivePermissionPolicy, PermissionMode, SessionRecord, } from "../../types.js"; -export type { - AcpOnPermissionRequest, - AcpPermissionDecision, - AcpPermissionRequest, -} from "../../types.js"; +export type { AcpPermissionDecision, AcpPermissionRequest } from "../../types.js"; export type AcpRuntimePromptMode = "prompt" | "steer"; @@ -202,13 +199,10 @@ export type AcpRuntimeOptions = { timeoutMs?: number; probeAgent?: string; verbose?: boolean; - /** - * Optional async hook for host-driven per-call permission gating. - * Forwarded as-is to the underlying `AcpClient`. Returning a - * decision short-circuits the mode-based resolver; returning - * `undefined` (or throwing) falls through to it. - */ - onPermissionRequest?: AcpOnPermissionRequest; + onPermissionRequest?: ( + req: AcpPermissionRequest, + ctx: { signal: AbortSignal }, + ) => Promise; }; export type AcpFileSessionStoreOptions = { diff --git a/src/types.ts b/src/types.ts index ee9cdea9..2d1a70c9 100644 --- a/src/types.ts +++ b/src/types.ts @@ -12,50 +12,19 @@ import type { export type { McpServer, SessionNotification } from "@agentclientprotocol/sdk"; import type { PromptInput } from "./prompt-content.js"; -/** - * Structured permission-request handed to a host's - * `onPermissionRequest` callback. Wraps the raw ACP request with a - * best-effort kind classification so hosts can branch UI without - * re-parsing. - */ export type AcpPermissionRequest = { sessionId: string; - /** Full original RequestPermissionRequest from the ACP SDK. */ raw: RequestPermissionRequest; - /** Inferred from `toolCall.kind` or `toolCall.title`; `"other"` when unknown. */ - inferredKind: ToolKind | "other"; + inferredKind: ToolKind | undefined; }; -/** - * Decision a host returns from `onPermissionRequest`. acpx maps this - * to the right ACP `RequestPermissionResponse` shape based on the - * options the agent advertised — hosts don't need to know about - * `optionId` strings. - * - * approve_once → pick `allow_once` (fall back to `allow_always`, - * or first option, in that order) - * approve_always → pick `allow_always` (fall back to `allow_once`, - * or first option) - * deny → pick `reject_once` (fall back to `reject_always`, - * or `cancelled`) - * cancel → respond with `{ outcome: "cancelled" }` - */ export type AcpPermissionDecision = - | { outcome: "approve_once" } - | { outcome: "approve_always" } - | { outcome: "deny" } + | { outcome: "allow_once" } + | { outcome: "allow_always" } + | { outcome: "reject_once" } + | { outcome: "reject_always" } | { outcome: "cancel" }; -/** - * Async hook invoked when the agent issues a permission request. - * Return a decision to bypass the mode-based resolver; return - * `undefined` (or throw) to fall through to it. - */ -export type AcpOnPermissionRequest = ( - req: AcpPermissionRequest, - ctx: { signal: AbortSignal }, -) => Promise; - export const EXIT_CODES = { SUCCESS: 0, ERROR: 1, @@ -226,14 +195,10 @@ export type AcpClientOptions = { onAcpOutputMessage?: (direction: AcpMessageDirection, message: AcpJsonRpcMessage) => void; onSessionUpdate?: (notification: SessionNotification) => void; onClientOperation?: (operation: ClientOperation) => void; - /** - * Optional async hook invoked when the agent issues a permission - * request. Return a decision to bypass the mode-based resolver; - * return `undefined` to fall through to it. Throwing also falls - * through (and is logged) so a buggy host UI can't take down the - * agent turn. - */ - onPermissionRequest?: AcpOnPermissionRequest; + 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 2bbdfe01..849e40ce 100644 --- a/test/client.test.ts +++ b/test/client.test.ts @@ -358,17 +358,15 @@ test("AcpClient handlePermissionRequest records approved decisions", async () => }); test("AcpClient onPermissionRequest decision short-circuits the mode-based resolver", async () => { - let modeResolverCalls = 0; + let callbackInvocations = 0; const client = makeClient({ - // Mode is approve-reads which would deny edit; the callback's - // approve_once decision must override that. permissionMode: "approve-reads", nonInteractivePermissions: "deny", onPermissionRequest: async (req) => { - modeResolverCalls += 1; + callbackInvocations += 1; assert.equal(req.sessionId, "session-cb-1"); assert.equal(req.inferredKind, "edit"); - return { outcome: "approve_once" }; + return { outcome: "allow_once" }; }, }); @@ -382,7 +380,7 @@ test("AcpClient onPermissionRequest decision short-circuits the mode-based resol optionId: "allow", }, }); - assert.equal(modeResolverCalls, 1); + assert.equal(callbackInvocations, 1); assert.deepEqual(client.getPermissionStats(), { requested: 1, approved: 1, @@ -405,7 +403,6 @@ test("AcpClient onPermissionRequest returning undefined falls through to mode-ba makePermissionRequest("session-cb-2", "edit"), ); - // approve-all path picks the allow option after the callback abstains. assert.deepEqual(response, { outcome: { outcome: "selected", @@ -421,7 +418,7 @@ test("AcpClient onPermissionRequest returning undefined falls through to mode-ba }); }); -test("AcpClient onPermissionRequest throws fall through (turn does not fail)", async () => { +test("AcpClient onPermissionRequest throws fall through to mode-based resolver", async () => { let callbackInvocations = 0; const client = makeClient({ permissionMode: "approve-all", @@ -435,7 +432,6 @@ test("AcpClient onPermissionRequest throws fall through (turn does not fail)", a makePermissionRequest("session-cb-3", "edit"), ); - // Mode-based fallback runs regardless of the host's bug. assert.deepEqual(response, { outcome: { outcome: "selected", @@ -451,7 +447,7 @@ test("AcpClient onPermissionRequest receives an AbortSignal that fires on sessio permissionMode: "approve-all", onPermissionRequest: async (_req, ctx) => { observedSignal = ctx.signal; - return { outcome: "approve_once" }; + return { outcome: "allow_once" }; }, }); @@ -462,9 +458,6 @@ test("AcpClient onPermissionRequest receives an AbortSignal that fires on sessio assert(observedSignal instanceof AbortSignal); assert.equal(observedSignal?.aborted, false); - // Fire the same cancellation path the runtime uses on session cancel. - // (The full `client.cancel()` path also aborts but requires a live - // connection; the private helper exercises just the abort wiring.) const internals = asInternals(client) as unknown as { abortAndDropPermissionSignal: (sessionId: string) => void; }; diff --git a/test/permissions.test.ts b/test/permissions.test.ts index 5c9d17b8..077e7585 100644 --- a/test/permissions.test.ts +++ b/test/permissions.test.ts @@ -198,59 +198,48 @@ test("classifyPermissionDecision maps selected outcomes to approved, denied, or ); }); -// ── decisionToResponse ────────────────────────────────────────────── - -test("decisionToResponse approve_once prefers allow_once over allow_always", () => { +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: "approve_once" }); + const response = decisionToResponse(request, { outcome: "allow_once" }); assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "once" } }); }); -test("decisionToResponse approve_always prefers allow_always over allow_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: "approve_always" }); + const response = decisionToResponse(request, { outcome: "allow_always" }); assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); }); -test("decisionToResponse approve_once falls back to allow_always when allow_once is missing", () => { +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: "approve_once" }); + const response = decisionToResponse(request, { outcome: "allow_once" }); assert.deepEqual(response, { outcome: { outcome: "selected", optionId: "always" } }); }); -test("decisionToResponse deny prefers reject_once and falls back to reject_always", () => { - const oneOnly = makeRequestWithTitle("tool", "edit", [ - { optionId: "allow", kind: "allow_once" }, - { optionId: "reject-once", kind: "reject_once" }, - { optionId: "reject-always", kind: "reject_always" }, - ]); - assert.deepEqual(decisionToResponse(oneOnly, { outcome: "deny" }), { - outcome: { outcome: "selected", optionId: "reject-once" }, - }); - +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: "deny" }), { + assert.deepEqual(decisionToResponse(onlyAlways, { outcome: "reject_once" }), { outcome: { outcome: "selected", optionId: "reject-always" }, }); }); -test("decisionToResponse deny cancels when no reject option exists", () => { +test("decisionToResponse cancels when no matching option exists", () => { const request = makeRequestWithTitle("tool", "edit", [{ optionId: "allow", kind: "allow_once" }]); - assert.deepEqual(decisionToResponse(request, { outcome: "deny" }), { + assert.deepEqual(decisionToResponse(request, { outcome: "reject_once" }), { outcome: { outcome: "cancelled" }, }); }); @@ -265,9 +254,7 @@ test("decisionToResponse cancel always returns cancelled", () => { }); }); -// ── inferToolKind export ───────────────────────────────────────────── - -test("inferToolKind exposes the title-based classifier for hosts", () => { +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"); From 1b1047c58f87a9620891dc21ab41a17691b93795 Mon Sep 17 00:00:00 2001 From: DaniAkash Date: Wed, 6 May 2026 19:45:43 +0530 Subject: [PATCH 3/4] docs: add CHANGELOG entry for onPermissionRequest --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d779d15..22f0d448 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. Thanks @DaniAkash. + ### Breaking ### Fixes From 1984b03397f1173c0b68333c2ff0e16d11cfa231 Mon Sep 17 00:00:00 2001 From: DaniAkash Date: Wed, 6 May 2026 19:53:23 +0530 Subject: [PATCH 4/4] chore: update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22f0d448..97227a1d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,7 +8,7 @@ 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. Thanks @DaniAkash. +- 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