feat: harden ask tool validation and fallback

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

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

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
 

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,
+					details: {
-				};
+						...toAskResult(validation.state),
-			}
+						cancelled: true,
-
+					},
-			if (!params.questions?.length) {
-				const details = errorResultDetails(params, true);
-				return {
-					content: [{ type: "text", text: "Error: No questions provided" }],
-					details,
 				};
 			}
 
@@ -106,3 +108,46 @@ function errorResultDetails(params: AskParams, cancelled: boolean): AskResult {
 		};
 	}
 }
+
+function validateParams(
+	params: AskParams
+):
+	| { ok: true; state: ReturnType<typeof createInitialState> }
+	| { 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<typeof createInitialState>
+): 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");
+}

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",
 	}),
 });

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<string>();
+	for (const [questionIndex, question] of questions.entries()) {
+		validateQuestion(question, questionIndex, questionIds);
+	}
+}
+
+function validateQuestion(
+	question: AskQuestionInput,
+	questionIndex: number,
+	questionIds: Set<string>
+): 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<string>();
+	for (const [optionIndex, option] of question.options.entries()) {
+		validateOption(
+			option,
+			optionIndex,
+			optionValues,
+			questionNumber,
+			questionType
+		);
+	}
+}
+
+function validateOption(
+	option: AskOption,
+	optionIndex: number,
+	optionValues: Set<string>,
+	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<string>,
+	value: string,
+	errorMessage: string
+): void {
+	if (seen.has(value)) {
+		throw new Error(errorMessage);
+	}
+	seen.add(value);
+}

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<string, unknown>[] = [];
+	registerAskTool({
+		registerTool(tool: unknown) {
+			tools.push(tool as Record<string, unknown>);
+		},
+	} as never);
+	return tools[0] as {
+		execute: (...args: any[]) => Promise<any>;
+		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");
+});

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);