From 71f575e16a63b2fcb98e3054912799252a75f0a3 Mon Sep 17 00:00:00 2001 From: "paseo-bot[bot]" <266920839+paseo-bot[bot]@users.noreply.github.com> Date: Thu, 15 Oct 2026 14:21:03 +0200 Subject: [PATCH] Keep malformed Claude question answers retryable (#6936) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: reject a question permission that would deliver no answer Answering an `AskUserQuestion` permission in any shape but `updatedInput.answers` keyed by one of its questions resolves as `allow` and delivers nothing. The waiting agent is told "The user did not answer the questions." — an affirmative falsehood rather than silence, so neither side can see that anything failed. The responder sees success; the agent believes the human declined and acts on that. The field it is most natural to reach for cannot work. `selectedActionId` is `z.string().optional()` with no membership check, and is read only for `plan` kinds; a question advertises `actions: undefined`, so there is no action id to name in the first place. It is also unrecoverable. `respondToPermission` deletes the request from `pendingPermissions` on entry, so the malformed answer consumes it and the retry fails with "No pending permission request". The guard runs before that delete, and `AgentManager` drops its own copy only once the call resolves, so both maps survive the throw and the request stays answerable. The answer contract is per provider, so the guard is in the Claude provider rather than in `AgentManager`. Claude keeps an answer only when its key is a question's full text or its header and its value is a non-empty string; Codex's `mapCodexQuestionResponseByHeader` reads headers only and deliberately selects each question's first option when nothing maps, which a shared guard would have rejected; OpenCode reads headers only as well. `resolveClaudeAskUserQuestionAnswers` is extracted from `normalizeClaudeAskUserQuestionUpdatedInput` with no change in behaviour, and both it and the guard now use it. So "would this deliver anything" is answered by the code that does the delivering, rather than by a second copy of its rule — which matters more than it looks, because that rule has two details easy to restate wrongly: `readNonEmptyString` tests `value.trim()` for emptiness but returns the *original* string, so a question named `" Which colour? "` is keyed by that padding; and an empty `updatedInput.questions` array is a list that wins over the request's own questions and maps nothing, not an absent one. Verified on a live daemon over the MCP `respond_to_permission` path against a seat whose question text is literally `" Which colour? "`: `selectedActionId`, the trimmed key, an empty echoed question list and a numeric answer are each rejected with the request still pending, and `{"answers":{" Which colour? ":"Ochre"}}` then reaches the agent. One known limit: the WebSocket path (`session.ts handleAgentPermissionResponse`) catches the throw and emits an `activity_log` error rather than failing the call, because a permission response is modelled as a notification. So `paseo permit allow` still prints `allowed` and exits 0 for a rejected shape. The destructive half is closed on every path — the request survives and the agent is never told a falsehood — but closing the reporting half needs a protocol change. * test(claude): cover the question-answer-required retry path A non-deliverable answer to an AskUserQuestion permission is rejected, but the pending request survives it: a corrected retry against the same request id is accepted, and the waiting agent receives the answer. Mutation testing on a fork found that the existing suite does not pin this ordering — moving the deliverability check to run after the request is deleted from pendingPermissions still leaves every existing test green. This test reds against that change. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Thomas Van Co-authored-by: Claude Sonnet 5 Co-authored-by: paseo-bot[bot] <266920839+paseo-bot[bot]@users.noreply.github.com> Co-authored-by: paseo-bot --- .../server/agent/providers/claude/agent.ts | 68 +++++++- .../claude/question-response.test.ts | 164 ++++++++++++++++++ 2 files changed, 223 insertions(+), 9 deletions(-) create mode 100644 packages/server/src/server/agent/providers/claude/question-response.test.ts diff --git a/packages/server/src/server/agent/providers/claude/agent.ts b/packages/server/src/server/agent/providers/claude/agent.ts index 61ac0e3ae0..8c5cc1a41c 100644 --- a/packages/server/src/server/agent/providers/claude/agent.ts +++ b/packages/server/src/server/agent/providers/claude/agent.ts @@ -203,24 +203,26 @@ function stripClaudeAskUserQuestionUiMetadata(input: AgentMetadata): AgentMetada }; } -export function normalizeClaudeAskUserQuestionUpdatedInput( +/** + * The answers this response will actually deliver, keyed by full question text, + * or null when there is no question list and answers map to work from. + * + * Shared by the normalizer and by the check that rejects an undeliverable + * answer, so the two can never disagree about what "deliverable" means. + */ +function resolveClaudeAskUserQuestionAnswers( updatedInput: AgentMetadata | undefined, fallbackInput: AgentMetadata | undefined, -): AgentMetadata { +): Record | null { const fallback = isMetadata(fallbackInput) ? fallbackInput : {}; const base = isMetadata(updatedInput) ? updatedInput : {}; - // Paseo's shared question UI serializes answers by question header, but Claude's - // AskUserQuestion tool expects answer keys to match the full question text. Merge - // the original request payload back in so provider callbacks that only return - // `{ answers }` still satisfy Claude's full tool input schema. - const merged = stripClaudeAskUserQuestionUiMetadata({ ...fallback, ...base }); const questions = (Array.isArray(base.questions) ? base.questions : null) ?? (Array.isArray(fallback.questions) ? fallback.questions : null); const answers = isMetadata(base.answers) ? base.answers : null; if (!questions || !answers) { - return merged; + return null; } const normalizedAnswers: Record = {}; @@ -244,7 +246,23 @@ export function normalizeClaudeAskUserQuestionUpdatedInput( } } - if (Object.keys(normalizedAnswers).length === 0) { + return normalizedAnswers; +} + +export function normalizeClaudeAskUserQuestionUpdatedInput( + updatedInput: AgentMetadata | undefined, + fallbackInput: AgentMetadata | undefined, +): AgentMetadata { + const fallback = isMetadata(fallbackInput) ? fallbackInput : {}; + const base = isMetadata(updatedInput) ? updatedInput : {}; + // Paseo's shared question UI serializes answers by question header, but Claude's + // AskUserQuestion tool expects answer keys to match the full question text. Merge + // the original request payload back in so provider callbacks that only return + // `{ answers }` still satisfy Claude's full tool input schema. + const merged = stripClaudeAskUserQuestionUiMetadata({ ...fallback, ...base }); + const normalizedAnswers = resolveClaudeAskUserQuestionAnswers(updatedInput, fallbackInput); + + if (!normalizedAnswers || Object.keys(normalizedAnswers).length === 0) { return merged; } @@ -254,6 +272,32 @@ export function normalizeClaudeAskUserQuestionUpdatedInput( }; } +class ClaudeQuestionAnswerRequiredError extends Error { + readonly code = "question_answer_required" as const; + + constructor(readonly requestId: string) { + super( + `Permission request '${requestId}' requires updatedInput.answers keyed by a ` + + `question's full text or header, with a non-empty string value. ` + + `The request is still pending; retry with that shape.`, + ); + this.name = "ClaudeQuestionAnswerRequiredError"; + } +} + +function assertClaudeQuestionAnswerDeliverable( + request: AgentPermissionRequest, + response: AgentPermissionResponse, +): void { + if (request.kind !== "question" || response.behavior !== "allow") { + return; + } + const answers = resolveClaudeAskUserQuestionAnswers(response.updatedInput, request.input); + if (!answers || Object.keys(answers).length === 0) { + throw new ClaudeQuestionAnswerRequiredError(request.id); + } +} + function isObjectRecord(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value); } @@ -2734,6 +2778,12 @@ class ClaudeAgentSession implements AgentSession { if (!pending) { throw new Error(`No pending permission request with id '${requestId}'`); } + + // Before the delete below, so a rejected answer leaves the request pending + // and answerable rather than consuming it for nothing. AgentManager only + // drops its own copy once this resolves, so both maps survive the throw. + assertClaudeQuestionAnswerDeliverable(pending.request, response); + this.pendingPermissions.delete(requestId); pending.cleanup?.(); diff --git a/packages/server/src/server/agent/providers/claude/question-response.test.ts b/packages/server/src/server/agent/providers/claude/question-response.test.ts new file mode 100644 index 0000000000..26909faab7 --- /dev/null +++ b/packages/server/src/server/agent/providers/claude/question-response.test.ts @@ -0,0 +1,164 @@ +import { describe, expect, test } from "vitest"; +import type { CanUseTool, Query, SDKMessage } from "@anthropic-ai/claude-agent-sdk"; +import { ClaudeAgentClient } from "./agent.js"; +import type { ClaudeQueryFactory } from "./query.js"; +import type { AgentPermissionResponse } from "../../agent-sdk-types.js"; +import { createTestLogger } from "../../../../test-utils/test-logger.js"; + +// The SDK dependency receives the same permission callback as a real Claude process. +// Keep its stream open until close; permission state belongs entirely to the session. +function createSdkFake() { + let canUseTool: CanUseTool | undefined; + let finish!: () => void; + const finished = new Promise((resolve) => { + finish = resolve; + }); + const queryFactory: ClaudeQueryFactory = ({ options }) => { + canUseTool = options.canUseTool; + return { + close: finish, + return: async () => { + finish(); + return { value: undefined, done: true }; + }, + [Symbol.asyncIterator]() { + return { + next: async (): Promise> => { + await finished; + return { value: undefined, done: true }; + }, + }; + }, + } as Query; + }; + return { + queryFactory, + ask: (input: Record) => { + if (!canUseTool) throw new Error("SDK permission callback was not configured"); + return canUseTool("AskUserQuestion", input, { + signal: new AbortController().signal, + toolUseID: "question-tool-call", + }); + }, + }; +} + +const questionInput = { + questions: [{ question: "Which colour?", header: "Colour", options: [], multiSelect: false }], +}; + +const invalidResponses: Array<[string, AgentPermissionResponse]> = [ + ["selectedActionId", { behavior: "allow", selectedActionId: "Viridian" }], + ["missing answers", { behavior: "allow", updatedInput: {} }], + ["unknown question", { behavior: "allow", updatedInput: { answers: { Hue: "Viridian" } } }], + ["blank answer", { behavior: "allow", updatedInput: { answers: { Colour: " " } } }], + ["non-string answer", { behavior: "allow", updatedInput: { answers: { Colour: 1 } } }], + [ + "empty echoed questions", + { behavior: "allow", updatedInput: { questions: [], answers: { Colour: "Viridian" } } }, + ], +]; + +describe("Claude question permission responses", () => { + test.each(invalidResponses)( + "rejects %s and delivers a corrected retry", + async (_label, response) => { + const sdk = createSdkFake(); + const session = await new ClaudeAgentClient({ + logger: createTestLogger(), + queryFactory: sdk.queryFactory, + resolveBinary: async () => "/test/claude/bin", + }).createSession({ provider: "claude", cwd: process.cwd(), modeId: "default" }); + try { + await session.startTurn("Ask which colour"); + const answer = sdk.ask(questionInput); + void answer.catch(() => undefined); + const request = session.getPendingPermissions()[0]!; + expect(request.kind).toBe("question"); + await expect(session.respondToPermission(request.id, response)).rejects.toMatchObject({ + code: "question_answer_required", + requestId: request.id, + }); + expect(session.getPendingPermissions()).toEqual([request]); + await session.respondToPermission(request.id, { + behavior: "allow", + updatedInput: { answers: { Colour: "Viridian" } }, + }); + await expect(answer).resolves.toMatchObject({ + behavior: "allow", + updatedInput: { answers: { "Which colour?": "Viridian" } }, + }); + expect(session.getPendingPermissions()).toEqual([]); + } finally { + await session.close(); + } + }, + ); + + test.each([ + ["header", questionInput, { Colour: "Viridian" }, { "Which colour?": "Viridian" }], + ["question text", questionInput, { "Which colour?": "Ochre" }, { "Which colour?": "Ochre" }], + [ + "padded question text", + { questions: [{ question: " Which colour? ", header: " Colour " }] }, + { " Which colour? ": "Ochre" }, + { " Which colour? ": "Ochre" }, + ], + [ + "padded header", + { questions: [{ question: " Which colour? ", header: " Colour " }] }, + { " Colour ": "Viridian" }, + { " Which colour? ": "Viridian" }, + ], + [ + "partial answer", + { questions: [...questionInput.questions, { question: "Ship it?", header: "Ship" }] }, + { Ship: "Yes" }, + { "Ship it?": "Yes" }, + ], + ])("delivers answers keyed by %s", async (_label, input, answers, expected) => { + const sdk = createSdkFake(); + const session = await new ClaudeAgentClient({ + logger: createTestLogger(), + queryFactory: sdk.queryFactory, + resolveBinary: async () => "/test/claude/bin", + }).createSession({ provider: "claude", cwd: process.cwd(), modeId: "default" }); + try { + await session.startTurn("Ask which colour"); + const answer = sdk.ask(input); + void answer.catch(() => undefined); + const request = session.getPendingPermissions()[0]!; + await session.respondToPermission(request.id, { + behavior: "allow", + updatedInput: { answers }, + }); + await expect(answer).resolves.toMatchObject({ + behavior: "allow", + updatedInput: { answers: expected }, + }); + expect(session.getPendingPermissions()).toEqual([]); + } finally { + await session.close(); + } + }); + + test("allows denying a question without answers", async () => { + const sdk = createSdkFake(); + const session = await new ClaudeAgentClient({ + logger: createTestLogger(), + queryFactory: sdk.queryFactory, + resolveBinary: async () => "/test/claude/bin", + }).createSession({ provider: "claude", cwd: process.cwd(), modeId: "default" }); + try { + await session.startTurn("Ask which colour"); + const answer = sdk.ask(questionInput); + void answer.catch(() => undefined); + const request = session.getPendingPermissions()[0]!; + await session.respondToPermission(request.id, { behavior: "deny", message: "No answer" }); + await expect(answer).resolves.toMatchObject({ behavior: "deny", message: "No answer" }); + expect(session.getPendingPermissions()).toEqual([]); + } finally { + await session.close(); + } + }); +});