diff --git a/CHANGELOG.md b/CHANGELOG.md index d9dfc94..52205d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,33 @@ session. If that session is gone by the time you come back, the app falls back to the first tab rather than showing you nothing. +- **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. + + 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/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..e2e8d06 100644 --- a/src/client/shell/chat/MessageBubble.tsx +++ b/src/client/shell/chat/MessageBubble.tsx @@ -6,6 +6,9 @@ import { ErrorBlock, ImageBlock, NoticeBlock, + ToolBlock, + askedQuestionFrom, + looksLikeAskCall, } from '../../../shared/chat-events.js'; import { ChatTranscript } from '../../chat/transcript.js'; import { compactCount, formatDuration } from '../../chat/tool-meta.js'; @@ -13,6 +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 } from './QuestionCard.js'; /** * One message in the transcript — prose, and nothing else. @@ -69,6 +73,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 +92,7 @@ export const MessageBubble = React.memo(function MessageBubble({ carriedIds = '', showThinking = true, showToolCalls = true, + onAnswerQuestion, }: MessageBubbleProps) { const id = message.id; @@ -236,6 +248,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 +335,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' || looksLikeAskCall(block.name, block.input)), + ).length; } /** @@ -376,6 +401,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' && looksLikeAskCall(block.name, block.input)) { + continue; + } if (block.kind === 'tool' && showToolCalls) { tools += 1; if (block.durationMs !== undefined) durationMs += block.durationMs; @@ -451,12 +482,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 +530,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 (looksLikeAskCall(block.name, block.input)) { + return ( + + ); + } return null; case 'plan': @@ -515,6 +564,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 = 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; + + 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..acfdb5c --- /dev/null +++ b/src/client/shell/chat/QuestionCard.tsx @@ -0,0 +1,321 @@ +import * as React from 'react'; +import { QuestionOption, QuestionRequest } 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)', +}; 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 e2a8b17..f39bf5c 100644 --- a/src/client/terminal/message-handler.ts +++ b/src/client/terminal/message-handler.ts @@ -230,6 +230,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/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 new file mode 100644 index 0000000..0ce5d03 --- /dev/null +++ b/src/server/chat/ask-mcp.ts @@ -0,0 +1,337 @@ +/** + * 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. ' + + '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: { + 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: ['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/registry.ts b/src/server/chat/registry.ts index 8f47c60..5b23220 100644 --- a/src/server/chat/registry.ts +++ b/src/server/chat/registry.ts @@ -31,10 +31,33 @@ 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. + * + * 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'; + interface RuntimeChatEntry { factory: ChatAdapterFactory; /** What the launcher shows before a session exists. */ advertised: Partial; + askChannel?: AskChannel; } /** @@ -47,6 +70,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 +86,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 +130,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 1037451..614fbc2 100644 --- a/src/server/chat/session.ts +++ b/src/server/chat/session.ts @@ -10,16 +10,32 @@ import { PermissionOption, PermissionRequest, QueuedTurn, + QuestionOption, + QuestionRequest, UserTurn, classifyTool, 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 { PermissionAsk, PermissionAnswer, PermissionBroker, permissionHookSettings } from './permission-broker.js'; +import { AdapterEvent, ChatAdapter, ChatAdapterOptions } from './adapter.js'; +import { + PermissionAsk, + PermissionAnswer, + PermissionBroker, + QuestionAsk, + QuestionReply, + permissionHookSettings, +} from './permission-broker.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. @@ -44,6 +60,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 +115,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 +174,28 @@ 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 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 already reported. + * + * 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 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 askCalls: Array<{ toolId: string; question?: 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 +292,57 @@ 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 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 // 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 && 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 + // 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; + } + 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, { @@ -264,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) @@ -304,6 +395,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 +430,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 +472,35 @@ 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') { + 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.askCalls = []; + } + 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 +735,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 +794,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 +824,118 @@ 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. + * + * 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.claimAskCall(question), + 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 +985,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 +998,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..e10cab4 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,164 @@ 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; + // 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, + }; +} + /** * 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 e289446..79d2d0d 100644 --- a/test/browser/checks.ts +++ b/test/browser/checks.ts @@ -237,6 +237,7 @@ async function run(): Promise { await checkTheFixedBarsNeverWrap(); await checkALiveAnswerAppearsAsItStreams(); await checkSilentStepsLeaveNoRowButKeepTheirTrace(); + await checkAQuestionIsAnsweredByClicking(); await checkThePhoneLayoutIsUsable(); await checkThePhoneShellSurfacesAreUsable(); await checkALongTabNameStaysInsideTheStrip(); @@ -247,6 +248,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..95d1dc1 --- /dev/null +++ b/test/chat-questions.test.js @@ -0,0 +1,765 @@ +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, + 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). +// +// 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('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('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 + // 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 () { + 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); + }); + }); +});