From 37aa484ffc4987f2c3671906cc23652ede5f1633 Mon Sep 17 00:00:00 2001 From: Aditya Umale Date: Fri, 21 Aug 2026 19:41:06 +0530 Subject: [PATCH] Show clickable options when a bot asks you a question MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Claude CLI has its own AskUserQuestion tool for asking the user to pick from a list. It reached us through the permission tool, so we treated it as "may I run this?" and drew an Allow/Deny box over a truncated blob of JSON. The buttons the bot meant you to press were never shown, and auto mode could answer the question for you. It is now read as what it is: a question. Each one becomes a normal option card with real buttons and each option's own explanation, and the answer goes back in the field the tool documents for it. A bare allow is not enough — the CLI then runs the tool in a headless session with no dialog and tells the model you did not answer, so the click is thrown away. Verified end to end against claude 2.1.238. Also here, all in the same area: - Auto mode can never answer a question for you. That was the whole point of asking. - A question asked inside a room now shows up. It used to draw nothing while the bot waited out a 15 minute timeout. - Closing a question sends an answer rather than a denial. The broker refuses a denial on a question, so closing one used to do nothing. - A card decides allow/deny from the card, never from the button's text. A question whose option happened to read "Allow" would have been sent as an approval and refused. - A malformed question is denied with a reason instead of falling back to the Allow/Deny box this change exists to remove. - A timeout note is no longer filed as your chosen answer. - A permission never takes its buttons from anything the model wrote. The phone renders those as the decision, where every one of them means deny while "always allow" writes a real grant first. Co-Authored-By: Claude Opus 5 (1M context) --- server/auto-approve.test.ts | 25 ++- server/auto-approve.ts | 14 ++ server/contracts.ts | 4 + server/drivers/claude.test.ts | 139 +++++++++++++ server/drivers/claude.ts | 88 +++++++- server/index.ts | 4 + server/permission-proxy.test.ts | 348 ++++++++++++++++++++++++++++++++ server/permission-proxy.ts | 153 ++++++++++++-- server/store.ts | 9 + server/thread-events.test.ts | 21 ++ server/thread-events.ts | 7 +- src/components/GroupView.tsx | 8 + src/components/OptionCard.tsx | 85 +++++--- src/lib/card-answer.test.ts | 63 ++++++ src/lib/card-answer.ts | 57 ++++++ src/state/store.tsx | 75 +++++-- 16 files changed, 1039 insertions(+), 61 deletions(-) create mode 100644 server/permission-proxy.test.ts create mode 100644 src/lib/card-answer.test.ts create mode 100644 src/lib/card-answer.ts diff --git a/server/auto-approve.test.ts b/server/auto-approve.test.ts index 17bad4eb8..022efdff6 100644 --- a/server/auto-approve.test.ts +++ b/server/auto-approve.test.ts @@ -4,7 +4,7 @@ // question is never answered by the machine. import { describe, expect, it } from "vitest"; -import { approvalKey, autoDecision, looksDestructive, looksSensitive } from "./auto-approve.ts"; +import { approvalKey, autoDecision, autoVerdict, looksDestructive, looksSensitive } from "./auto-approve.ts"; describe("looksDestructive", () => { const dangerous = [ @@ -148,3 +148,26 @@ describe("unattended turns", () => { expect(autoDecision(bot, "Bash", "git status", { unattended: false })).toBeTruthy(); }); }); + +describe("tools that ask a person", () => { + // A question normally arrives typed as a question and never reaches a + // verdict. This is the backstop for the path where one arrives typed as a + // permission: auto mode must not answer it, because auto-approving does + // not answer anything — the CLI runs the tool with no answers and the + // model is told "The user did not answer the questions." + const bot = { autoApprove: true, alwaysAllow: ["AskUserQuestion", "ask_user"] }; + + it("never auto-approves AskUserQuestion, in auto mode or by remembered grant", () => { + expect(autoDecision(bot, "AskUserQuestion", "Which framework should we use?")).toBeNull(); + expect(autoVerdict(bot, "AskUserQuestion", "Which framework?").source).toBe("no-grant"); + }); + + it("never auto-approves ask_user, bare or MCP-prefixed", () => { + expect(autoDecision(bot, "ask_user", "Ready to ship?")).toBeNull(); + expect(autoDecision(bot, "mcp__ogb__ask_user", "Ready to ship?")).toBeNull(); + }); + + it("still auto-approves an ordinary tool for the same bot", () => { + expect(autoDecision(bot, "Read", "/tmp/notes.md")).toBeTruthy(); + }); +}); diff --git a/server/auto-approve.ts b/server/auto-approve.ts index bf83565fa..7e79b9cd9 100644 --- a/server/auto-approve.ts +++ b/server/auto-approve.ts @@ -30,6 +30,16 @@ const SENSITIVE = [ /\bcredentials?\.json\b|\bserviceaccount\b/i, ]; +// Tools that ask a PERSON something. A question exists so that a human +// decides; auto mode answering one on their behalf defeats the only reason +// it was asked. They normally arrive typed as questions and never reach a +// verdict at all — this is the backstop for the path where one arrives +// mis-typed as a permission (a malformed AskUserQuestion call falls back to +// the permission path in permission-proxy). Auto-approving it there does not +// produce an answer: the CLI runs the tool with none and the model is told +// "The user did not answer the questions." — a question silently lost. +const ASKS_A_PERSON = new Set(["askuserquestion", "ask_user"]); + /** First matching pattern's source, so a verdict can NAME the rule that * made it — the decision log's whole value is "which rule", and deriving * the match a second time at the call site is how the log and the verdict @@ -118,6 +128,10 @@ export function autoVerdict( // into them const destructive = matchFirst(DESTRUCTIVE, summary) ?? matchFirst(DESTRUCTIVE, tool); const sensitive = destructive ? null : matchFirst(SENSITIVE, summary); + // A question is for a person, whatever channel it arrived on. + if (ASKS_A_PERSON.has(tool.replace(/^mcp__[^_]+__/, "").toLowerCase())) { + return { approve: null, source: "no-grant" }; + } // The grant is computed even when a hard block will refuse it: the row // worth auditing is "this WOULD have auto-approved, and only the block // stood in the way", which cannot be told apart from an ordinary diff --git a/server/contracts.ts b/server/contracts.ts index 63a051fa2..70045640a 100644 --- a/server/contracts.ts +++ b/server/contracts.ts @@ -109,6 +109,10 @@ export type RuntimeEvent = RuntimeEventBase & tool: string; summary: string; choices?: string[]; + /** questions: what each choice means, keyed by the choice's label */ + choiceHints?: Record; + /** questions: more than one choice may be picked (answers join with ", ") */ + multiSelect?: boolean; approvalScope?: "local-computer"; } | { diff --git a/server/drivers/claude.test.ts b/server/drivers/claude.test.ts index 0a8c896e1..ed5de79bf 100644 --- a/server/drivers/claude.test.ts +++ b/server/drivers/claude.test.ts @@ -640,6 +640,145 @@ describe("ClaudeDriver turns (fake CLI)", () => { await recorder.until((e) => e.type === "turn.completed"); }); + it("carries a question's choices, their explanations, and multi-select through to the card", async () => { + await create("hang"); + await instance.adapter.sendTurn({ threadId: "t-perm-q", text: "go" }); + await recorder.until((e) => e.type === "session.started"); + + const conn = await connectSocket(permissionSocketPath("t-perm-q")); + const nextAnswer = answerQueue(conn); + conn.write( + JSON.stringify({ + t: "ask", + id: "ask-q", + kind: "question", + tool: "AskUserQuestion", + input: { + question: "Which framework should we use?", + choices: ["React", "Vue"], + optionHints: { React: "What the app already uses" }, + multiSelect: true, + }, + }) + "\n", + ); + + const opened = await recorder.until((e) => e.type === "request.opened"); + expect(opened).toMatchObject({ + requestType: "question", + tool: "AskUserQuestion", + summary: "Which framework should we use?", + choices: ["React", "Vue"], + choiceHints: { React: "What the app already uses" }, + multiSelect: true, + }); + + // a question takes an answer, never an allow + await expect( + instance.adapter.respondToRequest("t-perm-q", "ask-q", { behavior: "answer", message: "React, Vue" }), + ).resolves.toBe("answered"); + expect(await nextAnswer()).toMatchObject({ behavior: "answer", message: "React, Vue" }); + + conn.end(); + await instance.adapter.interruptTurn("t-perm-q"); + await recorder.until((e) => e.type === "turn.completed"); + }); + + it("reads a QUESTION's nested questions[] instead of showing it as raw JSON", async () => { + await create("hang"); + await instance.adapter.sendTurn({ threadId: "t-perm-nest", text: "go" }); + await recorder.until((e) => e.type === "session.started"); + + const conn = await connectSocket(permissionSocketPath("t-perm-nest")); + // permission-proxy normalizes this shape before the broker sees it; if + // anything ever gets past it, the card must still be readable rather than + // a truncated `{"questions":[{"question":"…` blob with Allow/Deny under it + conn.write( + JSON.stringify({ + t: "ask", + id: "ask-nest", + kind: "question", + tool: "AskUserQuestion", + input: { + questions: [ + { + question: "What role are you targeting?", + options: [ + { label: "Software Engineer", description: "General SWE" }, + { label: "Product", description: "PM track" }, + ], + }, + ], + }, + }) + "\n", + ); + + const opened = await recorder.until((e) => e.type === "request.opened"); + expect(opened).toMatchObject({ + summary: "What role are you targeting?", + choices: ["Software Engineer", "Product"], + choiceHints: { "Software Engineer": "General SWE", Product: "PM track" }, + }); + expect((opened as { summary: string }).summary).not.toContain("{"); + + conn.end(); + await instance.adapter.interruptTurn("t-perm-nest"); + await recorder.until((e) => e.type === "turn.completed"); + }); + + it("never lets a PERMISSION take its buttons or its summary from a nested questions[]", async () => { + await create("hang"); + await instance.adapter.sendTurn({ threadId: "t-perm-lbl", text: "go" }); + await recorder.until((e) => e.type === "session.started"); + + const conn = await connectSocket(permissionSocketPath("t-perm-lbl")); + // A permission ask whose arguments happen to carry `questions`. The + // companion renders card.options AS the decision and maps every label + // that is not "Allow" to a denial, so model-authored labels here mean + // both buttons deny while "Always allow" writes a real grant first. + conn.write( + JSON.stringify({ + t: "ask", + id: "ask-lbl", + tool: "mcp__x__wire", + input: { + questions: [{ question: "Send the payroll export?", options: [{ label: "Yes" }, { label: "No" }] }], + to: "acct-9", + amount: 4200, + }, + }) + "\n", + ); + + const opened = await recorder.until((e) => e.type === "request.opened"); + expect(opened).toMatchObject({ requestType: "permission" }); + expect((opened as { choices?: string[] }).choices).toBeUndefined(); + // and the arguments a person is actually deciding on are still shown + expect((opened as { summary: string }).summary).toContain("acct-9"); + expect((opened as { summary: string }).summary).toContain("4200"); + + conn.end(); + await instance.adapter.interruptTurn("t-perm-lbl"); + await recorder.until((e) => e.type === "turn.completed"); + }); + + it("still shows an unknown permission tool's raw arguments — they are the whole decision", async () => { + await create("hang"); + await instance.adapter.sendTurn({ threadId: "t-perm-raw", text: "go" }); + await recorder.until((e) => e.type === "session.started"); + + const conn = await connectSocket(permissionSocketPath("t-perm-raw")); + conn.write( + JSON.stringify({ t: "ask", id: "ask-raw", tool: "mcp__x__wire", input: { amount: 4200, to: "acct-9" } }) + "\n", + ); + + const opened = await recorder.until((e) => e.type === "request.opened"); + expect(opened).toMatchObject({ requestType: "permission", summary: '{"amount":4200,"to":"acct-9"}' }); + expect((opened as { choices?: string[] }).choices).toBeUndefined(); + + conn.end(); + await instance.adapter.interruptTurn("t-perm-raw"); + await recorder.until((e) => e.type === "turn.completed"); + }); + it("answers to unknown or already-resolved asks resolve `unavailable` — typed, never a throw", async () => { await create("hang"); await instance.adapter.sendTurn({ threadId: "t-perm-2", text: "go" }); diff --git a/server/drivers/claude.ts b/server/drivers/claude.ts index f876906d5..5cb47c56b 100644 --- a/server/drivers/claude.ts +++ b/server/drivers/claude.ts @@ -214,16 +214,97 @@ function systemEndedReply(kind: Ask["kind"]): { behavior: AskBehavior; message: : { behavior: "deny", message: "OpenMausBot: the turn ended" }; } +const isRecord = (value: unknown): value is Record => + typeof value === "object" && value !== null && !Array.isArray(value); + /** One human-readable line for an ask — what the card subtitle shows. */ function askSummary(ask: Ask): string { const input = ask.input ?? {}; if (typeof input.question === "string") return input.question.slice(0, 300); if (typeof input.command === "string") return input.command.slice(0, 200); if (typeof input.url === "string") return input.url.slice(0, 200); + // A QUESTION can also carry its text one level down, under `questions` + // (the CLI's own AskUserQuestion shape). permission-proxy normalizes that + // before it reaches the broker, so this is the belt to that braces. + // + // Gated on the kind on purpose. A PERMISSION whose arguments happen to + // contain a `questions` array must never have the rest of them summarized + // away: you would be approving an action having read one friendly sentence + // instead of what it does. + if (ask.kind === "question") { + const nested = nestedQuestions(input); + if (nested.length) return nested.map((question) => question.question).join(" · ").slice(0, 300); + } + // Last resort, and deliberately still the raw arguments: for a permission + // ask on an unknown tool, its arguments are the only thing a person has to + // decide with, and hiding them behind a tidy label would make the decision + // worse rather than the card prettier. const text = JSON.stringify(input); return text === "{}" ? (ask.tool ?? "tool") : text.slice(0, 200); } +/** `questions[]` entries with a readable question and labelled options. */ +function nestedQuestions(input: Record): Array<{ question: string; options: unknown[] }> { + if (!Array.isArray(input.questions)) return []; + const out: Array<{ question: string; options: unknown[] }> = []; + for (const entry of input.questions) { + if (typeof entry !== "object" || entry === null) continue; + const { question, options } = entry as Record; + if (typeof question === "string" && question.trim()) { + out.push({ question, options: Array.isArray(options) ? options : [] }); + } + } + return out; +} + +/** + * The answer buttons a QUESTION card offers, and what each one means. + * + * Two shapes arrive: the flat `choices` list ask_user sends, and the + * `questions[].options[]` objects the CLI's AskUserQuestion uses — a label + * plus its own explanation. Reading only the flat one is why a native + * question arrived with no choices at all and fell back to Allow/Deny. + * + * Only ever called for a question. A permission's buttons are Allow/Deny and + * must not come from anything the model wrote: the companion renders + * `card.options` AS the decision, and it maps every label that is not "Allow" + * to a denial — so model-authored labels there mean both buttons deny while + * "Always allow" writes a real grant and then denies. + */ +function askChoices(input: Record): { + choices?: string[]; + choiceHints?: Record; + multiSelect?: boolean; +} { + const hints: Record = {}; + const labels: string[] = []; + // one loop over either shape: flat strings, or objects carrying the label + // and its explanation + const options = Array.isArray(input.choices) ? input.choices : (nestedQuestions(input)[0]?.options ?? []); + for (const option of options) { + if (typeof option === "string") { + labels.push(option); + continue; + } + if (!isRecord(option) || typeof option.label !== "string") continue; + labels.push(option.label); + if (typeof option.description === "string" && option.description.trim()) hints[option.label] = option.description; + } + if (isRecord(input.optionHints)) { + for (const [label, hint] of Object.entries(input.optionHints)) { + if (typeof hint === "string" && hint.trim()) hints[label] = hint; + } + } + const choices = labels.slice(0, 5); + const choiceHints = Object.fromEntries(Object.entries(hints).filter(([label]) => choices.includes(label))); + return { + choices: choices.length ? choices : undefined, + choiceHints: Object.keys(choiceHints).length ? choiceHints : undefined, + multiSelect: input.multiSelect === true ? true : undefined, + }; +} + + export function permissionSocketPath(threadId: string) { // A readable prefix alone is not unique: ids that agree on their first // characters ("t-perm-dup-1", "t-perm-dup-2") would share a socket. POSIX @@ -321,7 +402,10 @@ function createPermissionBroker(opts: { if (!pending.delete(askId)) return; clearTimeout(timer); try { - conn.write(JSON.stringify({ t: "answer", id: askId, behavior, message }) + "\n"); + // `source` travels with the answer: a proxy that cannot tell the + // human's words from a timeout note will file the timeout note as + // the human's words. + conn.write(JSON.stringify({ t: "answer", id: askId, behavior, message, source }) + "\n"); } catch {} opts.onResolve({ ...ask, behavior, source }); }; @@ -672,7 +756,7 @@ export const ClaudeDriver: ProviderDriver = { tool: ask.tool, summary: askSummary(ask), approvalScope: controlsHost ? "local-computer" : undefined, - choices: Array.isArray(ask.input?.choices) ? (ask.input.choices as string[]).slice(0, 5) : undefined, + ...(ask.kind === "question" ? askChoices(ask.input ?? {}) : {}), }); }, onResolve: (resolved) => { diff --git a/server/index.ts b/server/index.ts index 93b9bd9aa..69fc6a124 100644 --- a/server/index.ts +++ b/server/index.ts @@ -879,6 +879,10 @@ bus.subscribe((event: RuntimeEvent) => { : "Your bot has a question", subtitle: event.summary, options: event.choices?.length ? event.choices : permission ? ["Allow", "Deny"] : [], + // questions only: an Allow/Deny box has nothing to explain, and + // nothing to multi-select + optionHints: !permission ? event.choiceHints : undefined, + multiSelect: !permission && event.multiSelect ? true : undefined, requestId: event.requestId, tool: permission ? event.tool : undefined, // the exact grant "always allow" would remember, decided here so diff --git a/server/permission-proxy.test.ts b/server/permission-proxy.test.ts new file mode 100644 index 000000000..e52e827fa --- /dev/null +++ b/server/permission-proxy.test.ts @@ -0,0 +1,348 @@ +// Contract test for permission-proxy — the MCP stdio server the claude CLI +// spawns for --permission-prompt-tool. A fake broker on the socket stands in +// for the harness, so these assert the two halves of the wire the proxy owns: +// the ask it writes to the broker, and the JSON it hands back to the CLI. +// +// The case that matters most is the CLI's own AskUserQuestion. It arrives +// through `approve` looking like a permission, and answering it as one is why +// a multiple-choice question reached users as an Allow/Deny box over a +// truncated JSON blob. It has to leave here as a question, and come back as +// the `answers` object the tool documents — a bare allow makes the CLI run +// the tool, and a headless run has no dialog, so the click is discarded +// ("The user did not answer the questions."). +import { spawn, type ChildProcess } from "node:child_process"; +import { createServer, type Server, type Socket } from "node:net"; +import { mkdtempSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { brokerSocketPath } from "./procs.ts"; +import { removeTempDir } from "./testing/cleanup.ts"; + +const PROXY = join(dirname(fileURLToPath(import.meta.url)), "permission-proxy.ts"); + +/** A question with two labelled options and a per-option explanation — the + * shape the real CLI sends (verified against claude 2.1.238). */ +const QUESTION = { + questions: [ + { + question: "Which framework should we use?", + header: "Framework", + multiSelect: false, + options: [ + { label: "React", description: "What the app already uses" }, + { label: "Vue", description: "Smaller, but a rewrite" }, + ], + }, + ], +}; + +describe("permission proxy", () => { + let scratch: string; + let broker: Server; + let proxy: ChildProcess; + /** every ask the broker received, in order */ + let asks: any[]; + /** how the fake broker answers ask N — set per test */ + let answerWith: (ask: any, index: number) => Record | null; + /** live broker connections, so a test can drop one mid-ask */ + let conns: Socket[]; + const results = new Map(); + + const rpc = (msg: unknown) => proxy.stdin!.write(JSON.stringify(msg) + "\n"); + const waitFor = async (id: number, ms = 8000) => { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (results.has(id)) return results.get(id); + await new Promise((r) => setTimeout(r, 10)); + } + throw new Error(`no response for id ${id}; asks so far: ${JSON.stringify(asks)}`); + }; + const waitForAsks = async (count: number, ms = 8000) => { + const deadline = Date.now() + ms; + while (Date.now() < deadline) { + if (asks.length >= count) return; + await new Promise((r) => setTimeout(r, 10)); + } + throw new Error(`only ${asks.length} of ${count} asks arrived`); + }; + /** The tool result the CLI would read, parsed. */ + const resultJson = (res: any) => JSON.parse(res.result.content[0].text); + + beforeEach(async () => { + scratch = mkdtempSync(join(tmpdir(), "omb-perm-proxy-")); + asks = []; + conns = []; + answerWith = () => null; + const socketPath = brokerSocketPath(scratch, "test"); + broker = createServer((conn: Socket) => { + conns.push(conn); + let buf = ""; + conn.on("error", () => {}); + conn.on("data", (chunk) => { + buf += chunk; + let nl; + while ((nl = buf.indexOf("\n")) !== -1) { + const line = buf.slice(0, nl); + buf = buf.slice(nl + 1); + const ask = JSON.parse(line); + if (ask.t !== "ask") continue; + const index = asks.length; + asks.push(ask); + const answer = answerWith(ask, index); + if (answer) conn.write(JSON.stringify({ t: "answer", id: ask.id, ...answer }) + "\n"); + } + }); + }); + await new Promise((resolve) => broker.listen(socketPath, resolve)); + + proxy = spawn(process.execPath, ["--experimental-strip-types", PROXY, socketPath], { + stdio: ["pipe", "pipe", "pipe"], + }); + let out = ""; + proxy.stdout!.on("data", (chunk) => { + out += chunk; + let nl; + while ((nl = out.indexOf("\n")) !== -1) { + const line = out.slice(0, nl); + out = out.slice(nl + 1); + if (!line.trim()) continue; + try { + const msg = JSON.parse(line); + if (msg.id != null) results.set(msg.id, msg); + } catch { + /* not our frame */ + } + } + }); + rpc({ jsonrpc: "2.0", id: 1, method: "initialize", params: {} }); + await waitFor(1); + }, 20_000); + + afterEach(async () => { + results.clear(); + proxy?.kill(); + await new Promise((resolve) => broker.close(() => resolve())); + removeTempDir(scratch); + }); + + it("exposes approve and ask_user", async () => { + rpc({ jsonrpc: "2.0", id: 2, method: "tools/list" }); + const res = await waitFor(2); + expect(res.result.tools.map((tool: any) => tool.name)).toEqual(["approve", "ask_user"]); + }); + + it("asks AskUserQuestion as a QUESTION, with the option labels and their explanations", async () => { + answerWith = () => ({ behavior: "answer", message: "React", source: "user" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input: QUESTION } }, + }); + const res = await waitFor(2); + + // it left as a question — not a permission, so no card can ever offer + // an Allow the broker would refuse, and auto mode can never answer it + expect(asks).toHaveLength(1); + expect(asks[0]).toMatchObject({ + kind: "question", + tool: "AskUserQuestion", + input: { + question: "Which framework should we use?", + choices: ["React", "Vue"], + optionHints: { React: "What the app already uses", Vue: "Smaller, but a rewrite" }, + multiSelect: false, + }, + }); + + // and it comes back as the tool's own contract: the original questions, + // plus an answers object keyed by the question's text + expect(resultJson(res)).toEqual({ + behavior: "allow", + updatedInput: { ...QUESTION, answers: { "Which framework should we use?": "React" } }, + }); + }); + + it("asks each question of a multi-question call in turn and returns every answer", async () => { + const input = { + questions: [ + { question: "Which framework?", options: [{ label: "React" }, { label: "Vue" }] }, + { question: "Which features?", multiSelect: true, options: [{ label: "Auth" }, { label: "Search" }] }, + ], + }; + answerWith = (_ask, index) => ({ behavior: "answer", message: index === 0 ? "React" : "Auth, Search", source: "user" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input } }, + }); + const res = await waitFor(2); + + // one card per question, in order — never one answer copied over both + expect(asks.map((ask) => ask.input.question)).toEqual(["Which framework?", "Which features?"]); + expect(asks[1].input.multiSelect).toBe(true); + expect(resultJson(res).updatedInput.answers).toEqual({ + "Which framework?": "React", + "Which features?": "Auth, Search", + }); + }); + + it("files a timeout's own note as unanswered, not as the user's choice", async () => { + const input = { + questions: [ + { question: "Which framework?", options: [{ label: "React" }] }, + { question: "Which features?", options: [{ label: "Auth" }] }, + ], + }; + // what the broker actually sends on a timeout: `answer`, source "timeout", + // and a full sentence. A blank-message stand-in never exercised this, which + // is how the note ended up recorded as the person's chosen option. + answerWith = (_ask, index) => + index === 0 + ? { behavior: "answer", message: "React", source: "user" } + : { + behavior: "answer", + message: "OpenMausBot: nobody answered in time. Use your best judgment and continue.", + source: "timeout", + }; + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input } }, + }); + expect(resultJson(await waitFor(2)).updatedInput.answers).toEqual({ "Which framework?": "React" }); + }); + + it("treats the turn ending the same way — system words are not the user's answer", async () => { + answerWith = () => ({ + behavior: "answer", + message: "OpenMausBot: the turn is ending — wrap up.", + source: "system", + }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input: QUESTION } }, + }); + expect(resultJson(await waitFor(2)).updatedInput.answers).toEqual({}); + }); + + it("stops asking and denies when the broker is gone mid-question", async () => { + const input = { + questions: [ + { question: "First?", options: [{ label: "Yes" }] }, + { question: "Second?", options: [{ label: "Yes" }] }, + ], + }; + answerWith = () => ({ behavior: "deny", message: "OpenMausBot: permission broker unavailable" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input } }, + }); + const res = await waitFor(2); + expect(asks).toHaveLength(1); // never asked the second into a dead socket + expect(resultJson(res)).toMatchObject({ behavior: "deny" }); + }); + + it("denies an unanswerable AskUserQuestion instead of carding it as a permission", async () => { + // Nothing to pick, so nobody can answer it. It must NOT become an + // Allow/Deny card over raw JSON: allowing that makes the CLI run the tool + // headless, where it collects nothing and reports "The user did not answer + // the questions." — the click thrown away, which is the original bug. + answerWith = () => ({ behavior: "allow" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { + name: "approve", + arguments: { tool_name: "AskUserQuestion", input: { questions: [{ question: "No options?" }] } }, + }, + }); + const res = await waitFor(2); + expect(asks).toHaveLength(0); // nobody was interrupted for it + expect(resultJson(res)).toMatchObject({ behavior: "deny" }); + expect(resultJson(res).message).toContain("no answerable question"); + }); + + it("keeps the good questions when one entry in the same call is unanswerable", async () => { + const input = { + questions: [ + { question: "Which framework?", options: [{ label: "React" }, { label: "Vue" }] }, + { question: "Anything else?", options: [] }, + ], + }; + answerWith = () => ({ behavior: "answer", message: "React", source: "user" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "AskUserQuestion", input } }, + }); + const res = await waitFor(2); + // the answerable one is still asked, and the bad entry costs it nothing + expect(asks.map((ask) => ask.input.question)).toEqual(["Which framework?"]); + expect(resultJson(res).updatedInput.answers).toEqual({ "Which framework?": "React" }); + }); + + it("still brokers an ordinary permission, with the CLI's own rules on allow", async () => { + answerWith = () => ({ behavior: "allow", always: true }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { + name: "approve", + arguments: { + tool_name: "Bash", + input: { command: "git status" }, + permission_suggestions: [{ type: "addRules", rules: [{ toolName: "Bash" }] }], + }, + }, + }); + const res = await waitFor(2); + expect(asks[0]).toMatchObject({ tool: "Bash", input: { command: "git status" } }); + expect(asks[0].kind).toBeUndefined(); + expect(resultJson(res)).toEqual({ + behavior: "allow", + updatedInput: { command: "git status" }, + updatedPermissions: [{ type: "addRules", rules: [{ toolName: "Bash" }] }], + }); + }); + + it("still asks ask_user as a question and returns the words verbatim", async () => { + answerWith = () => ({ behavior: "answer", message: "ship it", source: "user" }); + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "ask_user", arguments: { question: "Ready?", choices: ["ship it", "wait"] } }, + }); + const res = await waitFor(2); + expect(asks[0]).toMatchObject({ kind: "question", tool: "ask_user", input: { question: "Ready?" } }); + // a question's answer is text, never a permission envelope + expect(res.result.content[0].text).toBe("ship it"); + }); + + it("denies every waiting ask when the broker dies", async () => { + answerWith = () => null; // never answers + rpc({ + jsonrpc: "2.0", + id: 2, + method: "tools/call", + params: { name: "approve", arguments: { tool_name: "Bash", input: { command: "sleep 1" } } }, + }); + await waitForAsks(1); + for (const conn of conns) conn.destroy(); + const res = await waitFor(2); + expect(resultJson(res)).toMatchObject({ behavior: "deny" }); + }); +}); diff --git a/server/permission-proxy.ts b/server/permission-proxy.ts index 9d160b7f1..f1600d0c6 100644 --- a/server/permission-proxy.ts +++ b/server/permission-proxy.ts @@ -10,6 +10,13 @@ // ask_user — the agent can pose a question mid-run and wait; the // human's words come back verbatim. // +// One tool arrives through `approve` that is not a permission at all: the +// CLI's own AskUserQuestion. It is the tool the model actually reaches for +// when it wants a person to choose, and acceptEdits will not run it unasked, +// so it lands here looking like "may I run a tool?" — which is how a question +// ended up on screen as an Allow/Deny box over a JSON blob. It is intercepted +// below and asked as what it is. +// // stdout is the MCP channel — never console.log here. import { connect } from "node:net"; import { randomUUID } from "node:crypto"; @@ -55,6 +62,129 @@ conn.on("data", (chunk) => { const send = (obj: unknown) => process.stdout.write(JSON.stringify(obj) + "\n"); +/** Hand one ask to the broker and wait for the human's answer. */ +function askBroker(ask: Record): Promise { + return new Promise((resolve) => { + waiting.set(String(ask.id), resolve); + if (conn.destroyed) return dead(); + try { + conn.write(JSON.stringify(ask) + "\n"); + } catch { + dead(); + } + }); +} + +// ── the CLI's own AskUserQuestion ────────────────────────────────────── +const ASK_USER_QUESTION = "AskUserQuestion"; + +const asRecord = (value: unknown): Record | null => + typeof value === "object" && value !== null && !Array.isArray(value) ? (value as Record) : null; + +/** A non-blank string, or null. */ +const nonBlank = (value: unknown): string | null => (typeof value === "string" && value.trim() ? value : null); + +/** One question, in the shape the broker and the option card understand. */ +interface NativeQuestion { + question: string; + choices: string[]; + /** label → that option's own explanation, shown under it on the card */ + optionHints: Record; + multiSelect: boolean; +} + +/** + * The answerable questions in an AskUserQuestion call. + * + * Unanswerable entries are SKIPPED, not fatal: one bad entry must not cost + * the user the good questions beside it. An empty result means the whole + * call was unanswerable, which the caller turns into a denial. + * + * There is deliberately no "fall through to the permission path" here. That + * path cannot answer this tool: allowing it makes the CLI run + * AskUserQuestion in a headless session with no dialog, and the model is + * told "The user did not answer the questions." — so the fallback offered a + * person an Allow/Deny box over raw JSON whose Allow silently threw their + * click away. That box is the bug this file exists to remove. + */ +function nativeQuestions(input: unknown): NativeQuestion[] { + const raw = asRecord(input)?.questions; + if (!Array.isArray(raw)) return []; + const questions: NativeQuestion[] = []; + for (const entry of raw) { + const question = parseQuestion(entry); + if (question) questions.push(question); + } + return questions; +} + +/** One `questions[]` entry, or null when it has no question or nothing to pick. */ +function parseQuestion(entry: unknown): NativeQuestion | null { + const fields = asRecord(entry); + const question = nonBlank(fields?.question); + if (!question) return null; + const choices: string[] = []; + const optionHints: Record = {}; + const options = Array.isArray(fields?.options) ? fields.options : []; + for (const option of options) { + // an object with a label is the documented form; a bare string is + // accepted too, so a looser caller degrades to plain buttons + const label = typeof option === "string" ? nonBlank(option) : nonBlank(asRecord(option)?.label); + if (!label || choices.includes(label)) continue; + choices.push(label); + const description = nonBlank(asRecord(option)?.description); + if (description) optionHints[label] = description; + } + if (!choices.length) return null; + return { question, choices, optionHints, multiSelect: fields?.multiSelect === true }; +} + +/** + * Ask a person each question, then answer the tool the way it documents. + * + * The answer is NOT a bare allow. A bare allow tells the CLI to go and run + * AskUserQuestion, and a headless run has no dialog to collect anything — + * the model gets "The user did not answer the questions." back and the + * click is thrown away. The `answers` object is the field the tool's own + * schema calls "User answers collected by the permission component": + * keyed by the question's text, one comma-joined string per question. + */ +async function answerNativeQuestions(input: unknown): Promise { + const questions = nativeQuestions(input); + if (!questions.length) { + return JSON.stringify({ + behavior: "deny", + message: + "OpenMausBot: this AskUserQuestion call had no answerable question (each one needs question text and at least one option), so nobody was shown it. Ask again with a well-formed call, or continue without it.", + }); + } + const answers: Record = {}; + for (const q of questions) { + const answer = await askBroker({ + t: "ask", + id: randomUUID(), + kind: "question", + tool: ASK_USER_QUESTION, + input: { question: q.question, choices: q.choices, optionHints: q.optionHints, multiSelect: q.multiSelect }, + }); + // A question is only ever denied when the broker is gone. It cannot + // collect the rest either, so stop and say so instead of asking into + // a socket nobody is reading. + if (answer.behavior === "deny") { + return JSON.stringify({ behavior: "deny", message: answer.message || "Denied from OpenMausBot" }); + } + const chosen = typeof answer.message === "string" ? answer.message.trim() : ""; + // A question nobody answered is left OUT, and that turns on WHO answered, + // not on whether there are words. The broker's own notes are words — the + // timeout's "nobody answered in time, use your best judgment" is a whole + // sentence — so filing anything non-blank under `answers` hands the model + // system text in the slot reserved for what the person chose. Omitted, the + // CLI simply reports that question as unanswered, which is the truth. + if (answer.source === "user" && chosen) answers[q.question] = chosen; + } + return JSON.stringify({ behavior: "allow", updatedInput: { ...(asRecord(input) ?? {}), answers } }); +} + const TOOLS = [ { name: "approve", @@ -104,6 +234,12 @@ async function handle(msg: any) { if (msg.method === "tools/call") { const name = msg.params?.name; const args = msg.params?.arguments ?? {}; + const reply = (text: string) => send({ jsonrpc: "2.0", id: msg.id, result: { content: [{ type: "text", text }] } }); + // AskUserQuestion is a question wearing a permission's clothes. It never + // continues into the permission path below — see nativeQuestions. + if (name === "approve" && args.tool_name === ASK_USER_QUESTION) { + return reply(await answerNativeQuestions(args.input)); + } const askId = randomUUID(); const isQuestion = name === "ask_user"; // the CLI may include its own suggested permission rules; on allow we @@ -114,18 +250,11 @@ async function handle(msg: any) { : Array.isArray(args.suggestions) ? args.suggestions : null; - const answer: any = await new Promise((resolve) => { - waiting.set(askId, resolve); - if (conn.destroyed) return dead(); - const ask = isQuestion + const answer: any = await askBroker( + isQuestion ? { t: "ask", id: askId, kind: "question", tool: "ask_user", input: { question: args.question, choices: args.choices } } - : { t: "ask", id: askId, tool: args.tool_name, input: args.input }; - try { - conn.write(JSON.stringify(ask) + "\n"); - } catch { - dead(); - } - }); + : { t: "ask", id: askId, tool: args.tool_name, input: args.input }, + ); let text = answer.message || "No answer was given — use your best judgment."; if (!isQuestion) { if (answer.behavior === "allow") { @@ -136,7 +265,7 @@ async function handle(msg: any) { text = JSON.stringify({ behavior: "deny", message: answer.message || "Denied from OpenMausBot" }); } } - return send({ jsonrpc: "2.0", id: msg.id, result: { content: [{ type: "text", text }] } }); + return reply(text); } if (String(msg.method ?? "").startsWith("notifications/")) return; if (msg.id != null) { diff --git a/server/store.ts b/server/store.ts index 449694b24..fb4682a81 100644 --- a/server/store.ts +++ b/server/store.ts @@ -38,6 +38,15 @@ export interface OptionCardData { title: string; subtitle: string; options: string[]; + /** what each option means, keyed by its label — a question that came with + * explanations (AskUserQuestion) shows them under the buttons. Kept beside + * `options` rather than inside it so every existing reader of the plain + * label list — the phone, call-mode narration, the sidebar preview — keeps + * working untouched. */ + optionHints?: Record; + /** the question takes more than one option; `answered` is then the chosen + * labels joined with ", ", which is the format the asking tool expects */ + multiSelect?: boolean; answered?: string; dismissed?: boolean; /** Present when this card is a live provider ask (approval/question). */ diff --git a/server/thread-events.test.ts b/server/thread-events.test.ts index c43f97e1b..ed3221332 100644 --- a/server/thread-events.test.ts +++ b/server/thread-events.test.ts @@ -99,6 +99,27 @@ describe("readThreadEvents", () => { expect(page.total).toEqual({ runtime: 3, native: 2 }); }); + it("keeps a question's choices, their explanations, and multi-select — and drops malformed ones", () => { + const eventsDir = tmp(); + const nativeDir = tmp(); + const base = { provider: "claude", threadId: "t1", type: "request.opened", requestType: "question", tool: "AskUserQuestion", summary: "Which framework?" }; + writeFileSync( + join(eventsDir, "t1.ndjson"), + line({ ...base, eventId: "rich", createdAt: "1", choices: ["React", "Vue"], choiceHints: { React: "already used" }, multiSelect: true }) + + // a hint map whose values are not strings is not the contract + line({ ...base, eventId: "bad-hints", createdAt: "2", choiceHints: { React: 7 } }) + + line({ ...base, eventId: "bad-multi", createdAt: "3", multiSelect: "yes" }), + ); + writeFileSync(join(nativeDir, "t1.ndjson"), ""); + const page = readThreadEvents({ eventsDir, nativeDir, threadId: "t1" }); + expect(page.entries.map((entry) => (entry.data as { eventId?: string }).eventId)).toEqual(["rich"]); + expect(page.entries[0]!.data).toMatchObject({ + choices: ["React", "Vue"], + choiceHints: { React: "already used" }, + multiSelect: true, + }); + }); + it("keeps walking backward when a corrupt tail record would otherwise consume the limit", () => { const eventsDir = tmp(); const nativeDir = tmp(); diff --git a/server/thread-events.ts b/server/thread-events.ts index ee7e8c6ab..cf523d966 100644 --- a/server/thread-events.ts +++ b/server/thread-events.ts @@ -159,6 +159,9 @@ const stringOrMissing = (value: unknown) => value === undefined || typeof value const stringOrNullOrMissing = (value: unknown) => value === undefined || value === null || typeof value === "string"; const numberOrNullOrMissing = (value: unknown) => value === undefined || value === null || typeof value === "number"; const stringsOrMissing = (value: unknown) => value === undefined || (Array.isArray(value) && value.every((item) => typeof item === "string")); +const booleanOrMissing = (value: unknown) => value === undefined || typeof value === "boolean"; +const stringRecordOrMissing = (value: unknown) => + value === undefined || (isRecord(value) && Object.values(value).every((item) => typeof item === "string")); function isRuntimeEvent(value: unknown): value is RuntimeEvent { if ( @@ -202,7 +205,9 @@ function isRuntimeEvent(value: unknown): value is RuntimeEvent { (value.requestType === "permission" || value.requestType === "question") && typeof value.tool === "string" && typeof value.summary === "string" && - stringsOrMissing(value.choices) + stringsOrMissing(value.choices) && + stringRecordOrMissing(value.choiceHints) && + booleanOrMissing(value.multiSelect) ); case "request.resolved": return ( diff --git a/src/components/GroupView.tsx b/src/components/GroupView.tsx index a28a02ee1..d83321eb9 100644 --- a/src/components/GroupView.tsx +++ b/src/components/GroupView.tsx @@ -20,6 +20,7 @@ import { effectiveDefaultResponder, groupResponseHint } from "@/lib/group-routin import { ChatMarkdown } from "./ChatMarkdown"; import { Composer } from "./Composer"; import { ConnectorCard } from "./ConnectorCard"; +import { OptionCard } from "./OptionCard"; import { GroupCallButton, GroupCallOverlay } from "./GroupCallView"; import { ReactionBar, ReactionChips } from "./Reactions"; import { ApprovalCard } from "./ApprovalCard"; @@ -117,6 +118,13 @@ const Transcript = memo(function Transcript({
+ ) : m.kind === "options" && m.card && m.from?.botId ? ( + // a QUESTION from a member. Without this branch the card fell + // through to null: invisible on screen, and the asking bot sat + // there until its 15-minute timeout answered for you +
+ +
) : m.kind === "activity" && m.tool ? (
([]); const card = message.card; + const chosen = useMemo(() => answeredLabels(card?.answered), [card?.answered]); if (!card || card.dismissed) return null; + const multi = card.multiSelect === true; const answer = (text: string) => { if (!text.trim()) return; - dispatch({ type: "answerCard", botId, messageId: message.id, answer: text.trim() }); + dispatch({ type: "answerCard", botId, messageId: message.id, answer: text.trim(), groupId }); }; + const send = () => answer(joinAnswers(card.options, picked)); + const toggle = (option: string) => + setPicked((current) => + current.includes(option) ? current.filter((item) => item !== option) : [...current, option], + ); return (
@@ -33,7 +45,7 @@ export function OptionCard({
+ {multi && !card.answered && ( +
Pick as many as apply, then send.
+ )} +
- {card.options.map((opt, i) => ( - - ))} + {card.options.map((opt, i) => { + const selected = card.answered ? chosen.has(opt) : picked.includes(opt); + const hint = card.optionHints?.[opt]; + return ( + + ); + })}
+ {multi && !card.answered && ( + + )} + {/* a permission ask has no free-text answer — the broker only accepts allow/deny, so typing here used to fail silently */} {!card.answered && !card.tool && ( diff --git a/src/lib/card-answer.test.ts b/src/lib/card-answer.test.ts new file mode 100644 index 000000000..0f6908d6d --- /dev/null +++ b/src/lib/card-answer.test.ts @@ -0,0 +1,63 @@ +import { describe, expect, it } from "vitest"; + +import { answerResponse, answeredLabels, dismissResponse, joinAnswers } from "./card-answer"; + +describe("answerResponse", () => { + it("answers a question with the chosen text", () => { + expect(answerResponse({}, "Software Engineer")).toEqual({ + behavior: "answer", + message: "Software Engineer", + }); + }); + + it("answers a question whose option is literally 'Allow' with the TEXT, not an allow", () => { + // the case the old label-matching rule got wrong: the broker refuses an + // allow on a question, the card closes unreachable, and the bot waits + // out its whole timeout + expect(answerResponse({}, "Allow")).toEqual({ behavior: "answer", message: "Allow" }); + expect(answerResponse({}, "Deny")).toEqual({ behavior: "answer", message: "Deny" }); + }); + + it("decides a permission card by its refusal, not by matching 'Allow'", () => { + expect(answerResponse({ tool: "Bash" }, "Allow")).toEqual({ behavior: "allow" }); + expect(answerResponse({ tool: "Bash" }, "Deny")).toEqual({ behavior: "deny" }); + // peer-approval cards offer this third option; the grant is written + // separately, so this one action is simply allowed + expect(answerResponse({ tool: "ask_bot" }, "Always allow")).toEqual({ behavior: "allow" }); + // a card that says "Yes" rather than "Allow" still lets the action run + expect(answerResponse({ tool: "Bash" }, "Yes")).toEqual({ behavior: "allow" }); + }); +}); + +describe("dismissResponse", () => { + it("denies a permission but ANSWERS a question", () => { + expect(dismissResponse({ tool: "Bash" })).toMatchObject({ behavior: "deny" }); + // a deny on a question is refused by the broker, so closing the card + // used to leave the bot waiting on a card that was already gone + expect(dismissResponse({})).toMatchObject({ behavior: "answer" }); + expect(dismissResponse({}).message).toContain("without answering"); + }); +}); + +describe("answeredLabels", () => { + it("reads a single answer and a comma-joined multi-select back", () => { + expect([...answeredLabels("React")]).toEqual(["React"]); + expect([...answeredLabels("Auth, Search")]).toEqual(["Auth", "Search"]); + expect([...answeredLabels(undefined)]).toEqual([]); + expect([...answeredLabels("")]).toEqual([]); + }); + + it("does not invent a label from stray separators", () => { + expect([...answeredLabels("Auth,,Search")]).toEqual(["Auth", "Search"]); + }); +}); + +describe("joinAnswers", () => { + it("joins in the card's option order, not click order", () => { + expect(joinAnswers(["Auth", "Search", "Billing"], ["Billing", "Auth"])).toBe("Auth, Billing"); + }); + + it("is empty when nothing is picked", () => { + expect(joinAnswers(["Auth"], [])).toBe(""); + }); +}); diff --git a/src/lib/card-answer.ts b/src/lib/card-answer.ts new file mode 100644 index 000000000..44123ec12 --- /dev/null +++ b/src/lib/card-answer.ts @@ -0,0 +1,57 @@ +// How an option card's answer reaches the harness. +// +// Two kinds of card share one component. A PERMISSION card answers +// allow/deny — it is a decision about an action. A QUESTION answers with the +// chosen text; the broker accepts nothing else for one, and refuses an +// allow/deny outright (server/drivers/claude.ts). Getting that wrong is not +// a cosmetic bug: the refused answer resolves `unavailable`, the card closes +// as unreachable, and the bot then waits out its whole 15-minute timeout. +// +// So the kind comes from the CARD, never from the label the user pressed. +// The old rule read the text — `answer === "Allow" ? "allow" : …` — which +// works only as long as no question ever offers an option called "Allow". +// A question's options are written by the model, so that is a matter of +// luck, and the CLI's own AskUserQuestion makes questions common. + +/** The only field that decides it: a permission card names its tool. */ +interface CardKind { + tool?: string; +} + +export interface CardResponse { + behavior: "allow" | "deny" | "answer"; + message?: string; +} + +/** The response for pressing one of a card's options. */ +export function answerResponse(card: CardKind, answer: string): CardResponse { + if (!card.tool) return { behavior: "answer", message: answer }; + // A permission card's own options are Allow / Deny / Always allow; the + // grant behind "always" is written separately, so anything that is not a + // refusal lets this one action through. + return { behavior: answer === "Deny" ? "deny" : "allow" }; +} + +/** The response for closing a card with its X. */ +export function dismissResponse(card: CardKind): CardResponse { + if (card.tool) return { behavior: "deny", message: "Dismissed by user." }; + // Closing a question is an answer — "I am not choosing" — not a denial. + return { + behavior: "answer", + message: "The user closed this question without answering. Use your best judgment and continue.", + }; +} + +/** The labels a settled card recorded. A multi-select answer is one + * comma-joined string (the format the asking tool expects), so reading the + * highlight back has to split it the same way it was written. */ +export function answeredLabels(answered?: string): Set { + if (!answered) return new Set(); + return new Set(answered.split(",").map((part) => part.trim()).filter(Boolean)); +} + +/** One answer string from a multi-select, in the card's own option order — + * not click order, so the answer reads the way the question was written. */ +export function joinAnswers(options: string[], picked: string[]): string { + return options.filter((option) => picked.includes(option)).join(", "); +} diff --git a/src/state/store.tsx b/src/state/store.tsx index 812d1fd0f..883f6acca 100644 --- a/src/state/store.tsx +++ b/src/state/store.tsx @@ -18,6 +18,7 @@ import type { MausColor, MausMotion } from "@/lib/mascot"; import type { BotAvatarCrop } from "../../shared/bot-avatar"; import type { Routine, RoutineInput, RoutineRun } from "@/lib/routines"; import type { WebhookAttempt, WebhookIngressStatus, WebhookTrigger } from "@/lib/webhooks"; +import { answerResponse, dismissResponse } from "@/lib/card-answer"; import { currentCall } from "@/lib/call"; import { showNotification, type NotificationTarget } from "@/lib/notify"; import { speaker } from "@/lib/tts"; @@ -29,6 +30,15 @@ export interface OptionCardData { title: string; subtitle: string; options: string[]; + /** what each option means, keyed by its label — a question that came with + * explanations (AskUserQuestion) shows them under the buttons. Kept beside + * `options` rather than inside it so every existing reader of the plain + * label list — the phone, call-mode narration, the sidebar preview — keeps + * working untouched. */ + optionHints?: Record; + /** the question takes more than one option; `answered` is then the chosen + * labels joined with ", ", which is the format the asking tool expects */ + multiSelect?: boolean; answered?: string; dismissed?: boolean; /** Present when this card is a live provider ask (approval/question). */ @@ -398,8 +408,10 @@ export type Action = | { type: "editMessage"; botId: string; messageId: string; text: string } | { type: "switchBranch"; botId: string; messageId: string } | { type: "threadActive"; threadId: string; activeLeafId: string } - | { type: "answerCard"; botId: string; messageId: string; answer: string } - | { type: "dismissCard"; botId: string; messageId: string } + // `groupId` when the card is in a room: the message lives on the room's + // list, and the answer goes to the room's thread + | { type: "answerCard"; botId: string; messageId: string; answer: string; groupId?: string } + | { type: "dismissCard"; botId: string; messageId: string; groupId?: string } // permission cards answer by THREAD, so a request raised inside a room // can be answered the same way as one in a 1:1 chat | { @@ -486,13 +498,21 @@ function withMascotMotion( }; } +function withPatchedCard(messages: Message[], messageId: string, patch: Partial): Message[] { + return messages.map((m) => (m.id === messageId && m.card ? { ...m, card: { ...m.card, ...patch } } : m)); +} + function patchCard(state: AppState, botId: string, messageId: string, patch: Partial): AppState { - return updateBot(state, botId, (b) => ({ - ...b, - messages: b.messages.map((m) => - m.id === messageId && m.card ? { ...m, card: { ...m.card, ...patch } } : m, + return updateBot(state, botId, (b) => ({ ...b, messages: withPatchedCard(b.messages, messageId, patch) })); +} + +function patchGroupCard(state: AppState, groupId: string, messageId: string, patch: Partial): AppState { + return { + ...state, + groups: state.groups.map((g) => + g.id === groupId ? { ...g, messages: withPatchedCard(g.messages, messageId, patch) } : g, ), - })); + }; } export function reducer(state: AppState, action: Action): AppState { @@ -595,12 +615,14 @@ export function reducer(state: AppState, action: Action): AppState { } // optimistic card settle; the server's message.patch confirms it later case "answerCard": + if (action.groupId) return patchGroupCard(state, action.groupId, action.messageId, { answered: action.answer }); return withMascotMotion( patchCard(state, action.botId, action.messageId, { answered: action.answer }), action.botId, "working", ); case "dismissCard": + if (action.groupId) return patchGroupCard(state, action.groupId, action.messageId, { dismissed: true }); return patchCard(state, action.botId, action.messageId, { dismissed: true }); case "decideRequest": return state; // the server's request.resolved patch settles the card @@ -1070,6 +1092,14 @@ export function StoreProvider({ children }: { children: ReactNode }) { rawDispatch({ type: "error", message: e instanceof Error ? e.message : String(e) }); setTimeout(() => rawDispatch({ type: "error", message: null }), 6000); }; + /** Where a card action's message lives, and the card on it. A card asked + * inside a room belongs to the room's list, never to one member's. */ + const cardTarget = (action: { botId: string; messageId: string; groupId?: string }) => { + const host = action.groupId + ? stateRef.current.groups.find((g) => g.id === action.groupId) + : stateRef.current.bots.find((b) => b.id === action.botId); + return { host, card: host?.messages.find((m) => m.id === action.messageId)?.card, inRoom: !!action.groupId }; + }; // fire-and-forget card persistence; the route is optional server-side const persistCard = (botId: string, messageId: string, patch: Partial) => { fetch(`/api/bots/${botId}/cards/${messageId}`, { @@ -1156,20 +1186,20 @@ export function StoreProvider({ children }: { children: ReactNode }) { break; } case "answerCard": { - const bot = stateRef.current.bots.find((b) => b.id === action.botId); - const card = bot?.messages.find((m) => m.id === action.messageId)?.card; - if (card?.requestId) { - const behavior = - action.answer === "Allow" ? "allow" : action.answer === "Deny" ? "deny" : "answer"; - api(`/api/bots/${action.botId}/respond`, { + const { host, card, inRoom } = cardTarget(action); + if (card?.requestId && host) { + // allow/deny for a permission, the chosen text for a question — + // decided from the card, never from the label (see card-answer.ts). + // by THREAD, so a card raised inside a room answers the same way + // a 1:1 one does + api(`/api/threads/${host.threadId}/respond`, { method: "POST", body: JSON.stringify({ requestId: card.requestId, - behavior, - message: behavior === "answer" ? action.answer : undefined, + ...answerResponse(card, action.answer), }), }).catch(showError); - } else { + } else if (!inRoom) { persistCard(action.botId, action.messageId, { answered: action.answer }); api(`/api/bots/${action.botId}/messages`, { method: "POST", @@ -1179,14 +1209,15 @@ export function StoreProvider({ children }: { children: ReactNode }) { break; } case "dismissCard": { - const bot = stateRef.current.bots.find((b) => b.id === action.botId); - const card = bot?.messages.find((m) => m.id === action.messageId)?.card; - if (card?.requestId) { - api(`/api/bots/${action.botId}/respond`, { + const { host, card, inRoom } = cardTarget(action); + if (card?.requestId && host) { + // a question is DECLINED rather than denied — the broker refuses + // a deny on one (see card-answer.ts) + api(`/api/threads/${host.threadId}/respond`, { method: "POST", - body: JSON.stringify({ requestId: card.requestId, behavior: "deny", message: "Dismissed by user." }), + body: JSON.stringify({ requestId: card.requestId, ...dismissResponse(card) }), }).catch(() => {}); - } else { + } else if (!inRoom) { persistCard(action.botId, action.messageId, { dismissed: true }); } break;