From cf623e57484b7f0d1bee67f73acb6b02c0c39c8b Mon Sep 17 00:00:00 2001 From: David Marshall Date: Thu, 14 May 2026 00:36:43 -0500 Subject: [PATCH 1/4] 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 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/modes/acp/acp-agent.ts | 221 ++++++++++++-- packages/coding-agent/test/acp-agent.test.ts | 271 +++++++++++++++++- 3 files changed, 460 insertions(+), 33 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d8186311b..4f8072b43 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -13,6 +13,7 @@ - Added per-line column cap shared across streaming tool outputs (`bash`, `ssh`, `python`, `js eval`) and the `read` tool. Lines wider than `tools.outputMaxColumns` bytes (default **768**) are ellipsis-truncated at write time and remaining bytes up to the next `\n` are dropped — bounded memory even on multi-MB single-line outputs (e.g. `cat /dev/urandom`). The cap lives on `OutputSink` as the new `maxColumns` option, persists state across chunk boundaries so split-mid-line writes still respect the budget, and exposes `columnDroppedBytes` / `columnTruncatedLines` on `OutputSummary`. Middle-elision byte math subtracts column drops so the "elided from middle" count stays honest. `read` reuses the same setting but trims its already-collected lines via `truncateLine`. Skipped when the read selector is `:raw`. The artifact file (`artifact://`) keeps the full uncapped stream. Set `tools.outputMaxColumns = 0` to disable. - Added Bun HTTP/2 fetch opt-in. Dev scripts (`bun run dev`, `bun run stats`) now pass `bun --experimental-http2-fetch` so every `fetch()` advertises `h2` in the TLS ALPN list and falls back to HTTP/1.1 when the server doesn't select it. Multiplexing collapses parallel requests to the same origin onto one TLS connection. For the installed `omp` binary, export `BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT=1` in your shell to enable the same behavior (the flag has to be set before Bun starts; `process.env` from inside JS is too late). Requires Bun **1.3.14**. - Added per-subagent cost display (`$X.XX` in the task progress tree and the session-observer stats line). Cost is accumulated incrementally from `message_end` events and shown only when non-zero, using the `statusLineCost` theme color. Providers that do not report per-turn cost data (e.g. subscription/OAuth usage) continue to show nothing. +- Added ACP elicitation bridge so skills/extensions calling `select`, `confirm`, or `input` on the extension UI context now produce real `unstable_createElicitation` form requests to the ACP client (rather than always resolving to `undefined` / `false`). The `acpExtensionUiContext` constant is promoted to `createAcpExtensionUiContext(connection, sessionId, clientCapabilities)` — invoked once per session inside `#configureExtensions` — and each method maps to a single-property `value` schema: `select` → `{type: "string", enum}`, `confirm` → `{type: "boolean"}` (joined `title` + `message` when the trimmed message is non-empty; otherwise just `title`), `input` → `{type: "string", description: placeholder?}` (ACP has no `placeholder` field on `StringPropertySchema`; empty / whitespace-only placeholders are treated as absent). `accept` responses narrow the returned `ElicitationContentValue` back to the method's declared type with a runtime `typeof` guard; `decline` / `cancel` / transport failures fall back to the prior stub return values. `dialogOptions.signal` is honored: an already-aborted signal short-circuits before any SDK round-trip, and an abort mid-flight races against the elicitation so the caller's promise resolves to the stub fallback (the ACP request itself keeps running on the client side — the SDK exposes no form-mode cancel surface; `unstable_completeElicitation` is URL-mode only — matching the in-flight pattern used by `requestRpcEditor`). `dialogOptions.timeout` is honored on parity with `RpcExtensionUIContext`: when the timer fires before the client responds, `onTimeout` is invoked and the caller resolves to the stub fallback. A throwing `onTimeout` is caught and logged (`logger.warn`) so the elicitation promise still settles. Late SDK rejections that arrive after abort/timeout are dropped silently to keep operator logs clean; transport failures still emit `logger.warn` with `{ sessionId, method, error }`. Calls are skipped when the client did not advertise `clientCapabilities.elicitation.form` during `initialize`, so non-elicitation clients are unaffected. `createAcpExtensionUiContext` is exported for tests. ### Changed diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index 9a34ccda0..ce0a55a3b 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -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; -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 { + const signal = dialogOptions?.signal; + if (signal?.aborted) { + return undefined; + } + const { promise, resolve } = Promise.withResolvers(); + 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; diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 8da9766ee..109f94c84 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -2,7 +2,14 @@ import { afterEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs"; import * as os from "node:os"; import * as path from "node:path"; -import type { AgentSideConnection, PromptRequest, SessionNotification } from "@agentclientprotocol/sdk"; +import type { + AgentSideConnection, + ClientCapabilities, + CreateElicitationRequest, + CreateElicitationResponse, + PromptRequest, + SessionNotification, +} from "@agentclientprotocol/sdk"; import { zForkSessionResponse, zLoadSessionResponse, @@ -13,7 +20,7 @@ import { import type { Model } from "@oh-my-pi/pi-ai"; import { getConfigRootDir, setAgentDir } from "@oh-my-pi/pi-utils"; import { resetSettingsForTest, Settings } from "../src/config/settings"; -import { ACP_BOOTSTRAP_RACE_GUARD_MS, AcpAgent } from "../src/modes/acp/acp-agent"; +import { ACP_BOOTSTRAP_RACE_GUARD_MS, AcpAgent, createAcpExtensionUiContext } from "../src/modes/acp/acp-agent"; import type { PlanModeState } from "../src/plan-mode/state"; import type { AgentSession, AgentSessionEvent } from "../src/session/agent-session"; import { SILENT_ABORT_MARKER } from "../src/session/messages"; @@ -886,4 +893,264 @@ describe("ACP agent", () => { harness.abortController.abort(); await Bun.sleep(0); }); + + describe("ACP elicitation bridge", () => { + const FORM_CAPABILITIES: ClientCapabilities = { elicitation: { form: {} } }; + + function createElicitConnection(handler: (req: CreateElicitationRequest) => Promise): { + connection: AgentSideConnection; + calls: CreateElicitationRequest[]; + } { + const calls: CreateElicitationRequest[] = []; + const connection = { + unstable_createElicitation: async (req: CreateElicitationRequest) => { + calls.push(req); + return handler(req); + }, + } as unknown as AgentSideConnection; + return { connection, calls }; + } + + it("translates select to a single-property string-enum elicitation", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "second" }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-select", FORM_CAPABILITIES); + + const result = await ctx.select("Pick one", ["first", "second", "third"]); + + expect(result).toBe("second"); + expect(calls).toHaveLength(1); + const request = calls[0]!; + expect(request.mode).toBe("form"); + expect(request.message).toBe("Pick one"); + if (request.mode !== "form" || !("sessionId" in request)) { + throw new Error("expected session-scoped form elicitation"); + } + expect(request.sessionId).toBe("session-select"); + expect(request.requestedSchema).toEqual({ + type: "object", + properties: { value: { type: "string", enum: ["first", "second", "third"] } }, + required: ["value"], + }); + }); + + it("translates confirm to a boolean elicitation and returns the accepted value", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: true }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-confirm", FORM_CAPABILITIES); + + const result = await ctx.confirm("Proceed?", "This will overwrite the file."); + + expect(result).toBe(true); + expect(calls).toHaveLength(1); + const request = calls[0]!; + if (request.mode !== "form") { + throw new Error("expected form-mode elicitation"); + } + expect(request.message).toBe("Proceed?\n\nThis will overwrite the file."); + expect(request.requestedSchema.properties?.value).toEqual({ type: "boolean" }); + expect(request.requestedSchema.required).toEqual(["value"]); + }); + + it("translates input to a string elicitation and surfaces the placeholder as description", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "claude" }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-input", FORM_CAPABILITIES); + + const result = await ctx.input("Your name?", "e.g. claude"); + + expect(result).toBe("claude"); + expect(calls).toHaveLength(1); + const request = calls[0]!; + if (request.mode !== "form") { + throw new Error("expected form-mode elicitation"); + } + expect(request.message).toBe("Your name?"); + expect(request.requestedSchema.properties?.value).toEqual({ + type: "string", + description: "e.g. claude", + }); + }); + + it("returns undefined / false for decline and cancel actions", async () => { + let nextAction: "decline" | "cancel" = "decline"; + const { connection } = createElicitConnection(async () => ({ action: nextAction })); + const ctx = createAcpExtensionUiContext(connection, "session-cancel", FORM_CAPABILITIES); + + for (const action of ["decline", "cancel"] as const) { + nextAction = action; + expect(await ctx.select("X", ["a"])).toBeUndefined(); + expect(await ctx.confirm("X", "Y")).toBe(false); + expect(await ctx.input("X")).toBeUndefined(); + } + }); + + it("falls back to the stubbed behaviour when the client does not advertise form elicitation", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "ignored" }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-nocaps", {}); + + expect(await ctx.select("X", ["a"])).toBeUndefined(); + expect(await ctx.confirm("X", "Y")).toBe(false); + expect(await ctx.input("X")).toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("treats transport-level elicitation failures as undecided input", async () => { + const { connection, calls } = createElicitConnection(async () => { + throw new Error("connection closed"); + }); + const ctx = createAcpExtensionUiContext(connection, "session-throw", FORM_CAPABILITIES); + + expect(await ctx.select("X", ["a"])).toBeUndefined(); + expect(await ctx.confirm("X", "Y")).toBe(false); + expect(await ctx.input("X")).toBeUndefined(); + expect(calls).toHaveLength(3); + }); + + it("skips the SDK call entirely when dialogOptions.signal is already aborted", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "ignored" }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-preabort", FORM_CAPABILITIES); + const controller = new AbortController(); + controller.abort(); + + expect(await ctx.select("X", ["a"], { signal: controller.signal })).toBeUndefined(); + expect(await ctx.confirm("X", "Y", { signal: controller.signal })).toBe(false); + expect(await ctx.input("X", undefined, { signal: controller.signal })).toBeUndefined(); + expect(calls).toHaveLength(0); + }); + + it("resolves to the stub fallback when dialogOptions.signal aborts mid-flight", async () => { + const { resolve, promise: never } = Promise.withResolvers(); + const { connection, calls } = createElicitConnection(() => never); + const ctx = createAcpExtensionUiContext(connection, "session-midabort", FORM_CAPABILITIES); + const controller = new AbortController(); + + const pending = ctx.select("X", ["a"], { signal: controller.signal }); + controller.abort(); + expect(await pending).toBeUndefined(); + expect(calls).toHaveLength(1); + // Resolve the never-promise so the bridge's `.then(finish)` chain settles + // and Bun's promise tracker doesn't flag a leaked pending promise. + resolve({ action: "decline" }); + }); + + it("returns the stub fallback when the client sends a wrong-typed accept payload", async () => { + // confirm expects a boolean; a string `value` must narrow to `false`. + const stringForBool = createElicitConnection(async () => ({ + action: "accept", + content: { value: "yes" }, + })); + const boolCtx = createAcpExtensionUiContext( + stringForBool.connection, + "session-wrongtype-bool", + FORM_CAPABILITIES, + ); + expect(await boolCtx.confirm("Proceed?", "")).toBe(false); + + // select expects a string; a boolean `value` must narrow to `undefined`. + const boolForString = createElicitConnection(async () => ({ + action: "accept", + content: { value: true }, + })); + const selectCtx = createAcpExtensionUiContext( + boolForString.connection, + "session-wrongtype-str", + FORM_CAPABILITIES, + ); + expect(await selectCtx.select("Pick", ["a"])).toBeUndefined(); + }); + + it("returns the stub fallback when accept arrives without the expected `value` key", async () => { + // content present but missing the `value` key — the bridge looks up + // `response.content.value` which is `undefined`, so the typeof guard fires. + const missingKey = createElicitConnection(async () => ({ + action: "accept", + content: { other: "noise" } as never, + })); + const ctx = createAcpExtensionUiContext(missingKey.connection, "session-missingkey", FORM_CAPABILITIES); + expect(await ctx.select("Pick", ["a"])).toBeUndefined(); + expect(await ctx.confirm("Proceed?", "")).toBe(false); + expect(await ctx.input("Name?")).toBeUndefined(); + }); + + it("returns the stub fallback when accept arrives with no content at all", async () => { + // content omitted entirely — the `!response.content` guard short-circuits + // before the per-method narrow has a chance to run. + const noContent = createElicitConnection(async () => ({ action: "accept" })); + const ctx = createAcpExtensionUiContext(noContent.connection, "session-nocontent", FORM_CAPABILITIES); + expect(await ctx.select("Pick", ["a"])).toBeUndefined(); + expect(await ctx.confirm("Proceed?", "")).toBe(false); + expect(await ctx.input("Name?")).toBeUndefined(); + }); + + it("fires onTimeout and resolves to the stub fallback when dialogOptions.timeout expires", async () => { + const { promise: never } = Promise.withResolvers(); + const { connection, calls } = createElicitConnection(() => never); + const ctx = createAcpExtensionUiContext(connection, "session-timeout", FORM_CAPABILITIES); + let timeoutFired = 0; + const result = await ctx.select("Pick", ["a"], { timeout: 1, onTimeout: () => timeoutFired++ }); + expect(result).toBeUndefined(); + expect(timeoutFired).toBe(1); + expect(calls).toHaveLength(1); + }); + + it("treats whitespace-only placeholder as absent on `input`", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "n" }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-ws-placeholder", FORM_CAPABILITIES); + + await ctx.input("Name?", " "); + + expect(calls).toHaveLength(1); + const request = calls[0]!; + if (request.mode !== "form") throw new Error("expected form-mode elicitation"); + expect(request.requestedSchema.properties?.value).toEqual({ type: "string" }); + }); + + it("sends `message === title` on `confirm` when the message is empty (no join)", async () => { + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: true }, + })); + const ctx = createAcpExtensionUiContext(connection, "session-confirm-empty", FORM_CAPABILITIES); + + await ctx.confirm("Proceed?", ""); + // Whitespace-only message must follow the same branch as empty — + // CHANGELOG says join only when the message is non-empty. + await ctx.confirm("Proceed?", " "); + + expect(calls).toHaveLength(2); + expect(calls[0]!.message).toBe("Proceed?"); + expect(calls[1]!.message).toBe("Proceed?"); + }); + + it("still resolves to the stub fallback when dialogOptions.onTimeout throws", async () => { + const { promise: never } = Promise.withResolvers(); + const { connection } = createElicitConnection(() => never); + const ctx = createAcpExtensionUiContext(connection, "session-timeout-throw", FORM_CAPABILITIES); + + const result = await ctx.select("Pick", ["a"], { + timeout: 1, + onTimeout: () => { + throw new Error("boom"); + }, + }); + + expect(result).toBeUndefined(); + }); + }); }); From 6b63903cf2a88b65a9725b32cb32e18cfe15f061 Mon Sep 17 00:00:00 2001 From: David Marshall Date: Thu, 14 May 2026 09:59:43 -0500 Subject: [PATCH 2/4] refactor(coding-agent/acp): read sessionId lazily on every elicitation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `AgentSession.sessionId` is a getter that reads through to `sessionManager.getSessionId()` and mutates when an extension command calls `ctx.newSession` or `ctx.switchSession` (both exposed in the same #configureExtensions block). Snapshotting the id once at factory time routed later elicitations to the pre-switch id — diverging from every other sessionUpdate call in this file, which already reads record.session.sessionId live. createAcpExtensionUiContext now takes `getSessionId: () => string` and calls it per elicitation. Caller passes `() => record.session.sessionId` so each select / confirm / input picks up the current id. Also simplifies elicitFromAcpClient: `onAbort` and `finish` had identical settlement bodies differing only by the resolve value, with a hand-rolled `removeEventListener` symmetry comment to keep them in sync. Collapsed to a single settle path — `onAbort = () => finish(undefined)` — so future changes to settlement apply to both paths automatically. Doc-comment tightened to call out that late SDK `accept` responses (not just rejections) after abort/timeout are also dropped silently. New regression test asserts that mutating the captured sessionId between elicitations is reflected in the next request. Co-Authored-By: omp --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/modes/acp/acp-agent.ts | 58 +++++++++-------- packages/coding-agent/test/acp-agent.test.ts | 65 ++++++++++++------- 3 files changed, 72 insertions(+), 53 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 4f8072b43..c908cfe57 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -13,7 +13,7 @@ - Added per-line column cap shared across streaming tool outputs (`bash`, `ssh`, `python`, `js eval`) and the `read` tool. Lines wider than `tools.outputMaxColumns` bytes (default **768**) are ellipsis-truncated at write time and remaining bytes up to the next `\n` are dropped — bounded memory even on multi-MB single-line outputs (e.g. `cat /dev/urandom`). The cap lives on `OutputSink` as the new `maxColumns` option, persists state across chunk boundaries so split-mid-line writes still respect the budget, and exposes `columnDroppedBytes` / `columnTruncatedLines` on `OutputSummary`. Middle-elision byte math subtracts column drops so the "elided from middle" count stays honest. `read` reuses the same setting but trims its already-collected lines via `truncateLine`. Skipped when the read selector is `:raw`. The artifact file (`artifact://`) keeps the full uncapped stream. Set `tools.outputMaxColumns = 0` to disable. - Added Bun HTTP/2 fetch opt-in. Dev scripts (`bun run dev`, `bun run stats`) now pass `bun --experimental-http2-fetch` so every `fetch()` advertises `h2` in the TLS ALPN list and falls back to HTTP/1.1 when the server doesn't select it. Multiplexing collapses parallel requests to the same origin onto one TLS connection. For the installed `omp` binary, export `BUN_FEATURE_FLAG_EXPERIMENTAL_HTTP2_CLIENT=1` in your shell to enable the same behavior (the flag has to be set before Bun starts; `process.env` from inside JS is too late). Requires Bun **1.3.14**. - Added per-subagent cost display (`$X.XX` in the task progress tree and the session-observer stats line). Cost is accumulated incrementally from `message_end` events and shown only when non-zero, using the `statusLineCost` theme color. Providers that do not report per-turn cost data (e.g. subscription/OAuth usage) continue to show nothing. -- Added ACP elicitation bridge so skills/extensions calling `select`, `confirm`, or `input` on the extension UI context now produce real `unstable_createElicitation` form requests to the ACP client (rather than always resolving to `undefined` / `false`). The `acpExtensionUiContext` constant is promoted to `createAcpExtensionUiContext(connection, sessionId, clientCapabilities)` — invoked once per session inside `#configureExtensions` — and each method maps to a single-property `value` schema: `select` → `{type: "string", enum}`, `confirm` → `{type: "boolean"}` (joined `title` + `message` when the trimmed message is non-empty; otherwise just `title`), `input` → `{type: "string", description: placeholder?}` (ACP has no `placeholder` field on `StringPropertySchema`; empty / whitespace-only placeholders are treated as absent). `accept` responses narrow the returned `ElicitationContentValue` back to the method's declared type with a runtime `typeof` guard; `decline` / `cancel` / transport failures fall back to the prior stub return values. `dialogOptions.signal` is honored: an already-aborted signal short-circuits before any SDK round-trip, and an abort mid-flight races against the elicitation so the caller's promise resolves to the stub fallback (the ACP request itself keeps running on the client side — the SDK exposes no form-mode cancel surface; `unstable_completeElicitation` is URL-mode only — matching the in-flight pattern used by `requestRpcEditor`). `dialogOptions.timeout` is honored on parity with `RpcExtensionUIContext`: when the timer fires before the client responds, `onTimeout` is invoked and the caller resolves to the stub fallback. A throwing `onTimeout` is caught and logged (`logger.warn`) so the elicitation promise still settles. Late SDK rejections that arrive after abort/timeout are dropped silently to keep operator logs clean; transport failures still emit `logger.warn` with `{ sessionId, method, error }`. Calls are skipped when the client did not advertise `clientCapabilities.elicitation.form` during `initialize`, so non-elicitation clients are unaffected. `createAcpExtensionUiContext` is exported for tests. +- Added ACP elicitation bridge so skills/extensions calling `select`, `confirm`, or `input` on the extension UI context now produce real `unstable_createElicitation` form requests to the ACP client (rather than always resolving to `undefined` / `false`). The `acpExtensionUiContext` constant is promoted to `createAcpExtensionUiContext(connection, getSessionId, clientCapabilities)` — invoked once per session inside `#configureExtensions`, with `getSessionId: () => string` so the live `record.session.sessionId` is read on every elicitation (the underlying id mutates when an extension command calls `ctx.newSession` / `ctx.switchSession`). Each method maps to a single-property `value` schema: `select` → `{type: "string", enum}`, `confirm` → `{type: "boolean"}` (joined `title` + `message` when the trimmed message is non-empty; otherwise just `title`), `input` → `{type: "string", description: placeholder?}` (ACP has no `placeholder` field on `StringPropertySchema`; empty / whitespace-only placeholders are treated as absent). `accept` responses narrow the returned `ElicitationContentValue` back to the method's declared type with a runtime `typeof` guard; `decline` / `cancel` / transport failures fall back to the prior stub return values. `dialogOptions.signal` is honored: an already-aborted signal short-circuits before any SDK round-trip, and an abort mid-flight races against the elicitation so the caller's promise resolves to the stub fallback (the ACP request itself keeps running on the client side — the SDK exposes no form-mode cancel surface; `unstable_completeElicitation` is URL-mode only — matching the in-flight pattern used by `requestRpcEditor`). `dialogOptions.timeout` is honored on parity with `RpcExtensionUIContext`: when the timer fires before the client responds, `onTimeout` is invoked and the caller resolves to the stub fallback. A throwing `onTimeout` is caught and logged (`logger.warn`) so the elicitation promise still settles. Late SDK rejections that arrive after abort/timeout are dropped silently to keep operator logs clean; transport failures still emit `logger.warn` with `{ sessionId, method, error }`. Calls are skipped when the client did not advertise `clientCapabilities.elicitation.form` during `initialize`, so non-elicitation clients are unaffected. `createAcpExtensionUiContext` is exported for tests. ### Changed diff --git a/packages/coding-agent/src/modes/acp/acp-agent.ts b/packages/coding-agent/src/modes/acp/acp-agent.ts index ce0a55a3b..5d506a77d 100644 --- a/packages/coding-agent/src/modes/acp/acp-agent.ts +++ b/packages/coding-agent/src/modes/acp/acp-agent.ts @@ -161,9 +161,9 @@ type CreateAcpSession = (cwd: string) => Promise; * * `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. + * the caller's promise resolves to the stub fallback. Late SDK responses that + * arrive after abort/timeout — both rejections and successful `accept`s — + * are dropped silently (no `logger.warn`) to keep operator logs clean. */ async function elicitFromAcpClient( connection: AgentSideConnection, @@ -180,17 +180,6 @@ async function elicitFromAcpClient( const { promise, resolve } = Promise.withResolvers(); 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; @@ -198,6 +187,7 @@ async function elicitFromAcpClient( signal?.removeEventListener("abort", onAbort); resolve(value); }; + const onAbort = () => finish(undefined); signal?.addEventListener("abort", onAbort, { once: true }); if (dialogOptions?.timeout !== undefined) { timeoutId = setTimeout(() => { @@ -243,16 +233,23 @@ async function elicitFromAcpClient( /** * 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. + * requests into ACP elicitations against `connection` for the session + * returned by `getSessionId()`. The id is read lazily at each elicitation + * because `AgentSession.sessionId` is a getter over `sessionManager` state + * that mutates when an extension command calls `ctx.newSession` / + * `ctx.switchSession` — snapshotting it once at factory time would route + * later elicitations to the pre-switch id. Live reads keep the bridge + * symmetric with every other `sessionUpdate` call in this file + * (`record.session.sessionId` is always evaluated at emit time). + * + * The non-elicitation surface (custom components, editor, theming, + * terminal input) remains stubbed — ACP clients render those themselves + * or not at all. Capability gating respects the client's `initialize` + * advertisement. */ export function createAcpExtensionUiContext( connection: AgentSideConnection, - sessionId: string, + getSessionId: () => string, clientCapabilities: ClientCapabilities | undefined, ): ExtensionUIContext { const supportsForm = clientCapabilities?.elicitation?.form != null; @@ -261,7 +258,7 @@ export function createAcpExtensionUiContext( if (!supportsForm) return undefined; const value = await elicitFromAcpClient( connection, - sessionId, + getSessionId(), "select", title, { type: "string", enum: options }, @@ -273,7 +270,7 @@ export function createAcpExtensionUiContext( if (!supportsForm) return false; const value = await elicitFromAcpClient( connection, - sessionId, + getSessionId(), "confirm", message.trim().length > 0 ? `${title}\n\n${message}` : title, { type: "boolean" }, @@ -285,7 +282,7 @@ export function createAcpExtensionUiContext( if (!supportsForm) return undefined; const value = await elicitFromAcpClient( connection, - sessionId, + getSessionId(), "input", title, // ACP's `StringPropertySchema` has no `placeholder` field, so we @@ -1763,10 +1760,15 @@ export class AcpAgent implements Agent { }, compact: instructionsOrOptions => runExtensionCompact(record.session, instructionsOrOptions), }, - // 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), + // Per-session getter: `record.session.sessionId` reads through to + // `sessionManager.getSessionId()` (it's a getter, not a field), so an + // extension command that calls `ctx.newSession` / `ctx.switchSession` + // — both exposed in the block just above — mutates the underlying id + // mid-flight. Reading lazily on each elicitation matches every other + // `sessionUpdate` call in this file. Hoisting the factory to an + // `AcpAgent` field would still be wrong because it would also lose + // the per-`record` binding. + createAcpExtensionUiContext(this.#connection, () => record.session.sessionId, this.#clientCapabilities), ); await extensionRunner.emit({ type: "session_start" }); record.extensionsConfigured = true; diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 109f94c84..c6400cf10 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -916,7 +916,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "second" }, })); - const ctx = createAcpExtensionUiContext(connection, "session-select", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-select", FORM_CAPABILITIES); const result = await ctx.select("Pick one", ["first", "second", "third"]); @@ -941,7 +941,7 @@ describe("ACP agent", () => { action: "accept", content: { value: true }, })); - const ctx = createAcpExtensionUiContext(connection, "session-confirm", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-confirm", FORM_CAPABILITIES); const result = await ctx.confirm("Proceed?", "This will overwrite the file."); @@ -961,7 +961,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "claude" }, })); - const ctx = createAcpExtensionUiContext(connection, "session-input", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-input", FORM_CAPABILITIES); const result = await ctx.input("Your name?", "e.g. claude"); @@ -981,7 +981,7 @@ describe("ACP agent", () => { it("returns undefined / false for decline and cancel actions", async () => { let nextAction: "decline" | "cancel" = "decline"; const { connection } = createElicitConnection(async () => ({ action: nextAction })); - const ctx = createAcpExtensionUiContext(connection, "session-cancel", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-cancel", FORM_CAPABILITIES); for (const action of ["decline", "cancel"] as const) { nextAction = action; @@ -996,7 +996,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "ignored" }, })); - const ctx = createAcpExtensionUiContext(connection, "session-nocaps", {}); + const ctx = createAcpExtensionUiContext(connection, () => "session-nocaps", {}); expect(await ctx.select("X", ["a"])).toBeUndefined(); expect(await ctx.confirm("X", "Y")).toBe(false); @@ -1008,7 +1008,7 @@ describe("ACP agent", () => { const { connection, calls } = createElicitConnection(async () => { throw new Error("connection closed"); }); - const ctx = createAcpExtensionUiContext(connection, "session-throw", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-throw", FORM_CAPABILITIES); expect(await ctx.select("X", ["a"])).toBeUndefined(); expect(await ctx.confirm("X", "Y")).toBe(false); @@ -1021,7 +1021,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "ignored" }, })); - const ctx = createAcpExtensionUiContext(connection, "session-preabort", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-preabort", FORM_CAPABILITIES); const controller = new AbortController(); controller.abort(); @@ -1034,7 +1034,7 @@ describe("ACP agent", () => { it("resolves to the stub fallback when dialogOptions.signal aborts mid-flight", async () => { const { resolve, promise: never } = Promise.withResolvers(); const { connection, calls } = createElicitConnection(() => never); - const ctx = createAcpExtensionUiContext(connection, "session-midabort", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-midabort", FORM_CAPABILITIES); const controller = new AbortController(); const pending = ctx.select("X", ["a"], { signal: controller.signal }); @@ -1052,11 +1052,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "yes" }, })); - const boolCtx = createAcpExtensionUiContext( - stringForBool.connection, - "session-wrongtype-bool", - FORM_CAPABILITIES, - ); + const boolCtx = createAcpExtensionUiContext(stringForBool.connection, () => "session-wrongtype-bool", FORM_CAPABILITIES); expect(await boolCtx.confirm("Proceed?", "")).toBe(false); // select expects a string; a boolean `value` must narrow to `undefined`. @@ -1064,11 +1060,7 @@ describe("ACP agent", () => { action: "accept", content: { value: true }, })); - const selectCtx = createAcpExtensionUiContext( - boolForString.connection, - "session-wrongtype-str", - FORM_CAPABILITIES, - ); + const selectCtx = createAcpExtensionUiContext(boolForString.connection, () => "session-wrongtype-str", FORM_CAPABILITIES); expect(await selectCtx.select("Pick", ["a"])).toBeUndefined(); }); @@ -1079,7 +1071,7 @@ describe("ACP agent", () => { action: "accept", content: { other: "noise" } as never, })); - const ctx = createAcpExtensionUiContext(missingKey.connection, "session-missingkey", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(missingKey.connection, () => "session-missingkey", FORM_CAPABILITIES); expect(await ctx.select("Pick", ["a"])).toBeUndefined(); expect(await ctx.confirm("Proceed?", "")).toBe(false); expect(await ctx.input("Name?")).toBeUndefined(); @@ -1089,7 +1081,7 @@ describe("ACP agent", () => { // content omitted entirely — the `!response.content` guard short-circuits // before the per-method narrow has a chance to run. const noContent = createElicitConnection(async () => ({ action: "accept" })); - const ctx = createAcpExtensionUiContext(noContent.connection, "session-nocontent", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(noContent.connection, () => "session-nocontent", FORM_CAPABILITIES); expect(await ctx.select("Pick", ["a"])).toBeUndefined(); expect(await ctx.confirm("Proceed?", "")).toBe(false); expect(await ctx.input("Name?")).toBeUndefined(); @@ -1098,7 +1090,7 @@ describe("ACP agent", () => { it("fires onTimeout and resolves to the stub fallback when dialogOptions.timeout expires", async () => { const { promise: never } = Promise.withResolvers(); const { connection, calls } = createElicitConnection(() => never); - const ctx = createAcpExtensionUiContext(connection, "session-timeout", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-timeout", FORM_CAPABILITIES); let timeoutFired = 0; const result = await ctx.select("Pick", ["a"], { timeout: 1, onTimeout: () => timeoutFired++ }); expect(result).toBeUndefined(); @@ -1111,7 +1103,7 @@ describe("ACP agent", () => { action: "accept", content: { value: "n" }, })); - const ctx = createAcpExtensionUiContext(connection, "session-ws-placeholder", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-ws-placeholder", FORM_CAPABILITIES); await ctx.input("Name?", " "); @@ -1126,7 +1118,7 @@ describe("ACP agent", () => { action: "accept", content: { value: true }, })); - const ctx = createAcpExtensionUiContext(connection, "session-confirm-empty", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-confirm-empty", FORM_CAPABILITIES); await ctx.confirm("Proceed?", ""); // Whitespace-only message must follow the same branch as empty — @@ -1141,7 +1133,7 @@ describe("ACP agent", () => { it("still resolves to the stub fallback when dialogOptions.onTimeout throws", async () => { const { promise: never } = Promise.withResolvers(); const { connection } = createElicitConnection(() => never); - const ctx = createAcpExtensionUiContext(connection, "session-timeout-throw", FORM_CAPABILITIES); + const ctx = createAcpExtensionUiContext(connection, () => "session-timeout-throw", FORM_CAPABILITIES); const result = await ctx.select("Pick", ["a"], { timeout: 1, @@ -1152,5 +1144,30 @@ describe("ACP agent", () => { expect(result).toBeUndefined(); }); + + it("reads the sessionId getter on every elicitation so mid-flight session changes are reflected", async () => { + // `record.session.sessionId` mutates when an extension command calls + // `ctx.switchSession` / `ctx.newSession`. Snapshotting it once at + // factory time would route later elicitations to the pre-switch id. + const { connection, calls } = createElicitConnection(async () => ({ + action: "accept", + content: { value: "ok" }, + })); + let currentSessionId = "session-before-switch"; + const ctx = createAcpExtensionUiContext(connection, () => currentSessionId, FORM_CAPABILITIES); + + await ctx.select("Pick", ["a"]); + currentSessionId = "session-after-switch"; + await ctx.confirm("Continue?", "post-switch"); + await ctx.input("Name?"); + + expect(calls).toHaveLength(3); + if (calls[0]!.mode !== "form" || calls[1]!.mode !== "form" || calls[2]!.mode !== "form") { + throw new Error("expected form-mode elicitations"); + } + expect(calls[0]!.sessionId).toBe("session-before-switch"); + expect(calls[1]!.sessionId).toBe("session-after-switch"); + expect(calls[2]!.sessionId).toBe("session-after-switch"); + }); }); }); From d7e8a7358bbdea04e305d198c4a9278ae57bb830 Mon Sep 17 00:00:00 2001 From: David Marshall Date: Thu, 14 May 2026 10:16:26 -0500 Subject: [PATCH 3/4] style(coding-agent/acp): wrap long createAcpExtensionUiContext test call-sites `biome check` enforces print-width on the two test-only call-sites that `ast_edit` collapsed onto a single line during the sessionId-getter rewrite. Auto-formatter wrap, no behavior change. Co-Authored-By: omp --- packages/coding-agent/test/acp-agent.test.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index c6400cf10..95448af19 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -1052,7 +1052,11 @@ describe("ACP agent", () => { action: "accept", content: { value: "yes" }, })); - const boolCtx = createAcpExtensionUiContext(stringForBool.connection, () => "session-wrongtype-bool", FORM_CAPABILITIES); + const boolCtx = createAcpExtensionUiContext( + stringForBool.connection, + () => "session-wrongtype-bool", + FORM_CAPABILITIES, + ); expect(await boolCtx.confirm("Proceed?", "")).toBe(false); // select expects a string; a boolean `value` must narrow to `undefined`. @@ -1060,7 +1064,11 @@ describe("ACP agent", () => { action: "accept", content: { value: true }, })); - const selectCtx = createAcpExtensionUiContext(boolForString.connection, () => "session-wrongtype-str", FORM_CAPABILITIES); + const selectCtx = createAcpExtensionUiContext( + boolForString.connection, + () => "session-wrongtype-str", + FORM_CAPABILITIES, + ); expect(await selectCtx.select("Pick", ["a"])).toBeUndefined(); }); From d47f55c7fdd216ae6be35c666c4e623e981d26d3 Mon Sep 17 00:00:00 2001 From: David Marshall Date: Thu, 14 May 2026 10:24:21 -0500 Subject: [PATCH 4/4] fix(coding-agent/acp): narrow CreateElicitationRequest variant before reading sessionId in live-getter test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CreateElicitationRequest` is a discriminated union — even after narrowing on `mode === "form"`, both `ElicitationRequestScope` (no `sessionId`) and `ElicitationSessionScope` (with `sessionId`) remain in the union. The new live-getter regression test was reading `calls[N]!.sessionId` directly, which CI tsgo rejected with TS2339. Per-element `if (!call || call.mode !== "form" || !("sessionId" in call))` narrows to the session-scoped variant. Spelled three times because loop-style narrows don't propagate to the assertions below. Matches the discriminator pattern used in the older 'translates select' test at line ~927. Co-Authored-By: omp --- packages/coding-agent/test/acp-agent.test.ts | 21 ++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/coding-agent/test/acp-agent.test.ts b/packages/coding-agent/test/acp-agent.test.ts index 95448af19..d7ada16e9 100644 --- a/packages/coding-agent/test/acp-agent.test.ts +++ b/packages/coding-agent/test/acp-agent.test.ts @@ -1170,12 +1170,21 @@ describe("ACP agent", () => { await ctx.input("Name?"); expect(calls).toHaveLength(3); - if (calls[0]!.mode !== "form" || calls[1]!.mode !== "form" || calls[2]!.mode !== "form") { - throw new Error("expected form-mode elicitations"); - } - expect(calls[0]!.sessionId).toBe("session-before-switch"); - expect(calls[1]!.sessionId).toBe("session-after-switch"); - expect(calls[2]!.sessionId).toBe("session-after-switch"); + // Each call must be a session-scoped form elicitation. Spelled as three + // separate narrows because `mode === "form"` alone leaves both + // `ElicitationRequestScope` and `ElicitationSessionScope` in the union — + // only `"sessionId" in call` picks the session-scoped variant — and + // loop-style narrows don't propagate to the assertions below. + const [first, second, third] = calls; + if (!first || first.mode !== "form" || !("sessionId" in first)) + throw new Error("first call missing sessionId"); + if (!second || second.mode !== "form" || !("sessionId" in second)) + throw new Error("second call missing sessionId"); + if (!third || third.mode !== "form" || !("sessionId" in third)) + throw new Error("third call missing sessionId"); + expect(first.sessionId).toBe("session-before-switch"); + expect(second.sessionId).toBe("session-after-switch"); + expect(third.sessionId).toBe("session-after-switch"); }); }); });