From 871c904265b5ad4d00cc3bd4615e3b0252f63bbb Mon Sep 17 00:00:00 2001 From: dnviti Date: Mon, 27 Jul 2026 01:06:27 +0200 Subject: [PATCH 1/3] feat(chat): let the model ask you a question and wait (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The only thing the agent could put in front of the user was an approval: allow or deny a tool it was about to run. Anything else it needed to know it asked in prose, and the user had to guess the wording it wanted back. It can now ask a choice-based question — single-select or multi-select — which renders in the conversation where it was asked, is answered by clicking, and blocks the turn until it is. The mechanism was established by probe, not by documentation. Claude's own AskUserQuestion tool does not exist in the headless stream-json channel this app drives: it is absent from the init tool list, a tool search does not find it, and the model falls back to prose (.work/probes/ask/). So the capability is supplied rather than recognised, as an MCP server this app hands the runtime inline (.work/probes/askmcp/). Its one tool blocks on `tools/call` until a person clicks, which is what makes the agent genuinely wait. The answer travels back over the unix socket the approval hook already uses, generalised from one kind of caller to two. Nothing is written to the user's own settings or MCP configuration: both are passed inline, session-scoped. The question tool is auto-allowed — asking someone to approve being asked a question is two prompts for one decision — and questions are asked even when approvals are bypassed, since that setting has never meant answering for the user. The card is drawn from the tool call that asked, which is already persisted and replayed, so it survives a reload, a rejoin and a server restart with the question and the answer intact. Verified end to end against the real CLI in both approval modes: the model asks, the browser answers, and the reply names exactly what was picked. --- CHANGELOG.md | 23 + src/client/chat/controller.ts | 12 + src/client/chat/transcript.ts | 23 + src/client/chat/turns.ts | 3 +- src/client/shell/chat/ChatSessionSidebar.tsx | 18 +- src/client/shell/chat/ChatView.tsx | 61 +- src/client/shell/chat/MessageBubble.tsx | 95 ++- src/client/shell/chat/MessageList.tsx | 4 + src/client/shell/chat/QuestionCard.tsx | 359 ++++++++++ src/client/shell/chat/SessionHeader.tsx | 8 + src/client/shell/chat/StreamRibbon.tsx | 10 +- src/client/terminal/message-handler.ts | 1 + src/client/ui/relay/Icon.tsx | 1 + src/server/chat/ask-mcp.ts | 323 +++++++++ src/server/chat/manager.ts | 12 + src/server/chat/permission-broker.ts | 85 ++- src/server/chat/session.ts | 231 ++++++- src/server/websocket/messages.ts | 36 + src/shared/chat-events.ts | 151 ++++ src/shared/chat-reducer.ts | 60 ++ test/browser/checks.ts | 195 ++++++ test/chat-permission-broker.test.js | 59 +- test/chat-questions.test.js | 684 +++++++++++++++++++ 23 files changed, 2405 insertions(+), 49 deletions(-) create mode 100644 src/client/shell/chat/QuestionCard.tsx create mode 100644 src/server/chat/ask-mcp.ts create mode 100644 test/chat-questions.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c1cbf9..ff84c40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,29 @@ ## [5.2.0] - Unreleased ### Added +- **The agent can ask you a question and wait for the answer.** Until now the + only thing it could put in front of you was an approval — allow or deny a tool + it was about to run. Anything else it needed to know, it had to ask in prose, + and you had to guess the wording it was hoping for. + + It can now ask a proper question with the answers already written out: which + of three approaches to take, which of the four candidate files you meant, which + of the problems it found to fix first. The question appears in the conversation + where it was asked, you answer by clicking, and the agent picks up from your + answer. Questions come in both kinds — pick exactly one, or tick several and + confirm. + + The card stays where it was after you answer, showing what was asked and what + you chose, so scrolling back past a decision shows the decision. If you close + the tab while one is waiting, it is still there — and still answerable — when + you come back. A question you would rather not answer can be skipped; the agent + is told so and carries on rather than sitting there blocked. + + Questions are asked even in sessions running with approvals bypassed: not being + asked before it acts has never meant having your questions answered for you. + + Available with Claude today. The plumbing underneath is runtime-neutral, so the + other runtimes need only their own handshake wired up. - **You can open what the agent handed off and watch it work.** A delegation used to be one line in the agents list: a name, a status badge, a duration. Whether it was a sub-agent reading three files or a workflow running a dozen diff --git a/src/client/chat/controller.ts b/src/client/chat/controller.ts index ff6a231..9d4d3e3 100644 --- a/src/client/chat/controller.ts +++ b/src/client/chat/controller.ts @@ -309,6 +309,17 @@ export class ChatController { this.send({ type: 'chat_queue_cancel', queuedId }); } + /** + * Answer a multiple-choice question the model asked. + * + * `skipped` is explicit rather than inferred from an empty list: "I picked + * none of these" and "I do not want to answer" reach the model as different + * sentences, and the agent is blocked either way until one of them arrives. + */ + answerQuestion(requestId: string, optionIds: string[], skipped = false): void { + this.send({ type: 'chat_question_answer', requestId, optionIds, skipped }); + } + respondPermission(requestId: string, optionId: string): void { this.send({ type: 'chat_permission_response', requestId, optionId }); } @@ -415,6 +426,7 @@ export class ChatController { state: 'starting', capabilities: NO_CHAT_CAPABILITIES, pendingPermissions: [], + pendingQuestions: [], firstSeq: 0, replayFrom: 0, cursor: 0, diff --git a/src/client/chat/transcript.ts b/src/client/chat/transcript.ts index 59f98c1..07d01c1 100644 --- a/src/client/chat/transcript.ts +++ b/src/client/chat/transcript.ts @@ -28,6 +28,7 @@ import { PermissionRequest, PlanItem, QueuedTurn, + QuestionRequest, NO_CHAT_CAPABILITIES, } from '../../shared/chat-events.js'; import { @@ -161,6 +162,7 @@ export class ChatTranscript { usage: snapshot.usage || {}, plan: snapshot.plan || [], pendingPermissions: snapshot.pendingPermissions || [], + pendingQuestions: snapshot.pendingQuestions || [], firstSeq: snapshot.firstSeq, cursor: snapshot.cursor, }); @@ -371,6 +373,27 @@ export class ChatTranscript { return this.state.pendingPermissions; } + /** Questions the model asked that nobody has answered yet. */ + get pendingQuestions(): QuestionRequest[] { + return this.state.pendingQuestions; + } + + /** + * The question waiting on the given tool call, if that call is the one asking. + * + * How a card drawn from a tool block finds out it is still live: the block is + * in the transcript either way, and this is the difference between a set of + * buttons and a record of what was already decided. + */ + questionFor(toolId: string): QuestionRequest | undefined { + return this.state.pendingQuestions.find((pending) => pending.toolId === toolId); + } + + /** Which options were picked for the question that call asked, once answered. */ + answerFor(toolId: string): string[] | undefined { + return this.state.answeredQuestions[toolId]; + } + get cursor(): number { return this.state.cursor; } diff --git a/src/client/chat/turns.ts b/src/client/chat/turns.ts index 96e95ea..1fb561e 100644 --- a/src/client/chat/turns.ts +++ b/src/client/chat/turns.ts @@ -109,7 +109,8 @@ function summarise( } } - const status: TurnStatus = isLast && chatState === 'awaiting_permission' + const status: TurnStatus = isLast + && (chatState === 'awaiting_permission' || chatState === 'awaiting_answer') ? 'waiting' : isLast && (streaming || chatState === 'thinking' || chatState === 'running' || chatState === 'starting') ? 'running' diff --git a/src/client/shell/chat/ChatSessionSidebar.tsx b/src/client/shell/chat/ChatSessionSidebar.tsx index 612c655..64e0f99 100644 --- a/src/client/shell/chat/ChatSessionSidebar.tsx +++ b/src/client/shell/chat/ChatSessionSidebar.tsx @@ -45,11 +45,18 @@ export interface ChatSessionSidebarProps { } /** States where the agent is actively doing something, as opposed to waiting. */ -const BUSY_STATES: ReadonlySet = new Set(['starting', 'thinking', 'running', 'awaiting_permission']); +const BUSY_STATES: ReadonlySet = new Set([ + 'starting', + 'thinking', + 'running', + 'awaiting_permission', + 'awaiting_answer', +]); function stateDotColor(state: ChatState | undefined): string { if (state === 'error') return 'var(--destructive)'; if (state === 'awaiting_permission') return 'var(--warning)'; + if (state === 'awaiting_answer') return 'var(--info)'; if (state && BUSY_STATES.has(state)) return 'var(--ansi-green)'; return 'var(--muted-foreground)'; } @@ -64,6 +71,7 @@ function stateDotColor(state: ChatState | undefined): string { function stateLabel(state: ChatState | undefined): string | undefined { switch (state) { case 'awaiting_permission': return 'needs approval'; + case 'awaiting_answer': return 'asked you something'; case 'error': return 'error'; case 'running': return 'running'; case 'thinking': return 'thinking'; @@ -321,7 +329,13 @@ function SessionRow({ {label ? ( {label} diff --git a/src/client/shell/chat/ChatView.tsx b/src/client/shell/chat/ChatView.tsx index 4cb372e..8486697 100644 --- a/src/client/shell/chat/ChatView.tsx +++ b/src/client/shell/chat/ChatView.tsx @@ -23,6 +23,7 @@ import { Composer } from './Composer.js'; import { MessageList, type MessageListHandle } from './MessageList.js'; import { hasVisibleContent, messageText } from './MessageBubble.js'; import { PermissionCard } from './PermissionCard.js'; +import { QuestionCard } from './QuestionCard.js'; import { PlanPanel } from './PlanPanel.js'; import { SessionHeader } from './SessionHeader.js'; import { LiveStreamRibbon } from './StreamRibbon.js'; @@ -152,6 +153,19 @@ export function ChatView({ const bypassPermissions = transcript.bypassing; const plan = transcript.plan; const pending = transcript.pendingPermissions; + // Only the questions that have nowhere else to be drawn. A question that + // names the call that asked it renders inside the conversation at that call, + // which is where it was asked; this is the safety net for one that could not + // be correlated, and without it that question would have no button anywhere + // and the turn behind it would never move. + const strayQuestions = React.useMemo( + () => + transcript.pendingQuestions.filter( + (request) => !request.toolId || !transcript.message(messageIdOfTool(transcript, request.toolId)), + ), + // eslint-disable-next-line react-hooks/exhaustive-deps + [transcript, version], + ); const exited = chatState === 'exited'; const unavailable = controller.unavailableReason; const busy = transcript.busy; @@ -304,6 +318,11 @@ export function ChatView({ (requestId: string, optionId: string) => controller.respondPermission(requestId, optionId), [controller], ); + const answerQuestion = React.useCallback( + (requestId: string, optionIds: string[], skipped: boolean) => + controller.answerQuestion(requestId, optionIds, skipped), + [controller], + ); const cancelQueued = React.useCallback( (queuedId: string) => controller.cancelQueued(queuedId), [controller], @@ -673,6 +692,7 @@ export function ChatView({ onRetry={retryTurn} showThinking={view.showThinking} showToolCalls={view.showToolCalls} + onAnswerQuestion={answerQuestion} /> {terminalOpen ? ( @@ -706,7 +726,7 @@ export function ChatView({ /> ) : null} - {busy || chatState === 'awaiting_permission' || chatState === 'error' ? ( + {busy || chatState === 'awaiting_permission' || chatState === 'awaiting_answer' || chatState === 'error' ? ( ) : null} + {strayQuestions.length > 0 ? ( +
+ {strayQuestions.map((request) => ( + + ))} +
+ ) : null} + {pending.length > 0 ? ( // assertive, not polite: nothing else the user does will move the // session forward until one of these is answered. @@ -952,7 +993,7 @@ function placeholderFor(state: ChatState, runtimeLabel: string, isPhone = false) if (state === 'exited') return 'This session has ended'; // No longer 'answer this before you can type': the composer stays live and // queues, so the sentence has to describe what happens rather than forbid it. - if (state === 'awaiting_permission') { + if (state === 'awaiting_permission' || state === 'awaiting_answer') { return isPhone ? 'Answer above, or type' : 'Answer above — or type the next message'; } if (state === 'thinking' || state === 'running') { @@ -1263,3 +1304,19 @@ function mobileBarHeight(): number { return Number.isFinite(parsed) ? parsed : MOBILE_BAR_FALLBACK_PX; } +/** + * Which message holds a given tool call, or '' when the transcript has none. + * + * Used only to decide whether a pending question already has a card inside the + * conversation. A question whose call is above the loaded window — or whose call + * never arrived — needs the pinned card instead, and asking the transcript is + * the only way to tell those apart from one that is on screen. + */ +function messageIdOfTool(transcript: ChatTranscript, toolId: string): string { + for (const message of transcript.messages) { + for (const block of message.blocks) { + if (block.kind === 'tool' && block.toolId === toolId) return message.id; + } + } + return ''; +} diff --git a/src/client/shell/chat/MessageBubble.tsx b/src/client/shell/chat/MessageBubble.tsx index 1bffe6a..40dc7a8 100644 --- a/src/client/shell/chat/MessageBubble.tsx +++ b/src/client/shell/chat/MessageBubble.tsx @@ -6,6 +6,8 @@ import { ErrorBlock, ImageBlock, NoticeBlock, + ToolBlock, + isAskQuestionTool, } from '../../../shared/chat-events.js'; import { ChatTranscript } from '../../chat/transcript.js'; import { compactCount, formatDuration } from '../../chat/tool-meta.js'; @@ -13,6 +15,7 @@ import { Icon } from '../../ui/relay/Icon.js'; import { PHONE_TEXT, TOUCH_GAP, TOUCH_TARGET, usePhone } from '../../ui/touch.js'; import { Markdown, markdownText } from './Markdown.js'; import { PlanPanel } from './PlanPanel.js'; +import { QuestionCard, questionFromToolInput } from './QuestionCard.js'; /** * One message in the transcript — prose, and nothing else. @@ -69,6 +72,13 @@ export interface MessageBubbleProps { showThinking?: boolean; /** Count tool calls in the work pill. */ showToolCalls?: boolean; + /** + * Answer a question the model asked from its card in the conversation. + * + * Must be referentially stable — this component is memoised, and a fresh + * closure per render would re-render the whole transcript on every token. + */ + onAnswerQuestion?: (requestId: string, optionIds: string[], skipped: boolean) => void; } export const MessageBubble = React.memo(function MessageBubble({ @@ -81,6 +91,7 @@ export const MessageBubble = React.memo(function MessageBubble({ carriedIds = '', showThinking = true, showToolCalls = true, + onAnswerQuestion, }: MessageBubbleProps) { const id = message.id; @@ -236,6 +247,8 @@ export const MessageBubble = React.memo(function MessageBubble({ key={i} block={block} plain={isUser} + transcript={transcript} + onAnswerQuestion={onAnswerQuestion} onRetry={onRetry ? () => onRetry(id) : undefined} caret={Boolean(current.streaming) && i === current.blocks.length - 1} /> @@ -321,9 +334,20 @@ const SR_ONLY: React.CSSProperties = { clip: 'rect(0 0 0 0)', }; -/** How many blocks this message would actually paint. */ +/** + * How many blocks this message would actually paint. + * + * Tool calls are machinery and live on the trace rail — with one exception. A + * question the model asked is a tool call only in the mechanical sense; what it + * actually is, is the agent talking to the user, and burying it on the rail + * would hide the one card the turn is blocked behind. + */ function visibleBlocks(message: ChatMessage): number { - return message.blocks.filter((block) => block.kind !== 'thinking' && block.kind !== 'tool').length; + return message.blocks.filter( + (block) => + block.kind !== 'thinking' + && (block.kind !== 'tool' || isAskQuestionTool(block.name)), + ).length; } /** @@ -376,6 +400,12 @@ function summariseWork( let firstId: string | undefined; for (const message of messages) { for (const block of message.blocks) { + // The question card is rendered in the conversation, so counting it here + // as well would put "1 command" on a turn whose only machinery is the + // question already on screen. + if (block.kind === 'tool' && isAskQuestionTool(block.name)) { + continue; + } if (block.kind === 'tool' && showToolCalls) { tools += 1; if (block.durationMs !== undefined) durationMs += block.durationMs; @@ -451,12 +481,16 @@ function BlockView({ block, plain, caret, + transcript, + onAnswerQuestion, onRetry, }: { block: ChatBlock; /** True for the user's own turn, where text is echoed literally. */ plain: boolean; caret: boolean; + transcript: ChatTranscript; + onAnswerQuestion?: (requestId: string, optionIds: string[], skipped: boolean) => void; onRetry?: () => void; }) { switch (block.kind) { @@ -495,7 +529,21 @@ function BlockView({ // rather than removing the cases: a block kind that silently fell through // to `default` would be indistinguishable from one nobody has handled yet. case 'thinking': + return null; + case 'tool': + // The one tool call that belongs in the conversation rather than on the + // rail: it *is* the agent addressing the user. Everything else about a + // tool call is machinery. + if (isAskQuestionTool(block.name)) { + return ( + + ); + } return null; case 'plan': @@ -515,6 +563,49 @@ function BlockView({ } } +/** + * A question the model asked, drawn from the call that asked it. + * + * The tool block is the durable record — the arguments *are* the question, and + * they are persisted and replayed like any other block — so this needs no side + * table to render from and survives a reload, a rejoin and a server restart. + * The transcript is consulted only for the two things the block cannot know: + * whether the question is still waiting, and which options were picked. + */ +function QuestionBlock({ + block, + transcript, + onAnswerQuestion, +}: { + block: ToolBlock; + transcript: ChatTranscript; + onAnswerQuestion?: (requestId: string, optionIds: string[], skipped: boolean) => void; +}): React.JSX.Element | null { + const asked = questionFromToolInput(block.input); + // Still streaming its arguments in, or malformed. Nothing to draw yet — and + // an empty bordered card would read as a question with no answers. + if (!asked) return null; + + const request = transcript.questionFor(block.toolId); + const answered = request ? undefined : transcript.answerFor(block.toolId); + + return ( + + ); +} + /** * A rule across the conversation, marking something that happened to it. * diff --git a/src/client/shell/chat/MessageList.tsx b/src/client/shell/chat/MessageList.tsx index 7ef0a73..4aba00d 100644 --- a/src/client/shell/chat/MessageList.tsx +++ b/src/client/shell/chat/MessageList.tsx @@ -64,6 +64,8 @@ export interface MessageListProps { */ showThinking?: boolean; showToolCalls?: boolean; + /** Answer a question the model asked, from its card in the conversation. */ + onAnswerQuestion?: (requestId: string, optionIds: string[], skipped: boolean) => void; } /** How close to the top still counts as asking for the previous page. */ @@ -93,6 +95,7 @@ export const MessageList = React.forwardRef onToggleTurn, showThinking = true, showToolCalls = true, + onAnswerQuestion, }, handle, ) { @@ -393,6 +396,7 @@ export const MessageList = React.forwardRef carriedIds={carriedIds} showThinking={showThinking} showToolCalls={showToolCalls} + onAnswerQuestion={onAnswerQuestion} /> ))} diff --git a/src/client/shell/chat/QuestionCard.tsx b/src/client/shell/chat/QuestionCard.tsx new file mode 100644 index 0000000..f6898c1 --- /dev/null +++ b/src/client/shell/chat/QuestionCard.tsx @@ -0,0 +1,359 @@ +import * as React from 'react'; +import { + QuestionOption, + QuestionRequest, + normalizeQuestionOptions, +} from '../../../shared/chat-events.js'; +import { Button } from '../../ui/relay/Button.js'; +import { Icon } from '../../ui/relay/Icon.js'; + +/** + * A question the model asked, answered by clicking. + * + * Distinct from `PermissionCard` in the one way that matters: an approval is + * this app gating the agent, so its options are always some arrangement of + * allow and deny and the card can style them by meaning. Here the options are + * whatever the model wrote — the app has no opinion about which is the safe one + * and must not imply it has, so every choice is weighted the same and none is + * pre-selected. + * + * Two modes, one component. Live, it is a set of buttons and the turn is + * blocked behind it. Afterwards it is the record of what was asked and what was + * answered, which is the whole reason it renders inside the conversation rather + * than in a tray that empties: scrolling back past a decision should show the + * decision. + */ + +export interface QuestionCardProps { + /** The live request, when this question is still waiting on an answer. */ + request?: QuestionRequest; + /** The question as the model posed it. Present in both modes. */ + question: string; + header?: string; + multiSelect: boolean; + options: QuestionOption[]; + /** + * Option ids already chosen, for a question that has been answered. + * + * An empty array means answered-with-nothing (skipped); `undefined` means not + * answered, which is a different card entirely. + */ + answered?: string[]; + /** Free text describing the outcome when the option ids are not available. */ + answerText?: string; + onAnswer?: (requestId: string, optionIds: string[], skipped: boolean) => void; +} + +export function QuestionCard({ + request, + question, + header, + multiSelect, + options, + answered, + answerText, + onAnswer, +}: QuestionCardProps) { + // Held locally as well as read from the transcript so the card settles the + // instant it is clicked. The server's answer follows a beat later — and is + // what a second browser watching the same conversation sees — but the person + // who clicked should not watch their own click take a round trip. + const [sent, setSent] = React.useState(null); + const [checked, setChecked] = React.useState([]); + + const live = Boolean(request && onAnswer) && sent === null && answered === undefined; + const picked = sent ?? answered; + + const answer = (optionIds: string[], skipped: boolean): void => { + if (!request || !onAnswer || sent !== null) return; + setSent(skipped ? [] : optionIds); + onAnswer(request.requestId, optionIds, skipped); + }; + + const toggle = (optionId: string): void => { + setChecked((current) => + current.includes(optionId) + ? current.filter((each) => each !== optionId) + : [...current, optionId], + ); + }; + + // Escape closes every dialog in this app. This is not a dialog — there is no + // prior state to fall back to, only an agent sitting and waiting — so a stray + // Escape must do nothing rather than appear to dismiss something that is + // still, in fact, blocking the turn. + const swallowEscape = (event: React.KeyboardEvent): void => { + if (event.key === 'Escape') event.stopPropagation(); + }; + + return ( +
+
+ +
+ {header ? ( + + {header} + + ) : null} + + {question} + + {live && multiSelect ? ( + + Pick as many as apply, then confirm. + + ) : null} +
+
+ + {live && multiSelect ? ( + + ) : live ? ( +
+ {options.map((option) => ( + + ))} +
+ ) : ( + + )} + + {live ? ( +
+ {multiSelect ? ( + + ) : null} + {/* Offered, but never as a way to make the card go away quietly: the + model is told the question was skipped and carries on, which is + the only ending that does not leave the turn hanging. */} + +
+ ) : null} +
+ ); +} + +function OptionText({ option }: { option: QuestionOption }): React.JSX.Element { + return ( + + {option.label} + {option.description ? ( + + {option.description} + + ) : null} + + ); +} + +function MultiSelect({ + options, + checked, + onToggle, +}: { + options: QuestionOption[]; + checked: string[]; + onToggle: (optionId: string) => void; +}): React.JSX.Element { + return ( +
+ {options.map((option) => { + const on = checked.includes(option.optionId); + return ( + + ); + })} +
+ ); +} + +/** + * What was asked and what was answered, after the fact. + * + * Every option is still listed rather than only the chosen one: "picked B" says + * much less on its own than "picked B out of A, B, C", and the whole point of + * leaving this in the transcript is that it still reads as a decision later. + */ +function Answered({ + options, + picked, + answerText, +}: { + options: QuestionOption[]; + picked?: string[]; + answerText?: string; +}): React.JSX.Element { + // No ids to match against — a card rebuilt from a replayed transcript, where + // the resolution is known only as the sentence the model was given. Showing + // that verbatim beats showing every option as unpicked. + if (!picked && answerText) { + return ( +
+ {options.map((option) => ( + + ))} + {answerText} +
+ ); + } + + const chosen = picked ?? []; + return ( +
+ {options.map((option) => ( + + ))} + {chosen.length === 0 ? ( + + Skipped without answering. + + ) : null} +
+ ); +} + +function PlainOption({ + option, + chosen, + muted = false, +}: { + option: QuestionOption; + chosen: boolean; + muted?: boolean; +}): React.JSX.Element { + return ( +
+ + + {chosen ? chosen : null} +
+ ); +} + +const SR_ONLY: React.CSSProperties = { + position: 'absolute', + width: 1, + height: 1, + overflow: 'hidden', + clip: 'rect(0 0 0 0)', +}; + +/** + * Read a question back out of the tool call that asked it. + * + * The transcript's record of a question *is* the tool block — the arguments the + * model sent are the question, and they are already persisted and replayed. So + * a card scrolled back to needs no side table to render from, which is what + * makes the record survive a reload and a server restart. + * + * Tolerant by contract: `input` is `unknown` everywhere else in this codebase + * for good reason, and a malformed call should render as nothing rather than + * throw inside the transcript. + */ +export function questionFromToolInput(input: unknown): { + question: string; + header?: string; + multiSelect: boolean; + options: QuestionOption[]; +} | null { + if (!input || typeof input !== 'object') return null; + const object = input as Record; + const question = typeof object.question === 'string' ? object.question.trim() : ''; + if (!question) return null; + + const options = normalizeQuestionOptions(object.options); + if (options.length === 0) return null; + + return { + question, + header: typeof object.header === 'string' && object.header.trim() ? object.header.trim() : undefined, + multiSelect: object.multiSelect === true, + options, + }; +} diff --git a/src/client/shell/chat/SessionHeader.tsx b/src/client/shell/chat/SessionHeader.tsx index 2fefc62..848b98e 100644 --- a/src/client/shell/chat/SessionHeader.tsx +++ b/src/client/shell/chat/SessionHeader.tsx @@ -86,6 +86,14 @@ const STATE_META: Record = { tint: 'var(--warning)', pulse: true, }, + // Its own word, not the approval one: "waiting for you" over a question about + // which of three approaches to take reads as though something needs allowing. + awaiting_answer: { + label: 'asked you a question', + color: 'var(--info)', + tint: 'var(--info)', + pulse: true, + }, exited: { label: 'exited', color: 'var(--muted-foreground)' }, error: { label: 'error', color: 'var(--destructive)', tint: 'var(--destructive)' }, }; diff --git a/src/client/shell/chat/StreamRibbon.tsx b/src/client/shell/chat/StreamRibbon.tsx index fda5da3..e6bf266 100644 --- a/src/client/shell/chat/StreamRibbon.tsx +++ b/src/client/shell/chat/StreamRibbon.tsx @@ -51,13 +51,19 @@ export function StreamRibbon({ }: StreamRibbonProps): React.JSX.Element { const isPhone = usePhone(); const tone: Tone = - state === 'error' ? 'error' : state === 'awaiting_permission' ? 'waiting' : 'working'; + state === 'error' + ? 'error' + : state === 'awaiting_permission' || state === 'awaiting_answer' + ? 'waiting' + : 'working'; const colour = TONE[tone]; const working = tone === 'working'; const text = label || (state === 'awaiting_permission' ? 'Waiting for you to answer the approval above' + : state === 'awaiting_answer' + ? 'Waiting for you to answer the question above' : state === 'starting' ? 'Starting' : state === 'thinking' @@ -243,7 +249,7 @@ function ribbonLabel( turn: TurnSummary | undefined, state: ChatState, ): string | undefined { - if (state === 'awaiting_permission') return undefined; + if (state === 'awaiting_permission' || state === 'awaiting_answer') return undefined; const scoped = turn ? activityForTurn(turn, events) : events; for (let i = scoped.length - 1; i >= 0; i -= 1) { const event = scoped[i]; diff --git a/src/client/terminal/message-handler.ts b/src/client/terminal/message-handler.ts index e72e4ba..f2bbb37 100644 --- a/src/client/terminal/message-handler.ts +++ b/src/client/terminal/message-handler.ts @@ -223,6 +223,7 @@ export class MessageHandler { tabs.updateTabStatus(sessionId, 'active'); return; case 'awaiting_permission': + case 'awaiting_answer': tabs.updateTabStatus(sessionId, 'idle'); if (background) tabs.updateUnreadIndicator(sessionId, true); return; diff --git a/src/client/ui/relay/Icon.tsx b/src/client/ui/relay/Icon.tsx index d118859..c80595e 100644 --- a/src/client/ui/relay/Icon.tsx +++ b/src/client/ui/relay/Icon.tsx @@ -11,6 +11,7 @@ import * as React from "react"; export const RELAY_ICONS: Record = { "chevron-down": "\n \n", "chevron-right": "\n \n", + "circle-help": "\n \n \n \n", "circle": "\n \n", "command": "\n \n", "copy": "\n \n \n", diff --git a/src/server/chat/ask-mcp.ts b/src/server/chat/ask-mcp.ts new file mode 100644 index 0000000..17bf084 --- /dev/null +++ b/src/server/chat/ask-mcp.ts @@ -0,0 +1,323 @@ +/** + * The MCP server that lets a model ask the browser a multiple-choice question. + * + * Spawned by the runtime, not by us — same shape as the approval hook, and for + * the same reason: the CLI owns the child process, and the only way back to the + * chat session that owns the conversation is the unix socket named in the + * environment. + * + * Why an MCP tool at all, rather than recognising a question the model is + * already able to ask: it cannot. Probed against Claude Code 2.1.220 (trace kept + * at `.work/probes/ask/`), the built-in `AskUserQuestion` tool is simply absent + * from a headless `-p --output-format stream-json` session — it is not in the + * `system/init` tool list, a tool search does not find it, and the model gives + * up and asks in prose. The capability has to be supplied, and MCP is how a + * runtime accepts one. The companion trace at `.work/probes/askmcp/` is the same + * conversation with this tool present: the model calls it, blocks on the reply, + * and continues from the answer. + * + * Blocking is the entire point. `tools/call` does not respond until a person has + * clicked something, which is what makes the agent genuinely wait rather than + * guess and apologise afterwards. + * + * Unlike the approval hook, this does *not* fail closed. A question that cannot + * reach anyone is answered with an error result telling the model to ask in + * prose instead: there is no unsafe direction here — nothing is gated on the + * answer — and a silent hang would be strictly worse than a degraded turn. + */ + +import * as net from 'net'; +import { createInterface } from 'readline'; +import { ASK_MCP_SERVER, ASK_QUESTION_TOOL } from '../../shared/chat-events.js'; + +/** What the browser sends back once someone has answered. */ +export interface QuestionAnswer { + /** Labels of the options picked, in the order they were offered. */ + labels: string[]; + /** True when the user declined to answer rather than picking nothing. */ + skipped?: boolean; + /** Set when the question could not be put to anyone. */ + error?: string; +} + +/** + * The tool as the model sees it. + * + * The description is doing real work: a tool the model never reaches for is the + * same as no tool at all, so it names the situations this is *for* rather than + * describing the parameters, which the schema already does. + */ +export const ASK_TOOL_DEFINITION = { + name: ASK_QUESTION_TOOL, + description: + 'Ask the user a question with a fixed set of answer options, and wait for their choice. ' + + 'Use this instead of guessing whenever the next step depends on a decision only the user can ' + + 'make and the plausible answers are known up front — which of several approaches to take, ' + + 'which of several candidate files or issues to act on, or any yes/no that would change what ' + + 'you do next. Prefer it over asking in prose: the user answers by clicking, so there is no ' + + 'wording to guess at. This call blocks until they answer.', + inputSchema: { + type: 'object', + properties: { + question: { + type: 'string', + description: 'The question, written to be answered by picking from the options below.', + }, + header: { + type: 'string', + description: 'A 2-4 word label for the question, used as the card heading.', + }, + multiSelect: { + type: 'boolean', + description: + 'True if the user may pick more than one option before confirming. Defaults to false, ' + + 'meaning exactly one.', + }, + options: { + type: 'array', + minItems: 2, + description: 'The answers on offer. Keep this to a short, fixed list the user can scan.', + items: { + type: 'object', + properties: { + label: { type: 'string', description: 'The option, in a few words.' }, + description: { + type: 'string', + description: 'One line on what picking this would mean. Optional but usually worth it.', + }, + }, + required: ['label'], + }, + }, + }, + required: ['question', 'options'], + }, +} as const; + +/** + * The client half of the session socket. + * + * One connection, reopened if it drops. A question is matched to its reply by + * id because the socket is per-session rather than per-call, and a model that + * asks two things in one turn would otherwise read the wrong answer. + */ +class AskChannel { + private socket: net.Socket | null = null; + private nextId = 0; + private readonly waiting = new Map void>(); + + constructor(private readonly socketPath: string) {} + + ask(question: unknown): Promise { + return new Promise((resolve) => { + const id = `ask-${process.pid}-${(this.nextId += 1)}`; + let socket: net.Socket; + try { + socket = this.connect(); + } catch (error: unknown) { + resolve({ labels: [], error: describe(error) }); + return; + } + + this.waiting.set(id, resolve); + const write = (): void => { + socket.write(`${JSON.stringify({ id, kind: 'question', question })}\n`); + }; + if (socket.pending) { + socket.once('connect', write); + } else { + write(); + } + }); + } + + private connect(): net.Socket { + if (this.socket && !this.socket.destroyed) return this.socket; + + const socket = net.createConnection(this.socketPath); + this.socket = socket; + socket.setEncoding('utf8'); + + let buffer = ''; + socket.on('data', (chunk: string) => { + buffer += chunk; + let at: number; + while ((at = buffer.indexOf('\n')) !== -1) { + const line = buffer.slice(0, at).trim(); + buffer = buffer.slice(at + 1); + if (!line) continue; + let reply: { id?: string; labels?: unknown; skipped?: boolean; error?: string }; + try { + reply = JSON.parse(line); + } catch { + continue; + } + // Approval traffic shares this socket; anything not addressed to a + // question we asked belongs to the hook and is not ours to consume. + const pending = reply.id ? this.waiting.get(reply.id) : undefined; + if (!pending || !reply.id) continue; + this.waiting.delete(reply.id); + pending({ + labels: Array.isArray(reply.labels) ? reply.labels.map(String) : [], + skipped: reply.skipped === true, + error: reply.error, + }); + } + }); + + const fail = (reason: string): void => { + this.socket = null; + // Everything still in flight is unanswerable now; say so once rather than + // leaving the model blocked on a socket that is not coming back. + for (const [id, resolve] of this.waiting) { + this.waiting.delete(id); + resolve({ labels: [], error: reason }); + } + }; + socket.on('error', () => fail('the browser could not be reached to ask this question')); + socket.on('close', () => fail('the chat session ended before this question was answered')); + + return socket; + } +} + +function describe(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +/** Render an answer as the sentence the model reads back as its tool result. */ +export function describeAnswer(answer: QuestionAnswer): { text: string; isError: boolean } { + if (answer.error) { + return { + text: + `The question could not be put to the user: ${answer.error}. ` + + 'Ask in your reply instead, in plain text, and carry on once they answer.', + isError: true, + }; + } + if (answer.skipped || answer.labels.length === 0) { + return { + text: + 'The user skipped this question without choosing. Do not ask it again — ' + + 'either proceed with the most reasonable option and say which you took, or ask them ' + + 'in plain text if you genuinely cannot continue.', + isError: false, + }; + } + return { + text: `The user selected: ${answer.labels.map((label) => `"${label}"`).join(', ')}`, + isError: false, + }; +} + +interface Rpc { + id?: string | number; + method?: string; + params?: Record; +} + +/** + * Serve MCP on the given streams until stdin closes. + * + * Exported and stream-injected so the tests can drive the real protocol over a + * pair of pipes rather than asserting against a hand-rolled imitation of it. + */ +export function serveAsk( + input: NodeJS.ReadableStream, + output: NodeJS.WritableStream, + ask: (question: unknown) => Promise, +): void { + const send = (message: unknown): void => { + output.write(`${JSON.stringify(message)}\n`); + }; + + createInterface({ input }).on('line', (line: string) => { + if (!line.trim()) return; + let message: Rpc; + try { + message = JSON.parse(line); + } catch { + return; + } + + const { id, method, params } = message; + + if (method === 'initialize') { + send({ + jsonrpc: '2.0', + id, + result: { + // Echoed rather than pinned: the negotiated version is whatever the + // client asked for, and hard-coding one means breaking on the next + // revision of a spec this server has no opinions about. + protocolVersion: (params?.protocolVersion as string) || '2025-06-18', + capabilities: { tools: {} }, + serverInfo: { name: 'ccweb', version: '1.0.0' }, + }, + }); + return; + } + + if (method === 'tools/list') { + send({ jsonrpc: '2.0', id, result: { tools: [ASK_TOOL_DEFINITION] } }); + return; + } + + if (method === 'tools/call') { + if (params?.name !== ASK_QUESTION_TOOL) { + send({ jsonrpc: '2.0', id, error: { code: -32601, message: `no tool named ${String(params?.name)}` } }); + return; + } + void ask(params?.arguments).then((answer) => { + const { text, isError } = describeAnswer(answer); + send({ jsonrpc: '2.0', id, result: { content: [{ type: 'text', text }], isError } }); + }); + return; + } + + // Notifications carry no id and want no reply; anything else gets an empty + // result rather than an error, so an unknown-but-harmless method from a + // future client does not fail the handshake. + if (id === undefined) return; + send({ jsonrpc: '2.0', id, result: {} }); + }); +} + +/** The env var naming the session socket, shared with the approval hook. */ +export const ASK_SOCKET_ENV = 'CCWEB_ASK_SOCKET'; + +/** + * The `--mcp-config` payload that hands this server to a runtime. + * + * Inline JSON rather than a file: nothing is written to the user's own MCP + * configuration, for exactly the reason the approval hook is passed inline too — + * this app is not the only thing using that CLI, and a session-scoped capability + * should not outlive the session or appear in anyone else's tool list. + */ +export function askMcpConfig(serverScript: string, socketPath: string): string { + return JSON.stringify({ + mcpServers: { + [ASK_MCP_SERVER]: { + command: process.execPath, + args: [serverScript], + env: { [ASK_SOCKET_ENV]: socketPath }, + }, + }, + }); +} + +function main(): void { + const socketPath = process.env[ASK_SOCKET_ENV]; + const channel = socketPath ? new AskChannel(socketPath) : null; + serveAsk(process.stdin, process.stdout, (question) => + channel + ? channel.ask(question) + : Promise.resolve({ labels: [], error: 'this session has no channel to the browser' }), + ); +} + +// Only when run as the spawned server, so importing this for its tool +// definition (or its tests) does not start reading stdin. +if (process.argv[1] && /ask-mcp\.(js|ts)$/.test(process.argv[1])) { + main(); +} diff --git a/src/server/chat/manager.ts b/src/server/chat/manager.ts index adfa038..9113d87 100644 --- a/src/server/chat/manager.ts +++ b/src/server/chat/manager.ts @@ -30,12 +30,14 @@ export interface ChatManagerDeps { export class ChatSessionManager { private readonly sessions = new Map(); private readonly hookScript: string; + private readonly askScript: string; constructor(private readonly deps: ChatManagerDeps) { // Resolved from this module's own location rather than cwd: the server is // routinely started from whatever directory the user happened to be in, // and the hook has to be found from the agent's working directory too. this.hookScript = path.join(__dirname, 'permission-hook.js'); + this.askScript = path.join(__dirname, 'ask-mcp.js'); } get store(): ChatStore { @@ -76,6 +78,7 @@ export class ChatSessionManager { store: this.deps.store, socketDir: path.join(this.deps.storageDir, 'cs'), hookScript: this.hookScript, + askScript: this.askScript, broadcast: this.deps.broadcast, resolveCommand: this.deps.resolveCommand, readFile: (sessionId, filePath) => this.readFile(sessionId, filePath), @@ -228,6 +231,15 @@ export class ChatSessionManager { return this.sessions.get(sessionId)?.respondPermission(requestId, optionId) ?? false; } + answerQuestion( + sessionId: string, + requestId: string, + optionIds: string[], + skipped = false, + ): boolean { + return this.sessions.get(sessionId)?.answerQuestion(requestId, optionIds, skipped) ?? false; + } + async stop(sessionId: string): Promise { const session = this.sessions.get(sessionId); if (!session) return; diff --git a/src/server/chat/permission-broker.ts b/src/server/chat/permission-broker.ts index f76f941..4597fe0 100644 --- a/src/server/chat/permission-broker.ts +++ b/src/server/chat/permission-broker.ts @@ -36,7 +36,40 @@ export interface PermissionAnswer { reason?: string; } -type Decider = (ask: PermissionAsk) => Promise; +/** + * A question the model asked, as it arrives off the socket. + * + * `unknown` on purpose at this layer: the shape is whatever the model passed to + * the MCP tool, and validating it is the session's job — the broker only routes. + */ +export interface QuestionAsk { + question?: unknown; + header?: unknown; + multiSelect?: unknown; + options?: unknown; +} + +/** What the browser answered, on its way back to the waiting tool call. */ +export interface QuestionReply { + labels: string[]; + skipped?: boolean; + error?: string; +} + +/** + * The two things that dial into a session's socket. + * + * One socket, two kinds of caller: the PreToolUse hook asking whether a tool may + * run, and the MCP server asking the user a question. They are kept apart here + * rather than being squeezed into one decider because the answers have nothing + * in common — a boolean with a reason versus a list of chosen labels — and a + * union that had to be narrowed at every call site would be worse than two + * fields. + */ +export interface BrokerHandlers { + permission: (ask: PermissionAsk) => Promise; + question: (ask: QuestionAsk) => Promise; +} /** * How long a unix socket path may actually be. @@ -50,6 +83,10 @@ type Decider = (ask: PermissionAsk) => Promise; */ const MAX_SOCKET_PATH_BYTES = 103; +function describeError(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + function socketPathFits(candidate: string): boolean { return Buffer.byteLength(candidate, 'utf8') <= MAX_SOCKET_PATH_BYTES; } @@ -59,7 +96,7 @@ export class PermissionBroker { private socketPath = ''; /** Set only when the socket had to be placed outside `socketDir`. */ private tempDir: string | null = null; - private decide: Decider | null = null; + private handlers: BrokerHandlers | null = null; private readonly open = new Set(); constructor(private readonly socketDir: string) {} @@ -80,10 +117,10 @@ export class PermissionBroker { * directory mode already stops another account from connecting, there is no * reason to make the path guessable by whatever else runs as this user. */ - async listen(decide: Decider): Promise { + async listen(handlers: BrokerHandlers): Promise { if (this.server) return this.socketPath; - this.decide = decide; + this.handlers = handlers; this.socketPath = this.reservePath(); const server = net.createServer((socket) => this.accept(socket)); @@ -171,7 +208,7 @@ export class PermissionBroker { } private handle(socket: net.Socket, line: string): void { - let payload: { id?: string; ask?: PermissionAsk }; + let payload: { id?: string; kind?: string; ask?: PermissionAsk; question?: QuestionAsk }; try { payload = JSON.parse(line); } catch { @@ -179,26 +216,48 @@ export class PermissionBroker { } const id = payload.id; - const ask = payload.ask; - if (!id || !ask) return; + if (!id) return; - const reply = (answer: PermissionAnswer): void => { + const reply = (answer: PermissionAnswer | QuestionReply): void => { if (socket.destroyed) return; socket.write(`${JSON.stringify({ id, ...answer })}\n`); }; - const decide = this.decide; - if (!decide) { + const handlers = this.handlers; + + // A question from the MCP server. Note the opposite failure direction from + // an approval below: nothing is gated on the answer, so a question that + // cannot be asked comes back as an error the model can route around rather + // than as a refusal, and never as silence. + if (payload.kind === 'question') { + const question = payload.question ?? {}; + if (!handlers) { + reply({ labels: [], error: 'this session is not accepting questions' }); + return; + } + handlers + .question(question) + .then(reply) + .catch((error: unknown) => { + reply({ labels: [], error: describeError(error) }); + }); + return; + } + + const ask = payload.ask; + if (!ask) return; + + if (!handlers) { reply({ allow: false, reason: 'the approval channel is not accepting decisions' }); return; } - decide(ask) + handlers + .permission(ask) .then(reply) .catch((error: unknown) => { - const message = error instanceof Error ? error.message : String(error); // Failing closed: an approval that cannot be asked has not been given. - reply({ allow: false, reason: `approval failed: ${message}` }); + reply({ allow: false, reason: `approval failed: ${describeError(error)}` }); }); } diff --git a/src/server/chat/session.ts b/src/server/chat/session.ts index 1037451..25d9623 100644 --- a/src/server/chat/session.ts +++ b/src/server/chat/session.ts @@ -10,14 +10,27 @@ import { PermissionOption, PermissionRequest, QueuedTurn, + QuestionOption, + QuestionRequest, UserTurn, classifyTool, defaultPermissionOptions, isAllowOption, + isAskQuestionTool, + normalizeQuestionOptions, + ASK_QUESTION_TOOL_NAME, } from '../../shared/chat-events.js'; import { isClearingCommand } from '../../shared/slash-commands.js'; import { AdapterEvent, ChatAdapter } from './adapter.js'; -import { PermissionAsk, PermissionAnswer, PermissionBroker, permissionHookSettings } from './permission-broker.js'; +import { + PermissionAsk, + PermissionAnswer, + PermissionBroker, + QuestionAsk, + QuestionReply, + permissionHookSettings, +} from './permission-broker.js'; +import { askMcpConfig } from './ask-mcp.js'; import { ChatStoreLike, ChatSessionRef } from './store.js'; import { createChatAdapter, supportsChat } from './registry.js'; @@ -44,6 +57,13 @@ export interface ChatSessionDeps { socketDir: string; /** Absolute path to the compiled permission hook script. */ hookScript: string; + /** + * Absolute path to the compiled MCP server that asks the user questions. + * + * Optional so a deployment that has not built it (or does not want the + * capability) simply runs without it, rather than failing to start a session. + */ + askScript?: string; /** Push an event to every browser watching this session. */ broadcast: (sessionId: string, message: Record) => void; /** Resolve the executable for a runtime, from the existing bridge lookup. */ @@ -92,6 +112,12 @@ interface PendingApproval { resolve?: (answer: PermissionAnswer) => void; } +/** A question put to the browser, and the tool call waiting on the answer. */ +interface PendingQuestion { + request: QuestionRequest; + resolve: (reply: QuestionReply) => void; +} + /** * Event kinds a resuming runtime may re-emit from history. * @@ -145,6 +171,27 @@ export class ChatSession { private state: ChatState = 'starting'; private capabilities: ChatCapabilities | null = null; private readonly pending = new Map(); + private readonly questions = new Map(); + /** + * Question tool calls the transcript has opened, oldest first, unclaimed. + * + * The MCP server that carries a question has no way to know the tool_use id of + * the call it is serving — it only ever sees the arguments — so the pairing is + * made here instead, from the block the adapter has already reported. + * + * A queue rather than a single slot, because a runtime may announce two tool + * calls in one message before running either: with one slot the second + * announcement would overwrite the first, the first question would claim the + * second call's id, and both cards would be drawn in the wrong place. Calls + * run in the order they were announced, so first in is first claimed. + * + * An unclaimed id left over from a call that never reached the MCP server + * would mispair the next question, so the queue is emptied at the end of every + * turn — by which point nothing can still be waiting to claim one. + */ + private readonly askToolIds: string[] = []; + /** True once this session actually handed a runtime the question tool. */ + private questionsEnabled = false; private runtime = ''; private bypass = false; private nativeSessionId: string | null = null; @@ -241,18 +288,43 @@ export class ChatSession { const env = { ...(options.env || {}) }; const extraArgs = [...(options.extraArgs || [])]; - // Claude gates tools through a PreToolUse hook rather than a protocol - // message, so its approval channel is a socket the hook dials back into. - // Skipped entirely when bypassing: there is nothing to ask. - if (!this.bypass && options.runtime === 'claude' && fs.existsSync(this.deps.hookScript)) { + // Claude reaches the browser over a unix socket for two different reasons: + // a PreToolUse hook asking whether a tool may run, and an MCP server asking + // the user a question. Both dial the same socket, so it is opened whenever + // either of them will be installed. + // + // The hook is skipped when bypassing — there is nothing to approve — but the + // question channel is not. Bypassing approvals means "stop asking me before + // you act"; it has never meant "answer my questions on my behalf", and a + // model that asks which of three approaches to take still needs a person. + const wantsHook = !this.bypass && options.runtime === 'claude' && fs.existsSync(this.deps.hookScript); + const askScript = this.deps.askScript; + const wantsAsk = options.runtime === 'claude' && Boolean(askScript) && fs.existsSync(askScript!); + + if (wantsHook || wantsAsk) { // One shared directory, not one per session. A directory named after the // session id cost 37 bytes of a 103-byte path budget, which is what put // the socket over the kernel's limit; the random socket filename already // carries the unguessability that directory was standing in for. this.broker = new PermissionBroker(this.deps.socketDir); - const socketPath = await this.broker.listen((ask) => this.askUser(ask)); - extraArgs.push('--settings', permissionHookSettings(this.deps.hookScript, socketPath)); - env.CCWEB_PERMISSION_SOCKET = socketPath; + const socketPath = await this.broker.listen({ + permission: (ask) => this.askUser(ask), + question: (ask) => this.askQuestion(ask), + }); + + if (wantsHook) { + extraArgs.push('--settings', permissionHookSettings(this.deps.hookScript, socketPath)); + env.CCWEB_PERMISSION_SOCKET = socketPath; + } + if (wantsAsk) { + extraArgs.push('--mcp-config', askMcpConfig(askScript!, socketPath)); + // Named explicitly rather than relying on the hook to wave it through: + // with approvals bypassed there is no hook at all, and without this the + // one tool whose whole purpose is to ask the user something would be the + // one tool the runtime refused to run. + extraArgs.push('--allowedTools', ASK_QUESTION_TOOL_NAME); + this.questionsEnabled = true; + } } const adapter = createChatAdapter(options.runtime, { @@ -304,6 +376,14 @@ export class ChatSession { if (!this.capabilities) { this.capabilities = adapter.capabilities; } + // Not an adapter capability: whether the model can ask a question is a fact + // about what this session wired up, not about what the runtime can parse. + // The same adapter has it or does not depending on whether the MCP server + // was built and found. + if (this.questionsEnabled && this.capabilities) { + this.capabilities = { ...this.capabilities, questions: true }; + this.ingest({ t: 'capabilities', capabilities: { questions: true } }); + } this.setState('idle'); } @@ -331,6 +411,16 @@ export class ChatSession { } as ChatEvent; if (stamped.t === 'session') { + // Patched on the event itself, not just on the copy kept here. Every + // reader of this log — this session, the browser's reducer, a snapshot + // replayed tomorrow — takes `session.capabilities` as a wholesale + // replacement, so a flag re-applied only locally would be true on the + // server and false in every browser. Whether the model can ask a question + // is a fact about what this session wired up; the runtime introducing + // itself knows nothing about it and must not be able to unset it. + if (this.questionsEnabled && !stamped.capabilities.questions) { + stamped.capabilities = { ...stamped.capabilities, questions: true }; + } if (stamped.nativeSessionId) { this.nativeSessionId = stamped.nativeSessionId; this.deps.onLifecycle?.(this.ref.id, { nativeSessionId: stamped.nativeSessionId }); @@ -363,6 +453,29 @@ export class ChatSession { if (stamped.t === 'capabilities' && this.capabilities) { this.capabilities = { ...this.capabilities, ...stamped.capabilities }; } + // A question tool call opening is the only chance to learn its id: by the + // time the MCP server relays the question itself, the arguments are all it + // knows. `block_start` carries the name, `tool` patches carry only the id, + // so this is the one event that can make the pairing. + if (stamped.t === 'block_start' && stamped.block.kind === 'tool' && isAskQuestionTool(stamped.block.name)) { + this.askToolIds.push(stamped.block.toolId); + } + if (stamped.t === 'turn_end') { + this.askToolIds.length = 0; + } + if (stamped.t === 'question') { + const existing = this.questions.get(stamped.request.requestId); + if (existing) { + // Same merge-don't-replace rule the approval path learned the hard way: + // `askQuestion` records the resolver before it emits this event, and + // overwriting the entry here would throw away the only thing that can + // unblock the waiting tool call. + this.questions.set(stamped.request.requestId, { request: stamped.request, resolve: existing.resolve }); + } + } + if (stamped.t === 'question_resolved') { + this.questions.delete(stamped.requestId); + } if (stamped.t === 'permission') { // Merged, never replaced. `askUser` records the resolver *before* it // emits this event, and a plain `set` here threw that resolver away — @@ -597,6 +710,19 @@ export class ChatSession { this.ingest({ t: 'permission_resolved', requestId, optionId: 'reject_once', allowed: false }); } this.pending.clear(); + // A question is moot once the turn it belongs to is cancelled, and a card + // left on screen would invite an answer with nothing left to receive it. + for (const [requestId, entry] of this.questions) { + entry.resolve({ labels: [], error: 'the turn was interrupted' }); + this.ingest({ + t: 'question_resolved', + requestId, + toolId: entry.request.toolId, + optionIds: [], + skipped: true, + }); + } + this.questions.clear(); if (dropped) { this.ingest({ t: 'error', @@ -643,6 +769,13 @@ export class ChatSession { * rather than running the tool and apologising afterwards. */ private askUser(ask: PermissionAsk): Promise { + // The one tool that must never be gated. Asking someone to approve being + // asked a question is two prompts for one decision, and the second of them + // is unanswerable in any useful sense — refusing it just blocks the model + // from talking to the person sitting in front of it. + if (isAskQuestionTool(ask.toolName)) { + return Promise.resolve({ allow: true, reason: 'the user is being asked directly' }); + } if (this.bypass) { return Promise.resolve({ allow: true, reason: 'permissions are bypassed for this session' }); } @@ -666,6 +799,79 @@ export class ChatSession { }); } + /** + * A question from the model, on its way to a person. + * + * The promise is the tool call: it resolves when someone clicks, and the MCP + * server does not answer the runtime until it does. Everything here is + * defensive about shape because the payload is whatever the model wrote — a + * question with no options is a question nobody can answer, and coming back + * with an error the model can read is better than putting an empty card on + * screen and blocking the turn behind it. + */ + private askQuestion(ask: QuestionAsk): Promise { + const question = typeof ask.question === 'string' ? ask.question.trim() : ''; + const options = normalizeQuestionOptions(ask.options); + + if (!question || options.length === 0) { + return Promise.resolve({ + labels: [], + error: 'the question needs a question and at least one option', + }); + } + + return new Promise((resolve) => { + const requestId = `ask-${crypto.randomUUID()}`; + const request: QuestionRequest = { + requestId, + // Claimed, not merely read: a second question must not attach itself to + // the same tool block, which would draw two cards in one place and + // leave the later one unanswerable. + toolId: this.askToolIds.shift(), + question, + header: typeof ask.header === 'string' && ask.header.trim() ? ask.header.trim() : undefined, + multiSelect: ask.multiSelect === true, + options, + ts: Date.now(), + }; + this.questions.set(requestId, { request, resolve }); + this.ingest({ t: 'question', request }); + this.setState('awaiting_answer'); + }); + } + + /** + * Record the answer a browser sent, and hand it to the waiting tool call. + * + * Returns false for a question this session does not have, which is what a + * second browser answering one that has already been answered looks like. + */ + answerQuestion(requestId: string, optionIds: string[], skipped = false): boolean { + const entry = this.questions.get(requestId); + if (!entry) return false; + + // Filtered against the offered options rather than trusted: the ids come + // from a browser, and the labels they resolve to are about to be handed + // straight to the model as fact. + const picked = entry.request.options.filter((option) => optionIds.includes(option.optionId)); + const answered = !skipped && picked.length > 0; + + entry.resolve({ + labels: picked.map((option) => option.label), + skipped: !answered, + }); + + this.questions.delete(requestId); + this.ingest({ + t: 'question_resolved', + requestId, + toolId: entry.request.toolId, + optionIds: picked.map((option) => option.optionId), + skipped: !answered, + }); + return true; + } + /** * Switch the live process to a different model, for the adapters that can. * @@ -715,6 +921,7 @@ export class ChatSession { state: this.live ? snapshot.state : 'exited', capabilities: this.capabilities || snapshot.capabilities, pendingPermissions: Array.from(this.pending.values()).map((entry) => entry.request), + pendingQuestions: Array.from(this.questions.values()).map((entry) => entry.request), queued: this.queuedTurns, live: this.live, nativeSessionId: this.nativeSessionId || undefined, @@ -727,6 +934,14 @@ export class ChatSession { approval.resolve?.({ allow: false, reason: 'the session was stopped' }); } this.pending.clear(); + // The MCP server's socket is about to go with the process, but it is the + // one waiting on these promises: resolving them here is what turns a + // shutdown into a tool result rather than a connection that simply stops + // answering. + for (const [, entry] of this.questions) { + entry.resolve({ labels: [], error: 'the session was stopped' }); + } + this.questions.clear(); this.clearQueue(); const adapter = this.adapter; diff --git a/src/server/websocket/messages.ts b/src/server/websocket/messages.ts index 94624e9..17d62c8 100644 --- a/src/server/websocket/messages.ts +++ b/src/server/websocket/messages.ts @@ -117,6 +117,12 @@ export interface ChatManagerLike { rememberModel(sessionId: string, model: string | undefined): void; cancelQueued(sessionId: string, queuedId: string): boolean; respondPermission(sessionId: string, requestId: string, optionId: string): boolean; + answerQuestion( + sessionId: string, + requestId: string, + optionIds: string[], + skipped?: boolean, + ): boolean; stop(sessionId: string): Promise; readPage( record: SessionRecord, @@ -141,6 +147,10 @@ interface IncomingMessage { text?: string; attachments?: unknown[]; optionId?: string; + /** Every option picked for a multiple-choice question the model asked. */ + optionIds?: unknown[]; + /** True when the user chose to answer a question with nothing. */ + skipped?: boolean; fromSeq?: number; agentKind?: string; /** Identifies one turn waiting in the send-ahead queue. */ @@ -240,6 +250,10 @@ export class MessageProcessor { this.handleChatPermission(wsInfo, data); break; + case 'chat_question_answer': + this.handleChatQuestion(wsInfo, data); + break; + case 'chat_history_request': await this.handleChatHistory(wsInfo, data); break; @@ -1350,6 +1364,28 @@ export class MessageProcessor { manager.respondPermission(session.id, requestId, optionId); } + /** + * Answer a multiple-choice question the model asked. + * + * Separate from the approval route rather than reusing it with a list: an + * approval is one decision out of a set this app defines, and a question is an + * arbitrary selection out of a set the model wrote. Routing both through one + * handler would mean a browser could answer a question with an approval id. + */ + private handleChatQuestion(wsInfo: WebSocketInfo, data: IncomingMessage): void { + const manager = this.deps.chatManager; + const session = this.chatSessionFor(wsInfo, data.sessionId); + if (!manager || !session) return; + + const requestId = typeof data.requestId === 'string' ? data.requestId : ''; + if (!requestId) return; + + const optionIds = Array.isArray(data.optionIds) + ? data.optionIds.filter((id): id is string => typeof id === 'string') + : []; + manager.answerQuestion(session.id, requestId, optionIds, data.skipped === true); + } + private async handleChatHistory( wsInfo: WebSocketInfo, data: IncomingMessage, diff --git a/src/shared/chat-events.ts b/src/shared/chat-events.ts index 7b6ea65..f6ffc7d 100644 --- a/src/shared/chat-events.ts +++ b/src/shared/chat-events.ts @@ -324,6 +324,52 @@ export interface PermissionRequest { ts: number; } +/** + * One selectable answer to a question the model asked. + * + * `optionId` is minted by this app rather than taken from the model, which only + * ever supplies a label: two options can legitimately carry the same words + * ("Yes, and stop" / "Yes, and stop") and an id derived from the text would make + * them the same button. + */ +export interface QuestionOption { + optionId: string; + label: string; + /** The model's own gloss on what picking this means. */ + description?: string; +} + +/** + * A question the model asked, waiting on a person. + * + * Deliberately *not* a `PermissionRequest`. An approval is the app gating the + * agent — the options are always some arrangement of allow and deny, and the + * answer's meaning is known before it is given. A question is the agent asking + * the user something the app has no opinion about, the options are whatever the + * model wrote, and the answer is content rather than a decision. Folding the two + * together would mean either teaching the approval card to render arbitrary + * options or teaching `isAllowOption` to answer for text it cannot interpret. + */ +export interface QuestionRequest { + requestId: string; + /** + * The tool call that asked, when it could be identified. + * + * Present so the card can be drawn where the question was actually asked + * rather than in a tray at the bottom. Optional because correlation is + * best-effort and an uncorrelated question must still be answerable — an + * agent blocked on a question with no button anywhere is a hung session. + */ + toolId?: string; + question: string; + /** A short label for the question, when the model supplied one. */ + header?: string; + /** True when more than one option may be picked before confirming. */ + multiSelect: boolean; + options: QuestionOption[]; + ts: number; +} + /** What a chat session is doing right now, for the header indicator. */ export type ChatState = | 'starting' @@ -331,6 +377,8 @@ export type ChatState = | 'thinking' | 'running' | 'awaiting_permission' + /** Blocked on a question the model asked, which only a person can answer. */ + | 'awaiting_answer' | 'exited' | 'error'; @@ -352,6 +400,13 @@ export interface ChatCapabilities { diffs: boolean; /** The runtime can ask before acting, and honour a refusal. */ permissions: boolean; + /** + * The model can put a multiple-choice question to the user and wait for it. + * + * Optional rather than required so a stored snapshot written before this + * existed still parses; absent reads as false everywhere it is consulted. + */ + questions?: boolean; interrupt: boolean; /** A session can be resumed after the process is gone. */ resume: boolean; @@ -452,6 +507,30 @@ export type ChatEvent = /** Set when the decision came from the bypass setting, not a person. */ automatic?: boolean; } + | { t: 'question'; seq: number; ts: number; request: QuestionRequest } + | { + t: 'question_resolved'; + seq: number; + ts: number; + requestId: string; + /** + * The tool call that asked, repeated from the request. + * + * Carried on the resolution as well so a card rebuilt from the log alone + * can find its own answer: the request is dropped from the pending list + * the moment it resolves, and the id would otherwise go with it. + */ + toolId?: string; + /** Every option the user picked, in the order the question offered them. */ + optionIds: string[]; + /** + * True when the user chose to answer nothing. + * + * The model is still told — it is blocked and something has to come back — + * but the transcript says "skipped" rather than inventing a selection. + */ + skipped?: boolean; + } | { t: 'state'; seq: number; ts: number; state: ChatState } | { t: 'error'; seq: number; ts: number; message: string; fatal?: boolean } | { @@ -539,6 +618,13 @@ export interface ChatSnapshot { usage?: ChatUsage; plan?: PlanItem[]; pendingPermissions: PermissionRequest[]; + /** + * Questions still waiting on an answer. + * + * Optional for the same reason `queued` is: a snapshot replayed by a server + * that predates this should read as "none pending", not as malformed. + */ + pendingQuestions?: QuestionRequest[]; /** * Turns typed ahead, still waiting. Optional so a snapshot replayed from the * store — which knows nothing about a live process — is not obliged to @@ -590,8 +676,73 @@ export const NO_CHAT_CAPABILITIES: ChatCapabilities = { usage: false, cost: false, plan: false, + questions: false, }; +/** The MCP server this app exposes to the runtimes it launches. */ +export const ASK_MCP_SERVER = 'ccweb'; + +/** The one tool that server offers: put a multiple-choice question to the user. */ +export const ASK_QUESTION_TOOL = 'ask_user_question'; + +/** + * What the tool is called once a runtime has namespaced it. + * + * Claude prefixes MCP tools as `mcp____`, and that prefixed name + * is what shows up in the transcript — so this is the string the UI matches on + * to draw a question card instead of a generic tool row. + */ +export const ASK_QUESTION_TOOL_NAME = `mcp__${ASK_MCP_SERVER}__${ASK_QUESTION_TOOL}`; + +/** + * Whether a tool name refers to this app's question tool. + * + * Suffix rather than equality: runtimes namespace MCP tools differently (and + * have changed the separator before), so the bare name is the part that can be + * relied on. Nothing else in the transcript is called this. + */ +/** + * Turn whatever a model passed as `options` into answerable choices. + * + * Shared rather than implemented on each side, and that is the whole point: the + * server mints the ids it will later be answered with, and the browser mints the + * same ids again when it rebuilds a card from the tool call in a replayed + * transcript. Two copies of this that drifted by one dropped entry would put the + * tick on the wrong option, which is a lie about what the user chose. + * + * Ids are positional rather than derived from the labels: two options may + * legitimately read the same, and a label-derived id would collapse them into + * one button that answers for both. Anything unusable is dropped instead of + * rendered as an empty choice, and a bare string is accepted as its own label + * because that is what a model reaches for when the schema slips its mind. + */ +export function normalizeQuestionOptions(raw: unknown): QuestionOption[] { + if (!Array.isArray(raw)) return []; + const options: QuestionOption[] = []; + for (const entry of raw) { + let label = ''; + let description: string | undefined; + if (typeof entry === 'string') { + label = entry.trim(); + } else if (entry && typeof entry === 'object') { + const object = entry as Record; + const text = object.label ?? object.name ?? object.value ?? object.title; + if (typeof text === 'string') label = text.trim(); + if (typeof object.description === 'string' && object.description.trim()) { + description = object.description.trim(); + } + } + if (!label) continue; + options.push({ optionId: `opt-${options.length}`, label, description }); + } + return options; +} + +export function isAskQuestionTool(name: string | undefined): boolean { + if (!name) return false; + return name === ASK_QUESTION_TOOL || name.endsWith(`__${ASK_QUESTION_TOOL}`); +} + /** * Tool-name → kind mapping shared by every adapter. * diff --git a/src/shared/chat-reducer.ts b/src/shared/chat-reducer.ts index 5a8edf8..74f3567 100644 --- a/src/shared/chat-reducer.ts +++ b/src/shared/chat-reducer.ts @@ -25,6 +25,7 @@ import { ChatUsage, PermissionRequest, PlanItem, + QuestionRequest, TextBlock, ThinkingBlock, ToolBlock, @@ -50,6 +51,26 @@ export interface TranscriptState { usage: ChatUsage; plan: PlanItem[]; pendingPermissions: PermissionRequest[]; + /** + * Questions the model asked that nobody has answered yet. + * + * Held beside the transcript rather than inside a message because a question + * outlives the block that asked it: the agent stays blocked across a reload, + * and this is what a rejoining browser reads to know there is still a card to + * draw. The record of what was *asked and answered* lives in the tool block, + * which is where scrolling back finds it. + */ + pendingQuestions: QuestionRequest[]; + /** + * Answers already given, keyed by the tool call that asked. + * + * Keyed on `toolId` rather than `requestId` because the card that needs it is + * drawn from a tool block, and by the time it asks, the request — the only + * thing that knew both ids — has been dropped from the pending list. Falls + * back to the request id for a question that could not be correlated to a + * call, which is answered from the pinned card instead. + */ + answeredQuestions: Record; /** Lowest seq present. Non-zero once the log head has been trimmed. */ firstSeq: number; /** Highest seq applied. Events at or below this are ignored as replays. */ @@ -93,6 +114,8 @@ export function createTranscript( usage: {}, plan: [], pendingPermissions: [], + pendingQuestions: [], + answeredQuestions: {}, firstSeq: 0, cursor: 0, currentTurnId: null, @@ -384,6 +407,37 @@ export function applyChatEvent(state: TranscriptState, event: ChatEvent): Transc return { messageIndex: null, structural: false, meta: true, applied: true }; } + case 'question': { + const already = state.pendingQuestions.some( + (pending) => pending.requestId === event.request.requestId, + ); + if (!already) { + state.pendingQuestions.push(event.request); + } + // Not folded into `awaiting_permission`: the composer, the header and the + // stop button all read this, and "waiting for approval" over a question + // about which of three approaches to take is simply the wrong sentence. + state.state = 'awaiting_answer'; + return { messageIndex: null, structural: false, meta: true, applied: true }; + } + + case 'question_resolved': { + const asked = state.pendingQuestions.find((pending) => pending.requestId === event.requestId); + state.pendingQuestions = state.pendingQuestions.filter( + (pending) => pending.requestId !== event.requestId, + ); + // Kept after the fact so the card keeps showing what was picked once the + // request itself is gone, without waiting for the runtime to echo a tool + // result back. + state.answeredQuestions[event.toolId ?? asked?.toolId ?? event.requestId] = event.skipped + ? [] + : [...event.optionIds]; + if (state.state === 'awaiting_answer' && state.pendingQuestions.length === 0) { + state.state = 'running'; + } + return { messageIndex: null, structural: false, meta: true, applied: true }; + } + case 'state': { state.state = event.state; return { messageIndex: null, structural: false, meta: true, applied: true }; @@ -416,6 +470,12 @@ export function applyChatEvent(state: TranscriptState, event: ChatEvent): Transc state.index = {}; state.plan = []; state.lastError = undefined; + // The cards go with the conversation they were asked in. A question + // card left behind would be drawn against a tool block that is no + // longer on screen, and answering it would reach a turn that no longer + // exists — the session resolves them at the same moment on its side. + state.pendingQuestions = []; + state.answeredQuestions = {}; // And no paging back past it. The log still holds what was said — this // is a view, not a delete — but offering "load earlier messages" right // after someone asked for a clean window would undo the thing they diff --git a/test/browser/checks.ts b/test/browser/checks.ts index d3fe6a8..a46be0d 100644 --- a/test/browser/checks.ts +++ b/test/browser/checks.ts @@ -236,6 +236,7 @@ async function run(): Promise { await checkTheFixedBarsNeverWrap(); await checkALiveAnswerAppearsAsItStreams(); await checkSilentStepsLeaveNoRowButKeepTheirTrace(); + await checkAQuestionIsAnsweredByClicking(); await checkThePhoneLayoutIsUsable(); await checkThePhoneShellSurfacesAreUsable(); @@ -245,6 +246,200 @@ async function run(): Promise { document.body.appendChild(pre); } +/** + * A question the model asked is answered by clicking, and stays as a record. + * + * Here rather than in a unit test because every claim is about the rendered + * conversation: that the card appears *inside* the transcript at the call that + * asked (not in a tray under it), that a multi-select needs a confirm and a + * single-select does not, and that the card survives the answer instead of + * vanishing. A jsdom assertion on props would pass for a card rendered into + * nowhere. + */ +async function checkAQuestionIsAnsweredByClicking(): Promise { + const host = document.createElement('div'); + host.style.cssText = 'width:900px;height:700px;position:absolute;top:0;left:0;display:flex'; + document.body.appendChild(host); + + const sent: Array> = []; + const controller = new ChatController('browser-check', { + send: (message: Record) => { + sent.push(message); + }, + } as never); + + const askBlock = (toolId: string, input: unknown): Record => ({ + kind: 'tool', + toolId, + name: 'mcp__ccweb__ask_user_question', + toolKind: 'other', + status: 'running', + input, + }); + + controller.handle({ + type: 'chat_snapshot', + sessionId: 'browser-check', + snapshot: { + sessionId: 'browser-check', + runtime: 'claude', + state: 'awaiting_answer', + capabilities: { + streaming: true, thinking: true, toolCalls: true, diffs: true, permissions: true, + questions: true, interrupt: true, resume: true, fork: false, attachments: true, + usage: true, cost: true, plan: false, + }, + messages: [ + { + id: 'm1', seq: 1, turnId: 't1', role: 'user', ts: 1, + blocks: [{ kind: 'text', text: 'Fix the parser.' }], + }, + { + id: 'm2', seq: 2, turnId: 't1', role: 'assistant', ts: 2, + blocks: [ + askBlock('tool-single', { + question: 'Which approach should I take?', + header: 'Approach', + multiSelect: false, + options: [ + { label: 'Rewrite it', description: 'Slower but cleaner' }, + { label: 'Patch it', description: 'Faster, more debt' }, + ], + }), + ], + }, + { + id: 'm3', seq: 3, turnId: 't1', role: 'assistant', ts: 3, + blocks: [ + askBlock('tool-multi', { + question: 'Which rules should I apply?', + multiSelect: true, + options: [{ label: 'semicolons' }, { label: 'trailing commas' }, { label: 'single quotes' }], + }), + ], + }, + ], + pendingPermissions: [], + pendingQuestions: [ + { + requestId: 'q-single', toolId: 'tool-single', question: 'Which approach should I take?', + header: 'Approach', multiSelect: false, ts: 2, + options: [ + { optionId: 'opt-0', label: 'Rewrite it', description: 'Slower but cleaner' }, + { optionId: 'opt-1', label: 'Patch it', description: 'Faster, more debt' }, + ], + }, + { + requestId: 'q-multi', toolId: 'tool-multi', question: 'Which rules should I apply?', + multiSelect: true, ts: 3, + options: [ + { optionId: 'opt-0', label: 'semicolons' }, + { optionId: 'opt-1', label: 'trailing commas' }, + { optionId: 'opt-2', label: 'single quotes' }, + ], + }, + ], + firstSeq: 1, + replayFrom: 1, + cursor: 3, + live: true, + bypassPermissions: false, + }, + } as never); + + const root = createRoot(host); + root.render( + React.createElement(ChatView, { + controller, + runtime: 'claude', + runtimeLabel: 'Claude Code', + workingDir: '/tmp/project', + view: DEFAULT_CHAT_VIEW, + onViewChange: () => {}, + } as never), + ); + await wait(400); + + const cards = Array.from(host.querySelectorAll('[data-question-card]')) as HTMLElement[]; + check('both questions render as cards', cards.length === 2, `found ${cards.length}`); + if (cards.length !== 2) return; + + const [single, multi] = cards; + + // Inside the conversation, at the call that asked — the thing that makes the + // exchange still read as a conversation when scrolled back to. A card in a + // pinned tray would be outside this element entirely. + const inTranscript = single.closest('[data-message-id="m2"]'); + check('the card sits in the message that asked', !!inTranscript); + + const optionText = (card: HTMLElement): string => card.textContent || ''; + check( + 'the single-choice card offers every option', + optionText(single).includes('Rewrite it') && optionText(single).includes('Patch it'), + ); + check('option descriptions are shown', optionText(single).includes('Slower but cleaner')); + + // A single-choice question answers on the click itself; a multi-select must + // not, or the first tick would send the answer and the rest would go nowhere. + const singleButtons = Array.from(single.querySelectorAll('button')) as HTMLButtonElement[]; + const patch = singleButtons.find((b) => (b.textContent || '').includes('Patch it')); + check('each option is its own control', !!patch); + + const boxes = Array.from(multi.querySelectorAll('input[type="checkbox"]')) as HTMLInputElement[]; + check('the multi-select card offers checkboxes', boxes.length === 3, `found ${boxes.length}`); + const confirm = (Array.from(multi.querySelectorAll('button')) as HTMLButtonElement[]) + .find((b) => (b.textContent || '').trim() === 'Confirm'); + check('the multi-select card has a confirm', !!confirm); + check('confirm is disabled before anything is picked', !!confirm && confirm.disabled); + + if (boxes.length === 3 && confirm) { + boxes[0].click(); + boxes[2].click(); + await wait(150); + check('confirm becomes available once something is picked', !confirm.disabled); + check('picking a checkbox does not answer on its own', sent.length === 0, `${sent.length} sent`); + confirm.click(); + await wait(150); + } + + if (patch) { + patch.click(); + await wait(200); + } + + const answers = sent.filter((m) => m.type === 'chat_question_answer'); + check('both answers reach the server', answers.length === 2, `${answers.length} sent`); + const multiAnswer = answers.find((m) => m.requestId === 'q-multi'); + const singleAnswer = answers.find((m) => m.requestId === 'q-single'); + check( + 'the multi-select answer carries every pick', + !!multiAnswer && JSON.stringify(multiAnswer.optionIds) === JSON.stringify(['opt-0', 'opt-2']), + JSON.stringify(multiAnswer?.optionIds), + ); + check( + 'the single-choice answer carries the one pick', + !!singleAnswer && JSON.stringify(singleAnswer.optionIds) === JSON.stringify(['opt-1']), + JSON.stringify(singleAnswer?.optionIds), + ); + + await wait(200); + const after = Array.from(host.querySelectorAll('[data-question-card]')) as HTMLElement[]; + // The card must not vanish on being answered: scrolling back past a decision + // should show the decision, which is the acceptance criterion a tray-based + // card fails. + check('the cards stay in the conversation after answering', after.length === 2, `${after.length} left`); + check( + 'an answered card stops offering buttons', + after.every((card) => card.getAttribute('data-question-card') === 'answered'), + after.map((c) => c.getAttribute('data-question-card')).join(','), + ); + check('the answered card still shows what was asked', (after[0].textContent || '').includes('Which approach')); + check('the answered card shows what was chosen', (after[0].textContent || '').includes('Patch it')); + + root.unmount(); + host.remove(); +} + /** * A program that asks the terminal what it supports must not be able to stop it * rendering. diff --git a/test/chat-permission-broker.test.js b/test/chat-permission-broker.test.js index 8b32a0f..39e7f70 100644 --- a/test/chat-permission-broker.test.js +++ b/test/chat-permission-broker.test.js @@ -32,6 +32,17 @@ const PAYLOAD = { tool_use_id: 'toolu_01EaYEk7bykuwhCC86fZeTcC', }; +/** + * A question handler for the approval tests, which never ask one. + * + * The broker takes both handlers because one socket carries both kinds of + * caller; these tests are about the approval half, so this stands in for the + * other and would fail loudly if an approval were ever routed to it. + */ +const NO_QUESTIONS = async () => { + throw new Error('these tests never ask a question'); +}; + /** Run the hook exactly as the CLI does: payload on stdin, JSON on stdout. */ function runHook(socketPath, payload = PAYLOAD) { return new Promise((resolve) => { @@ -85,9 +96,12 @@ describe('chat permission broker', function () { it('carries an approval from the hook to a decider and back', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); const seen = []; - const socketPath = await broker.listen(async (ask) => { - seen.push(ask); - return { allow: true, reason: 'approved in the browser' }; + const socketPath = await broker.listen({ + permission: async (ask) => { + seen.push(ask); + return { allow: true, reason: 'approved in the browser' }; + }, + question: NO_QUESTIONS, }); const { decision } = await runHook(socketPath); @@ -103,10 +117,10 @@ describe('chat permission broker', function () { it('passes a refusal through with the reason the user gave', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ + const socketPath = await broker.listen({ permission: async () => ({ allow: false, reason: 'not this time', - })); + }), question: NO_QUESTIONS }); const { decision } = await runHook(socketPath); @@ -118,7 +132,7 @@ describe('chat permission broker', function () { // There is no terminal to prompt on, so an "ask" decision would block the // turn against a prompt nobody can see. broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); const { decision } = await runHook(socketPath); assert.ok(['allow', 'deny'].includes(decision.permissionDecision)); }); @@ -136,13 +150,14 @@ describe('chat permission broker', function () { it('denies when the session goes away mid-question', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen( - () => + const socketPath = await broker.listen({ + permission: () => new Promise(() => { // Never resolves: stands in for a user who never answers while the // session is torn down underneath them. }), - ); + question: NO_QUESTIONS, + }); const pending = runHook(socketPath); await new Promise((r) => setTimeout(r, 400)); @@ -155,8 +170,11 @@ describe('chat permission broker', function () { it('denies when the decider throws', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => { - throw new Error('decider exploded'); + const socketPath = await broker.listen({ + permission: async () => { + throw new Error('decider exploded'); + }, + question: NO_QUESTIONS, }); const { decision } = await runHook(socketPath); @@ -166,7 +184,7 @@ describe('chat permission broker', function () { it('denies when the payload is not valid JSON', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); const result = await new Promise((resolve) => { const child = spawn(process.execPath, [HOOK], { @@ -188,7 +206,7 @@ describe('chat permission broker', function () { describe('socket hygiene', function () { it('creates the socket private to this user and removes it on close', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); assert.strictEqual(fs.statSync(socketPath).mode & 0o777, 0o600); assert.strictEqual(fs.statSync(path.join(root, 'sockets')).mode & 0o777, 0o700); @@ -207,7 +225,7 @@ describe('chat permission broker', function () { const deep = path.join(root, 'x'.repeat(80), 'chat-sockets'); broker = new PermissionBroker(deep); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); assert.ok( Buffer.byteLength(socketPath, 'utf8') <= 103, @@ -230,22 +248,25 @@ describe('chat permission broker', function () { it('keeps the preferred directory when the path does fit', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); assert.strictEqual(path.dirname(socketPath), path.join(root, 'sockets')); }); it('does not put the session id in the socket name', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); - const socketPath = await broker.listen(async () => ({ allow: true })); + const socketPath = await broker.listen({ permission: async () => ({ allow: true }), question: NO_QUESTIONS }); assert.ok(!socketPath.includes(PAYLOAD.session_id)); }); it('handles several questions on one session socket', async function () { broker = new PermissionBroker(path.join(root, 'sockets')); let n = 0; - const socketPath = await broker.listen(async () => { - n += 1; - return { allow: n % 2 === 1, reason: `call ${n}` }; + const socketPath = await broker.listen({ + permission: async () => { + n += 1; + return { allow: n % 2 === 1, reason: `call ${n}` }; + }, + question: NO_QUESTIONS, }); const decisions = await Promise.all([ diff --git a/test/chat-questions.test.js b/test/chat-questions.test.js new file mode 100644 index 0000000..7d1f5f8 --- /dev/null +++ b/test/chat-questions.test.js @@ -0,0 +1,684 @@ +const assert = require('assert'); +const fs = require('fs'); +const os = require('os'); +const path = require('path'); +const { spawn } = require('child_process'); +const { PassThrough } = require('stream'); + +const { ChatSession } = require('../dist/server/chat/session.js'); +const { PermissionBroker } = require('../dist/server/chat/permission-broker.js'); +const { serveAsk, describeAnswer, ASK_TOOL_DEFINITION, askMcpConfig } = require('../dist/server/chat/ask-mcp.js'); +const { + applyChatEvent, + createTranscript, +} = require('../dist/shared/chat-reducer.js'); +const { + isAskQuestionTool, + normalizeQuestionOptions, + ASK_QUESTION_TOOL_NAME, +} = require('../dist/shared/chat-events.js'); + +// Choice-based questions from the model, end to end (issue #42). +// +// The mechanism is not obvious and was established by probe rather than by +// reading docs: Claude's own AskUserQuestion tool does not exist in the headless +// stream-json channel this app drives (`.work/probes/ask/`), so the capability +// is supplied as an MCP tool instead (`.work/probes/askmcp/`). What makes it +// work is that `tools/call` blocks — the model genuinely waits on a person — +// which is the property most of these tests are about. + +const ROOT = path.join(__dirname, '..'); +const ASK_SERVER = path.join(ROOT, 'dist', 'server', 'chat', 'ask-mcp.js'); + +const QUESTION = { + question: 'Which approach should I take?', + header: 'Approach', + multiSelect: false, + options: [ + { label: 'Rewrite it', description: 'Slower but cleaner' }, + { label: 'Patch it', description: 'Faster, more debt' }, + ], +}; + +function memoryStore() { + const events = []; + return { + events, + append(_ref, batch) { + events.push(...batch); + }, + async stat() { + return { firstSeq: 1, cursor: events.length }; + }, + async read() { + return { events: [], firstSeq: 1, from: 1, cursor: events.length }; + }, + async snapshot() { + return { + sessionId: 's1', runtime: 'claude', messages: [], state: 'idle', + capabilities: {}, pendingPermissions: [], firstSeq: 1, replayFrom: 1, + cursor: events.length, live: true, bypassPermissions: false, + }; + }, + }; +} + +function session({ bypass = false } = {}) { + const store = memoryStore(); + const s = new ChatSession( + { id: 's1', ownerUserId: 7 }, + { + store, + socketDir: fs.mkdtempSync(path.join(os.tmpdir(), 'ask-')), + // Absent, so no real broker is stood up: askQuestion is driven directly, + // which is the seam the broker would call anyway. + hookScript: path.join(ROOT, 'does-not-exist.js'), + broadcast: () => {}, + resolveCommand: () => 'claude', + }, + ); + s.bypass = bypass; + return { s, store }; +} + +/** Resolve, or fail loudly rather than letting mocha time out with no clue. */ +function within(promise, what, ms = 1000) { + return Promise.race([ + promise, + new Promise((_, reject) => setTimeout(() => reject(new Error(what)), ms)), + ]); +} + +describe('asking the user a choice-based question', function () { + describe('the MCP tool the model actually calls', function () { + /** Drive the real protocol over pipes, the way the runtime does. */ + function drive(answers) { + const input = new PassThrough(); + const output = new PassThrough(); + const lines = []; + let resolveLine = null; + output.setEncoding('utf8'); + let buffer = ''; + output.on('data', (chunk) => { + buffer += chunk; + let at; + while ((at = buffer.indexOf('\n')) !== -1) { + lines.push(JSON.parse(buffer.slice(0, at))); + buffer = buffer.slice(at + 1); + if (resolveLine) { + const go = resolveLine; + resolveLine = null; + go(); + } + } + }); + + const asked = []; + serveAsk(input, output, async (question) => { + asked.push(question); + return answers(question); + }); + + return { + asked, + lines, + send(message) { + input.write(`${JSON.stringify(message)}\n`); + }, + next() { + if (lines.length) return Promise.resolve(lines[lines.length - 1]); + return within( + new Promise((resolve) => { + resolveLine = () => resolve(lines[lines.length - 1]); + }), + 'the MCP server never replied', + ); + }, + }; + } + + it('advertises exactly one tool, with a schema the model can fill in', async function () { + const mcp = drive(() => ({ labels: [] })); + mcp.send({ jsonrpc: '2.0', id: 1, method: 'tools/list' }); + + const reply = await mcp.next(); + assert.strictEqual(reply.result.tools.length, 1); + const [tool] = reply.result.tools; + assert.ok(isAskQuestionTool(tool.name)); + // multiSelect is the whole second half of the issue; a schema without it + // means the model can only ever ask single-choice questions. + assert.ok(tool.inputSchema.properties.multiSelect); + assert.deepStrictEqual(tool.inputSchema.required, ['question', 'options']); + }); + + it('echoes the protocol version the client offered rather than pinning one', async function () { + const mcp = drive(() => ({ labels: [] })); + mcp.send({ jsonrpc: '2.0', id: 1, method: 'initialize', params: { protocolVersion: '2099-01-01' } }); + + const reply = await mcp.next(); + assert.strictEqual(reply.result.protocolVersion, '2099-01-01'); + }); + + it('does not answer the call until the user has', async function () { + let release; + const mcp = drive(() => new Promise((resolve) => { release = resolve; })); + mcp.send({ jsonrpc: '2.0', id: 7, method: 'tools/call', params: { name: 'ask_user_question', arguments: QUESTION } }); + + // The property the whole feature rests on: a blocked tool call is what + // makes the agent wait instead of guessing an answer and moving on. + await new Promise((resolve) => setTimeout(resolve, 50)); + assert.deepStrictEqual(mcp.lines, [], 'the call answered before anyone did'); + + release({ labels: ['Patch it'] }); + const reply = await mcp.next(); + assert.strictEqual(reply.id, 7); + assert.match(reply.result.content[0].text, /Patch it/); + assert.notStrictEqual(reply.result.isError, true); + }); + + it('passes the model’s own question through untouched', async function () { + const mcp = drive(() => ({ labels: ['Rewrite it'] })); + mcp.send({ jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'ask_user_question', arguments: QUESTION } }); + + await mcp.next(); + assert.deepStrictEqual(mcp.asked[0], QUESTION); + }); + + it('refuses a tool it does not serve instead of answering for it', async function () { + const mcp = drive(() => ({ labels: [] })); + mcp.send({ jsonrpc: '2.0', id: 3, method: 'tools/call', params: { name: 'rm_rf', arguments: {} } }); + + const reply = await mcp.next(); + assert.ok(reply.error, 'an unknown tool should not come back as a result'); + }); + }); + + describe('what the model is told', function () { + it('names the options the user picked', function () { + const { text, isError } = describeAnswer({ labels: ['Patch it', 'Rewrite it'] }); + assert.match(text, /"Patch it", "Rewrite it"/); + assert.strictEqual(isError, false); + }); + + it('says a skip was a skip, and tells the model not to ask again', function () { + const { text, isError } = describeAnswer({ labels: [], skipped: true }); + assert.match(text, /skipped/i); + assert.match(text, /not ask it again/i); + // Not an error: the user answering "none of these" is a normal outcome, + // and flagging it as a failure invites the model to retry the question. + assert.strictEqual(isError, false); + }); + + it('tells the model to fall back to prose when nobody could be reached', function () { + // The opposite failure direction from an approval, and deliberately so: + // nothing is gated on a question, so failing closed would block a turn + // for no safety gain. + const { text, isError } = describeAnswer({ labels: [], error: 'the browser is gone' }); + assert.match(text, /plain text/i); + assert.strictEqual(isError, true); + }); + }); + + describe('the session', function () { + it('puts the question in the transcript and blocks until it is answered', async function () { + const { s, store } = session(); + const waiting = s.askQuestion(QUESTION); + + const asked = store.events.find((e) => e.t === 'question'); + assert.ok(asked, 'the question should reach the transcript'); + assert.strictEqual(asked.request.question, QUESTION.question); + assert.strictEqual(asked.request.options.length, 2); + assert.strictEqual(s.currentState ?? s.state, 'awaiting_answer'); + + s.answerQuestion(asked.request.requestId, [asked.request.options[1].optionId]); + + const reply = await within(waiting, 'the question was never answered'); + assert.deepStrictEqual(reply.labels, ['Patch it']); + assert.ok(store.events.some((e) => e.t === 'question_resolved' && e.skipped !== true)); + }); + + it('carries every pick back for a multi-select question', async function () { + const { s, store } = session(); + const waiting = s.askQuestion({ + question: 'Which of these should I apply?', + multiSelect: true, + options: ['semicolons', 'trailing commas', 'single quotes'], + }); + + const asked = store.events.find((e) => e.t === 'question').request; + assert.strictEqual(asked.multiSelect, true); + s.answerQuestion(asked.requestId, [asked.options[0].optionId, asked.options[2].optionId]); + + const reply = await within(waiting, 'the multi-select answer never came back'); + assert.deepStrictEqual(reply.labels, ['semicolons', 'single quotes']); + }); + + it('ignores option ids it never offered', async function () { + const { s, store } = session(); + const waiting = s.askQuestion(QUESTION); + const asked = store.events.find((e) => e.t === 'question').request; + + // The ids come off a socket. The labels they resolve to are handed to the + // model as a statement of fact about what the user chose, so an id that + // was never offered must not be able to invent one. + s.answerQuestion(asked.requestId, ['opt-99', asked.options[0].optionId]); + + const reply = await within(waiting, 'the filtered answer never came back'); + assert.deepStrictEqual(reply.labels, ['Rewrite it']); + }); + + it('treats an answer of nothing at all as a skip', async function () { + const { s, store } = session(); + const waiting = s.askQuestion(QUESTION); + const asked = store.events.find((e) => e.t === 'question').request; + + s.answerQuestion(asked.requestId, [], true); + + const reply = await within(waiting, 'the skip never came back'); + assert.strictEqual(reply.skipped, true); + assert.deepStrictEqual(reply.labels, []); + assert.ok(store.events.some((e) => e.t === 'question_resolved' && e.skipped === true)); + }); + + it('refuses a question with nothing to pick from, rather than showing an empty card', async function () { + const { s, store } = session(); + const reply = await within( + s.askQuestion({ question: 'Well?', options: [] }), + 'a malformed question should be answered, not hang', + ); + assert.ok(reply.error); + assert.ok(!store.events.some((e) => e.t === 'question'), 'nothing should be put on screen'); + }); + + it('asks even when tool approvals are bypassed', async function () { + // Bypassing means "stop asking me before you act". It has never meant + // "answer my questions for me", and a session that auto-answered would + // silently pick for the user. + const { s, store } = session({ bypass: true }); + const waiting = s.askQuestion(QUESTION); + + const asked = store.events.find((e) => e.t === 'question'); + assert.ok(asked, 'a bypassed session must still put the question to the user'); + s.answerQuestion(asked.request.requestId, [asked.request.options[0].optionId]); + await within(waiting, 'the bypassed session never delivered the answer'); + }); + + it('never asks permission to ask a question', async function () { + const { s } = session(); + const decision = await within( + s.askUser({ toolName: ASK_QUESTION_TOOL_NAME, toolInput: QUESTION }), + 'the question tool was gated behind an approval', + ); + assert.strictEqual(decision.allow, true); + }); + + it('pairs a question with the tool call that asked it', async function () { + const { s, store } = session(); + s.ingest({ + t: 'block_start', + msgId: 'm1', + index: 0, + block: { kind: 'tool', toolId: 'toolu_42', name: ASK_QUESTION_TOOL_NAME, toolKind: 'other', status: 'pending' }, + }); + + s.askQuestion(QUESTION); + const asked = store.events.find((e) => e.t === 'question').request; + // Without this the card has nowhere to live in the conversation and falls + // back to the pinned region. + assert.strictEqual(asked.toolId, 'toolu_42'); + + s.answerQuestion(asked.requestId, [asked.options[0].optionId]); + const resolved = store.events.find((e) => e.t === 'question_resolved'); + // Repeated on the resolution so a card rebuilt from the log alone can + // still find its own answer. + assert.strictEqual(resolved.toolId, 'toolu_42'); + }); + + it('does not pair a second question with a call already claimed', async function () { + const { s, store } = session(); + s.ingest({ + t: 'block_start', + msgId: 'm1', + index: 0, + block: { kind: 'tool', toolId: 'toolu_42', name: ASK_QUESTION_TOOL_NAME, toolKind: 'other', status: 'pending' }, + }); + s.askQuestion(QUESTION); + s.askQuestion({ ...QUESTION, question: 'And then?' }); + + const asked = store.events.filter((e) => e.t === 'question').map((e) => e.request); + assert.strictEqual(asked[0].toolId, 'toolu_42'); + // Two cards in one place, one of them unanswerable, is worse than one + // card in the pinned fallback. + assert.strictEqual(asked[1].toolId, undefined); + }); + + it('keeps two calls announced together in the order they were announced', async function () { + // A runtime may announce both tool calls in one message before running + // either. Held in a single slot, the second announcement overwrote the + // first, the first question claimed the second call's id, and both cards + // were drawn against the wrong question. + const { s, store } = session(); + for (const toolId of ['toolu_a', 'toolu_b']) { + s.ingest({ + t: 'block_start', + msgId: 'm1', + index: 0, + block: { kind: 'tool', toolId, name: ASK_QUESTION_TOOL_NAME, toolKind: 'other', status: 'pending' }, + }); + } + + s.askQuestion({ ...QUESTION, question: 'first?' }); + s.askQuestion({ ...QUESTION, question: 'second?' }); + + const asked = store.events.filter((e) => e.t === 'question').map((e) => e.request); + assert.strictEqual(asked[0].toolId, 'toolu_a'); + assert.strictEqual(asked[1].toolId, 'toolu_b'); + }); + + it('does not pair a question with a call left over from an earlier turn', async function () { + // An announced call that never reached the MCP server would otherwise sit + // in the queue forever and mispair the next turn's question. + const { s, store } = session(); + s.ingest({ + t: 'block_start', + msgId: 'm1', + index: 0, + block: { kind: 'tool', toolId: 'toolu_stale', name: ASK_QUESTION_TOOL_NAME, toolKind: 'other', status: 'pending' }, + }); + s.ingest({ t: 'turn_end', turnId: 't1' }); + + s.askQuestion(QUESTION); + assert.strictEqual(store.events.find((e) => e.t === 'question').request.toolId, undefined); + }); + + it('keeps the question capability when the runtime introduces itself', function () { + // Claude's `init` lands after start() returns and replaces the capability + // record wholesale. Patched only on the session's own copy, the flag was + // true on the server and false in every browser reading the same log. + const { s, store } = session(); + s.questionsEnabled = true; + s.ingest({ + t: 'session', + capabilities: { streaming: true, permissions: false }, + }); + + const announced = store.events.find((e) => e.t === 'session'); + assert.strictEqual(announced.capabilities.questions, true); + assert.strictEqual(s.capabilities.questions, true); + }); + + it('leaves the capability off for a session that never wired the tool up', function () { + const { s, store } = session(); + s.ingest({ t: 'session', capabilities: { streaming: true, permissions: false } }); + assert.notStrictEqual(store.events.find((e) => e.t === 'session').capabilities.questions, true); + }); + + it('does not mistake an ordinary tool call for a question', function () { + const { s, store } = session(); + s.ingest({ + t: 'block_start', + msgId: 'm1', + index: 0, + block: { kind: 'tool', toolId: 'toolu_read', name: 'Read', toolKind: 'read', status: 'pending' }, + }); + s.askQuestion(QUESTION); + assert.strictEqual(store.events.find((e) => e.t === 'question').request.toolId, undefined); + }); + + it('releases a question the interrupted turn was waiting on', async function () { + const { s } = session(); + s.adapter = { + alive: true, + async interrupt() {}, + async stop() {}, + respondPermission() {}, + }; + const waiting = s.askQuestion(QUESTION); + + await s.interrupt(); + + const reply = await within(waiting, 'interrupt left the model blocked on a question'); + // An error rather than a skip: the user did not decline to answer, the + // turn was cancelled underneath the question, and the model should not be + // told someone considered it and passed. + assert.match(reply.error, /interrupted/); + }); + + it('releases a question when the session stops', async function () { + const { s } = session(); + const waiting = s.askQuestion(QUESTION); + await s.stop(); + const reply = await within(waiting, 'stopping left the model blocked on a question'); + assert.ok(reply.error); + }); + + it('offers a pending question to a browser that rejoins', async function () { + const { s } = session(); + s.askQuestion(QUESTION); + const snapshot = await s.snapshot(); + // The acceptance criterion about closing the tab and coming back: the + // pending question has to ride the snapshot or the card never returns. + assert.strictEqual(snapshot.pendingQuestions.length, 1); + assert.strictEqual(snapshot.pendingQuestions[0].question, QUESTION.question); + }); + }); + + describe('the transcript reducer', function () { + const seq = (() => { + let n = 0; + return () => (n += 1); + })(); + + function apply(state, event) { + applyChatEvent(state, { seq: seq(), ts: Date.now(), ...event }); + return state; + } + + it('holds a question until it is answered, and says the session is waiting', function () { + const state = createTranscript({}); + apply(state, { + t: 'question', + request: { requestId: 'q1', toolId: 't1', question: 'Which?', multiSelect: false, options: [{ optionId: 'opt-0', label: 'A' }], ts: 1 }, + }); + + assert.strictEqual(state.pendingQuestions.length, 1); + assert.strictEqual(state.state, 'awaiting_answer'); + + apply(state, { t: 'question_resolved', requestId: 'q1', toolId: 't1', optionIds: ['opt-0'] }); + assert.strictEqual(state.pendingQuestions.length, 0); + // Keyed by the call that asked, because that is all the card drawn from a + // tool block knows once the request itself is gone. + assert.deepStrictEqual(state.answeredQuestions.t1, ['opt-0']); + }); + + it('records a skip as answered-with-nothing, not as unanswered', function () { + const state = createTranscript({}); + apply(state, { + t: 'question', + request: { requestId: 'q2', toolId: 't2', question: 'Which?', multiSelect: false, options: [], ts: 1 }, + }); + apply(state, { t: 'question_resolved', requestId: 'q2', toolId: 't2', optionIds: [], skipped: true }); + + // An empty array and `undefined` mean different things to the card: one + // draws "skipped", the other draws buttons. + assert.deepStrictEqual(state.answeredQuestions.t2, []); + }); + + it('drops the cards when the conversation is cleared', function () { + // The card is drawn against a tool block. `/clear` empties the transcript, + // so a card left behind would hang off a message that is no longer there — + // and answering it would reach a turn that no longer exists. + const state = createTranscript({}); + apply(state, { + t: 'question', + request: { requestId: 'q4', toolId: 't4', question: 'Which?', multiSelect: false, options: [], ts: 1 }, + }); + apply(state, { t: 'question_resolved', requestId: 'q4', toolId: 't4', optionIds: [] }); + apply(state, { + t: 'question', + request: { requestId: 'q5', toolId: 't5', question: 'And now?', multiSelect: false, options: [], ts: 1 }, + }); + + apply(state, { t: 'marker', kind: 'cleared' }); + assert.deepStrictEqual(state.pendingQuestions, []); + assert.deepStrictEqual(state.answeredQuestions, {}); + }); + + it('does not add the same question twice when a snapshot overlaps the stream', function () { + const state = createTranscript({}); + const request = { requestId: 'q3', question: 'Which?', multiSelect: false, options: [], ts: 1 }; + apply(state, { t: 'question', request }); + apply(state, { t: 'question', request }); + assert.strictEqual(state.pendingQuestions.length, 1); + }); + }); + + describe('option ids', function () { + it('are minted the same way on both sides of the wire', function () { + // The server mints these when the question goes out; the browser mints + // them again when it rebuilds a card from the tool call in a replayed + // transcript. If the two ever disagreed by one dropped entry, the tick + // would land on an option the user did not choose. + const raw = ['A', { label: 'B', description: 'bee' }, { nonsense: true }, { label: ' ' }, 'C']; + assert.deepStrictEqual(normalizeQuestionOptions(raw), [ + { optionId: 'opt-0', label: 'A', description: undefined }, + { optionId: 'opt-1', label: 'B', description: 'bee' }, + { optionId: 'opt-2', label: 'C', description: undefined }, + ]); + }); + + it('keep two identically-worded options apart', function () { + const options = normalizeQuestionOptions(['Yes', 'Yes']); + assert.notStrictEqual(options[0].optionId, options[1].optionId); + }); + + it('survive a payload that is not a list at all', function () { + assert.deepStrictEqual(normalizeQuestionOptions('nope'), []); + assert.deepStrictEqual(normalizeQuestionOptions(undefined), []); + }); + }); + + describe('the runtime handshake', function () { + it('hands the runtime an inline server rather than editing the user’s MCP config', function () { + const config = JSON.parse(askMcpConfig('/opt/app/ask-mcp.js', '/tmp/s.sock')); + const server = config.mcpServers.ccweb; + assert.strictEqual(server.command, process.execPath); + assert.deepStrictEqual(server.args, ['/opt/app/ask-mcp.js']); + assert.strictEqual(server.env.CCWEB_ASK_SOCKET, '/tmp/s.sock'); + }); + + it('matches the namespaced name a runtime reports the call under', function () { + // What the transcript actually contains, and therefore what the UI has to + // match on to draw a card instead of a generic tool row. + assert.ok(isAskQuestionTool('mcp__ccweb__ask_user_question')); + assert.ok(isAskQuestionTool(ASK_TOOL_DEFINITION.name)); + assert.ok(!isAskQuestionTool('Bash')); + assert.ok(!isAskQuestionTool(undefined)); + }); + }); + + describe('over the real socket', function () { + let broker = null; + let root = ''; + + beforeEach(function () { + root = fs.mkdtempSync(path.join(os.tmpdir(), 'askbroker-')); + }); + + afterEach(function () { + if (broker) broker.close(); + broker = null; + fs.rmSync(root, { recursive: true, force: true }); + }); + + /** Speak MCP to the real spawned server, exactly as a runtime would. */ + function runServer(socketPath, calls) { + return new Promise((resolve) => { + const child = spawn(process.execPath, [ASK_SERVER], { + env: { ...process.env, CCWEB_ASK_SOCKET: socketPath }, + stdio: ['pipe', 'pipe', 'pipe'], + }); + const replies = []; + let buffer = ''; + child.stdout.setEncoding('utf8'); + child.stdout.on('data', (chunk) => { + buffer += chunk; + let at; + while ((at = buffer.indexOf('\n')) !== -1) { + replies.push(JSON.parse(buffer.slice(0, at))); + buffer = buffer.slice(at + 1); + if (replies.length === calls.length) { + child.kill(); + resolve(replies); + } + } + }); + for (const call of calls) child.stdin.write(`${JSON.stringify(call)}\n`); + }); + } + + it('carries a question out and an answer back through the spawned server', async function () { + broker = new PermissionBroker(path.join(root, 'sockets')); + const seen = []; + const socketPath = await broker.listen({ + permission: async () => ({ allow: false }), + question: async (ask) => { + seen.push(ask); + return { labels: ['Patch it'] }; + }, + }); + + const [reply] = await within( + runServer(socketPath, [ + { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'ask_user_question', arguments: QUESTION } }, + ]), + 'the spawned MCP server never answered', + 5000, + ); + + assert.strictEqual(seen.length, 1); + assert.strictEqual(seen[0].question, QUESTION.question); + assert.match(reply.result.content[0].text, /Patch it/); + }); + + it('keeps two questions on one socket apart', async function () { + broker = new PermissionBroker(path.join(root, 'sockets')); + const socketPath = await broker.listen({ + permission: async () => ({ allow: false }), + // Answered out of order on purpose: the socket is per session, not per + // call, so a reply that could not be matched to its own question would + // hand the model somebody else's answer. + question: async (ask) => + new Promise((resolve) => + setTimeout(() => resolve({ labels: [`answer to ${ask.question}`] }), ask.question === 'first?' ? 120 : 10), + ), + }); + + const replies = await within( + runServer(socketPath, [ + { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'ask_user_question', arguments: { question: 'first?', options: ['a'] } } }, + { jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'ask_user_question', arguments: { question: 'second?', options: ['a'] } } }, + ]), + 'the spawned MCP server never answered both', + 5000, + ); + + const byId = new Map(replies.map((r) => [r.id, r.result.content[0].text])); + assert.match(byId.get(1), /answer to first\?/); + assert.match(byId.get(2), /answer to second\?/); + }); + + it('tells the model to ask in prose when the session socket is gone', async function () { + const [reply] = await within( + runServer(path.join(root, 'nothing-here.sock'), [ + { jsonrpc: '2.0', id: 1, method: 'tools/call', params: { name: 'ask_user_question', arguments: QUESTION } }, + ]), + 'a dead socket left the model blocked forever', + 5000, + ); + // The one thing that must never happen here is silence. + assert.strictEqual(reply.result.isError, true); + assert.match(reply.result.content[0].text, /plain text/i); + }); + }); +}); From 3ce7743e1759386402e86ce8a2dbc2890e741082 Mon Sep 17 00:00:00 2001 From: dnviti Date: Mon, 27 Jul 2026 01:20:45 +0200 Subject: [PATCH 2/3] feat(chat): ask ACP agents the same question (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wires the question channel into the ACP adapter, where servers are offered in the handshake (`session/new`) rather than on the command line. Same script, same socket, same tool; which of the two routes a runtime takes is now a property of its registry row, so a runtime without a verified route reports `questions: false` instead of being handed a flag nobody has watched it parse. Probed against omp before writing any of it, and the probe paid for itself three times over — none of this is visible from the Claude path alone: - omp names the very same tool `mcp__ccweb_ask_user_question`, with a *single* underscore. The matcher required two, so every ACP question would have failed to render a card. - ACP has no tool-name field at all. The adapter uses the agent's own title for the block ("Asking tabs vs spaces preference"), and the real name turns up inside the arguments instead. Recognition now consults both, and the question payload is read from either the arguments themselves or omp's envelope, which carries them as a JSON string beside a path. - omp's model got the option schema wrong on its first attempt, had the call rejected before it ever reached this server, and retried. That leaves two announced calls for one question, and claiming by announcement order pinned the card to the attempt that failed. Correlation is now by question text, falling back to order only when a runtime reports no arguments to match on. The schema now accepts a bare string as an option as well as an object, since that rejected first attempt was a round trip spent on a shape this server always understood. kimi is wired by the same row but is unverified: it returns `end_turn` with no output for any prompt here, with or without this change. Verified end to end through the real ChatSession: omp asks both a single- and a multi-select question, both correlate to real tool ids, and the reply names exactly what was picked. Claude re-verified unchanged. 1423 unit tests, 315 browser checks, 0 failures. --- CHANGELOG.md | 5 +- src/client/shell/chat/MessageBubble.tsx | 13 +-- src/client/shell/chat/QuestionCard.tsx | 40 +-------- src/server/chat/adapter.ts | 9 ++ src/server/chat/adapters/acp.ts | 34 +++++++- src/server/chat/ask-mcp.ts | 32 ++++++-- src/server/chat/registry.ts | 26 ++++++ src/server/chat/session.ts | 104 +++++++++++++++++++----- src/shared/chat-events.ts | 93 ++++++++++++++++++++- test/chat-questions.test.js | 69 ++++++++++++++++ 10 files changed, 346 insertions(+), 79 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ff84c40..53ef91d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,8 +24,9 @@ Questions are asked even in sessions running with approvals bypassed: not being asked before it acts has never meant having your questions answered for you. - Available with Claude today. The plumbing underneath is runtime-neutral, so the - other runtimes need only their own handshake wired up. + Works with Claude and with the ACP agents (kimi, omp), each through its own + handshake. Codex, pi and grok report the capability as unavailable rather than + offering a button that would do nothing. - **You can open what the agent handed off and watch it work.** A delegation used to be one line in the agents list: a name, a status badge, a duration. Whether it was a sub-agent reading three files or a workflow running a dozen diff --git a/src/client/shell/chat/MessageBubble.tsx b/src/client/shell/chat/MessageBubble.tsx index 40dc7a8..e2e8d06 100644 --- a/src/client/shell/chat/MessageBubble.tsx +++ b/src/client/shell/chat/MessageBubble.tsx @@ -7,7 +7,8 @@ import { ImageBlock, NoticeBlock, ToolBlock, - isAskQuestionTool, + askedQuestionFrom, + looksLikeAskCall, } from '../../../shared/chat-events.js'; import { ChatTranscript } from '../../chat/transcript.js'; import { compactCount, formatDuration } from '../../chat/tool-meta.js'; @@ -15,7 +16,7 @@ import { Icon } from '../../ui/relay/Icon.js'; import { PHONE_TEXT, TOUCH_GAP, TOUCH_TARGET, usePhone } from '../../ui/touch.js'; import { Markdown, markdownText } from './Markdown.js'; import { PlanPanel } from './PlanPanel.js'; -import { QuestionCard, questionFromToolInput } from './QuestionCard.js'; +import { QuestionCard } from './QuestionCard.js'; /** * One message in the transcript — prose, and nothing else. @@ -346,7 +347,7 @@ function visibleBlocks(message: ChatMessage): number { return message.blocks.filter( (block) => block.kind !== 'thinking' - && (block.kind !== 'tool' || isAskQuestionTool(block.name)), + && (block.kind !== 'tool' || looksLikeAskCall(block.name, block.input)), ).length; } @@ -403,7 +404,7 @@ function summariseWork( // The question card is rendered in the conversation, so counting it here // as well would put "1 command" on a turn whose only machinery is the // question already on screen. - if (block.kind === 'tool' && isAskQuestionTool(block.name)) { + if (block.kind === 'tool' && looksLikeAskCall(block.name, block.input)) { continue; } if (block.kind === 'tool' && showToolCalls) { @@ -535,7 +536,7 @@ function BlockView({ // The one tool call that belongs in the conversation rather than on the // rail: it *is* the agent addressing the user. Everything else about a // tool call is machinery. - if (isAskQuestionTool(block.name)) { + if (looksLikeAskCall(block.name, block.input)) { return ( void; }): React.JSX.Element | null { - const asked = questionFromToolInput(block.input); + const asked = askedQuestionFrom(block.input); // Still streaming its arguments in, or malformed. Nothing to draw yet — and // an empty bordered card would read as a question with no answers. if (!asked) return null; diff --git a/src/client/shell/chat/QuestionCard.tsx b/src/client/shell/chat/QuestionCard.tsx index f6898c1..acfdb5c 100644 --- a/src/client/shell/chat/QuestionCard.tsx +++ b/src/client/shell/chat/QuestionCard.tsx @@ -1,9 +1,5 @@ import * as React from 'react'; -import { - QuestionOption, - QuestionRequest, - normalizeQuestionOptions, -} from '../../../shared/chat-events.js'; +import { QuestionOption, QuestionRequest } from '../../../shared/chat-events.js'; import { Button } from '../../ui/relay/Button.js'; import { Icon } from '../../ui/relay/Icon.js'; @@ -323,37 +319,3 @@ const SR_ONLY: React.CSSProperties = { overflow: 'hidden', clip: 'rect(0 0 0 0)', }; - -/** - * Read a question back out of the tool call that asked it. - * - * The transcript's record of a question *is* the tool block — the arguments the - * model sent are the question, and they are already persisted and replayed. So - * a card scrolled back to needs no side table to render from, which is what - * makes the record survive a reload and a server restart. - * - * Tolerant by contract: `input` is `unknown` everywhere else in this codebase - * for good reason, and a malformed call should render as nothing rather than - * throw inside the transcript. - */ -export function questionFromToolInput(input: unknown): { - question: string; - header?: string; - multiSelect: boolean; - options: QuestionOption[]; -} | null { - if (!input || typeof input !== 'object') return null; - const object = input as Record; - const question = typeof object.question === 'string' ? object.question.trim() : ''; - if (!question) return null; - - const options = normalizeQuestionOptions(object.options); - if (options.length === 0) return null; - - return { - question, - header: typeof object.header === 'string' && object.header.trim() ? object.header.trim() : undefined, - multiSelect: object.multiSelect === true, - options, - }; -} diff --git a/src/server/chat/adapter.ts b/src/server/chat/adapter.ts index e5d2bc6..ef53f87 100644 --- a/src/server/chat/adapter.ts +++ b/src/server/chat/adapter.ts @@ -48,6 +48,15 @@ export interface ChatAdapterOptions { bypassPermissions?: boolean; /** Native session id to resume, when the runtime and the log both have one. */ resumeSessionId?: string; + /** + * The MCP server that lets the model ask the user a multiple-choice question. + * + * Handed to adapters whose runtime takes MCP servers through the protocol + * rather than the command line. Claude gets the same server as a `--mcp-config` + * argument instead, built by the session; either way the server is the same + * script and the same socket. + */ + askMcpServer?: { name: string; command: string; args: string[]; env: Record }; emit: (event: AdapterEvent) => void; /** * Read a file on the agent's behalf. diff --git a/src/server/chat/adapters/acp.ts b/src/server/chat/adapters/acp.ts index 7f31827..cb1b93b 100644 --- a/src/server/chat/adapters/acp.ts +++ b/src/server/chat/adapters/acp.ts @@ -329,6 +329,33 @@ export class AcpChatAdapter extends JsonRpcChatAdapter { } } + /** + * The MCP servers this client offers the agent. + * + * Only ever the question server, and only when the session wired one up. ACP + * spells a server's environment as a list of name/value pairs rather than an + * object, which is the one place its shape differs from every other config + * this app writes. + * + * Verified against omp: the entry is accepted on `session/new`, the tool + * appears to the model, and calling it reaches this app's socket. Note the + * agent renames it on the way through — omp reports the call as + * `mcp__ccweb_ask_user_question`, with a single underscore — which is why + * nothing downstream matches on an exact tool name. + */ + private mcpServers(): Array> { + const server = this.options.askMcpServer; + if (!server) return []; + return [ + { + name: server.name, + command: server.command, + args: server.args, + env: Object.entries(server.env).map(([name, value]) => ({ name, value })), + }, + ]; + } + private applyInitialize(init: Record): void { const agent = record(init.agentCapabilities); const prompt = record(agent.promptCapabilities); @@ -353,7 +380,7 @@ export class AcpChatAdapter extends JsonRpcChatAdapter { const loaded = await this.call('session/load', { sessionId: resumeId, cwd: this.options.workingDir, - mcpServers: [], + mcpServers: this.mcpServers(), }); this.nativeSessionId = resumeId; return loaded; @@ -365,7 +392,10 @@ export class AcpChatAdapter extends JsonRpcChatAdapter { try { const created = record( - await this.call('session/new', { cwd: this.options.workingDir, mcpServers: [] }), + await this.call('session/new', { + cwd: this.options.workingDir, + mcpServers: this.mcpServers(), + }), ); this.nativeSessionId = str(created.sessionId) || null; return created; diff --git a/src/server/chat/ask-mcp.ts b/src/server/chat/ask-mcp.ts index 17bf084..0ce5d03 100644 --- a/src/server/chat/ask-mcp.ts +++ b/src/server/chat/ask-mcp.ts @@ -76,17 +76,31 @@ export const ASK_TOOL_DEFINITION = { options: { type: 'array', minItems: 2, - description: 'The answers on offer. Keep this to a short, fixed list the user can scan.', + description: + 'The answers on offer. Keep this to a short, fixed list the user can scan. ' + + 'Each may be an object with a label and an optional description, or just the ' + + 'label as a plain string.', + // A bare string is accepted as well as an object, because omp's model + // sent `["Tabs", "Spaces"]`, had the call rejected by schema validation + // before it ever reached this server, and had to burn a second round + // trip retrying. The shape was always understood here; only the schema + // objected. items: { - type: 'object', - properties: { - label: { type: 'string', description: 'The option, in a few words.' }, - description: { - type: 'string', - description: 'One line on what picking this would mean. Optional but usually worth it.', + anyOf: [ + { type: 'string', description: 'The option, in a few words.' }, + { + type: 'object', + properties: { + label: { type: 'string', description: 'The option, in a few words.' }, + description: { + type: 'string', + description: + 'One line on what picking this would mean. Optional but usually worth it.', + }, + }, + required: ['label'], }, - }, - required: ['label'], + ], }, }, }, diff --git a/src/server/chat/registry.ts b/src/server/chat/registry.ts index 8f47c60..da56712 100644 --- a/src/server/chat/registry.ts +++ b/src/server/chat/registry.ts @@ -31,10 +31,23 @@ import { PiChatAdapter } from './adapters/pi.js'; export type ChatAdapterFactory = (options: ChatAdapterOptions) => ChatAdapter; +/** + * How a runtime is handed the MCP server that asks the user questions. + * + * `cli` — as a `--mcp-config` argument at spawn (claude). + * `protocol` — in the handshake, as part of `session/new` (ACP agents). + * + * Absent means the runtime has no verified way to accept one, and it simply + * reports `questions: false` rather than being handed a flag nobody has watched + * it parse. + */ +export type AskChannel = 'cli' | 'protocol'; + interface RuntimeChatEntry { factory: ChatAdapterFactory; /** What the launcher shows before a session exists. */ advertised: Partial; + askChannel?: AskChannel; } /** @@ -47,6 +60,7 @@ interface RuntimeChatEntry { function acp(runtime: string, acpArgs: string[]): RuntimeChatEntry { return { factory: (options) => new AcpChatAdapter({ ...options, runtime, acpArgs }), + askChannel: 'protocol', advertised: { streaming: true, thinking: true, @@ -62,6 +76,7 @@ function acp(runtime: string, acpArgs: string[]): RuntimeChatEntry { const RUNTIMES: Record = { claude: { factory: (options) => new ClaudeChatAdapter(options), + askChannel: 'cli', advertised: { streaming: true, thinking: true, @@ -105,6 +120,17 @@ const RUNTIMES: Record = { omp: acp('omp', ['acp']), }; +/** + * How this runtime takes the question server, or undefined if it does not. + * + * Only the runtimes it has actually been watched working on: claude and the ACP + * agents. Codex, pi and grok are one probe away, and get `questions: false` + * until someone runs it. + */ +export function askChannelFor(runtime: string): AskChannel | undefined { + return RUNTIMES[runtime]?.askChannel; +} + /** Whether this runtime can be driven as a chat at all. */ export function supportsChat(runtime: string): boolean { return runtime in RUNTIMES; diff --git a/src/server/chat/session.ts b/src/server/chat/session.ts index 25d9623..614fbc2 100644 --- a/src/server/chat/session.ts +++ b/src/server/chat/session.ts @@ -17,11 +17,14 @@ import { defaultPermissionOptions, isAllowOption, isAskQuestionTool, + looksLikeAskCall, + askedQuestionFrom, normalizeQuestionOptions, + ASK_MCP_SERVER, ASK_QUESTION_TOOL_NAME, } from '../../shared/chat-events.js'; import { isClearingCommand } from '../../shared/slash-commands.js'; -import { AdapterEvent, ChatAdapter } from './adapter.js'; +import { AdapterEvent, ChatAdapter, ChatAdapterOptions } from './adapter.js'; import { PermissionAsk, PermissionAnswer, @@ -30,9 +33,9 @@ import { QuestionReply, permissionHookSettings, } from './permission-broker.js'; -import { askMcpConfig } from './ask-mcp.js'; +import { ASK_SOCKET_ENV, askMcpConfig } from './ask-mcp.js'; import { ChatStoreLike, ChatSessionRef } from './store.js'; -import { createChatAdapter, supportsChat } from './registry.js'; +import { askChannelFor, createChatAdapter, supportsChat } from './registry.js'; /** * One chat conversation, owned by the server. @@ -173,23 +176,24 @@ export class ChatSession { private readonly pending = new Map(); private readonly questions = new Map(); /** - * Question tool calls the transcript has opened, oldest first, unclaimed. + * Question tool calls the transcript has opened and nothing has claimed yet. * * The MCP server that carries a question has no way to know the tool_use id of * the call it is serving — it only ever sees the arguments — so the pairing is - * made here instead, from the block the adapter has already reported. + * made here instead, from the block the adapter already reported. * - * A queue rather than a single slot, because a runtime may announce two tool - * calls in one message before running either: with one slot the second - * announcement would overwrite the first, the first question would claim the - * second call's id, and both cards would be drawn in the wrong place. Calls - * run in the order they were announced, so first in is first claimed. + * Matched on the question text first, and only then on announcement order. + * Order alone is not enough, which omp demonstrated: its model got the option + * schema wrong, the call was rejected before it ever reached the MCP server, + * and it retried. That leaves *two* announced calls for one question, and + * claiming the oldest pinned the card to the attempt that failed. Newest match + * first is the answer, because a retry is the later of the two. * - * An unclaimed id left over from a call that never reached the MCP server - * would mispair the next question, so the queue is emptied at the end of every - * turn — by which point nothing can still be waiting to claim one. + * An unclaimed entry left by a call that never reached the server would + * mispair a later question, so the list is emptied at the end of every turn — + * by which point nothing can still be waiting to claim one. */ - private readonly askToolIds: string[] = []; + private askCalls: Array<{ toolId: string; question?: string }> = []; /** True once this session actually handed a runtime the question tool. */ private questionsEnabled = false; private runtime = ''; @@ -299,7 +303,9 @@ export class ChatSession { // model that asks which of three approaches to take still needs a person. const wantsHook = !this.bypass && options.runtime === 'claude' && fs.existsSync(this.deps.hookScript); const askScript = this.deps.askScript; - const wantsAsk = options.runtime === 'claude' && Boolean(askScript) && fs.existsSync(askScript!); + const askChannel = askChannelFor(options.runtime); + const wantsAsk = Boolean(askChannel) && Boolean(askScript) && fs.existsSync(askScript!); + let askMcpServer: ChatAdapterOptions['askMcpServer']; if (wantsHook || wantsAsk) { // One shared directory, not one per session. A directory named after the @@ -316,7 +322,7 @@ export class ChatSession { extraArgs.push('--settings', permissionHookSettings(this.deps.hookScript, socketPath)); env.CCWEB_PERMISSION_SOCKET = socketPath; } - if (wantsAsk) { + if (wantsAsk && askChannel === 'cli') { extraArgs.push('--mcp-config', askMcpConfig(askScript!, socketPath)); // Named explicitly rather than relying on the hook to wave it through: // with approvals bypassed there is no hook at all, and without this the @@ -325,6 +331,18 @@ export class ChatSession { extraArgs.push('--allowedTools', ASK_QUESTION_TOOL_NAME); this.questionsEnabled = true; } + if (wantsAsk && askChannel === 'protocol') { + // ACP agents take their MCP servers in the handshake rather than on the + // command line, so this goes to the adapter and is sent with + // `session/new`. Same script, same socket, same tool. + askMcpServer = { + name: ASK_MCP_SERVER, + command: process.execPath, + args: [askScript!], + env: { [ASK_SOCKET_ENV]: socketPath }, + }; + this.questionsEnabled = true; + } } const adapter = createChatAdapter(options.runtime, { @@ -336,6 +354,7 @@ export class ChatSession { env, bypassPermissions: this.bypass, resumeSessionId: options.resumeSessionId, + askMcpServer, emit: (event) => this.ingest(event), readFile: this.deps.readFile ? (filePath) => this.deps.readFile!(this.ref.id, filePath) @@ -457,11 +476,17 @@ export class ChatSession { // time the MCP server relays the question itself, the arguments are all it // knows. `block_start` carries the name, `tool` patches carry only the id, // so this is the one event that can make the pairing. - if (stamped.t === 'block_start' && stamped.block.kind === 'tool' && isAskQuestionTool(stamped.block.name)) { - this.askToolIds.push(stamped.block.toolId); + if (stamped.t === 'block_start' && stamped.block.kind === 'tool') { + this.noteAskCall(stamped.block.toolId, stamped.block.name, stamped.block.input); + } + // Claude streams its arguments in as JSON fragments, so a question tool call + // is announced before anything says what it asks. The parsed input lands + // later as a patch, which is the first point the text is knowable. + if (stamped.t === 'tool' && stamped.patch.input !== undefined) { + this.noteAskCall(stamped.toolId, stamped.patch.name, stamped.patch.input); } if (stamped.t === 'turn_end') { - this.askToolIds.length = 0; + this.askCalls = []; } if (stamped.t === 'question') { const existing = this.questions.get(stamped.request.requestId); @@ -799,6 +824,45 @@ export class ChatSession { }); } + /** + * Remember a tool call that might be a question, or fill in what it asks. + * + * Called for every tool block, so the cheap name check comes first. A call + * already known is updated rather than duplicated: the same id is reported + * twice — once on announcement and again when its arguments finish arriving. + */ + private noteAskCall(toolId: string, name: string | undefined, input: unknown): void { + const existing = this.askCalls.find((call) => call.toolId === toolId); + if (!existing && !looksLikeAskCall(name, input)) return; + + const question = askedQuestionFrom(input)?.question; + if (existing) { + if (question) existing.question = question; + return; + } + this.askCalls.push({ toolId, question }); + } + + /** + * Which announced call a question belongs to, if any. + * + * Claimed as it is answered, so a second question cannot attach itself to the + * same block — two cards in one place, one of them unanswerable, is worse than + * one card in the pinned fallback. + */ + private claimAskCall(question: string): string | undefined { + // Newest text match wins; see the note on `askCalls` for why a retry makes + // that the right end to start from. + for (let at = this.askCalls.length - 1; at >= 0; at -= 1) { + if (this.askCalls[at].question === question) { + return this.askCalls.splice(at, 1)[0].toolId; + } + } + // Nothing matched on text — a runtime that reports no arguments, or reports + // them in a shape nothing here parses. Order is the fallback, oldest first. + return this.askCalls.shift()?.toolId; + } + /** * A question from the model, on its way to a person. * @@ -827,7 +891,7 @@ export class ChatSession { // Claimed, not merely read: a second question must not attach itself to // the same tool block, which would draw two cards in one place and // leave the later one unanswerable. - toolId: this.askToolIds.shift(), + toolId: this.claimAskCall(question), question, header: typeof ask.header === 'string' && ask.header.trim() ? ask.header.trim() : undefined, multiSelect: ask.multiSelect === true, diff --git a/src/shared/chat-events.ts b/src/shared/chat-events.ts index f6ffc7d..e10cab4 100644 --- a/src/shared/chat-events.ts +++ b/src/shared/chat-events.ts @@ -740,7 +740,98 @@ export function normalizeQuestionOptions(raw: unknown): QuestionOption[] { export function isAskQuestionTool(name: string | undefined): boolean { if (!name) return false; - return name === ASK_QUESTION_TOOL || name.endsWith(`__${ASK_QUESTION_TOOL}`); + // Suffix match on a separator of either width. Claude namespaces MCP tools as + // `mcp____`; omp reports the same tool as + // `mcp__ccweb_ask_user_question`, with one underscore. Both were observed — + // an exact-name table would have silently failed for one of them. + return name === ASK_QUESTION_TOOL || /(^|_)ask_user_question$/.test(name); +} + +/** + * Whether a tool block is this app's question tool, however the runtime named it. + * + * The name alone is not enough. ACP has no separate tool-name field at all: the + * adapter uses the agent's own title for the block ("Asking tabs vs spaces + * preference"), and the real tool name turns up inside the arguments instead + * (omp puts it in `rawInput.path`). So the arguments are consulted too. + */ +export function looksLikeAskCall(name: string | undefined, input: unknown): boolean { + if (isAskQuestionTool(name)) return true; + if (input === undefined || input === null) return false; + try { + return JSON.stringify(input).includes(ASK_QUESTION_TOOL); + } catch { + return false; + } +} + +/** A question as it can be read back out of the call that asked it. */ +export interface AskedQuestion { + question: string; + header?: string; + multiSelect: boolean; + options: QuestionOption[]; +} + +/** + * Read a question back out of a tool call's arguments. + * + * Shared between the session — which pairs an incoming question with the call + * that asked it — and the browser, which rebuilds the card from a replayed + * transcript. Two implementations that disagreed about which options survive + * would put the tick on an option the user did not choose. + * + * Two shapes are accepted because two were observed: the arguments themselves, + * and an envelope carrying them as a JSON string (omp reports the call as + * `{ path: 'xd://mcp__ccweb_ask_user_question', content: '{...}' }`). Tolerant + * by contract — `input` is `unknown` everywhere else in this file for good + * reason, and a malformed call should render as nothing rather than throw. + */ +export function askedQuestionFrom(input: unknown): AskedQuestion | null { + const object = asRecord(input); + if (!object) return null; + + const direct = readQuestion(object); + if (direct) return direct; + + // An envelope. `content` is the field omp uses; the others cost nothing to + // accept and save a second round of probing if another agent picks one. + for (const key of ['content', 'arguments', 'input', 'params']) { + const inner = object[key]; + if (typeof inner === 'string') { + try { + const parsed = readQuestion(asRecord(JSON.parse(inner))); + if (parsed) return parsed; + } catch { + // Not JSON, so not a question. Keep looking. + } + } else if (inner && typeof inner === 'object') { + const parsed = readQuestion(asRecord(inner)); + if (parsed) return parsed; + } + } + return null; +} + +function asRecord(value: unknown): Record | undefined { + return value && typeof value === 'object' && !Array.isArray(value) + ? (value as Record) + : undefined; +} + +function readQuestion(object: Record | undefined): AskedQuestion | null { + if (!object) return null; + const question = typeof object.question === 'string' ? object.question.trim() : ''; + if (!question) return null; + const options = normalizeQuestionOptions(object.options); + if (options.length === 0) return null; + return { + question, + header: + typeof object.header === 'string' && object.header.trim() ? object.header.trim() : undefined, + multiSelect: object.multiSelect === true, + options, + }; } /** diff --git a/test/chat-questions.test.js b/test/chat-questions.test.js index 7d1f5f8..4d1e051 100644 --- a/test/chat-questions.test.js +++ b/test/chat-questions.test.js @@ -14,9 +14,12 @@ const { } = require('../dist/shared/chat-reducer.js'); const { isAskQuestionTool, + looksLikeAskCall, + askedQuestionFrom, normalizeQuestionOptions, ASK_QUESTION_TOOL_NAME, } = require('../dist/shared/chat-events.js'); +const { askChannelFor } = require('../dist/server/chat/registry.js'); // Choice-based questions from the model, end to end (issue #42). // @@ -567,14 +570,80 @@ describe('asking the user a choice-based question', function () { assert.strictEqual(server.env.CCWEB_ASK_SOCKET, '/tmp/s.sock'); }); + it('knows which runtimes have a verified way to take the server', function () { + // Only the ones this has actually been watched working on. A runtime that + // gets a flag nobody has seen it parse is a capability claim with nothing + // behind it. + assert.strictEqual(askChannelFor('claude'), 'cli'); + assert.strictEqual(askChannelFor('kimi'), 'protocol'); + assert.strictEqual(askChannelFor('omp'), 'protocol'); + assert.strictEqual(askChannelFor('codex'), undefined); + assert.strictEqual(askChannelFor('nonesuch'), undefined); + }); + it('matches the namespaced name a runtime reports the call under', function () { // What the transcript actually contains, and therefore what the UI has to // match on to draw a card instead of a generic tool row. assert.ok(isAskQuestionTool('mcp__ccweb__ask_user_question')); + // One underscore, not two. This is how omp reports the very same tool, + // and an exact-name table would have silently failed for it while + // passing for Claude. + assert.ok(isAskQuestionTool('mcp__ccweb_ask_user_question')); assert.ok(isAskQuestionTool(ASK_TOOL_DEFINITION.name)); assert.ok(!isAskQuestionTool('Bash')); assert.ok(!isAskQuestionTool(undefined)); }); + + it('recognises a call an ACP agent renamed past recognition', function () { + // ACP has no tool-name field: the adapter uses the agent's own title for + // the block, so the name is prose. The real name turns up in the + // arguments instead, which is why those are consulted too. + const rawInput = { + path: 'xd://mcp__ccweb_ask_user_question', + content: JSON.stringify({ question: 'Tabs or spaces?', options: ['Tabs', 'Spaces'] }), + }; + assert.ok(!isAskQuestionTool('Asking tabs vs spaces preference')); + assert.ok(looksLikeAskCall('Asking tabs vs spaces preference', rawInput)); + assert.ok(!looksLikeAskCall('Reading a file', { path: '/etc/hosts' })); + }); + + it('reads the question back out of either shape a runtime reports', function () { + const direct = askedQuestionFrom({ + question: 'Tabs or spaces?', + header: 'Indent', + multiSelect: true, + options: [{ label: 'Tabs' }, { label: 'Spaces', description: 'wider' }], + }); + assert.strictEqual(direct.question, 'Tabs or spaces?'); + assert.strictEqual(direct.multiSelect, true); + assert.strictEqual(direct.options[1].description, 'wider'); + + // omp's envelope: the arguments arrive as a JSON string beside a path. + const wrapped = askedQuestionFrom({ + path: 'xd://mcp__ccweb_ask_user_question', + content: JSON.stringify({ question: 'Tabs or spaces?', options: ['Tabs', 'Spaces'] }), + }); + assert.strictEqual(wrapped.question, 'Tabs or spaces?'); + assert.deepStrictEqual(wrapped.options.map((o) => o.label), ['Tabs', 'Spaces']); + assert.strictEqual(wrapped.multiSelect, false); + }); + + it('reads nothing out of a call that is not a question', function () { + assert.strictEqual(askedQuestionFrom(undefined), null); + assert.strictEqual(askedQuestionFrom('nope'), null); + assert.strictEqual(askedQuestionFrom({ question: 'no options?' }), null); + assert.strictEqual(askedQuestionFrom({ content: 'not json' }), null); + }); + + it('accepts bare string options in the tool schema', function () { + // omp's model sent `["Tabs","Spaces"]`, the call was rejected by schema + // validation before it ever reached this server, and it burned a round + // trip retrying. The shape was always understood; only the schema objected. + const item = ASK_TOOL_DEFINITION.inputSchema.properties.options.items; + assert.ok(Array.isArray(item.anyOf), 'options items should accept more than one shape'); + assert.ok(item.anyOf.some((shape) => shape.type === 'string')); + assert.ok(item.anyOf.some((shape) => shape.type === 'object')); + }); }); describe('over the real socket', function () { From a1257ef8a3f3c2642811e63a8609f800016eeed6 Mon Sep 17 00:00:00 2001 From: dnviti Date: Mon, 27 Jul 2026 01:26:49 +0200 Subject: [PATCH 3/3] test(chat): keep a runtime's own ask tool out of this one's path (#42) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit kimi turned out to ship a native `AskUserQuestion`, and in a headless ACP session it answers itself with "the user dismissed this" without anybody being asked. Its MCP support is fine — verified: it spawns this app's server, exposes the tool as `mcp__ccweb__ask_user_question`, and round-trips an answer when the model picks it. It just often picks its own instead. That makes two rules load-bearing that were previously only incidentally correct, so they are now covered: a runtime's own ask-the-user tool must not be auto-approved by the rule that exists for this app's tool, and must not draw a question card — there is no pending question behind it, so the card would render as already-answered with nothing in it. Refusing the native call through the ACP permission channel was tried and does not redirect the model: the turn simply ends, because ACP carries no reason back. Recorded in the registry rather than worked around, since wiring kimi anyway is still strictly better than not. --- CHANGELOG.md | 9 ++++++--- src/server/chat/registry.ts | 10 ++++++++++ test/chat-questions.test.js | 12 ++++++++++++ 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53ef91d..5520cf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,12 @@ Questions are asked even in sessions running with approvals bypassed: not being asked before it acts has never meant having your questions answered for you. - Works with Claude and with the ACP agents (kimi, omp), each through its own - handshake. Codex, pi and grok report the capability as unavailable rather than - offering a button that would do nothing. + Works with Claude and with omp, each through its own handshake. kimi can reach + it too, but often prefers its own built-in question tool, which answers itself + without asking anyone — so questions there are hit and miss, and that is kimi's + behaviour rather than something this app can steer. Codex, pi and grok report + the capability as unavailable rather than offering a button that would do + nothing. - **You can open what the agent handed off and watch it work.** A delegation used to be one line in the agents list: a name, a status badge, a duration. Whether it was a sub-agent reading three files or a workflow running a dozen diff --git a/src/server/chat/registry.ts b/src/server/chat/registry.ts index da56712..5b23220 100644 --- a/src/server/chat/registry.ts +++ b/src/server/chat/registry.ts @@ -40,6 +40,16 @@ export type ChatAdapterFactory = (options: ChatAdapterOptions) => ChatAdapter; * Absent means the runtime has no verified way to accept one, and it simply * reports `questions: false` rather than being handed a flag nobody has watched * it parse. + * + * A wired channel is not a promise the model will use it. kimi accepts the + * server, spawns it and exposes the tool — verified — but ships a native + * `AskUserQuestion` of its own that it often reaches for first, and that one + * answers itself with "the user dismissed this" without anybody being asked. + * Nothing here can redirect it: refusing the native call through the permission + * channel just ends the turn, because ACP carries no reason back to the model. + * Wiring it anyway is still strictly better than not — when the model does pick + * this tool the question reaches a person, and when it does not, the outcome is + * the one kimi would have produced regardless. */ export type AskChannel = 'cli' | 'protocol'; diff --git a/test/chat-questions.test.js b/test/chat-questions.test.js index 4d1e051..95d1dc1 100644 --- a/test/chat-questions.test.js +++ b/test/chat-questions.test.js @@ -594,6 +594,18 @@ describe('asking the user a choice-based question', function () { assert.ok(!isAskQuestionTool(undefined)); }); + it('does not mistake a runtime’s own ask-the-user tool for this one', function () { + // kimi ships a native `AskUserQuestion`, and in a headless ACP session it + // answers itself with "user dismissed" without anyone being asked. It must + // not be matched here: this app can neither auto-approve it (that rule + // exists because *our* tool is unanswerable behind an approval) nor draw a + // card for it (there is no pending question to answer, so the card would + // render as already-answered with no answer in it). + assert.ok(!isAskQuestionTool('AskUserQuestion')); + assert.ok(!looksLikeAskCall('AskUserQuestion', { questions: [{ question: 'Tabs or spaces?' }] })); + assert.strictEqual(askedQuestionFrom({ questions: [{ question: 'Tabs or spaces?' }] }), null); + }); + it('recognises a call an ACP agent renamed past recognition', function () { // ACP has no tool-name field: the adapter uses the agent's own title for // the block, so the name is prose. The real name turns up in the