diff --git a/src/servers/api/chat/types.ts b/src/servers/api/chat/types.ts index 5206f25e..dcc4f220 100644 --- a/src/servers/api/chat/types.ts +++ b/src/servers/api/chat/types.ts @@ -153,6 +153,21 @@ export type ClientMessage = requestId: string; approve: boolean; feedback?: string; + } + | { + /** + * The user's answer to an `AskUserQuestion` card. + * + * `toolCallId` is the id the client already received with `tool:start` for that very tool call, so + * answering is addressed rather than positional — two questions in one turn, or a second browser + * replaying a stale click, both land where they belong. + * + * `answer` is prose, not an index: what the agent gets back is a sentence, so a picked option, a + * multi-select and a typed "something else" all travel the same way. + */ + type: 'question:answer'; + toolCallId: string; + answer: string; }; // The `Task` tool's id, when this piece of output came from a subagent rather than the agent you are diff --git a/src/servers/api/chat/websocket.ts b/src/servers/api/chat/websocket.ts index 92539c03..227db196 100644 --- a/src/servers/api/chat/websocket.ts +++ b/src/servers/api/chat/websocket.ts @@ -243,6 +243,8 @@ export function message(ws: ServerWebSocket, raw: string | Buffer): void await handlePlanMode(ws, clientMsg); } else if (clientMsg.type === 'plan:decide') { handlePlanDecision(ws, clientMsg); + } else if (clientMsg.type === 'question:answer') { + handleQuestionAnswer(ws, clientMsg); } } catch (err) { logger.error('Error handling WebSocket message', { email: ws.data.email, error: String(err) }); @@ -1270,6 +1272,25 @@ function handlePlanDecision( }); } +/** + * Relay the user's answer to the question the agent is blocked on. + * + * Silent like its plan sibling, and for a stronger reason: the answer's own receipt is the `tool:result` + * the sidecar emits when this unblocks it, which is durable and reaches every socket on this conversation. + * A direct reply would confirm to the one browser that clicked, and leave a second pane showing a question + * that is already answered. + */ +function handleQuestionAnswer(ws: ServerWebSocket, msg: { toolCallId: string; answer: string }): void { + const sessionId = wsToSessionMap.get(ws); + if (!sessionId) return; + sidecar.answerClaudeQuestion({ + sessionKey: sessionId, + userId: ws.data.userId, + toolCallId: msg.toolCallId, + answer: msg.answer, + }); +} + async function handleStop(ws: ServerWebSocket): Promise { const sessionId = wsToSessionMap.get(ws); diff --git a/src/servers/sidecar-registry.ts b/src/servers/sidecar-registry.ts index 12fa1da7..6227d930 100644 --- a/src/servers/sidecar-registry.ts +++ b/src/servers/sidecar-registry.ts @@ -314,6 +314,18 @@ export function decideClaudePlan(params: { sendFire('claude', { type: 'claude:plan-decision', id: nextId(), ...params }); } +// The user's answer to an AskUserQuestion. Fire-and-forget for the same reason as the plan decision: the +// visible result is the tool's own output arriving in the durable log, where every socket watching this +// conversation sees it — including the one that did not click. +export function answerClaudeQuestion(params: { + sessionKey: string; + userId: number; + toolCallId: string; + answer: string; +}): void { + sendFire('claude', { type: 'claude:question-answer', id: nextId(), ...params }); +} + /** * Is the agent still running a turn for this session? * diff --git a/src/servers/sidecar/claude/claude-manager.ts b/src/servers/sidecar/claude/claude-manager.ts index f44cb25c..447c4f1d 100644 --- a/src/servers/sidecar/claude/claude-manager.ts +++ b/src/servers/sidecar/claude/claude-manager.ts @@ -270,6 +270,16 @@ type PersistentSession = { string, { resolve: (decision: { approve: boolean; feedback?: string }) => void; timer: ReturnType } >; + /** + * AskUserQuestion calls waiting on a human, keyed by the harness's `tool_use_id`. + * + * Same hazard as `pendingPlans` — each entry is a promise the agent is blocked on — and settled from the + * same places, via `settleAllQuestions`. Keyed on the tool call id rather than an id we invent because + * the UI already has that one: it arrives with `tool:start` and comes back on the answer, so nothing has + * to be correlated by position or by "the newest pending question", which is what breaks when two + * questions are asked in one turn. + */ + pendingQuestions: Map void; timer: ReturnType }>; /** The tail of the child's stderr, so its exit can carry a reason instead of just a number. */ stderrTail: string; /** Whether the child ever produced a single message. False plus a non-zero exit means it never started. */ @@ -509,6 +519,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat pendingTasks: new Set(), planMode: params.planMode === true, pendingPlans: new Map(), + pendingQuestions: new Map(), interrupted: false, stderrTail: '', sawOutput: false, @@ -579,6 +590,46 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // returns immediately and never throws: it is a notification, and it must not be able to stall or // fail the compaction it is announcing. hooks: { + /** + * The answer channel for `AskUserQuestion`, and the reason it is a HOOK rather than `canUseTool`. + * + * Without one the harness answers the tool itself, instantly, with the literal string "The user did + * not answer the questions." — before the card has painted, let alone been read. The agent then acts + * on a refusal that never happened: on 2026-08-19 it asked the same question three times in a row + * while the answers were sitting on screen. + * + * `canUseTool` cannot carry this. Under `bypassPermissions` — which is every session here — the SDK + * shadows it and never calls it (measured against 0.2.59: a bypassing session's AskUserQuestion + * reached the PreToolUse hook and never reached the callback). Hooks run regardless of permission + * mode, which is exactly the property needed. + * + * The mechanism is `permissionDecision: 'deny'` with the answer as its reason, because the harness + * feeds that reason back as the TOOL RESULT verbatim. "Deny" reads wrong and is right: the tool is + * prevented from resolving itself, and what replaces its output is the human's answer. + * + * The matcher timeout is deliberately longer than `QUESTION_ANSWER_TIMEOUT_MS` so that OUR timer is + * always the one that fires. A hook killed by the harness would fall back to the built-in lie; a + * hook that times out here answers in words that are true. + */ + PreToolUse: [ + { + matcher: 'AskUserQuestion', + timeout: Math.round((QUESTION_ANSWER_TIMEOUT_MS + 5 * 60 * 1000) / 1000), + hooks: [ + async (input) => { + if (!('tool_use_id' in input) || typeof input.tool_use_id !== 'string') return { continue: true }; + const answer = await questionGate(session, input.tool_use_id); + return { + hookSpecificOutput: { + hookEventName: 'PreToolUse', + permissionDecision: 'deny', + permissionDecisionReason: answer, + }, + }; + }, + ], + }, + ], PreCompact: [ { hooks: [ @@ -741,6 +792,7 @@ function createSession(params: ClaudeSpawnStreamingParams, onEvent: (event: Chat // Before the session leaves the map, so nothing is left holding a promise whose resolver has just // become unreachable. settleAllPlans(session, 'The session ended before the plan was answered.'); + settleAllQuestions(session, 'The session ended before the question was answered.'); sessions.delete(sessionKey); } })(); @@ -819,6 +871,15 @@ async function applyModelChange(session: PersistentSession, model: string | unde */ const PLAN_DECISION_TIMEOUT_MS = 20 * 60 * 1000; +/** + * How long a question waits for a person before the agent is told, truthfully, that it has no answer yet. + * + * Matched to the plan timeout because the situation is the same one: a human who has walked away, and a turn + * that must end rather than hang. What differs is the wording — an unanswered question is not a refusal, and + * saying so invites the agent to re-ask, which is the behaviour this whole channel exists to stop. + */ +const QUESTION_ANSWER_TIMEOUT_MS = 20 * 60 * 1000; + /** * Hold `ExitPlanMode` open while a human decides. * @@ -874,6 +935,61 @@ async function planGate( return { behavior: 'allow', updatedInput: toolInput }; } +/** + * Park an `AskUserQuestion` until a person answers it, and hand back what they said. + * + * Returns the string that BECOMES THE TOOL RESULT, so every branch has to read as an answer to the agent + * rather than as a status report about the plumbing. + * + * No `question:asked` event is emitted, and that is the point of keying on `tool_use_id`: the tool call is + * already in the session log as an ordinary `tool:start`, durable and ordered, carrying the questions. A + * second copy would be a second source of truth for the same card, and they would disagree the moment one + * of them was replayed. + */ +async function questionGate(session: PersistentSession, toolCallId: string): Promise { + return new Promise((resolve) => { + const timer = setTimeout(() => { + session.pendingQuestions.delete(toolCallId); + console.warn(`[claude:question:${session.sessionKey}] no answer in 20m`); + resolve( + 'The user has not answered yet — they may be away from the screen. The question is still on theirs, ' + + 'so do NOT ask it again. Continue with your best judgement, or stop and wait for their next message.', + ); + }, QUESTION_ANSWER_TIMEOUT_MS); + + session.pendingQuestions.set(toolCallId, { resolve, timer }); + }); +} + +/** + * Resolve every outstanding question, because the session or turn it belonged to is going away. + * + * Same contract as `settleAllPlans` and for the same reason: an unresolved entry is a turn that never ends. + */ +function settleAllQuestions(session: PersistentSession, reason: string): void { + for (const [toolCallId, pending] of session.pendingQuestions) { + clearTimeout(pending.timer); + session.pendingQuestions.delete(toolCallId); + pending.resolve(reason); + } +} + +/** + * The user's answer to a pending question. False when nothing is waiting under that tool call id. + * + * A miss is ordinary, exactly as it is for plans: a reconnected client replaying its last action, or a + * second pane answering a question the first already answered. + */ +export function answerClaudeQuestion(sessionKey: string, userId: number, toolCallId: string, answer: string): boolean { + const session = ownedSession(sessionKey, userId); + const pending = session?.pendingQuestions.get(toolCallId); + if (!session || !pending) return false; + clearTimeout(pending.timer); + session.pendingQuestions.delete(toolCallId); + pending.resolve(answer); + return true; +} + /** * Resolve every outstanding plan as refused. * @@ -957,6 +1073,7 @@ export async function interruptClaudeSession(sessionKey: string, userId: number) // Stop cannot reach a turn that is blocked on a plan — the agent is not running, it is waiting on us — // so the promise has to be released for the interrupt to have anything to interrupt. settleAllPlans(session, 'The turn was stopped before the plan was answered.'); + settleAllQuestions(session, 'The turn was stopped before the question was answered.'); try { await session.query.interrupt(); session.isGenerating = false; @@ -976,6 +1093,7 @@ export function killClaudeSession(sessionKey: string, userId: number): boolean { if (session.idleTimer) clearTimeout(session.idleTimer); if (session.stallTimer) clearTimeout(session.stallTimer); settleAllPlans(session, 'The session was closed before the plan was answered.'); + settleAllQuestions(session, 'The session was closed before the question was answered.'); try { session.abort.abort(); } catch { diff --git a/src/servers/sidecar/claude/user-instance.ts b/src/servers/sidecar/claude/user-instance.ts index b1552823..db8f0f0c 100644 --- a/src/servers/sidecar/claude/user-instance.ts +++ b/src/servers/sidecar/claude/user-instance.ts @@ -301,6 +301,13 @@ async function handleCommand(cmd: SidecarCommand, reply: ReplyFn) { claudeManager.decideClaudePlan(cmd.sessionKey, cmd.userId, cmd.requestId, cmd.approve, cmd.feedback); break; + // Answers through the tool result the agent is blocked on, not through a reply — the same reasoning as + // the plan commands above, one layer deeper: what the user needs to see is the question's own card + // filling in, and that arrives as the `tool:result` this unblocks. + case 'claude:question-answer': + claudeManager.answerClaudeQuestion(cmd.sessionKey, cmd.userId, cmd.toolCallId, cmd.answer); + break; + case 'claude:list': reply({ type: 'claude:sessions', id: cmd.id, sessions: claudeManager.listSessions(cmd.userId) }); break; diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 06ef96a6..2ebde5c6 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -80,6 +80,22 @@ export type SidecarCommand = approve: boolean; feedback?: string; } + /** + * The user's answer to an `AskUserQuestion` the agent is blocked on. + * + * Sibling of `claude:plan-decision` and the second command that resolves a promise the AGENT is waiting + * on. `toolCallId` is the harness's own `tool_use_id`, which the client already has from `tool:start` — + * so a stale answer replayed by a reconnecting client lands on the question it was meant for, or on + * nothing at all, and can never resolve a different one. + */ + | { + type: 'claude:question-answer'; + id: string; + sessionKey: string; + userId: number; + toolCallId: string; + answer: string; + } // Which session key owns this transcript? The map lives on the agent's disk, so only it can answer — // see `findClaudeSessionKey` in sidecar-registry, and `attach` in the chat socket for why it is asked. | { type: 'claude:find-session'; id: string; claudeSessionId: string; userId: number } diff --git a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts index 919ad97c..7dfe209c 100644 --- a/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts +++ b/src/workspaces/officerdev/src/apps/Chat/EmbeddableChat/useEmbeddableChat.ts @@ -81,6 +81,7 @@ export function useEmbeddableChat(params: UseEmbeddableChatParams, onMessageComp setThinkingLevel, setPlanMode, decidePlan, + answerQuestion, sendPrompt, stopGeneration: stopTurn, loadOlder, @@ -589,6 +590,7 @@ export function useEmbeddableChat(params: UseEmbeddableChatParams, onMessageComp setThinkingLevel, setPlanMode, decidePlan, + answerQuestion, sendPrompt, stopGeneration, input, diff --git a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx index 0ef3780c..914d7614 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/MessageBubble.tsx @@ -122,7 +122,12 @@ const ReadAloudButton = ({ id, text }: { id: string; text: string }) => { type MessageBubbleProps = { message: ChatMessage; - onAnswer?: (text: string) => void; + /** + * Answer a question the agent is parked on. Takes the tool call id as well as the text because that id + * is how the sidecar finds the promise to resolve, and this component is the last place that still knows + * it — the caller sees a `ChatMessage` union whose user variant has no tool call at all. + */ + onAnswer?: (text: string, toolCallId: string) => void; onRetry?: (prompt: string) => void; /** Answer a plan the agent is parked on. Absent in read-only views, where the card renders without buttons. */ onDecidePlan?: (requestId: string, approve: boolean, feedback?: string) => void; @@ -231,7 +236,7 @@ export const MessageBubble = ({ message, onAnswer, onRetry, onDecidePlan, defaul // was arriving correctly and the UI simply did not recognise it. Nothing errored, which is why it // read as "the model never asks" rather than as a bug. if (QUESTION_TOOLS.has(message.toolName) && onAnswer) { - return ; + return onAnswer(text, message.toolCallId)} />; } return ; diff --git a/src/workspaces/officerdev/src/apps/Chat/components/MessageList.tsx b/src/workspaces/officerdev/src/apps/Chat/components/MessageList.tsx index fc9280f0..edee25af 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/MessageList.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/MessageList.tsx @@ -25,6 +25,7 @@ export const MessageList = ({ manager }: MessageListProps) => { jumpToBottom, sendPrompt, decidePlan, + answerQuestion, scrollViewportRef, bottomRef, isLoadingOlder, @@ -130,7 +131,7 @@ export const MessageList = ({ manager }: MessageListProps) => { {row.kind === 'message' && ( sendPrompt(text)} + onAnswer={(text, toolCallId) => answerQuestion(toolCallId, text)} onRetry={(prompt) => sendPrompt(prompt)} onDecidePlan={decidePlan} // Deliberately not `isGenerating`: a turn's work stays open after it finishes, and diff --git a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx index 02757cf0..6b7cf5c5 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.test.tsx @@ -106,29 +106,45 @@ describe('the escape hatch', () => { }); }); -// The regression that made this component dead on arrival, pinned from both sides. +// What an `output` means, which reversed on 2026-08-19 and is the reason this block reads oddly. // -// AskUserQuestion has no answer channel: it resolves ITSELF, immediately, with the string below — so a -// real question always carries an output by the time it renders. The component gated on that output, and -// the test here asserted the resulting greyed-out state was correct. Both were wrong in the same -// direction, so the suite passed and every question in the product was unclickable. -describe('a question the SDK has already self-resolved', () => { - test('is still answerable — the output ends the tool call, not the conversation', () => { +// BEFORE: AskUserQuestion had no answer channel and resolved ITSELF, immediately, with "The user did not +// answer the questions." — so every real question carried an output before it was even readable. The +// component gated on that output, this test asserted the resulting greyed-out card was correct, and the +// suite stayed green over a feature nobody could click. +// +// AFTER: the sidecar parks the tool in a `PreToolUse` hook and feeds the human's answer back as its result. +// An output now means the question is settled — answered, timed out, or ended with its session — so the +// card is correctly read-only, and the pre-fix transcripts carrying the old string read as settled too, +// which they are: nobody can answer them now. +describe('a question that already carries an output', () => { + test('is read-only — the output is the answer, or the reason there will not be one', () => { const answers: string[] = []; - const resolved = { + const answered = { + ...ask([{ question: 'Which?', header: 'Store', options: OPTIONS }]), + output: 'Postgres', + } as never; + render( answers.push(t)} />); + fireEvent.click(screen.getByText('Postgres')); + expect(answers).toEqual([]); + }); + + test('including a transcript from before the fix, whose output is the harness answering for the user', () => { + const answers: string[] = []; + const stale = { ...ask([{ question: 'Which?', header: 'Store', options: OPTIONS }]), output: 'The user did not answer the questions.', } as never; - render( answers.push(t)} />); + render( answers.push(t)} />); fireEvent.click(screen.getByText('Postgres')); - expect(answers).toEqual(['Postgres']); + expect(answers).toEqual([]); }); - test('but answering twice is refused — the latch is what we sent, not what the tool returned', () => { + test('while a LIVE question — no output yet — is answerable exactly once', () => { const answers: string[] = []; render( answers.push(t)} />, ); diff --git a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx index f17def64..fdb9777c 100644 --- a/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx +++ b/src/workspaces/officerdev/src/apps/Chat/components/QuestionActivity.tsx @@ -67,16 +67,24 @@ export const QuestionActivity = ({ message, onAnswer }: QuestionActivityProps) = const questions = input.questions; if (!questions || questions.length === 0) return null; - // Deliberately NOT gated on `message.output`. - // - // AskUserQuestion resolves itself the instant it is called — the SDK has no answer channel, so it - // returns "The user did not answer the questions." before these buttons have painted. Gating on the - // output therefore disabled every option on every question, always: the tool was rendered, greyed out, - // and unanswerable. A test asserted that as correct, so the suite stayed green over a dead feature. - // - // The output says the TOOL CALL is over, which is true and irrelevant. The answer travels back as an - // ordinary chat message, and a message can always be sent. `sent` is the only real latch. - const isDisabled = sent; + /* + Gated on `message.output` again, because as of 2026-08-19 the output finally means what it says. + + The history matters, since the obvious reading of this line is that it is the old bug coming back. + AskUserQuestion used to resolve ITSELF the instant it was called — no answer channel existed, so the + harness answered on your behalf with "The user did not answer the questions." before these buttons had + painted. Gating on that disabled every question, always, and a test asserted the dead feature as + correct. So the gate was removed and answers were sent as ordinary chat messages. + + The sidecar now parks the tool in a `PreToolUse` hook until a person answers, and feeds the answer back + as the tool result. So an output is now proof of exactly one thing — this question has been dealt with: + answered, timed out, or ended with its session. All three should be read-only. + + It also fixes staleness for free. A replayed transcript renders every past question with its answer + already in `output`, so old cards can no longer be clicked as if they were live — which is what a + refresh used to show, and what mobile hit too. + */ + const isDisabled = sent || message.output !== undefined; /** * Record one answer, and send once EVERY question has one. diff --git a/src/workspaces/officerdev/src/hooks/useChat.ts b/src/workspaces/officerdev/src/hooks/useChat.ts index 759c88f9..68bd0cd3 100644 --- a/src/workspaces/officerdev/src/hooks/useChat.ts +++ b/src/workspaces/officerdev/src/hooks/useChat.ts @@ -777,6 +777,19 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, send({ type: 'plan:decide', requestId, approve, ...(feedback ? { feedback } : {}) }); } + /** + * Answer an `AskUserQuestion` the agent is blocked on. + * + * Note what this is NOT: sending the answer as a chat message. That was the old shape, forced by there + * being no answer channel at all — the harness had already resolved the tool with "The user did not + * answer the questions." before the card painted. The answer now goes back as the tool's own result, so + * the agent reads it as the reply to what it asked instead of as a new instruction arriving out of + * nowhere. + */ + function answerQuestion(toolCallId: string, answer: string) { + send({ type: 'question:answer', toolCallId, answer }); + } + function stopGeneration() { send({ type: 'stop' }); } @@ -804,6 +817,7 @@ export function useChat(initialSessionId?: string, initialModel?: string | null, setThinkingLevel, setPlanMode, decidePlan, + answerQuestion, sendPrompt, stopGeneration, disconnectSession,