From 934d590dedc452f40b230ab1fca6d31dc43e3687 Mon Sep 17 00:00:00 2001 From: li-xiu-qi Date: Mon, 26 Oct 2026 15:43:40 +0800 Subject: [PATCH] feat(rpc): support clarify_user dialogs Route RPC questionnaires through the existing select and input fallback, and propagate cancellation signals to pending answers. Reject empty or whitespace freeform answers, responses that do not match an offered option label, duplicate question ids, and duplicate option labels with explicit errors. Render error results separately from user cancellation, and ignore late dialog responses after an abort. Update RPC documentation and regression coverage. Fixes #253 --- packages/coding-agent/docs/rpc.md | 4 + .../src/features/step-questionnaire.ts | 74 ++- .../regressions/253-rpc-clarify-user.test.ts | 472 ++++++++++++++++++ 3 files changed, 530 insertions(+), 20 deletions(-) create mode 100644 packages/coding-agent/test/suite/regressions/253-rpc-clarify-user.test.ts diff --git a/packages/coding-agent/docs/rpc.md b/packages/coding-agent/docs/rpc.md index 303f08aa..4dae0fb5 100644 --- a/packages/coding-agent/docs/rpc.md +++ b/packages/coding-agent/docs/rpc.md @@ -1204,6 +1204,10 @@ Some `ExtensionUIContext` methods are not supported or degraded in RPC mode beca Note: `ctx.mode` is `"rpc"` and `ctx.hasUI` is `true` in RPC mode because the dialog and fire-and-forget methods are functional via the extension UI sub-protocol. Use `ctx.mode === "tui"` to guard TUI-specific features like `custom()` that require a real terminal. +The built-in `clarify_user` tool accepts question groups and emits one existing `select` or `input` request per question in RPC mode. Dialogs carry the question text and option labels; question ids, labels, reasons, and group structure remain in the tool result rather than the dialog payload. Structured option values, trimmed non-empty freeform answers, cancellation, and abort signals are preserved. Empty freeform answers and responses that do not match an offered option label return an explicit error result. The error result still sets `cancelled: true`; clients should display the error text rather than treating it as a user cancellation. The tool does not use `custom()` in RPC mode. The native TUI dialog is unchanged, and non-interactive `print` and `json` modes continue to reject clarification requests. + +After an abort, the runtime settles the pending dialog and ignores late responses to that request id, but it does not emit a separate invalidation event. Clients must dismiss any pending clarification dialog when the turn is aborted or ends. + ### Extension UI Requests (stdout) All requests have `type: "extension_ui_request"`, a unique `id`, and a `method` field. diff --git a/packages/coding-agent/src/features/step-questionnaire.ts b/packages/coding-agent/src/features/step-questionnaire.ts index 7301fc9a..a3b56733 100644 --- a/packages/coding-agent/src/features/step-questionnaire.ts +++ b/packages/coding-agent/src/features/step-questionnaire.ts @@ -208,41 +208,56 @@ function answerText(questions: readonly NormalizedQuestion[], answers: readonly .join("\n\n"); } -async function executeFallback( - questions: NormalizedQuestion[], - ctx: ExtensionContext, -): Promise { +interface FallbackOutcome { + details: StepQuestionnaireDetails; + error?: string; +} + +async function executeFallback(questions: NormalizedQuestion[], ctx: ExtensionContext): Promise { const answers: StepQuestionnaireAnswer[] = []; + const finish = (cancelled: boolean, error?: string): FallbackOutcome => ({ + details: { questions, answers, cancelled }, + ...(error === undefined ? {} : { error }), + }); + const readFreeform = async (question: NormalizedQuestion): Promise<{ value?: string; error?: string }> => { + const answer = await ctx.ui.input(question.question, undefined, { signal: ctx.signal }); + if (answer === undefined) return {}; + const value = answer.trim(); + return value ? { value } : { error: "Error: clarify_user requires a non-empty freeform answer" }; + }; for (const question of questions) { if (question.options.length === 0) { - const answer = await ctx.ui.input(question.question); - if (answer === undefined) return { questions, answers, cancelled: true }; + const result = await readFreeform(question); + if (result.error) return finish(true, result.error); + if (result.value === undefined) return finish(true); answers.push({ id: question.id, - value: answer, - label: answer, + value: result.value, + label: result.value, wasCustom: true, }); continue; } const labels = question.options.map((option) => option.label); if (question.allowFreeform) labels.push("Type a custom answer"); - const selected = await ctx.ui.select(question.question, labels); - if (selected === undefined) return { questions, answers, cancelled: true }; + const selected = await ctx.ui.select(question.question, labels, { signal: ctx.signal }); + if (selected === undefined) return finish(true); const index = labels.indexOf(selected); if (index === question.options.length) { - const answer = await ctx.ui.input(question.question); - if (answer === undefined) return { questions, answers, cancelled: true }; + const result = await readFreeform(question); + if (result.error) return finish(true, result.error); + if (result.value === undefined) return finish(true); answers.push({ id: question.id, - value: answer, - label: answer, + value: result.value, + label: result.value, wasCustom: true, }); continue; } + if (index < 0) return finish(true, "Error: clarify_user received an option that was not offered"); const option = question.options[index]; - if (!option) return { questions, answers, cancelled: true }; + if (!option) return finish(true, "Error: clarify_user received an option that was not offered"); answers.push({ id: question.id, value: option.value, @@ -251,7 +266,7 @@ async function executeFallback( index: index + 1, }); } - return { questions, answers, cancelled: false }; + return finish(false); } async function executeNativeDialog( @@ -523,7 +538,8 @@ async function executeQuestionnaire( telemetry?: StepTelemetryReporter, ): Promise> { const questions = normalizeQuestions(params); - if (!ctx.hasUI || ctx.mode !== "tui") { + const supportsQuestionnaire = ctx.mode === "tui" || ctx.mode === "rpc"; + if (!ctx.hasUI || !supportsQuestionnaire) { return response({ questions, answers: [], cancelled: true }, "Error: clarify_user requires an interactive UI"); } if (questions.length === 0 || questions.some((item) => !item.question)) { @@ -535,12 +551,26 @@ async function executeQuestionnaire( "Error: allow_freeform=false requires at least one option", ); } + if (new Set(questions.map((item) => item.id)).size !== questions.length) { + return response({ questions, answers: [], cancelled: true }, "Error: question ids must be unique"); + } + for (const question of questions) { + const labels = question.options.map((option) => option.label.trim()); + if (new Set(labels).size !== labels.length) { + return response( + { questions, answers: [], cancelled: true }, + "Error: option labels must be unique within each question", + ); + } + } const askedAt = Date.now(); - const details = - typeof ctx.ui.custom === "function" - ? await executeNativeDialog(questions, ctx) + const execution = + ctx.mode === "tui" && typeof ctx.ui.custom === "function" + ? { details: await executeNativeDialog(questions, ctx) } : await executeFallback(questions, ctx); + const { details, error } = execution; + if (error) return response(details, error); const outcome = details.cancelled ? "cancelled" : details.answers.some((answer) => answer.wasCustom) @@ -583,6 +613,10 @@ export function registerStepClarifyUserExtension(pi: ExtensionAPI, telemetry?: S const block = result.content.find((item) => item.type === "text"); return new Text(block?.type === "text" ? block.text : "", 0, 0); } + const errorText = result.content.find( + (item): item is { type: "text"; text: string } => item.type === "text" && item.text.startsWith("Error:"), + )?.text; + if (errorText) return new Text(theme.fg("warning", errorText), 0, 0); if (details.cancelled) { return new Text(theme.fg("warning", "Clarification cancelled"), 0, 0); } diff --git a/packages/coding-agent/test/suite/regressions/253-rpc-clarify-user.test.ts b/packages/coding-agent/test/suite/regressions/253-rpc-clarify-user.test.ts new file mode 100644 index 00000000..1f836d75 --- /dev/null +++ b/packages/coding-agent/test/suite/regressions/253-rpc-clarify-user.test.ts @@ -0,0 +1,472 @@ +import type { AgentToolResult } from "@step-harness/agent-core"; +import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import type { AgentSessionRuntime } from "../../../src/core/agent-session-runtime.ts"; +import type { ExtensionContext } from "../../../src/core/extensions/index.ts"; +import { + registerStepClarifyUserExtension, + type StepQuestionnaireDetails, +} from "../../../src/features/step-questionnaire.ts"; +import { runRpcMode } from "../../../src/modes/rpc/rpc-mode.ts"; +import { createHarness, type Harness } from "../harness.ts"; + +const rpcIo = vi.hoisted(() => ({ + outputLines: [] as string[], + lineHandler: undefined as ((line: string) => void) | undefined, +})); + +vi.mock("../../../src/core/output-guard.js", () => ({ + flushRawStdout: vi.fn(async () => {}), + takeOverStdout: vi.fn(), + waitForRawStdoutBackpressure: vi.fn(async () => {}), + writeRawStdout: (line: string) => { + rpcIo.outputLines.push(line); + }, +})); + +vi.mock("../../../src/theme/theme.js", () => ({ theme: {} })); + +vi.mock("../../../src/modes/rpc/jsonl.js", () => ({ + attachJsonlLineReader: vi.fn((_stream: NodeJS.ReadableStream, onLine: (line: string) => void) => { + rpcIo.lineHandler = onLine; + return () => { + rpcIo.lineHandler = undefined; + }; + }), + serializeJsonLine: (value: unknown) => `${JSON.stringify(value)}\n`, +})); + +type NodeListener = Parameters[1]; +type ListenerSnapshot = { + stdinEnd: NodeListener[]; + signals: Map; +}; +type DialogRequest = { + type: "extension_ui_request"; + id: string; + method: "select" | "input"; + title: string; + options?: string[]; +}; + +function takeListenerSnapshot(): ListenerSnapshot { + const signals: NodeJS.Signals[] = + process.platform === "win32" ? ["SIGTERM", "SIGINT"] : ["SIGTERM", "SIGINT", "SIGHUP"]; + return { + stdinEnd: process.stdin.listeners("end") as NodeListener[], + signals: new Map(signals.map((signal) => [signal, process.listeners(signal) as NodeListener[]])), + }; +} + +function restoreListeners(snapshot: ListenerSnapshot): void { + for (const listener of process.stdin.listeners("end") as NodeListener[]) { + if (!snapshot.stdinEnd.includes(listener)) process.stdin.off("end", listener); + } + for (const [signal, previousListeners] of snapshot.signals) { + for (const listener of process.listeners(signal) as NodeListener[]) { + if (!previousListeners.includes(listener)) process.off(signal, listener); + } + } +} + +function outputRecords(): Array> { + return rpcIo.outputLines + .flatMap((line) => line.split("\n")) + .filter((line) => line.trim().length > 0) + .map((line) => JSON.parse(line) as Record); +} + +function dialogRequests(): DialogRequest[] { + return outputRecords().filter( + (record): record is DialogRequest => + record.type === "extension_ui_request" && (record.method === "select" || record.method === "input"), + ) as DialogRequest[]; +} + +function createRuntimeHost(harness: Harness): AgentSessionRuntime { + return { + session: harness.session, + newSession: vi.fn(async () => ({ cancelled: true })), + switchSession: vi.fn(async () => ({ cancelled: true })), + fork: vi.fn(async () => ({ cancelled: true, selectedText: "" })), + dispose: vi.fn(async () => {}), + setRebindSession: vi.fn(), + } as unknown as AgentSessionRuntime; +} + +describe("RPC clarify_user regression (#253)", () => { + const harnesses: Harness[] = []; + let listenerSnapshot: ListenerSnapshot; + + beforeEach(() => { + listenerSnapshot = takeListenerSnapshot(); + rpcIo.outputLines = []; + rpcIo.lineHandler = undefined; + }); + + afterEach(() => { + for (const harness of harnesses.splice(0)) harness.cleanup(); + restoreListeners(listenerSnapshot); + expect(process.stdin.listeners("end")).toEqual(listenerSnapshot.stdinEnd); + for (const [signal, previousListeners] of listenerSnapshot.signals) { + expect(process.listeners(signal)).toEqual(previousListeners); + } + rpcIo.outputLines = []; + rpcIo.lineHandler = undefined; + }); + + async function createQuestionnaireHarness(): Promise { + const harness = await createHarness({ extensionFactories: [registerStepClarifyUserExtension] }); + harnesses.push(harness); + return harness; + } + + async function startRpc(): Promise { + const harness = await createQuestionnaireHarness(); + void runRpcMode(createRuntimeHost(harness)); + await vi.waitFor(() => expect(rpcIo.lineHandler).toBeDefined()); + return harness; + } + + function clarifyTool(harness: Harness) { + const tool = harness.session.extensionRunner + .getAllRegisteredTools() + .find(({ definition }) => definition.name === "clarify_user"); + if (!tool) throw new Error("clarify_user is not registered"); + return tool.definition; + } + + async function waitForDialog(index: number): Promise { + await vi.waitFor(() => expect(dialogRequests().length).toBeGreaterThan(index)); + const request = dialogRequests()[index]; + if (!request) throw new Error(`Missing dialog request ${index}`); + return request; + } + + function respond(request: DialogRequest, response: { value?: string; cancelled?: boolean }): void { + rpcIo.lineHandler?.(JSON.stringify({ type: "extension_ui_response", id: request.id, ...response })); + } + + test("collects option and freeform answers through the RPC dialog protocol", async () => { + const harness = await startRpc(); + const execution = clarifyTool(harness).execute( + "clarify-253", + { + questions: [ + { + id: "deployment", + question: "Deploy the fix?", + options: [ + { label: "Yes", value: "yes" }, + { label: "No", value: "no" }, + ], + allow_freeform: false, + }, + { + id: "reason", + question: "Which kind of change is it?", + options: [{ label: "Bug fix", value: "bug" }], + }, + ], + }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + ); + + const deployRequest = await waitForDialog(0); + expect(deployRequest).toMatchObject({ method: "select", title: "Deploy the fix?", options: ["Yes", "No"] }); + respond(deployRequest, { value: "Yes" }); + + const reasonRequest = await waitForDialog(1); + expect(reasonRequest).toMatchObject({ + method: "select", + title: "Which kind of change is it?", + options: ["Bug fix", "Type a custom answer"], + }); + respond(reasonRequest, { value: "Type a custom answer" }); + + const customRequest = await waitForDialog(2); + expect(customRequest).toMatchObject({ method: "input", title: "Which kind of change is it?" }); + respond(customRequest, { value: " Production is broken " }); + + const result = (await execution) as AgentToolResult; + expect(result.details).toMatchObject({ + cancelled: false, + answers: [ + { id: "deployment", value: "yes", label: "Yes", wasCustom: false, index: 1 }, + { id: "reason", value: "Production is broken", label: "Production is broken", wasCustom: true }, + ], + }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: expect.stringContaining("matched_option: Yes"), + }); + expect(result.content[0]).toMatchObject({ type: "text", text: expect.stringContaining("source: freeform") }); + expect(dialogRequests()).toHaveLength(3); + }); + + test("returns cancellation from an RPC dialog without issuing another question", async () => { + const harness = await startRpc(); + const execution = clarifyTool(harness).execute( + "clarify-cancel", + { + questions: [ + { id: "first", question: "First question", options: [{ label: "One", value: "one" }] }, + { id: "second", question: "Second question" }, + ], + }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + ); + + respond(await waitForDialog(0), { cancelled: true }); + const result = (await execution) as AgentToolResult; + expect(result.details).toMatchObject({ cancelled: true, answers: [] }); + expect(result.content[0]).toMatchObject({ type: "text", text: "User clarification cancelled" }); + expect(dialogRequests()).toHaveLength(1); + }); + + test("cancels a pending RPC dialog when tool execution is aborted", async () => { + const harness = await startRpc(); + const controller = new AbortController(); + const context = { + ...harness.session.extensionRunner.createContext(), + signal: controller.signal, + } as ExtensionContext; + const execution = clarifyTool(harness).execute( + "clarify-abort", + { question: "Wait for an answer", options: [{ label: "One", value: "one" }] }, + controller.signal, + undefined, + context, + ); + let settled = false; + const trackedExecution = execution.finally(() => { + settled = true; + }); + const request = await waitForDialog(0); + + controller.abort(); + const result = (await trackedExecution) as AgentToolResult; + expect(settled).toBe(true); + expect(result.details).toMatchObject({ cancelled: true, answers: [] }); + expect(result.content[0]).toMatchObject({ type: "text", text: "User clarification cancelled" }); + expect(dialogRequests()).toHaveLength(1); + const outputCount = rpcIo.outputLines.length; + respond(request, { value: "One" }); + expect(rpcIo.outputLines).toHaveLength(outputCount); + }); + + test("validates questionnaire arguments before emitting an RPC dialog", async () => { + const harness = await startRpc(); + const result = (await clarifyTool(harness).execute( + "clarify-invalid", + { question: "Choose one", options: [], allow_freeform: false }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + )) as AgentToolResult; + + expect(result.details?.cancelled).toBe(true); + expect(result.content[0]).toMatchObject({ + type: "text", + text: "Error: allow_freeform=false requires at least one option", + }); + expect(dialogRequests()).toHaveLength(0); + }); + + test("rejects an empty freeform answer without reporting user cancellation", async () => { + const harness = await startRpc(); + const execution = clarifyTool(harness).execute( + "clarify-empty", + { question: "Any notes?" }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + ); + + respond(await waitForDialog(0), { value: "" }); + const result = (await execution) as AgentToolResult; + expect(result.details).toMatchObject({ cancelled: true, answers: [] }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: "Error: clarify_user requires a non-empty freeform answer", + }); + expect(dialogRequests()).toHaveLength(1); + const renderResult = clarifyTool(harness).renderResult; + expect(renderResult).toBeDefined(); + const rendered = renderResult?.( + result, + {} as never, + { fg: (_color: string, text: string) => text } as unknown as never, + {} as never, + ); + expect(rendered?.render(80).join("\n")).toContain("Error: clarify_user requires a non-empty freeform answer"); + }); + + test("rejects whitespace after the custom option without reporting user cancellation", async () => { + const harness = await startRpc(); + const execution = clarifyTool(harness).execute( + "clarify-whitespace", + { question: "Choose one", options: [{ label: "One", value: "one" }] }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + ); + + respond(await waitForDialog(0), { value: "Type a custom answer" }); + respond(await waitForDialog(1), { value: " " }); + const result = (await execution) as AgentToolResult; + expect(result.details).toMatchObject({ cancelled: true, answers: [] }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: "Error: clarify_user requires a non-empty freeform answer", + }); + expect(dialogRequests()).toHaveLength(2); + }); + + test("rejects an unoffered option without reporting user cancellation", async () => { + const harness = await startRpc(); + const execution = clarifyTool(harness).execute( + "clarify-unoffered", + { + questions: [ + { id: "first", question: "First question", options: [{ label: "One", value: "one" }] }, + { id: "second", question: "Second question" }, + ], + }, + undefined, + undefined, + harness.session.extensionRunner.createContext(), + ); + + respond(await waitForDialog(0), { value: "Not offered" }); + const result = (await execution) as AgentToolResult; + expect(result.details).toMatchObject({ cancelled: true, answers: [] }); + expect(result.content[0]).toMatchObject({ + type: "text", + text: "Error: clarify_user received an option that was not offered", + }); + expect(dialogRequests()).toHaveLength(1); + }); + + test("rejects duplicate question ids and option labels before any dialog", async () => { + const harness = await createQuestionnaireHarness(); + const baseContext = harness.session.extensionRunner.createContext(); + const context = { + ...baseContext, + mode: "rpc", + hasUI: true, + ui: { + ...baseContext.ui, + select: async () => { + throw new Error("duplicate labels must be rejected before select"); + }, + input: async () => { + throw new Error("duplicate ids must be rejected before input"); + }, + }, + } as ExtensionContext; + const tool = clarifyTool(harness); + + const duplicateIds = (await tool.execute( + "clarify-duplicate-ids", + { + questions: [ + { id: "same", question: "First question" }, + { id: "same", question: "Second question" }, + ], + }, + undefined, + undefined, + context, + )) as AgentToolResult; + expect(duplicateIds.details).toMatchObject({ cancelled: true, answers: [] }); + expect(duplicateIds.content[0]).toMatchObject({ + type: "text", + text: "Error: question ids must be unique", + }); + + const duplicateLabels = (await tool.execute( + "clarify-duplicate-labels", + { + question: "Choose one", + options: [ + { label: "Same", value: "one" }, + { label: "Same", value: "two" }, + ], + }, + undefined, + undefined, + context, + )) as AgentToolResult; + expect(duplicateLabels.details).toMatchObject({ cancelled: true, answers: [] }); + expect(duplicateLabels.content[0]).toMatchObject({ + type: "text", + text: "Error: option labels must be unique within each question", + }); + }); + + test("keeps the native TUI dialog when custom UI is available", async () => { + const harness = await createQuestionnaireHarness(); + const baseContext = harness.session.extensionRunner.createContext(); + let customCalls = 0; + const context = { + ...baseContext, + mode: "tui", + hasUI: true, + ui: { + ...baseContext.ui, + select: async () => { + throw new Error("RPC fallback must not replace the native TUI dialog"); + }, + custom: async () => { + customCalls += 1; + return { + questions: [ + { + id: "answer", + label: "Question", + question: "Choose one", + options: [{ label: "Native", value: "native" }], + allowFreeform: false, + }, + ], + answers: [{ id: "answer", value: "native", label: "Native", wasCustom: false, index: 1 }], + cancelled: false, + } satisfies StepQuestionnaireDetails; + }, + }, + } as ExtensionContext; + + const result = (await clarifyTool(harness).execute( + "clarify-tui", + { question: "Choose one", options: [{ label: "Native", value: "native" }], allow_freeform: false }, + undefined, + undefined, + context, + )) as AgentToolResult; + expect(customCalls).toBe(1); + expect(result.details).toMatchObject({ cancelled: false, answers: [{ value: "native", label: "Native" }] }); + }); + + test("keeps rejecting clarification in non-interactive mode", async () => { + const harness = await createQuestionnaireHarness(); + const context = harness.session.extensionRunner.createContext(); + expect(context).toMatchObject({ mode: "print", hasUI: false }); + + const result = (await clarifyTool(harness).execute( + "clarify-print", + { question: "Choose one", options: [{ label: "One", value: "one" }] }, + undefined, + undefined, + context, + )) as AgentToolResult; + expect(result.details?.cancelled).toBe(true); + expect(result.content[0]).toMatchObject({ + type: "text", + text: "Error: clarify_user requires an interactive UI", + }); + }); +});