jb/pi-ask
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: align ask_user payload validation

Browse Source
This commit is contained in:
Erik Kotsiuba
2026-08-12 14:18:22 +02:00
parent 0145055445
commit 852e625d86
10 changed files with 242 additions and 75 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/contract.md
+6 -4
View File
@@ -32,8 +32,9 @@ This document defines the stable external behavior. It does not explain internal
- 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`, ...
- blank optional `title`, question `label`, option `description`, and option `preview` fields are treated as omitted
- question `label` falls back to `Q1`, `Q2`, ...
- option `label` is required in the public schema; before schema validation, a missing or blank string label is derived from a non-empty `value` by replacing hyphens and underscores with spaces and capitalizing the first character
- `type` defaults to `single`
- `required` defaults to `false`
- `required` is metadata only; it never blocks submission
@@ -161,7 +162,8 @@ This document defines the stable external behavior. It does not explain internal
## Output rules
- `cancelled: true` means the user dismissed the flow, UI was unavailable, or the payload was invalid before UI opened
- invalid payloads return `error.kind === "invalid_input"` with structured `issues` and a transcript-friendly `Invalid ask_user payload:` message
- semantically invalid payloads that reach tool execution return `error.kind === "invalid_input"` with structured `issues` and a transcript-friendly `Invalid ask_user payload:` message; their rendered status is `Invalid tool payload`
- payloads missing schema-required fields fail Pi's schema validation before tool execution and use Pi's standard tool-error result without structured `details`
- `mode: "submit"` is normal completion; `mode: "elaborate"` means the user asked the agent to continue with follow-up clarification based on notes
- unanswered questions are omitted from `answers`
- in `mode: "elaborate"`, `answers` contains only committed answers; note-only entries move to `elaboration.items`
@@ -259,7 +261,7 @@ Dirty dismiss:
The rich ask flow uses `ctx.ui.custom()` and opens only in TUI mode. In print, JSON, RPC, or any other non-TUI mode, the tool returns a `Needs user input: ask_user requires interactive TUI mode.` message in `content` and a cancelled result in `details` instead of opening custom UI.
Validation is handled inside the tool so malformed calls produce the same structured error shape as other invalid payloads instead of relying on pre-execution schema failures.
The public tool schema requires question `id` and `prompt` plus option `value` and `label`, so missing required fields fail before execution. The tool still validates trimmed text, uniqueness, question types, option counts, and preview requirements during execution and returns structured issues for those failures. Result rendering falls back to Pi's raw tool-error text when schema validation prevents execution.
The ask flow subscribes to runtime settings updates while open. In practice, this means changing `Auto-submit when answered without notes`, `Confirm dismiss when dirty`, `Double-press review shortcuts`, `Notifications`, `Show footer hints`, resetting config to defaults, or reloading config-backed keymaps can affect the in-progress ask flow immediately instead of only future asks when the change is saved or otherwise applied in memory. Load-time migrations and invalid config handling do not rewrite, rename, or back up the config file; invalid files load defaults for the session and show a notice. `Present single-select as multi-select` is applied when an ask flow is created and does not rewrite question semantics for an already-open flow; use `main.changeQuestionType` for live per-question changes.
src/ask-tool-helpers.ts
+11 -11
View File
@@ -13,13 +13,14 @@ import type {
} from "./types.ts";
export const ASK_TOOL_DESCRIPTION =
"Interactive clarification tool for cases where the next step depends on user preferences, missing requirements, or choosing between multiple valid directions. Ask a short structured interview, collect normalized answers, and continue using those answers explicitly instead of guessing. Supports single-select, multi-select, and preview-pane questions. Always include a machine-readable `value` for every option. Use `preview` only when every option includes `preview` text; descriptions alone are not enough.";
"Interactive clarification tool for cases where the next step depends on user preferences, missing requirements, or choosing between multiple valid directions. Ask a short structured interview, collect normalized answers, and continue using those answers explicitly instead of guessing. Supports single-select, multi-select, and preview-pane questions. Always include a stable `id` and non-empty `prompt` for every question, plus a machine-readable `value` and visible `label` for every option. Use `preview` only when every option includes `preview` text; descriptions alone are not enough.";
export const ASK_TOOL_PROMPT_GUIDELINES = [
"Use `ask_user` before making preference-sensitive decisions about scope, tone, UX, naming, architecture, docs, or implementation direction.",
"When multiple valid directions exist, call `ask_user` with 1-3 concise questions instead of committing to one path on your own.",
"When calling `ask_user`, prefer one focused decision per question. Use short labels. Provide clear, distinct options. Do not add filler options.",
"When calling `ask_user`, always include a non-empty machine-readable `value` for every option.",
"When calling `ask_user`, always include a stable `id` and non-empty `prompt` for every question.",
"When calling `ask_user`, always include a non-empty machine-readable `value` and visible `label` for every option.",
"When calling `ask_user`, choose question `type` from the question semantics: `single` means one answer is expected, `multi` means multiple answers could reasonably be selected, and `preview` means options need preview-pane detail.",
'When calling `ask_user`, use `type: "preview"` only when every option includes non-empty `preview` text. Option descriptions do not satisfy this requirement.',
"After an `ask_user` elaboration or follow-up note, prefer another structured `ask_user` follow-up if a choice is still needed instead of switching to plain-text multiple choice in chat.",
@@ -88,7 +89,7 @@ export function renderAskToolCall(args: unknown, theme: ToolTheme) {
? params.questions
.map(
(question: AskQuestionInput, index) =>
question.label || `Q${index + 1}`
question.label?.trim() || `Q${index + 1}`
)
.join(", ")
: "";
@@ -112,17 +113,16 @@ export function renderAskToolResult(
theme: ToolTheme
) {
const details = result.details;
if (!details) {
if (!(details && Array.isArray(details.questions))) {
const text = result.content[0];
return new Text(text?.type === "text" ? (text.text ?? "") : "", 0, 0);
}
if (details.error) {
return new Text(theme.fg("warning", "Invalid input"), 0, 0);
}
if (details.cancelled) {
return new Text(theme.fg("warning", "Cancelled"), 0, 0);
}
return new Text(renderResultText(details), 0, 0);
const text = renderResultText(details);
return new Text(
details.error || details.cancelled ? theme.fg("warning", text) : text,
0,
0
);
}
function errorResultDetails(
src/ask-tool.ts
+2
View File
@@ -16,6 +16,7 @@ import {
import { getAskConfigStore } from "./config/store.ts";
import type { RemoteAskRuntime } from "./remote-ask.ts";
import { AskParamsSchema } from "./schema.ts";
import { prepareAskParams } from "./state/normalize.ts";
import type { AskParams } from "./types.ts";
import { runAskFlow } from "./ui/controller.ts";
@@ -31,6 +32,7 @@ export function registerAskTool(
"Clarify ambiguous or preference-sensitive decisions with a short interactive interview before proceeding",
promptGuidelines: [...ASK_TOOL_PROMPT_GUIDELINES],
parameters: AskParamsSchema,
prepareArguments: (args) => prepareAskParams(args) as AskParams,
execute: (toolCallId, params, signal, onUpdate, ctx) =>
executeAskTool(
pi,
src/result.ts
+1 -1
View File
@@ -8,7 +8,7 @@ import type { AskResult } from "./types.ts";
export function renderResultText(result: AskResult): string {
if (result.error) {
return "Invalid input";
return "Invalid tool payload";
}
if (result.cancelled) {
return CANCELLED_RESULT_TEXT;
src/schema.ts
+8 -16
View File
@@ -1,17 +1,13 @@
import { Type } from "typebox";
export const AskOptionSchema = Type.Object({
value: Type.Optional(
Type.String({
value: Type.String({
description:
"Required machine-readable value returned for this option in the result",
})
),
label: Type.Optional(
Type.String({
}),
label: Type.String({
description: "Required short visible option label shown in the list",
})
),
}),
description: Type.Optional(
Type.String({
description: "Optional one-line explanation to help the user choose",
@@ -26,23 +22,19 @@ export const AskOptionSchema = Type.Object({
});
export const AskQuestionSchema = Type.Object({
id: Type.Optional(
Type.String({
id: Type.String({
description:
"Required stable question identifier used as the key in returned answers",
})
),
}),
label: Type.Optional(
Type.String({
description: "Short tab label, e.g. Goal, Audience, Tone, Scope",
})
),
prompt: Type.Optional(
Type.String({
prompt: Type.String({
description:
"Required direct question shown to the user; ask about one decision at a time",
})
),
}),
type: Type.Optional(
Type.String({
description:
src/state/create.ts
+1 -1
View File
@@ -12,7 +12,7 @@ export function createInitialState(
options: CreateInitialStateOptions = {}
) {
return createBaseState({
title: params.title,
title: params.title?.trim() || undefined,
questions: normalizeQuestions(params, options),
});
}
src/state/normalize.ts
+47 -31
View File
@@ -38,6 +38,51 @@ export function collectValidationIssues(
return collector.issues;
}
export function prepareAskParams(input: unknown): unknown {
if (!(isRecord(input) && Array.isArray(input.questions))) {
return input;
}
return {
...input,
questions: input.questions.map((question) => {
if (!(isRecord(question) && Array.isArray(question.options))) {
return question;
}
return {
...question,
options: question.options.map(prepareOption),
};
}),
};
}
function prepareOption(input: unknown): unknown {
if (!isRecord(input)) {
return input;
}
if (
input.label !== undefined &&
!(typeof input.label === "string" && !input.label.trim())
) {
return input;
}
if (typeof input.value !== "string") {
return input;
}
const words = input.value.replace(/[_-]+/g, " ").replace(/\s+/g, " ").trim();
if (!words) {
return input;
}
return {
...input,
label: words.charAt(0).toUpperCase() + words.slice(1),
};
}
function isRecord(value: unknown): value is Record<string, unknown> {
return !!value && typeof value === "object" && !Array.isArray(value);
}
function normalizeQuestion(
question: AskQuestionInput,
index: number,
@@ -65,8 +110,8 @@ function normalizeOption(option: AskOption): AskOption {
return {
value: option.value.trim(),
label: option.label.trim(),
description: option.description?.trim(),
preview: option.preview?.trim(),
description: option.description?.trim() || undefined,
preview: option.preview?.trim() || undefined,
...(option.freeform ? { freeform: true } : {}),
};
}
@@ -118,12 +163,6 @@ function validateQuestion(
`${questionPath}.id`,
`Question ${questionNumber}: duplicate question id "${questionId}"`
);
assertOptionalText(
question.label,
collector,
`${questionPath}.label`,
`Question ${questionNumber}: label must not be empty`
);
assertRequired(
question.prompt?.trim(),
collector,
@@ -218,18 +257,6 @@ function validateOption(
`${optionPath}.label`,
`${prefix}: label is required`
);
assertOptionalText(
option.description,
collector,
`${optionPath}.description`,
`${prefix}: description must not be empty`
);
assertOptionalText(
option.preview,
collector,
`${optionPath}.preview`,
`${prefix}: preview must not be empty`
);
if (questionType === "preview") {
assertRequired(
optionPreview,
@@ -287,17 +314,6 @@ function assertRequired(
}
}
function assertOptionalText(
value: string | undefined,
collector: IssueCollector,
path: string,
message: string
) {
if (value !== undefined && !value.trim()) {
collector.add(path, message);
}
}
function assertUnique(
seen: Set<string>,
value: string | undefined,
tests/ask-tool.test.ts
+127 -2
View File
@@ -1,5 +1,6 @@
import assert from "node:assert/strict";
import test from "node:test";
import { validateToolArguments } from "@earendil-works/pi-ai/base";
import { registerAskTool } from "../src/ask-tool.ts";
import type { AskParams } from "../src/types.ts";
@@ -14,6 +15,7 @@ const PREVIEW_RULE_RE =
/questions\[0\]\.options\[0\]\.preview: Question 1, option 1: preview questions require preview text for every option; add preview text or use type "single" instead/;
const MISSING_OPTION_VALUE_RE =
/questions\[0\]\.options\[0\]\.value: Question 1, option 1: value is required/;
const MISSING_OPTION_LABEL_RE = /label/;
const EMPTY_QUESTIONS_RE = /questions: At least one question is required/;
const INVALID_TYPE_RE =
/questions\[0\]\.type: Question 1: invalid type "grid"; expected "single", "multi", or "preview"/;
@@ -37,6 +39,8 @@ function registerMockTool() {
entries,
tool: tools[0] as {
execute: (...args: any[]) => Promise<any>;
parameters: Record<string, any>;
prepareArguments: (args: unknown) => unknown;
renderCall: (args: unknown, theme: any) => { text: string };
renderResult: (
result: any,
@@ -304,7 +308,7 @@ test("ask tool reports preview validation with structured issues", async () => {
id: "layout",
prompt: "Pick layout",
type: "preview",
options: [{ value: "compact", label: "Compact" }],
options: [{ value: "compact", label: "Compact", preview: " " }],
},
],
},
@@ -327,6 +331,109 @@ test("ask tool reports preview validation with structured issues", async () => {
assert.match(result.content[0].text, PREVIEW_RULE_RE);
});
test("ask tool accepts blank optional presentation fields", async () => {
const { tool } = registerMockTool();
const params = validateToolArguments(tool as never, {
type: "toolCall",
id: "call-blank-optionals",
name: "ask_user",
arguments: {
title: " ",
questions: [
{
id: "scope",
label: " ",
prompt: "Pick scope",
options: [
{
value: "small",
label: "Small",
description: " ",
preview: "\t",
},
],
},
],
},
}) as AskParams;
const result = await tool.execute(
"call-blank-optionals",
params,
undefined,
noop,
makeCtx(false)
);
assert.equal(result.details.error, undefined);
assert.equal(result.details.title, undefined);
assert.equal(result.details.questions[0].label, "Q1");
});
test("ask tool prepares missing and blank option labels before schema validation", async () => {
const { tool } = registerMockTool();
const raw = {
questions: [
{
id: "encryption",
prompt: "Choose encryption",
options: [
{ value: "region-local-kms-reencrypt-dek" },
{ value: "follow_up", label: " " },
{ value: "existing", label: "Existing label" },
],
},
],
};
assert.equal(Value.Check(AskParamsSchema, raw), false);
const prepared = tool.prepareArguments(raw) as AskParams;
assert.equal(Value.Check(AskParamsSchema, prepared), true);
assert.deepEqual(
prepared.questions[0]?.options.map((option) => option.label),
["Region local kms reencrypt dek", "Follow up", "Existing label"]
);
const result = await tool.execute(
"call-prepared-labels",
prepared,
undefined,
noop,
makeCtx(false)
);
assert.equal(result.details.error, undefined);
});
test("public schema requires semantic identifiers and labels", () => {
const { tool } = registerMockTool();
assert.deepEqual(tool.parameters.required, ["questions"]);
const questionSchema = tool.parameters.properties.questions.items;
assert.deepEqual(questionSchema.required, ["id", "prompt", "options"]);
assert.deepEqual(questionSchema.properties.options.items.required, [
"value",
"label",
]);
const missingLabel = {
questions: [
{
id: "scope",
prompt: "Pick scope",
options: [{ value: "small" }],
},
],
};
assert.throws(
() =>
validateToolArguments(tool as never, {
type: "toolCall",
id: "call-missing-label",
name: "ask_user",
arguments: missingLabel,
}),
MISSING_OPTION_LABEL_RE
);
});
test("ask tool transcript renderers summarize call and cancelled result", () => {
const { tool } = registerMockTool();
const theme = {
@@ -377,5 +484,23 @@ test("ask tool transcript renderers summarize call and cancelled result", () =>
undefined,
theme
).text;
assert.equal(invalidText, "Invalid input");
assert.equal(invalidText, "Invalid tool payload");
const schemaErrorText = tool.renderResult(
{
content: [
{
type: "text",
text: 'Validation failed for tool "ask_user": missing label',
},
],
details: {},
},
undefined,
theme
).text;
assert.equal(
schemaErrorText,
'Validation failed for tool "ask_user": missing label'
);
});
tests/result.test.ts
+2 -2
View File
@@ -568,7 +568,7 @@ test("elaborate continuation preserves unrelated committed answers", () => {
});
});
test("invalid input result renders as invalid input", () => {
test("invalid tool payload result is attributed to the tool call", () => {
const result = {
title: "Interview",
cancelled: true,
@@ -581,7 +581,7 @@ test("invalid input result renders as invalid input", () => {
},
};
assert.equal(renderResultText(result), "Invalid input");
assert.equal(renderResultText(result), "Invalid tool payload");
});
test("cancelled result renders as cancelled", () => {
tests/state.test.ts
+30
View File
@@ -254,6 +254,36 @@ test("normalize trims ids, prompts, and option text", () => {
});
});
test("normalize treats blank optional presentation text as absent", () => {
const state = createInitialState({
title: " ",
questions: [
{
id: "scope",
label: " ",
prompt: "Pick scope",
options: [
{
value: "small",
label: "Small",
description: " ",
preview: "\t",
},
],
},
],
});
assert.equal(state.title, undefined);
assert.equal(state.questions[0].label, "Q1");
assert.deepEqual(state.questions[0].options[0], {
value: "small",
label: "Small",
description: undefined,
preview: undefined,
});
});
test("single-select number shortcut stores answer and advances to next tab", () => {
let state = createInitialState(sampleParams());
state = applyNumberShortcut(state, 2);