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;