diff --git a/README.md b/README.md index cab2254..222a4f4 100644 --- a/README.md +++ b/README.md @@ -150,13 +150,18 @@ The returned `details.answers[questionId]` object may include: Behavior details: +- payloads are validated before the UI opens: question ids must be unique, option values must be unique within a question, and required text fields must not be blank +- preview questions require preview text for every option +- `required` defaults to `false` and remains advisory only - question-level notes are submitted whenever authored - option notes can be authored for any active option during the UI flow - only notes for currently selected options are included in the submitted result - deselecting an option keeps its note in UI state, so re-selecting it restores the note - empty note text clears the note +- on single-select questions, saving a free-form answer replaces selected options for that question - on multi-select questions, saving a free-form answer keeps other selected options intact -- on multi-select questions, submitted `values` and `labels` include both selected options and the free-form answer when both exist +- on multi-select questions, submitted `values` and `labels` keep selected options first and append the free-form answer last when both exist +- unanswered questions are omitted from `details.answers` - when the free-form option is selected, it becomes an inline input row with the selected-tab background style spanning the full width - while editing a note or free-form answer, arrow keys and `Tab` stay inside the editor so the typing cursor can move naturally - free-form answer editors support pi-style `@` file path autocomplete for quickly mentioning project files @@ -165,6 +170,8 @@ Behavior details: - use `n` for the active option note and `Shift+N` for the current question note - `Space` toggles the active option on single-select questions too, but does not auto-advance +If pi runs without UI, `ask_user` returns a `Needs user input` message plus normalized pending questions in `details.questions` so a caller can re-ask them manually. + ## Local development ### Run locally in pi diff --git a/docs/architecture.md b/docs/architecture.md index fe167c5..af23aa0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -53,7 +53,8 @@ The codebase is split so the implementation reads through file boundaries and na - submit is never blocked by unanswered questions - single-select answers serialize as arrays -- free-form answers replace selected options for that question +- single-select free-form answers replace selected options for that question +- multi-select free-form answers augment selected options instead of clearing them - preview questions do not get the synthetic custom-answer option - deselected option notes stay in UI state - only selected option notes are emitted in the final result diff --git a/docs/contract.md b/docs/contract.md index 2c44d1d..4cb6b3a 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -28,9 +28,16 @@ This document defines the stable external behavior. It does not explain internal ## Input rules - at least one question is required +- every question must have non-empty trimmed `id` and `prompt` +- every question must have at least one option +- question ids must be unique within one tool call +- option `value`s must be unique within a question +- optional `label`, `description`, and `preview` fields must not be blank when provided - `label` falls back to `Q1`, `Q2`, ... - `type` defaults to `single` +- `required` defaults to `false` - `required` is metadata only; it never blocks submission +- preview questions require preview text for every option - non-`preview` questions get an internal `Type your own` option ## Output @@ -64,11 +71,14 @@ This document defines the stable external behavior. It does not explain internal ## Output rules -- `cancelled: true` means the user dismissed the flow or UI was unavailable +- `cancelled: true` means the user dismissed the flow, UI was unavailable, or the payload was invalid before UI opened +- unanswered questions are omitted from `answers` - single-select answers still use arrays - `indices` are 1-based rendered option positions - `customText` stores the free-form answer +- on single-select questions, saving free-form text clears selected options for that question - on multi-select questions, `values` and `labels` include both selected options and `customText` when both are present +- on multi-select questions, selected options keep their original order and `customText` is appended last - saving or clearing free-form text on a multi-select question does not clear other selected options - `note` stores a question-level note - `optionNotes` includes only notes for selected options @@ -110,7 +120,9 @@ Editing flow: ## Non-interactive mode -If `ctx.hasUI === false`, the tool returns an error message in `content` and a cancelled result in `details`. +If `ctx.hasUI === false`, the tool returns a `Needs user input` message in `content` and a cancelled result in `details`. + +The fallback message includes normalized pending questions and options so the caller can re-ask them manually. `details.questions` still contains normalized question metadata, while `details.answers` stays empty until a user responds. ## Source of truth diff --git a/src/ask-tool.ts b/src/ask-tool.ts index 6aa21ad..8f4bb32 100644 --- a/src/ask-tool.ts +++ b/src/ask-tool.ts @@ -25,24 +25,26 @@ export function registerAskTool(pi: ExtensionAPI) { parameters: AskParamsSchema, async execute(_toolCallId, params: AskParams, _signal, _onUpdate, ctx) { + const validation = validateParams(params); + if (!validation.ok) { + return { + content: [{ type: "text", text: `Error: ${validation.error}` }], + details: errorResultDetails(params, true), + }; + } + if (!ctx.hasUI) { - const details = errorResultDetails(params, true); return { content: [ { type: "text", - text: "Error: UI not available (running in non-interactive mode)", + text: formatNonInteractiveMessage(validation.state), }, ], - details, - }; - } - - if (!params.questions?.length) { - const details = errorResultDetails(params, true); - return { - content: [{ type: "text", text: "Error: No questions provided" }], - details, + details: { + ...toAskResult(validation.state), + cancelled: true, + }, }; } @@ -106,3 +108,46 @@ function errorResultDetails(params: AskParams, cancelled: boolean): AskResult { }; } } + +function validateParams( + params: AskParams +): + | { ok: true; state: ReturnType } + | { ok: false; error: string } { + try { + return { + ok: true, + state: createInitialState(params), + }; + } catch (error) { + return { + ok: false, + error: + error instanceof Error ? error.message : "Invalid ask_user payload", + }; + } +} + +function formatNonInteractiveMessage( + state: ReturnType +): string { + const lines = [ + "Needs user input: ask_user requires interactive UI.", + "Run same tool call in interactive session, or ask user these questions manually:", + ]; + + for (const [index, question] of state.questions.entries()) { + lines.push(`${index + 1}. ${question.label}: ${question.prompt}`); + for (const option of question.options) { + lines.push(` - ${option.label} [${option.value}]`); + } + if (question.type !== "preview") { + lines.push(" - Type your own [custom]"); + } + } + + lines.push( + "details.questions contains normalized pending questions. details.answers stays empty until user responds." + ); + return lines.join("\n"); +} diff --git a/src/schema.ts b/src/schema.ts index 860fe84..2fb2c66 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -2,19 +2,23 @@ import { Type } from "@sinclair/typebox"; export const AskOptionSchema = Type.Object({ value: Type.String({ + minLength: 1, description: "Machine-readable value returned for this option in the result", }), label: Type.String({ + minLength: 1, description: "Short visible option label shown in the list", }), description: Type.Optional( Type.String({ + minLength: 1, description: "Optional one-line explanation to help the user choose", }) ), preview: Type.Optional( Type.String({ + minLength: 1, description: "Optional preview content shown in the dedicated preview pane for preview questions", }) @@ -23,15 +27,18 @@ export const AskOptionSchema = Type.Object({ export const AskQuestionSchema = Type.Object({ id: Type.String({ + minLength: 1, description: "Stable question identifier used as the key in returned answers", }), label: Type.Optional( Type.String({ + minLength: 1, description: "Short tab label, e.g. Goal, Audience, Tone, Scope", }) ), prompt: Type.String({ + minLength: 1, description: "Direct question shown to the user; ask about one decision at a time", }), @@ -51,6 +58,7 @@ export const AskQuestionSchema = Type.Object({ }) ), options: Type.Array(AskOptionSchema, { + minItems: 1, description: "Answer options; prefer 2-4 clear choices when possible", }), }); @@ -58,11 +66,13 @@ export const AskQuestionSchema = Type.Object({ export const AskParamsSchema = Type.Object({ title: Type.Optional( Type.String({ + minLength: 1, description: "Optional short title shown above the clarification flow, e.g. README direction", }) ), questions: Type.Array(AskQuestionSchema, { + minItems: 1, description: "Questions to ask in the interactive clarification flow", }), }); diff --git a/src/state/normalize.ts b/src/state/normalize.ts index c4bce30..9238dd1 100644 --- a/src/state/normalize.ts +++ b/src/state/normalize.ts @@ -1,6 +1,12 @@ -import type { AskParams, AskQuestion, AskQuestionInput } from "../types.ts"; +import type { + AskOption, + AskParams, + AskQuestion, + AskQuestionInput, +} from "../types.ts"; export function normalizeQuestions(params: AskParams): AskQuestion[] { + validateQuestions(params.questions); return params.questions.map((question, index) => normalizeQuestion(question, index) ); @@ -11,9 +17,137 @@ function normalizeQuestion( index: number ): AskQuestion { return { - ...question, + id: question.id.trim(), label: question.label?.trim() || `Q${index + 1}`, + prompt: question.prompt.trim(), type: question.type ?? "single", - required: question.required !== false, + required: question.required ?? false, + options: question.options.map(normalizeOption), }; } + +function normalizeOption(option: AskOption): AskOption { + return { + value: option.value.trim(), + label: option.label.trim(), + description: option.description?.trim(), + preview: option.preview?.trim(), + }; +} + +function validateQuestions(questions: AskParams["questions"]): void { + if (questions.length === 0) { + throw new Error("At least one question is required"); + } + + const questionIds = new Set(); + for (const [questionIndex, question] of questions.entries()) { + validateQuestion(question, questionIndex, questionIds); + } +} + +function validateQuestion( + question: AskQuestionInput, + questionIndex: number, + questionIds: Set +): void { + const questionNumber = questionIndex + 1; + const questionId = question.id.trim(); + const questionType = question.type ?? "single"; + + assertNonEmpty(questionId, `Question ${questionNumber}: id is required`); + assertUnique( + questionIds, + questionId, + `Question ${questionNumber}: duplicate question id "${questionId}"` + ); + assertOptionalNonEmpty( + question.label, + `Question ${questionNumber}: label must not be empty` + ); + assertNonEmpty( + question.prompt.trim(), + `Question ${questionNumber}: prompt is required` + ); + assertHasItems( + question.options, + `Question ${questionNumber}: at least one option is required` + ); + + const optionValues = new Set(); + for (const [optionIndex, option] of question.options.entries()) { + validateOption( + option, + optionIndex, + optionValues, + questionNumber, + questionType + ); + } +} + +function validateOption( + option: AskOption, + optionIndex: number, + optionValues: Set, + questionNumber: number, + questionType: AskQuestion["type"] +): void { + const optionNumber = optionIndex + 1; + const prefix = `Question ${questionNumber}, option ${optionNumber}`; + const optionValue = option.value.trim(); + + assertNonEmpty(optionValue, `${prefix}: value is required`); + assertUnique( + optionValues, + optionValue, + `${prefix}: duplicate option value "${optionValue}"` + ); + assertNonEmpty(option.label.trim(), `${prefix}: label is required`); + assertOptionalNonEmpty( + option.description, + `${prefix}: description must not be empty` + ); + assertOptionalNonEmpty( + option.preview, + `${prefix}: preview must not be empty` + ); + if (questionType === "preview") { + assertNonEmpty( + option.preview?.trim(), + `${prefix}: preview questions require preview text for every option` + ); + } +} + +function assertHasItems(items: unknown[], errorMessage: string): void { + if (items.length === 0) { + throw new Error(errorMessage); + } +} + +function assertNonEmpty(value: string | undefined, errorMessage: string): void { + if (!value) { + throw new Error(errorMessage); + } +} + +function assertOptionalNonEmpty( + value: string | undefined, + errorMessage: string +): void { + if (value !== undefined && !value.trim()) { + throw new Error(errorMessage); + } +} + +function assertUnique( + seen: Set, + value: string, + errorMessage: string +): void { + if (seen.has(value)) { + throw new Error(errorMessage); + } + seen.add(value); +} diff --git a/tests/ask-tool.test.ts b/tests/ask-tool.test.ts new file mode 100644 index 0000000..3e2c7d3 --- /dev/null +++ b/tests/ask-tool.test.ts @@ -0,0 +1,144 @@ +import assert from "node:assert/strict"; +import test from "node:test"; +import { registerAskTool } from "../src/ask-tool.ts"; +import type { AskParams } from "../src/types.ts"; + +const NON_INTERACTIVE_MESSAGE_RE = + /Needs user input: ask_user requires interactive UI\./; +const FIRST_QUESTION_RE = /1\. Goal: What should I optimize for\?/; +const SPEED_OPTION_RE = /- Speed \[speed\]/; +const CUSTOM_OPTION_RE = /- Type your own \[custom\]/; +const DUPLICATE_ID_RE = /duplicate question id "scope"/; +const HAS_UI = "hasUI"; +const noop = () => { + // intentional test callback +}; + +function registerMockTool() { + const tools: Record[] = []; + registerAskTool({ + registerTool(tool: unknown) { + tools.push(tool as Record); + }, + } as never); + return tools[0] as { + execute: (...args: any[]) => Promise; + renderCall: (args: unknown, theme: any) => { text: string }; + renderResult: ( + result: any, + options: unknown, + theme: any + ) => { text: string }; + }; +} + +function makeCtx(hasUi: boolean): unknown { + return { [HAS_UI]: hasUi }; +} + +function sampleParams(): AskParams { + return { + title: "Clarify next step", + questions: [ + { + id: "goal", + label: "Goal", + prompt: "What should I optimize for?", + options: [ + { value: "speed", label: "Speed" }, + { value: "safety", label: "Safety" }, + ], + }, + ], + }; +} + +test("ask tool returns pending questions in non-interactive mode", async () => { + const tool = registerMockTool(); + + const result = await tool.execute( + "call-1", + sampleParams(), + undefined, + noop, + makeCtx(false) + ); + + assert.equal(result.details.cancelled, true); + assert.deepEqual(result.details.questions, [ + { + id: "goal", + label: "Goal", + prompt: "What should I optimize for?", + type: "single", + }, + ]); + assert.deepEqual(result.details.answers, {}); + assert.match(result.content[0].text, NON_INTERACTIVE_MESSAGE_RE); + assert.match(result.content[0].text, FIRST_QUESTION_RE); + assert.match(result.content[0].text, SPEED_OPTION_RE); + assert.match(result.content[0].text, CUSTOM_OPTION_RE); +}); + +test("ask tool rejects invalid payloads before UI opens", async () => { + const tool = registerMockTool(); + + const result = await tool.execute( + "call-1", + { + title: "Clarify", + questions: [ + { + id: "scope", + prompt: "Pick scope", + options: [{ value: "small", label: "Small" }], + }, + { + id: "scope", + prompt: "Pick tone", + options: [{ value: "direct", label: "Direct" }], + }, + ], + }, + undefined, + noop, + makeCtx(true) + ); + + assert.equal(result.details.cancelled, true); + assert.equal(result.details.questions.length, 0); + assert.match(result.content[0].text, DUPLICATE_ID_RE); +}); + +test("ask tool transcript renderers summarize call and cancelled result", () => { + const tool = registerMockTool(); + const theme = { + bold: (text: string) => text, + fg: (_token: string, text: string) => text, + }; + + const callText = tool.renderCall( + { + questions: [ + { label: "Goal", options: [], prompt: "Q?", id: "goal" }, + { label: "Tone", options: [], prompt: "Q?", id: "tone" }, + ], + }, + theme + ).text; + assert.equal(callText, "ask_user 2 question(s) (Goal, Tone)"); + + const resultText = tool.renderResult( + { + content: [{ type: "text", text: "ignored" }], + details: { + cancelled: true, + questions: [], + answers: {}, + }, + }, + undefined, + theme + ).text; + assert.equal(resultText, "Cancelled"); +}); diff --git a/tests/state.test.ts b/tests/state.test.ts index b4c4be1..6bf32cc 100644 --- a/tests/state.test.ts +++ b/tests/state.test.ts @@ -1,6 +1,12 @@ import assert from "node:assert/strict"; import test from "node:test"; import { createInitialState } from "../src/state/create.ts"; + +const DUPLICATE_QUESTION_ID_RE = /duplicate question id "scope"/; +const DUPLICATE_OPTION_VALUE_RE = /duplicate option value "small"/; +const PREVIEW_REQUIRED_RE = + /preview questions require preview text for every option/; + import { toAskResult } from "../src/state/result.ts"; import { getRenderableOptions } from "../src/state/selectors.ts"; import { @@ -59,7 +65,7 @@ test("normalize defaults via initial state", () => { assert.equal(state.questions[0].label, "Q1"); assert.equal(state.questions[0].type, "single"); - assert.equal(state.questions[0].required, true); + assert.equal(state.questions[0].required, false); assert.equal( getRenderableOptions(state.questions[0]).at(-1)?.label, "Type your own" @@ -92,6 +98,92 @@ test("preview questions keep their type and option previews", () => { assert.equal(getRenderableOptions(state.questions[0]).length, 1); }); +test("createInitialState rejects duplicate question ids", () => { + assert.throws( + () => + createInitialState({ + questions: [ + { + id: "scope", + prompt: "What scope?", + options: [{ value: "small", label: "Small" }], + }, + { + id: "scope", + prompt: "What tone?", + options: [{ value: "direct", label: "Direct" }], + }, + ], + }), + DUPLICATE_QUESTION_ID_RE + ); +}); + +test("createInitialState rejects duplicate option values within one question", () => { + assert.throws( + () => + createInitialState({ + questions: [ + { + id: "scope", + prompt: "What scope?", + options: [ + { value: "small", label: "Small" }, + { value: "small", label: "Also small" }, + ], + }, + ], + }), + DUPLICATE_OPTION_VALUE_RE + ); +}); + +test("preview questions require preview text for every option", () => { + assert.throws( + () => + createInitialState({ + questions: [ + { + id: "layout", + prompt: "Pick layout", + type: "preview", + options: [{ value: "compact", label: "Compact" }], + }, + ], + }), + PREVIEW_REQUIRED_RE + ); +}); + +test("normalize trims ids, prompts, and option text", () => { + const state = createInitialState({ + questions: [ + { + id: " scope ", + label: " Scope ", + prompt: " What scope? ", + options: [ + { + value: " small ", + label: " Small ", + description: " Lowest risk ", + }, + ], + }, + ], + }); + + assert.equal(state.questions[0].id, "scope"); + assert.equal(state.questions[0].label, "Scope"); + assert.equal(state.questions[0].prompt, "What scope?"); + assert.deepEqual(state.questions[0].options[0], { + value: "small", + label: "Small", + description: "Lowest risk", + preview: undefined, + }); +}); + test("single-select number shortcut stores answer and advances to next tab", () => { let state = createInitialState(sampleParams()); state = applyNumberShortcut(state, 2);