From 7819eff6a160881202bf020851a301ef32d50811 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Padez?= Date: Tue, 18 Aug 2026 19:01:03 +0000 Subject: [PATCH] chat: render AskUserQuestion, instead of dropping it as raw JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The model has been asking questions in Officer all along. None of them ever rendered. MessageBubble dispatched the interactive question UI on `toolName === 'question'` — OpenCode's name for it. Claude Code's tool is `AskUserQuestion`, so every question from a Claude session fell through to the generic ToolActivity and printed its input as JSON in a bubble. Nothing threw, nothing logged, and the payload was arriving correctly the whole time, so it read as "the model never asks" rather than as a bug. Both harnesses send the same shape under different names; only the name was ever wrong. ── What this deliberately does NOT do ── It does not make the agent wait. Checked against the installed SDK rather than assumed: AskUserQuestion is implemented inside the bundled cli.js (with its own "No answer provided" fallback), appears nowhere in sdk.d.ts, and there is no control method or hook that can return a tool RESULT. canUseTool and PreToolUse can only allow or deny, and canUseTool is shadowed entirely under bypassPermissions, which is how Officer runs. So the tool self-resolves as unanswered the moment it is called, whatever the client does. The two ways to pretend otherwise — forging a tool result, or smuggling the choice through a denial reason — both lie to the model about what happened. Neither is here. The question is rendered, and the answer goes back as the next message, exactly what a person would have typed. The tool contract is untouched. The cost is real and stated in the file: the agent will usually have carried on and reads the answer a beat late. The day the SDK grows an answer channel, this component is the piece that already knows how to ask. ── Also fixed while in there ── State was single-valued and shared across questions, but the schema allows up to four. With more than one, the first click sent a partial answer and locked the rest. Now keyed by question index: nothing is sent until every question has an answer, and a multi-question reply is labelled by header so it cannot be ambiguous. Multi-select joins with commas, which is what the tool's own output schema documents. `multiSelect` (Claude Code) and `multiple` (OpenCode) are normalised in one place rather than at each read site. 6 tests pinning both harness shapes, the multi-question rule, the client-side "Other" escape hatch (both schemas tell the model not to offer one, because providing it is the client's job), and that an already-resolved question is read-only. Co-Authored-By: Claude Opus 5 --- .../apps/Chat/components/MessageBubble.tsx | 10 +- .../Chat/components/QuestionActivity.test.tsx | 120 ++++++++ .../apps/Chat/components/QuestionActivity.tsx | 278 ++++++++++-------- 3 files changed, 289 insertions(+), 119 deletions(-) create mode 100644 src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx diff --git a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx index 25bf98e8..4881be90 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx @@ -25,6 +25,9 @@ import { errorText } from 'helpers/error-text'; import type { ChatMessage } from '../types'; import { ToolActivity } from './ToolActivity'; import { QuestionActivity } from './QuestionActivity'; + +/** The tool each harness uses to ask the human something. Same payload, different name. */ +const QUESTION_TOOLS = new Set(['question', 'AskUserQuestion']); import { getRawUrl } from '../../FileViewer/file-types'; import { useFilesAPI } from '../../../hooks/useFilesAPI'; import { CopyButton } from './CopyButton'; @@ -195,7 +198,12 @@ export const MessageBubble = ({ message, onAnswer, onRetry, onDecidePlan, defaul } case 'tool': - if (message.toolName === 'question' && onAnswer) { + // Both harnesses ask the human the same way and call it something different: OpenCode's tool is + // `question`, Claude Code's is `AskUserQuestion`. Only the first was matched, so every question from + // a Claude session fell through to ToolActivity and rendered as raw JSON in a bubble — the payload + // was arriving correctly and the UI simply did not recognise it. Nothing errored, which is why it + // read as "the model never asks" rather than as a bug. + if (QUESTION_TOOLS.has(message.toolName) && onAnswer) { return ; } return ; diff --git a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx new file mode 100644 index 00000000..5f674076 --- /dev/null +++ b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx @@ -0,0 +1,120 @@ +import { describe, expect, test } from 'bun:test'; +import { render, fireEvent, screen } from '@testing-library/react'; +import { QuestionActivity } from './QuestionActivity'; +import type { ChatMessage } from '../types'; + +// The bug this file exists for: a Claude session's questions rendered as raw JSON for months because the +// dispatch matched only OpenCode's tool name. Nothing threw — it read as "the model never asks". So what +// is pinned here is the SHAPE both harnesses send, not the styling. + +const ask = (questions: unknown): Extract => + ({ role: 'tool', toolName: 'AskUserQuestion', toolCallId: 't1', toolInput: { questions } }) as never; + +const OPTIONS = [ + { label: 'Postgres', description: 'the one already running' }, + { label: 'SQLite', description: 'no server to run' }, +]; + +describe('the two harnesses disagree on one field name', () => { + test("Claude Code's `multiSelect` is honoured — several options, one submit", () => { + const answers: string[] = []; + render( + answers.push(t)} + />, + ); + fireEvent.click(screen.getByText('Postgres')); + fireEvent.click(screen.getByText('SQLite')); + // Nothing sent until submit: that is what multi-select means. + expect(answers).toEqual([]); + fireEvent.click(screen.getByText(/Submit \(2 selected\)/)); + // Comma-separated, as the tool's own output schema documents. + expect(answers).toEqual(['Postgres, SQLite']); + }); + + test("OpenCode's `multiple` means the same thing", () => { + const answers: string[] = []; + render( + answers.push(t)} + />, + ); + fireEvent.click(screen.getByText('Postgres')); + fireEvent.click(screen.getByText(/Submit \(1 selected\)/)); + expect(answers).toEqual(['Postgres']); + }); + + test('neither field set means single-select — one click answers', () => { + const answers: string[] = []; + render( + answers.push(t)} + />, + ); + fireEvent.click(screen.getByText('SQLite')); + expect(answers).toEqual(['SQLite']); + }); +}); + +describe('more than one question', () => { + // The schema allows up to four. State used to be shared across them, so the first click sent a partial + // answer and locked the rest — invisible while only one was ever asked. + test('nothing is sent until every question has an answer', () => { + const answers: string[] = []; + render( + answers.push(t)} + />, + ); + + fireEvent.click(screen.getByText('Postgres')); + expect(answers).toEqual([]); // one of two — still waiting + + fireEvent.click(screen.getByText('Bun')); + // Labelled by header, so a two-part reply cannot be ambiguous. + expect(answers).toEqual(['Store: Postgres\nRuntime: Bun']); + }); +}); + +describe('the escape hatch', () => { + // Both schemas tell the model NOT to offer an "Other" option, because providing one is the client's job. + test('free text answers the question', () => { + const answers: string[] = []; + render( + answers.push(t)} + />, + ); + fireEvent.change(screen.getByPlaceholderText('Other...'), { target: { value: 'DuckDB' } }); + fireEvent.click(screen.getByText('Send')); + expect(answers).toEqual(['DuckDB']); + }); +}); + +describe('a question the harness already resolved', () => { + test('is read-only — its answer cannot be changed', () => { + const answers: string[] = []; + const resolved = { + ...ask([{ question: 'Which?', header: 'Store', options: OPTIONS }]), + output: 'The user did not answer the questions.', + } as never; + render( answers.push(t)} />); + fireEvent.click(screen.getByText('Postgres')); + expect(answers).toEqual([]); + }); +}); diff --git a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx index 082ba08c..4dd02b4e 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx @@ -6,13 +6,23 @@ type ToolMessage = Extract; type QuestionOption = { label: string; - description: string; + /** Why you would pick this. Optional in practice, though both harnesses document it as required. */ + description?: string; }; +/** + * One question, as it arrives on the wire. + * + * Two harnesses send this, and they disagree on exactly one field name: Claude Code's + * `AskUserQuestion` says `multiSelect`, OpenCode's `question` says `multiple`. Both are accepted and + * normalised at the top of the component rather than anywhere a renderer has to remember it — see + * `isMulti`. Everything else (`question`, `header`, `options[{label, description}]`) is identical. + */ type Question = { question: string; header: string; - multiple: boolean; + multiple?: boolean; + multiSelect?: boolean; options: QuestionOption[]; }; @@ -21,146 +31,178 @@ type QuestionActivityProps = { onAnswer: (text: string) => void; }; +const isMulti = (q: Question): boolean => q.multiSelect ?? q.multiple ?? false; + +/** + * The interactive rendering of a harness asking the human something. + * + * ── Why the answer is a chat message rather than a tool result ── + * + * The honest version of this would resolve the tool call itself: the model asks, the turn blocks, the + * human picks, the tool returns. That is what the terminal does, and it is not available to us. Checked + * against the installed SDK rather than assumed: `AskUserQuestion` is implemented inside the bundled CLI + * (`cli.js`, with its own "No answer provided" fallback), it appears nowhere in `sdk.d.ts`, and there is + * no control method or hook that can return a tool RESULT — `canUseTool` and `PreToolUse` can only allow + * or deny, and `canUseTool` is shadowed entirely under `bypassPermissions`, which is how Officer runs. + * + * So the tool self-resolves as "no answer" the moment it is called, whatever we do. The two ways to + * pretend otherwise — forging a result, or smuggling the choice through a denial reason — both lie to the + * model about what happened. This does neither. It renders what was asked and sends the human's choice as + * the next message, which is exactly what a person would have typed. The tool contract is untouched. + * + * The cost is real and worth stating: the agent does not WAIT. It will usually have carried on, and reads + * the answer a beat later. That is a limitation of the SDK, not of this component, and the day the SDK + * grows an answer channel this is the piece that already knows how to ask. + */ export const QuestionActivity = ({ message, onAnswer }: QuestionActivityProps) => { - const [selectedOptions, setSelectedOptions] = useState>(new Set()); - const [otherText, setOtherText] = useState(''); - const [answered, setAnswered] = useState(false); - const [answeredText, setAnsweredText] = useState(''); + // Keyed by question INDEX, because a payload may carry up to four and they are answered independently. + // This was single-valued state shared across every question, so with more than one the first click sent + // a partial answer and locked the rest — invisible while only one question was ever asked. + const [answers, setAnswers] = useState>({}); + const [selected, setSelected] = useState>>({}); + const [otherText, setOtherText] = useState>({}); + const [sent, setSent] = useState(false); const input = message.toolInput as { questions?: Question[] }; const questions = input.questions; if (!questions || questions.length === 0) return null; + // A tool call with an output has already been resolved by the harness; nothing here can change it. const pending = message.output === undefined; + const isDisabled = sent || !pending; - const handleSelect = (question: Question, label: string) => { - if (answered || !pending) return; + /** + * Record one answer, and send once EVERY question has one. + * + * The format follows the tool's own output schema, which maps question text to answer and joins a + * multi-select with commas. With a single question the bare answer reads like something a person + * typed; with several, each is labelled by its header so the reply cannot be ambiguous. + */ + const record = (qi: number, text: string) => { + if (isDisabled) return; + const next = { ...answers, [qi]: text }; + setAnswers(next); + if (Object.keys(next).length < questions.length) return; - if (question.multiple) { - setSelectedOptions((prev) => { - const next = new Set(prev); - if (next.has(label)) next.delete(label); - else next.add(label); - return next; - }); - } else { - const text = label; - setAnswered(true); - setAnsweredText(text); - onAnswer(text); - } + setSent(true); + onAnswer(questions.length === 1 ? next[0]! : questions.map((q, i) => `${q.header}: ${next[i]}`).join('\n')); }; - const handleSubmitMultiple = () => { - if (selectedOptions.size === 0 || answered || !pending) return; - const text = Array.from(selectedOptions).join(', '); - setAnswered(true); - setAnsweredText(text); - onAnswer(text); + const toggle = (qi: number, label: string) => { + if (isDisabled) return; + setSelected((prev) => { + const set = new Set(prev[qi] ?? []); + if (set.has(label)) set.delete(label); + else set.add(label); + return { ...prev, [qi]: set }; + }); }; - const handleSubmitOther = () => { - const text = otherText.trim(); - if (!text || answered || !pending) return; - setAnswered(true); - setAnsweredText(text); - onAnswer(text); - }; - - const isDisabled = answered || !pending; - return (
- {questions.map((q, qi) => ( -
-
- - {q.header} -
+ {questions.map((q, qi) => { + const multi = isMulti(q); + const answer = answers[qi]; + const picked = selected[qi] ?? new Set(); + const done = answer !== undefined || isDisabled; -
-

{q.question}

- -
- {q.options.map((opt) => { - const isSelected = answered - ? answeredText === opt.label || answeredText.split(', ').includes(opt.label) - : selectedOptions.has(opt.label); - - return ( - - ); - })} + return ( +
+
+ + {q.header} + {questions.length > 1 && ( + + {qi + 1}/{questions.length} + + )}
- {/* "Other" free-text option */} - {!isDisabled && ( -
- setOtherText(ev.target.value)} - onKeyDown={(ev) => { - if (ev.key === 'Enter') { +
+

{q.question}

+ +
+ {q.options.map((opt) => { + const isSelected = + answer !== undefined ? answer.split(', ').includes(opt.label) : picked.has(opt.label); + + return ( + + ); + })} +
+ + {/* "Other" — provided by the client on purpose. Both schemas say the model must NOT offer one + as an option, precisely because the escape hatch is the UI's job. */} + {!done && ( +
+ setOtherText((prev) => ({ ...prev, [qi]: ev.target.value }))} + onKeyDown={(ev) => { + if (ev.key !== 'Enter') return; ev.preventDefault(); - handleSubmitOther(); - } - }} - placeholder="Other..." - className="flex-1 px-3 py-1.5 rounded-lg border border-border text-sm text-foreground placeholder:text-muted-foreground focus:outline-none focus:border-duck-teal/40" - /> + const text = (otherText[qi] ?? '').trim(); + if (text) record(qi, text); + }} + placeholder="Other..." + className="flex-1 px-3 py-1.5 rounded-lg border border-border text-sm text-foreground placeholder:text-muted-foreground focus:outline-none focus:border-duck-teal/40" + /> + +
+ )} + + {multi && !done && ( -
- )} + )} - {/* Submit button for multi-select */} - {q.multiple && !isDisabled && ( - - )} - - {/* Answered indicator */} - {isDisabled && answeredText && ( -
- - Answered: {answeredText} -
- )} + {answer !== undefined && ( +
+ + Answered: {answer} +
+ )} +
-
- ))} + ); + })}
); };