snowspeeder/pi-web
Archived
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(ui): streamline ask_user question forms

Browse Source
This commit is contained in:
Federico Jaramillo Martinez
2026-07-27 10:55:49 +02:00
parent 4e47c23f23
commit 0e83146315
21 changed files with 261 additions and 209 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/server/sessions/askUserTool.test.ts
+6 -4
View File
@@ -29,7 +29,7 @@ function toolOverStore(askIds: string[] = ["ask-1", "ask-2"]) {
const twoQuestions = {
questions: [
{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }, { value: "sqlite", label: "SQLite" }] },
{ id: "why", question: "Why?", options: [], allowOther: true },
{ id: "why", question: "Why?", options: [] },
],
};
@@ -53,6 +53,7 @@ describe("createAskUserToolDefinition", () => {
required: ["questions"],
properties: { questions: { type: "array", minItems: 1, maxItems: 20 } },
});
expect(tool.parameters).not.toHaveProperty("properties.questions.items.properties.allowOther");
});
it("opens the ask for the calling session and terminates the run instead of awaiting the user", async () => {
@@ -63,7 +64,7 @@ describe("createAskUserToolDefinition", () => {
expect(open).toHaveBeenCalledWith({
sessionId: "session-1",
questions: [
{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }, { value: "sqlite", label: "SQLite" }] },
{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }, { value: "sqlite", label: "SQLite" }], allowOther: true },
{ id: "why", question: "Why?", options: [], allowOther: true },
],
});
@@ -75,7 +76,7 @@ describe("createAskUserToolDefinition", () => {
it("defaults a question without options to an empty option list so free text alone is expressible", async () => {
const { open, tool } = toolOverStore();
await tool.execute("call-free", { questions: [{ id: "note", question: "Anything else?", allowOther: true }] }, undefined, undefined, ctxFor("session-1"));
await tool.execute("call-free", { questions: [{ id: "note", question: "Anything else?" }] }, undefined, undefined, ctxFor("session-1"));
expect(open).toHaveBeenCalledWith({
sessionId: "session-1",
@@ -83,7 +84,7 @@ describe("createAskUserToolDefinition", () => {
});
});
it("forwards per-question detail, option detail, and multi-select without inventing defaults", async () => {
it("adds custom answers while preserving detail, option detail, and multi-select", async () => {
const { open, tool } = toolOverStore();
await tool.execute("call-rich", {
@@ -103,6 +104,7 @@ describe("createAskUserToolDefinition", () => {
question: "Which targets?",
detail: "Pick every platform we should build for.",
options: [{ value: "web", label: "Web", detail: "Chromium and Firefox" }, { value: "cli", label: "CLI" }],
allowOther: true,
multiple: true,
}],
});
src/server/sessions/askUserTool.ts
+6 -7
View File
@@ -17,7 +17,7 @@ export interface AskUserInvocation {
}
export interface AskUserToolDeps {
/** Registers the ask as the session's open one; rejects question sets the user could not answer. */
/** Registers the ask as the session's open one; rejects malformed question sets. */
open(input: AskUserInvocation): Promise<PendingAskOpenResult>;
}
@@ -53,10 +53,7 @@ const AskUserQuestionParams = Type.Object({
})),
options: Type.Optional(Type.Array(AskUserOptionParams, {
maxItems: ASK_USER_OPTION_LIMIT,
description: "Options to choose from. Omit only when free text is the whole answer, and then set allowOther.",
})),
allowOther: Type.Optional(Type.Boolean({
description: "Offer a free-text field alongside the options.",
description: "Options to choose from. Omit when free text is the whole answer; the browser always adds a Custom choice.",
})),
multiple: Type.Optional(Type.Boolean({
description: "Allow several options at once. Default: one answer per question.",
@@ -73,13 +70,15 @@ const AskUserParams = Type.Object({
/** Shapes one schema question into the domain question; the store owns validation. */
function toQuestion(param: Static<typeof AskUserQuestionParams>): AskUserQuestion {
const { detail, options, allowOther, multiple } = param;
const { detail, options, multiple } = param;
return {
id: param.id,
question: param.question,
...(detail === undefined ? {} : { detail }),
options: (options ?? []).map(toOption),
...(allowOther === undefined ? {} : { allowOther }),
// Keep the compatibility marker on the daemon wire even though the model no
// longer chooses whether a question accepts a custom answer.
allowOther: true,
...(multiple === undefined ? {} : { multiple }),
};
}
src/server/sessions/pendingAskStore.test.ts
+16 -14
View File
@@ -45,7 +45,7 @@ describe("PendingAskStore validation", () => {
question: "Which database?",
detail: "Only the primary store matters here.",
options: [{ value: "pg", label: "Postgres", detail: "Existing cluster" }],
allowOther: true,
allowOther: false,
multiple: false,
},
],
@@ -79,7 +79,6 @@ describe("PendingAskStore validation", () => {
expect(reject([question("q1"), question("q1")])).toThrow(/Duplicate question id q1/);
expect(reject([question(" ")])).toThrow(/question id must not be empty/);
expect(reject([question("q1", { question: " " })])).toThrow(/text of question q1 must not be empty/);
expect(reject([question("q1", { options: [] })])).toThrow(/must offer options or allow other text/);
expect(reject([question("q1", { options: [{ value: "a", label: "A" }, { value: "a", label: "Again" }] })]))
.toThrow(/Duplicate option value a in question q1/);
expect(reject([question("q1", { options: [{ value: "a", label: " " }] })]))
@@ -90,9 +89,9 @@ describe("PendingAskStore validation", () => {
expect(store.pendingAsk(sessionId)).toBeUndefined();
});
it("accepts a question that only offers free text", () => {
it("accepts an optionless question and adds the custom-answer compatibility marker", () => {
const store = testStore();
const { ask } = store.open({ sessionId, questions: [question("q1", { options: [], allowOther: true })] });
const { ask } = store.open({ sessionId, questions: [question("q1", { options: [] })] });
expect(ask.questions[0]).toEqual({ id: "q1", question: "Question q1?", options: [], allowOther: true });
});
});
@@ -143,7 +142,7 @@ describe("PendingAskStore submit", () => {
it("treats an empty answer as leaving the question untouched", () => {
const store = testStore();
const { ask } = store.open({ sessionId, questions: [question("q1"), question("q2", { allowOther: true })] });
const { ask } = store.open({ sessionId, questions: [question("q1"), question("q2")] });
const result = store.submit(sessionId, ask.askId, {
answers: [{ id: "q1", values: [] }, { id: "q2", values: [], otherText: " " }],
@@ -161,11 +160,11 @@ describe("PendingAskStore submit", () => {
expect(store.pendingAsk(sessionId)?.askId).toBe(ask.askId);
});
it("accepts several values and coexisting other text for a multi-select question", () => {
it("accepts several values and coexisting custom text for a multi-select question", () => {
const store = testStore();
const { ask } = store.open({
sessionId,
questions: [question("q1", { multiple: true, allowOther: true })],
questions: [question("q1", { multiple: true })],
});
const result = store.submit(sessionId, ask.askId, {
@@ -181,17 +180,20 @@ describe("PendingAskStore submit", () => {
});
});
it("rejects other text for a question that does not allow it", () => {
it("accepts custom text for every question", () => {
const store = testStore();
const { ask } = openTwoQuestions(store);
expect(() => store.submit(sessionId, ask.askId, { answers: [{ id: "q1", values: [], otherText: "custom" }] }))
.toThrow(/Question q1 does not accept other text/);
const result = store.submit(sessionId, ask.askId, { answers: [{ id: "q1", values: [], otherText: "custom" }] });
expect(result).toMatchObject({ status: "closed", outcome: { answeredCount: 1 } });
if (result.status !== "closed") throw new Error("expected the ask to close");
expect(result.outcome.questions[0]).toMatchObject({ answered: true, values: [], otherText: "custom" });
});
it("answers a free-text-only question with other text alone", () => {
it("answers a free-text-only question with custom text alone", () => {
const store = testStore();
const { ask } = store.open({ sessionId, questions: [question("q1", { options: [], allowOther: true })] });
const { ask } = store.open({ sessionId, questions: [question("q1", { options: [] })] });
const result = store.submit(sessionId, ask.askId, { answers: [{ id: "q1", values: [], otherText: "a note" }] });
@@ -294,7 +296,7 @@ describe("ask outcome rendering", () => {
const store = testStore();
const { ask } = store.open({
sessionId,
questions: [question("q1"), question("q2", { allowOther: true }), question("q3")],
questions: [question("q1"), question("q2"), question("q3")],
});
const result = store.submit(sessionId, ask.askId, {
answers: [{ id: "q1", values: ["yes"] }, { id: "q2", values: [], otherText: "something else" }],
@@ -307,7 +309,7 @@ describe("ask outcome rendering", () => {
"- q1: Question q1?",
" Answered: selected yes",
"- q2: Question q2?",
` Answered: other: "something else"`,
` Answered: custom: "something else"`,
"- q3: Question q3?",
" Unanswered.",
"",
src/server/sessions/pendingAskStore.ts
+5 -10
View File
@@ -47,7 +47,7 @@ export type PendingAskCloseResult =
| { status: "closed"; outcome: AskUserOutcome }
| { status: "stale" };
/** Rejected input: the model asked something unanswerable, or an answer does not fit its question. */
/** Rejected input: a question set is malformed, or an answer does not fit its question. */
export class PendingAskValidationError extends Error {
constructor(message: string) {
super(message);
@@ -181,7 +181,7 @@ function questionLines(record: AskUserQuestionRecord): string[] {
const header = `- ${record.question.id}: ${record.question.question}`;
if (!record.answered) return [header, " Unanswered."];
const parts = [...record.values.map((value) => `selected ${value}`)];
if (record.otherText !== undefined) parts.push(`other: ${JSON.stringify(record.otherText)}`);
if (record.otherText !== undefined) parts.push(`custom: ${JSON.stringify(record.otherText)}`);
return [header, ` Answered: ${parts.join("; ")}`];
}
@@ -240,12 +240,6 @@ function validateQuestion(question: AskUserQuestion, id: string): AskUserQuestio
if (question.options.length > ASK_USER_OPTION_LIMIT) {
throw new PendingAskValidationError(`Question ${id} must not offer more than ${ASK_USER_OPTION_LIMIT.toString()} options`);
}
const allowOther = question.allowOther === true;
// A question with neither options nor a free-text field cannot be answered at
// all, which would make its "unanswered" report meaningless.
if (question.options.length === 0 && !allowOther) {
throw new PendingAskValidationError(`Question ${id} must offer options or allow other text`);
}
const seenValues = new Set<string>();
const options = question.options.map((option) => {
const value = requireId(option.value, `option value of question ${id}`);
@@ -259,7 +253,9 @@ function validateQuestion(question: AskUserQuestion, id: string): AskUserQuestio
question: requireText(question.question, `text of question ${id}`),
...(detail === undefined ? {} : { detail: requireText(detail, `detail of question ${id}`) }),
options,
...(allowOther ? { allowOther: true } : {}),
// Every question accepts a custom answer. Retain the marker so older web
// clients also expose the field when connected to this daemon.
allowOther: true,
...(question.multiple === true ? { multiple: true } : {}),
};
}
@@ -307,7 +303,6 @@ function validateAnswer(question: AskUserQuestion, answer: AskUserAnswer): AskUs
function normalizeOtherText(question: AskUserQuestion, otherText: string | undefined): string | undefined {
if (otherText === undefined) return undefined;
if (question.allowOther !== true) throw new PendingAskValidationError(`Question ${question.id} does not accept other text`);
if (otherText.length > ASK_USER_OTHER_TEXT_MAX_LENGTH) {
throw new PendingAskValidationError(`Other text of question ${question.id} exceeds its length limit`);
}
src/server/sessions/piSessionService.askUser.test.ts
+16 -9
View File
@@ -7,7 +7,7 @@ import { CapturingSessionEventHub, emptyArchiveStore, fakeRuntime, runtimeCreato
const TEST_AGENT_DIR = "/tmp/pi-web-test-agent";
const ACTIVE_SESSION_ID = "session-1";
const questions = [{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }] }];
const questions = [{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }], allowOther: true }];
/**
* Service over a clocked store with sequential ask ids, so asks are named
@@ -98,13 +98,14 @@ describe("PiSessionService.openAsk", () => {
await service.dispose();
});
it("rejects an unanswerable question set without opening it", async () => {
it("opens an optionless question with a custom answer", async () => {
const { service, store, events } = askService();
await expect(service.openAsk({ sessionId: "session-1", questions: [{ id: "empty", question: "No way to answer?", options: [] }] }))
.rejects.toThrow(PendingAskValidationError);
expect(store.pendingAsk("session-1")).toBeUndefined();
expect(askEvents(events)).toEqual([]);
const result = await service.openAsk({ sessionId: "session-1", questions: [{ id: "empty", question: "Anything else?", options: [] }] });
expect(result.ask.questions).toEqual([{ id: "empty", question: "Anything else?", options: [], allowOther: true }]);
expect(store.pendingAsk("session-1")).toEqual(result.ask);
expect(askEvents(events)).toHaveLength(1);
await service.dispose();
});
@@ -163,11 +164,16 @@ describe("PiSessionService ask status projection", () => {
});
describe("PiSessionService.submitAsk", () => {
it("delivers the answers as a follow-up custom message that wakes the session", async () => {
it("delivers a custom answer as a follow-up message that wakes the session", async () => {
const { service, store, events, fake } = askService({ withActiveSession: true });
await service.openAsk({ sessionId: ACTIVE_SESSION_ID, questions });
await service.openAsk({
sessionId: ACTIVE_SESSION_ID,
questions: [{ id: "db", question: "Which database?", options: [{ value: "pg", label: "Postgres" }], allowOther: false }],
});
const response = await service.submitAsk(sessionRef(ACTIVE_SESSION_ID), "ask-1", { answers: [{ id: "db", values: ["pg"] }] });
const response = await service.submitAsk(sessionRef(ACTIVE_SESSION_ID), "ask-1", {
answers: [{ id: "db", values: [], otherText: "DuckDB" }],
});
expect(response).toMatchObject({ result: "closed", outcome: { askId: "ask-1", reason: "submitted", answeredCount: 1, unansweredIds: [] } });
expect(response.sessionStatus.sessionId).toBe(ACTIVE_SESSION_ID);
@@ -176,6 +182,7 @@ describe("PiSessionService.submitAsk", () => {
expect(delivered?.message.customType).toBe(ASK_USER_ANSWERS_CUSTOM_TYPE);
expect(delivered?.message.display).toBe(true);
expect(delivered?.message.content).toContain("The user submitted answers to your questions.");
expect(delivered?.message.content).toContain(`custom: "DuckDB"`);
expect(delivered?.message.content).toContain("Answered 1 of 1");
expect(delivered?.message.details).toMatchObject({ askId: "ask-1", reason: "submitted" });
expect(delivered?.options).toEqual({ triggerTurn: true, deliverAs: "followUp" });