feat(coding-agent/acp): bridged ExtensionUIContext to ACP unstable_createElicitation

Promotes the stub acpExtensionUiContext to a createAcpExtensionUiContext factory invoked per session inside #configureExtensions. select / confirm / input each map to a single-property `value` schema and round-trip through a shared elicitFromAcpClient helper that mirrors RpcExtensionUIContext.#createDialogPromise: - capability gating on clientCapabilities.elicitation.form - runtime typeof narrowing on accept payloads (wrong-type / missing key / no content all fall back to the stub return values) - dialogOptions.signal: pre-aborted short-circuits before any SDK call; mid-flight abort races the in-flight elicitation. Symmetric removeEventListener on both onAbort and finish paths. - dialogOptions.timeout: setTimeout(.unref()) settles the promise via onTimeout + stub fallback. A throwing onTimeout is caught and logged so the elicitation promise still settles. - late SDK rejections after abort/timeout are dropped silently; transport failures log via logger.warn with { sessionId, method, error }. Empty/whitespace-only placeholders on `input` and empty/whitespace-only messages on `confirm` are treated as absent (trim-aware), matching the behavior documented in the CHANGELOG bullet. 15 new tests cover request shape, decline/cancel, missing capability, transport failure, pre-abort, mid-flight abort, wrong-typed accept, missing `value` key, no content, timeout, whitespace placeholder, empty-message join, and throwing onTimeout. Co-Authored-By: omp <noreply@oh-my-pi.dev>
2026-05-14 00:36:43 -05:00
parent 71f3997afa
commit cf623e5748
3 changed files with 460 additions and 33 deletions
@@ -9,6 +9,9 @@ import {
 	type ClientCapabilities,
 	type CloseSessionRequest,
 	type CloseSessionResponse,
+	type CreateElicitationResponse,
+	type ElicitationContentValue,
+	type ElicitationPropertySchema,
 	type ForkSessionRequest,
 	type ForkSessionResponse,
 	type InitializeRequest,
@@ -44,7 +47,7 @@ import { logger, VERSION } from "@oh-my-pi/pi-utils";
 import { disableProvider, enableProvider, reset as resetCapabilities } from "../../capability";
 import { Settings } from "../../config/settings";
 import { clearPluginRootsAndCaches, resolveActiveProjectRegistryPath } from "../../discovery/helpers";
-import type { ExtensionUIContext } from "../../extensibility/extensions";
+import type { ExtensionUIContext, ExtensionUIDialogOptions } from "../../extensibility/extensions";
 import { runExtensionCompact } from "../../extensibility/extensions/compact-handler";
 import { buildSkillPromptMessage, getSkillSlashCommandName } from "../../extensibility/skills";
 import { loadSlashCommands } from "../../extensibility/slash-commands";
@@ -138,35 +141,188 @@ type MCPSourceMap = {

 type CreateAcpSession = (cwd: string) => Promise<AgentSession>;

-const acpExtensionUiContext: ExtensionUIContext = {
-	select: async () => undefined,
-	confirm: async () => false,
-	input: async () => undefined,
-	notify: (message, type) => {
-		logger.debug("ACP extension notification", { message, type });
-	},
-	onTerminalInput: () => () => {},
-	setStatus: () => {},
-	setWorkingMessage: () => {},
-	setWidget: () => {},
-	setFooter: () => {},
-	setHeader: () => {},
-	setTitle: () => {},
-	custom: async () => undefined as never,
-	pasteToEditor: () => {},
-	setEditorText: () => {},
-	getEditorText: () => "",
-	editor: async () => undefined,
-	setEditorComponent: () => {},
-	get theme() {
-		return theme;
-	},
-	getAllThemes: async () => [],
-	getTheme: async () => undefined,
-	setTheme: async () => ({ success: false, error: "Theme changes are unavailable in ACP mode" }),
-	getToolsExpanded: () => false,
-	setToolsExpanded: () => {},
-};
+/**
+ * Bridge a single ExtensionUIContext call to the ACP `unstable_createElicitation`
+ * surface. Skills/extensions ask for one value at a time (a chosen option, a
+ * confirmation, a piece of text), so every elicitation here uses a one-property
+ * `value` schema; the caller narrows the resulting `ElicitationContentValue`
+ * back to its concrete primitive type.
+ *
+ * `dialogOptions.signal` short-circuits the elicitation if it is already
+ * aborted and races the in-flight request against the abort event. The SDK
+ * exposes no `cancel_elicitation` surface for form-mode elicitations
+ * (`unstable_completeElicitation` is URL-mode only), so the ACP request itself
+ * keeps running on the client side until the user dismisses it — but
+ * resolving the local promise unblocks the caller (matches the RPC mode
+ * pattern in `requestRpcEditor`). The abort listener is removed once the
+ * elicitation settles so that callers which reuse the same signal across many
+ * elicitations (e.g. `ask` multi-select loops) don't accumulate listeners and
+ * trip Node's `MaxListeners` warning.
+ *
+ * `dialogOptions.timeout` mirrors `RpcExtensionUIContext.#createDialogPromise`:
+ * when the timer fires before the client responds, `onTimeout` is invoked and
+ * the caller's promise resolves to the stub fallback. Late SDK rejections that
+ * arrive after abort/timeout are dropped silently (no `logger.warn`) to keep
+ * operator logs clean.
+ */
+async function elicitFromAcpClient(
+	connection: AgentSideConnection,
+	sessionId: string,
+	method: "select" | "confirm" | "input",
+	message: string,
+	property: ElicitationPropertySchema,
+	dialogOptions: ExtensionUIDialogOptions | undefined,
+): Promise<ElicitationContentValue | undefined> {
+	const signal = dialogOptions?.signal;
+	if (signal?.aborted) {
+		return undefined;
+	}
+	const { promise, resolve } = Promise.withResolvers<CreateElicitationResponse | undefined>();
+	let settled = false;
+	let timeoutId: NodeJS.Timeout | undefined;
+	const onAbort = () => {
+		if (settled) return;
+		settled = true;
+		if (timeoutId !== undefined) clearTimeout(timeoutId);
+		// `{ once: true }` auto-detaches this listener after firing, but call
+		// `removeEventListener` explicitly so the cleanup is symmetric with
+		// `finish()` and the doc-comment above is literally true even if the
+		// option is ever changed.
+		signal?.removeEventListener("abort", onAbort);
+		resolve(undefined);
+	};
+	const finish = (value: CreateElicitationResponse | undefined) => {
+		if (settled) return;
+		settled = true;
+		if (timeoutId !== undefined) clearTimeout(timeoutId);
+		signal?.removeEventListener("abort", onAbort);
+		resolve(value);
+	};
+	signal?.addEventListener("abort", onAbort, { once: true });
+	if (dialogOptions?.timeout !== undefined) {
+		timeoutId = setTimeout(() => {
+			if (settled) return;
+			try {
+				dialogOptions.onTimeout?.();
+			} catch (error) {
+				// A throwing `onTimeout` must not leave the elicitation promise
+				// pending — settle it via `finish` below regardless.
+				logger.warn("ACP elicitation onTimeout threw", { sessionId, method, error });
+			}
+			finish(undefined);
+		}, dialogOptions.timeout);
+		// A long pending timeout alone shouldn't keep the event loop alive when
+		// the rest of the agent has shut down — matches `job-manager.ts` /
+		// `executor.ts` timer hygiene. Connection + session lifetimes keep the
+		// loop alive on the happy path.
+		timeoutId.unref();
+	}
+	connection
+		.unstable_createElicitation({
+			mode: "form",
+			sessionId,
+			message,
+			requestedSchema: {
+				type: "object",
+				properties: { value: property },
+				required: ["value"],
+			},
+		})
+		.then(finish, error => {
+			// Caller may already have moved on via abort/timeout; suppress noise.
+			if (settled) return;
+			logger.warn("ACP elicitation failed", { sessionId, method, error });
+			finish(undefined);
+		});
+	const response = await promise;
+	if (!response || response.action !== "accept" || !response.content) {
+		return undefined;
+	}
+	return response.content.value;
+}
+
+/**
+ * Build an {@link ExtensionUIContext} that translates skill/extension UI
+ * requests into ACP elicitations against `connection` for `sessionId`. The
+ * non-elicitation surface (custom components, editor, theming, terminal input)
+ * remains stubbed — ACP clients render those themselves or not at all. The
+ * factory is invoked per session so each `select` / `confirm` / `input` call
+ * is scoped to the session that triggered it, and capability gating respects
+ * the client's `initialize` advertisement.
+ */
+export function createAcpExtensionUiContext(
+	connection: AgentSideConnection,
+	sessionId: string,
+	clientCapabilities: ClientCapabilities | undefined,
+): ExtensionUIContext {
+	const supportsForm = clientCapabilities?.elicitation?.form != null;
+	return {
+		select: async (title, options, dialogOptions) => {
+			if (!supportsForm) return undefined;
+			const value = await elicitFromAcpClient(
+				connection,
+				sessionId,
+				"select",
+				title,
+				{ type: "string", enum: options },
+				dialogOptions,
+			);
+			return typeof value === "string" ? value : undefined;
+		},
+		confirm: async (title, message, dialogOptions) => {
+			if (!supportsForm) return false;
+			const value = await elicitFromAcpClient(
+				connection,
+				sessionId,
+				"confirm",
+				message.trim().length > 0 ? `${title}\n\n${message}` : title,
+				{ type: "boolean" },
+				dialogOptions,
+			);
+			return typeof value === "boolean" ? value : false;
+		},
+		input: async (title, placeholder, dialogOptions) => {
+			if (!supportsForm) return undefined;
+			const value = await elicitFromAcpClient(
+				connection,
+				sessionId,
+				"input",
+				title,
+				// ACP's `StringPropertySchema` has no `placeholder` field, so we
+				// surface the placeholder text as `description` — the closest
+				// semantic field a client can render alongside the input.
+				// Empty / whitespace-only placeholders are treated as absent.
+				{ type: "string", ...(placeholder?.trim() ? { description: placeholder } : {}) },
+				dialogOptions,
+			);
+			return typeof value === "string" ? value : undefined;
+		},
+		notify: (message, type) => {
+			logger.debug("ACP extension notification", { message, type });
+		},
+		onTerminalInput: () => () => {},
+		setStatus: () => {},
+		setWorkingMessage: () => {},
+		setWidget: () => {},
+		setFooter: () => {},
+		setHeader: () => {},
+		setTitle: () => {},
+		custom: async () => undefined as never,
+		pasteToEditor: () => {},
+		setEditorText: () => {},
+		getEditorText: () => "",
+		editor: async () => undefined,
+		setEditorComponent: () => {},
+		get theme() {
+			return theme;
+		},
+		getAllThemes: async () => [],
+		getTheme: async () => undefined,
+		setTheme: async () => ({ success: false, error: "Theme changes are unavailable in ACP mode" }),
+		getToolsExpanded: () => false,
+		setToolsExpanded: () => {},
+	};
+}

 export class AcpAgent implements Agent {
 	#connection: AgentSideConnection;
@@ -1607,7 +1763,10 @@ export class AcpAgent implements Agent {
 				},
 				compact: instructionsOrOptions => runExtensionCompact(record.session, instructionsOrOptions),
 			},
-			acpExtensionUiContext,
+			// Per-session: the factory bakes `sessionId` into every elicitation, so
+			// hoisting this to a field on AcpAgent would silently route all
+			// elicitations to the first session that ever ran.
+			createAcpExtensionUiContext(this.#connection, record.session.sessionId, this.#clientCapabilities),
 		);
 		await extensionRunner.emit({ type: "session_start" });
 		record.extensionsConfigured = true;