From 1bc4a3b2e49849086ae8e6c3f4e6fed00cdca795 Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 14:44:30 -0300 Subject: [PATCH 1/6] Open the Bot's screen only for work there is something to watch The live screen was announced before every call to a Bot's computer. Two kinds of call had nothing to show for it: one that touches files or runs a command, which never reaches the screen, and one a policy refuses, which is the Bot's intent rather than anything that happened. Asking a Bot a question it answered in words could therefore open its computer on a still page. Off-screen paths no longer announce themselves, as a list of exceptions rather than of permits: a browser path added later and never listed goes on announcing, so forgetting the list opens a pane rather than silently stopping one. A refusal now withdraws its announcement. Retraction is per announcement rather than per run, because a Bot that opens a page and then has a click refused made both calls in one run, and closing on the run would shut a pane showing the page the first call loaded. An action that was permitted and failed keeps its screen: the page may have moved. The route's effect moves into useAutoWatchScreen so the rules can be stated in tests without a router, a channel and a transcript around them. --- .../computer/use-auto-watch-screen.ts | 77 ++++++++ app/src/lib/computers/call.ts | 33 +++- app/src/lib/copilot/computer-activity.ts | 55 +++++- .../_authed/_app/channel/$channelId.tsx | 35 ++-- app/tests/auto-watch-screen.test.tsx | 166 ++++++++++++++++++ .../computer-screen-announcement.test.ts | 135 ++++++++++++++ 6 files changed, 476 insertions(+), 25 deletions(-) create mode 100644 app/src/components/computer/use-auto-watch-screen.ts create mode 100644 app/tests/auto-watch-screen.test.tsx create mode 100644 app/tests/computer-screen-announcement.test.ts diff --git a/app/src/components/computer/use-auto-watch-screen.ts b/app/src/components/computer/use-auto-watch-screen.ts new file mode 100644 index 000000000..b3195ba95 --- /dev/null +++ b/app/src/components/computer/use-auto-watch-screen.ts @@ -0,0 +1,77 @@ +import { useEffect, useRef } from "react"; +import { + onComputerActivity, + onComputerActivityWithdrawn, +} from "@/lib/copilot/computer-activity"; + +/** + * The live screen opening itself beside a conversation, once per run, and closing again when the + * call that opened it turns out to have been refused. + * + * A HOOK RATHER THAN AN EFFECT IN THE ROUTE, because what it decides is worth stating in tests + * without a router, a channel and a chat transcript around it. The route keeps what is genuinely + * its own: the pane is a URL flag it shares with the Bot's profile. + * + * The person's pane comes first. It does not open over something they opened (`busy`), it does not + * reopen what they are already watching, and a run they closed stays closed for the rest of it. + */ +export function useAutoWatchScreen({ + botId, + watching, + busy, + open, + close, +}: { + botId: string | undefined; + /** The pane is currently showing this Bot's screen. */ + watching: boolean; + /** The pane is showing something else the person asked for, which auto-open must not take. */ + busy: boolean; + open: () => void; + close: () => void; +}): { dismiss: () => void } { + /** The run the Bot is in, so a dismissal can name the one it applies to. */ + const run = useRef(null); + const dismissed = useRef(null); + /** + * The announcement this pane opened for, or null when it opened for nothing. + * + * Only this call may close it again. A pane the person opened never has one, and a pane showing + * a page some earlier call loaded holds that call's announcement rather than a later one's. + */ + const openedFor = useRef(null); + + // Read inside subscriptions that outlive the render they were made in. + const latest = useRef({ watching, busy, open, close }); + latest.current = { watching, busy, open, close }; + + useEffect(() => { + if (!botId) return; + const stopActivity = onComputerActivity((activity) => { + if (activity.botId !== botId) return; + run.current = activity.epoch; + if (dismissed.current === activity.epoch) return; + const { watching: isWatching, busy: isBusy, open: show } = latest.current; + if (isWatching || isBusy) return; + openedFor.current = activity.announcement; + show(); + }); + const stopWithdrawals = onComputerActivityWithdrawn((withdrawal) => { + if (withdrawal.botId !== botId) return; + if (openedFor.current !== withdrawal.announcement) return; + openedFor.current = null; + latest.current.close(); + }); + return () => { + stopActivity(); + stopWithdrawals(); + }; + }, [botId]); + + return { + dismiss: () => { + dismissed.current = run.current; + openedFor.current = null; + }, + }; +} diff --git a/app/src/lib/computers/call.ts b/app/src/lib/computers/call.ts index d28fc77ea..44ce4f1bc 100644 --- a/app/src/lib/computers/call.ts +++ b/app/src/lib/computers/call.ts @@ -1,8 +1,29 @@ import { tryClient } from "@/lib/client"; -import { reportComputerActivity } from "@/lib/copilot/computer-activity"; +import { + reportComputerActivity, + withdrawComputerActivity, +} from "@/lib/copilot/computer-activity"; export type ToolOutcome = Record & { ok: boolean }; +/** + * The calls that never reach the Bot's screen. + * + * Reading a file or running a command happens on the computer, not on the display, so announcing + * them opened the live screen on whatever still page was already there — a pane that arrived for an + * action nobody could see, which is indistinguishable from one that arrived for no reason. + * + * A LIST OF EXCEPTIONS, NOT A LIST OF PERMITS, and deliberately so: a browser path added later and + * never added here goes on announcing itself, which is what every browser path already does. The + * failure mode of forgetting this list is a pane that opens, not one that silently stops. + */ +const OFF_SCREEN_PATHS = new Set([ + "/files/list", + "/files/read", + "/files/write", + "/exec", +]); + /** * Exported for the test that covers what a Bot is told when a call is refused. * @@ -21,7 +42,7 @@ export async function callComputer( signal?: AbortSignal, ): Promise { // Announce before the call so the screen can open while the action is running. - reportComputerActivity(botId); + const run = OFF_SCREEN_PATHS.has(path) ? null : reportComputerActivity(botId); let response: Response; try { response = await tryClient(`/api/computers/${botId}${path}`, { @@ -47,6 +68,14 @@ export async function callComputer( > | null; if (!response.ok) { + /* + * A REFUSAL IS AN INTENTION, NOT AN ACTION. 403 is a policy, a capability switch or a network + * rule saying no, and it says so before the browser is touched: there is nothing on the screen + * to see, because nothing was done to it. Every other failure reached the computer and may have + * moved it, so those keep the screen they opened. + */ + if (response.status === 403 && run !== null) + withdrawComputerActivity(botId, run); return { ok: false, reason: (body?.error as string) ?? "That did not work.", diff --git a/app/src/lib/copilot/computer-activity.ts b/app/src/lib/copilot/computer-activity.ts index 4bd05c4ee..024e8cdd7 100644 --- a/app/src/lib/copilot/computer-activity.ts +++ b/app/src/lib/copilot/computer-activity.ts @@ -4,22 +4,60 @@ const QUIET_GAP_MS = 10_000; -type Activity = { botId: string; epoch: number }; +type Activity = { + botId: string; + /** The run, for surfaces that open once per run and honour a dismissal for the rest of it. */ + epoch: number; + /** + * This call, for taking an announcement back. + * + * A RUN IS TOO COARSE TO RETRACT. A Bot that opens a page and then has a click refused made both + * calls in one run; closing on the run would shut a pane that is showing the page the first call + * loaded. A pane remembers the announcement that opened it, so only that call can close it. + */ + announcement: number; +}; let epoch = 0; +let announcement = 0; let lastAt = 0; let lastBot: string | null = null; const listeners = new Set<(activity: Activity) => void>(); +const withdrawals = new Set<(withdrawal: Withdrawal) => void>(); -/** Called wherever a Bot's computer is reached, so auto-open surfaces share one activity source. */ -export function reportComputerActivity(botId: string): void { +type Withdrawal = { botId: string; announcement: number }; + +/** + * Called wherever a Bot's computer is reached, so auto-open surfaces share one activity source. + * + * Returns this call's announcement, which is what a caller hands + * {@link withdrawComputerActivity} when the action turns out not to have happened. + */ +export function reportComputerActivity(botId: string): number { const now = Date.now(); // A different Bot is always a new run, however recently the last one acted. if (botId !== lastBot || now - lastAt > QUIET_GAP_MS) epoch += 1; lastAt = now; lastBot = botId; - const activity = { botId, epoch }; + announcement += 1; + const activity = { botId, epoch, announcement }; for (const listener of listeners) listener(activity); + return announcement; +} + +/** + * Take an announcement back: the action it was made for was refused, so nothing happened. + * + * The announcement goes out BEFORE the request, because a page is worth watching while it loads, + * and that is the whole reason this exists. A refusal arrives after it, by which point a pane has + * already opened on a screen the Bot never touched. + */ +export function withdrawComputerActivity( + botId: string, + forAnnouncement: number, +): void { + const withdrawal = { botId, announcement: forAnnouncement }; + for (const listener of withdrawals) listener(withdrawal); } export function onComputerActivity( @@ -30,3 +68,12 @@ export function onComputerActivity( listeners.delete(listener); }; } + +export function onComputerActivityWithdrawn( + listener: (withdrawal: Withdrawal) => void, +): () => void { + withdrawals.add(listener); + return () => { + withdrawals.delete(listener); + }; +} diff --git a/app/src/routes/_authed/_app/channel/$channelId.tsx b/app/src/routes/_authed/_app/channel/$channelId.tsx index 5bc28436b..4e60ec43a 100644 --- a/app/src/routes/_authed/_app/channel/$channelId.tsx +++ b/app/src/routes/_authed/_app/channel/$channelId.tsx @@ -2,7 +2,6 @@ import { IconSettings } from "@tabler/icons-react"; import { useQuery } from "@tanstack/react-query"; import { createFileRoute, Navigate } from "@tanstack/react-router"; import { motion, useReducedMotion } from "motion/react"; -import { useEffect, useRef } from "react"; import { z } from "zod"; import { AgentProfile } from "@/components/agents/agent-profile"; import { BotPausedBanner } from "@/components/bot-profile/pause-banner"; @@ -10,12 +9,12 @@ import { ChannelAvatar } from "@/components/channels/avatar"; import { ChannelChat } from "@/components/channels/channel-chat"; import { ComputerChatControls } from "@/components/computer/computer-controls"; import { ComputerViewPanel } from "@/components/computer/computer-panel"; +import { useAutoWatchScreen } from "@/components/computer/use-auto-watch-screen"; import { DetailPanel } from "@/components/layout/detail-panel"; import { SidebarToggle } from "@/components/layout/sidebar-toggle"; import { Button } from "@/components/ui/button"; import { useMarkOpenChannelRead } from "@/lib/channels/mark-read"; import { type AgentChannel, channelQueryOptions } from "@/lib/channels/queries"; -import { onComputerActivity } from "@/lib/copilot/computer-activity"; const chatSearchSchema = z.object({ settings: z.boolean().optional(), @@ -50,28 +49,26 @@ function RouteComponent() { useMarkOpenChannelRead(channelId); // Browser activity may auto-open the screen once per run unless this run was dismissed. - const dismissedEpoch = useRef(null); - const runEpoch = useRef(null); - useEffect(() => { - if (!agentId) return; - return onComputerActivity((activity) => { - if (activity.botId !== agentId) return; - runEpoch.current = activity.epoch; - if (dismissedEpoch.current === activity.epoch) return; + const autoWatch = useAutoWatchScreen({ + botId: agentId, + watching: isWatching, + busy: isSettingsOpen, + open: () => navigate({ - search: (previous) => - previous.watch === true || previous.settings === true - ? previous - : { ...previous, settings: undefined, watch: true }, - }); - }); - }, [agentId, navigate]); + search: (previous) => ({ + ...previous, + settings: undefined, + watch: true, + }), + }), + close: () => + navigate({ search: (previous) => ({ ...previous, watch: undefined }) }), + }); // Settings and watch share one pane; opening either clears the other URL flag. const show = (next: "settings" | "watch" | null) => { // Dismissal applies only to the current browser-activity run. - if (next !== "watch" && isWatching) - dismissedEpoch.current = runEpoch.current; + if (next !== "watch" && isWatching) autoWatch.dismiss(); return navigate({ search: (previous) => ({ ...previous, diff --git a/app/tests/auto-watch-screen.test.tsx b/app/tests/auto-watch-screen.test.tsx new file mode 100644 index 000000000..aec164cec --- /dev/null +++ b/app/tests/auto-watch-screen.test.tsx @@ -0,0 +1,166 @@ +import { + afterAll, + afterEach, + beforeAll, + describe, + expect, + test, +} from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { useAutoWatchScreen } from "@/components/computer/use-auto-watch-screen"; +import { + reportComputerActivity, + withdrawComputerActivity, +} from "@/lib/copilot/computer-activity"; +import { settleReactWork } from "./settle-react-work"; + +/** + * When a Bot's screen opens itself beside the conversation, and when it closes again. + * + * The pane is shared with the Bot's profile and it is the person's pane first: it opens for work + * they can watch, it does not steal the pane from something they opened, and a run they dismissed + * stays dismissed until the next one. + */ + +beforeAll(() => GlobalRegistrator.register()); +afterEach(cleanup); +afterAll(async () => { + await settleReactWork(); + GlobalRegistrator.unregister(); +}); + +function paneFor(options: { watching?: boolean; busy?: boolean } = {}) { + const opened: number[] = []; + const closed: number[] = []; + let watching = options.watching ?? false; + const view = renderHook( + (props: { watching: boolean }) => + useAutoWatchScreen({ + botId: "bot-1", + watching: props.watching, + busy: options.busy ?? false, + open: () => { + opened.push(1); + watching = true; + view.rerender({ watching }); + }, + close: () => { + closed.push(1); + watching = false; + view.rerender({ watching }); + }, + }), + { initialProps: { watching } }, + ); + return { + opened, + closed, + /** What closing the pane by hand does: the pane shuts, and this run stops auto-opening. */ + dismiss: () => + act(() => { + view.result.current.dismiss(); + watching = false; + view.rerender({ watching }); + }), + }; +} + +const announce = (botId = "bot-1") => { + let announcement = 0; + act(() => { + announcement = reportComputerActivity(botId); + }); + return announcement; +}; + +describe("a Bot reaching its computer", () => { + test("opens the screen beside the conversation", () => { + const pane = paneFor(); + + announce(); + + expect(pane.opened).toHaveLength(1); + }); + + test("does not take a pane the person opened for something else", () => { + const pane = paneFor({ busy: true }); + + announce(); + + expect(pane.opened).toHaveLength(0); + }); + + test("does not reopen a screen already being watched", () => { + const pane = paneFor({ watching: true }); + + announce(); + + expect(pane.opened).toHaveLength(0); + }); +}); + +describe("an announcement taken back", () => { + test("closes the screen it opened", () => { + const pane = paneFor(); + const announcement = announce(); + + act(() => withdrawComputerActivity("bot-1", announcement)); + + expect(pane.closed).toHaveLength(1); + }); + + test("leaves a screen some other call opened", () => { + const pane = paneFor(); + const opening = announce(); + + // A later refused call in the same run: its announcement is not the one holding the pane open. + act(() => withdrawComputerActivity("bot-1", opening + 99)); + + expect(pane.closed).toHaveLength(0); + }); + + test("leaves a screen the person was already watching", () => { + const pane = paneFor({ watching: true }); + const announcement = announce(); + + act(() => withdrawComputerActivity("bot-1", announcement)); + + // Nothing auto-opened, so there is nothing to take back. + expect(pane.closed).toHaveLength(0); + }); +}); + +describe("a run the person dismissed", () => { + test("does not open again for the rest of it", () => { + const pane = paneFor(); + announce(); + pane.dismiss(); + + announce(); + + expect(pane.opened).toHaveLength(1); + }); + + test("stops being dismissed once a new run starts", () => { + const pane = paneFor(); + announce(); + pane.dismiss(); + + // Another Bot acting ends this Bot's run, so its next action is a new one. + announce("other-bot"); + announce(); + + expect(pane.opened).toHaveLength(2); + }); +}); + +describe("another Bot's computer", () => { + test("is not this conversation's business", () => { + const pane = paneFor(); + + announce("other-bot"); + + expect(pane.opened).toHaveLength(0); + }); +}); diff --git a/app/tests/computer-screen-announcement.test.ts b/app/tests/computer-screen-announcement.test.ts new file mode 100644 index 000000000..fe3c6e8d9 --- /dev/null +++ b/app/tests/computer-screen-announcement.test.ts @@ -0,0 +1,135 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { callComputer } from "@/lib/computers/call"; +import { + onComputerActivity, + onComputerActivityWithdrawn, +} from "@/lib/copilot/computer-activity"; + +/** + * Which computer calls open the live screen, and which take it back. + * + * The screen used to open for every call to a Bot's computer, announced before the request went out + * so a page could be watched while it loaded. Two kinds of call have nothing to show for that: one + * that touches files or runs a command, which never reaches the screen at all, and one a policy + * refuses, which is the Bot's intent rather than anything that happened. Both left a person looking + * at a still screen for an action that was not there. + */ + +const realFetch = globalThis.fetch; + +type Seen = { botId: string; announcement: number }; + +let opened: Seen[] = []; +let withdrawn: Seen[] = []; +let stop: (() => void)[] = []; + +beforeEach(() => { + opened = []; + withdrawn = []; + stop = [ + onComputerActivity(({ botId, announcement }) => + opened.push({ botId, announcement }), + ), + onComputerActivityWithdrawn((withdrawal) => + withdrawn.push({ ...withdrawal }), + ), + ]; +}); + +afterEach(() => { + for (const off of stop) off(); + globalThis.fetch = realFetch; +}); + +function serverAnswering(status: number, body: unknown) { + globalThis.fetch = (async () => + new Response(body === undefined ? null : JSON.stringify(body), { + status, + headers: { "content-type": "application/json" }, + })) as unknown as typeof fetch; +} + +describe("a call that happens on the screen", () => { + test("announces itself, so the page can be watched while it loads", async () => { + serverAnswering(200, { url: "https://example.com", title: "Example" }); + + await callComputer("bot-navigate", "/navigate", { + method: "POST", + body: { url: "https://example.com" }, + }); + + expect(opened.map((seen) => seen.botId)).toEqual(["bot-navigate"]); + expect(withdrawn).toEqual([]); + }); + + test("still announces when it fails, because the page may have moved", async () => { + // Not a refusal: the action was permitted, reached the browser and did not work. What is on + // screen after it is unknown, so the screen stays the honest place to look. + serverAnswering(500, { error: "That did not work." }); + + await callComputer("bot-failed", "/click", { method: "POST" }); + + expect(opened.map((seen) => seen.botId)).toEqual(["bot-failed"]); + expect(withdrawn).toEqual([]); + }); +}); + +describe("a call a policy refused", () => { + test("takes its announcement back, because nothing happened", async () => { + serverAnswering(403, { + error: "That is not allowed here.", + rule: 'url.host == "notion.so"', + }); + + const outcome = await callComputer("bot-refused", "/navigate", { + method: "POST", + body: { url: "https://notion.so" }, + }); + + expect(outcome.refused).toBe(true); + expect(opened.map((seen) => seen.botId)).toEqual(["bot-refused"]); + // This exact announcement, so only a pane that opened for it closes again. + expect(withdrawn).toEqual([ + { botId: "bot-refused", announcement: opened[0]?.announcement ?? -1 }, + ]); + }); + + test("leaves a pane that a successful call in the same run opened", async () => { + serverAnswering(200, { url: "https://example.com" }); + await callComputer("bot-mixed", "/navigate", { method: "POST" }); + + serverAnswering(403, { error: "That is not allowed here." }); + await callComputer("bot-mixed", "/click", { method: "POST" }); + + // Two announcements, and the one withdrawn is the refused click rather than the page. + expect(opened).toHaveLength(2); + expect(withdrawn).toEqual([ + { botId: "bot-mixed", announcement: opened[1]?.announcement ?? -1 }, + ]); + expect(withdrawn[0]?.announcement).not.toBe(opened[0]?.announcement); + }); +}); + +describe("a call that never reaches the screen", () => { + test.each(["/files/list", "/files/read", "/files/write", "/exec"])( + "%s does not open it", + async (path) => { + serverAnswering(200, { ok: true }); + + await callComputer("bot-offscreen", path, { method: "POST" }); + + expect(opened).toEqual([]); + expect(withdrawn).toEqual([]); + }, + ); + + test("an unrecognised path still opens it", async () => { + // The default has to stay "announce": a browser action added later and not listed here should + // behave as every browser action does, rather than silently stop opening the screen. + serverAnswering(200, { ok: true }); + + await callComputer("bot-unknown", "/something-new", { method: "POST" }); + + expect(opened.map((seen) => seen.botId)).toEqual(["bot-unknown"]); + }); +}); From 20c716ae5a895b0df96f50b603d9c9fba15ec92a Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 14:44:37 -0300 Subject: [PATCH 2/6] Stop offering computer tools this deployment would refuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Admin → Controls already decided whether anybody may drive a Bot's browser, use its files and shell, or reach the network, and the computer boundary enforced all three on every computer_* call. The app asked only the other question — whose computer it is — so a Bot was handed computer_navigate with cloudBrowser switched off, reached for it, was refused, and spent the turn saying so. computerToolsOffered mirrors the server's own split in decideFromSnapshot, which is by intent rather than by tool: files and commands need cloudComputer, everything else on the computer needs cloudBrowser, and opening a page needs cloudNetwork on top. Unresolved reads as allowed in both halves, since the server is the one that decides and this only declines to offer what it would certainly refuse. report_refusal stays on the ownership question alone: recording that a Bot declined something is audit bookkeeping rather than a computer action. --- app/src/lib/copilot/computer-available.ts | 75 +++++++++++++++++-- app/src/lib/copilot/computer-tools.tsx | 38 ++++++---- app/src/lib/copilot/sign-in-tool.tsx | 6 +- app/src/lib/enterprise/queries.ts | 26 +++++++ app/tests/computer-tools-offered.test.ts | 91 +++++++++++++++++++++++ 5 files changed, 213 insertions(+), 23 deletions(-) create mode 100644 app/tests/computer-tools-offered.test.ts diff --git a/app/src/lib/copilot/computer-available.ts b/app/src/lib/copilot/computer-available.ts index fc6f976db..65a75e727 100644 --- a/app/src/lib/copilot/computer-available.ts +++ b/app/src/lib/copilot/computer-available.ts @@ -1,14 +1,66 @@ import { useQuery } from "@tanstack/react-query"; import { agentQueryOptions } from "@/lib/agents/queries"; +import { + type CapabilityKey, + myCapabilitiesQueryOptions, +} from "@/lib/enterprise/queries"; import { useDeclaredBotId } from "./active-bot"; +/** Which computer tools this conversation offers the Bot. */ +export type ComputerToolsOffered = { + /** Acting on the page that is open: reading it, clicking, typing, scrolling. */ + browser: boolean; + /** Opening a page, which is the browser plus the network. */ + navigate: boolean; + /** The computer behind the screen: its files and its shell. */ + computer: boolean; +}; + +/** + * Whether this chat may use the Bot's computer, and which parts of it. + * + * TWO QUESTIONS, NOT ONE. The first is whose computer it is: the computer carries its owner's + * signed-in browser, and these tools run from this browser on the person's own session, so the + * server refuses every computer route (and every sign-in into it) to a Team Bot's teammates. The + * second is what the deployment allows anybody at all — the capability switches an administrator + * set, which the computer boundary enforces on every `computer_*` call in `decideFromSnapshot`. + * + * MIRRORING THAT SERVER-SIDE SPLIT IS THE POINT, and it is a split by intent rather than by tool: + * files and commands need `cloudComputer`, everything else on the computer needs `cloudBrowser`, + * and opening a page needs `cloudNetwork` on top. A Bot offered a tool whose capability is off does + * not fail safe into silence — it reaches for the tool, gets a refusal, and spends the turn saying + * so, which is how asking a Bot a question about an app it holds no tools for ended with its screen + * opening on nothing. + * + * Pure, and separated from the hook below, because every one of those rules is worth stating in a + * test without a router, a query client and a profile fetch around it. + */ +export function computerToolsOffered({ + profile, + capabilities, +}: { + /** Undefined while it loads: the server is the one that decides, so unresolved offers. */ + profile: { visibility: string; canManage: boolean } | undefined; + capabilities: Partial> | undefined; +}): ComputerToolsOffered { + const mine = !profile || profile.visibility === "public" || profile.canManage; + // An unresolved switch reads as allowed, for the same reason an unresolved profile does. + const allows = (capability: CapabilityKey) => + capabilities?.[capability] !== false; + const browser = mine && allows("cloudBrowser"); + return { + browser, + navigate: browser && allows("cloudNetwork"), + computer: mine && allows("cloudComputer"), + }; +} + /** - * Whether this chat may use the Bot's computer: a public Bot, or one this person owns or administers. + * Whether this browser may drive the Bot's computer at all, capabilities aside. * - * Not a Team Bot's teammates. The computer carries its owner's signed-in browser, and the computer - * tools run from this browser on the person's own session, so the server refuses every computer - * route (and every sign-in into it) to anyone else. True while the profile loads: the server is the - * one that decides, and this only stops offering tools that would be refused. + * The first of the two questions above, on its own. `report_refusal` rides on it: recording that a + * Bot declined something is audit bookkeeping rather than a computer action, so it stays offered + * wherever the Bot is this person's to drive, whatever the computer switches say. */ export function useComputerAvailable(): boolean { const declared = useDeclaredBotId(); @@ -18,3 +70,16 @@ export function useComputerAvailable(): boolean { }); return !profile || profile.visibility === "public" || profile.canManage; } + +export function useComputerToolsOffered(): ComputerToolsOffered { + const declared = useDeclaredBotId(); + const { data: profile } = useQuery({ + ...agentQueryOptions(declared ?? ""), + enabled: Boolean(declared), + }); + const { data: mine } = useQuery(myCapabilitiesQueryOptions()); + return computerToolsOffered({ + profile, + capabilities: mine?.capabilities, + }); +} diff --git a/app/src/lib/copilot/computer-tools.tsx b/app/src/lib/copilot/computer-tools.tsx index 33e62bb40..2e6d2ccc6 100644 --- a/app/src/lib/copilot/computer-tools.tsx +++ b/app/src/lib/copilot/computer-tools.tsx @@ -13,7 +13,10 @@ import { } from "@/lib/computers/handoff"; import { useActiveBotHolder } from "./active-bot"; import { useFrontendTool } from "./approval-tools"; -import { useComputerAvailable } from "./computer-available"; +import { + useComputerAvailable, + useComputerToolsOffered, +} from "./computer-available"; /** * Frontend registrations for computer tools, including inline rendering and policy-refusal display. @@ -184,11 +187,16 @@ function didNotWork(outcome: ComputerOutcome): boolean { export function ComputerTools() { const bot = useActiveBotHolder(); - // Not offered to a Team Bot's teammates; see useComputerAvailable. + /* + * Not offered to a Team Bot's teammates, and not offered where this deployment's capability + * switches would refuse them; see computerToolsOffered. + */ + const offered = useComputerToolsOffered(); + // Audit bookkeeping rather than a computer action, so it answers only the first question. const available = useComputerAvailable(); useFrontendTool({ - available, + available: offered.navigate, name: "computer_navigate", description: "Open a web page on your own computer so the person can watch. Use this when asked to look " + @@ -257,7 +265,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_read", description: "Read the page currently open on your computer, without opening anything. Use this after you " + @@ -274,7 +282,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_snapshot", description: "List the things on the current page you can act on: fields, buttons, links and checkboxes, " + @@ -308,7 +316,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_type", description: "Enter text into a field on the page. Give the ref of the field from your most recent " + @@ -359,7 +367,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_click", description: "Click something on the page: a button, a link, a checkbox or a radio option. Give the ref " + @@ -406,7 +414,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_key", description: "Press a key, such as Enter, Tab or Escape. Give a ref to press it while a particular field " + @@ -448,7 +456,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_request_secret", description: "Ask the person for ONE value you must not be told: a password, a one-time code, a card number. " + @@ -546,7 +554,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_request_help", description: "Ask the person to take control of your computer and do something you cannot: sign in, enter a " + @@ -575,7 +583,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.computer, name: "computer_list_files", description: "List what is in your workspace: every file and folder you have saved, with sizes. Call this " + @@ -623,7 +631,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.computer, name: "computer_read_file", description: "Read a file you saved earlier in your own workspace. Paths are relative to your workspace, " + @@ -669,7 +677,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.computer, name: "computer_run_command", description: "Run a shell command on your own computer. Use this for anything the browser cannot do: " + @@ -755,7 +763,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.computer, name: "computer_write_file", description: "Save a file in your own workspace so you still have it later. Paths are relative to your " + @@ -823,7 +831,7 @@ export function ComputerTools() { }); useFrontendTool({ - available, + available: offered.browser, name: "computer_scroll", description: "Scroll the page down, or up with a negative amount, to bring more of a long page into view.", diff --git a/app/src/lib/copilot/sign-in-tool.tsx b/app/src/lib/copilot/sign-in-tool.tsx index 5ed2eb683..621e99bcc 100644 --- a/app/src/lib/copilot/sign-in-tool.tsx +++ b/app/src/lib/copilot/sign-in-tool.tsx @@ -21,7 +21,7 @@ import { } from "@/lib/passwords/queries"; import { useActiveBotHolder } from "./active-bot"; import { useFrontendTool } from "./approval-tools"; -import { useComputerAvailable } from "./computer-available"; +import { useComputerToolsOffered } from "./computer-available"; /** How long the open chat waits for the person to answer. The server's request lasts as long. */ const WAIT_MS = 15 * 60_000; @@ -67,8 +67,8 @@ export function SignInTool() { const bot = useActiveBotHolder(); // Stable for the provider's life, so the SDK's once-registered handler may keep it. const queryClient = useQueryClient(); - // A sign-in is typed into the Bot's computer; see useComputerAvailable. - const available = useComputerAvailable(); + // A sign-in is typed into the Bot's browser; see computerToolsOffered. + const available = useComputerToolsOffered().browser; useFrontendTool({ available, name: "computer_request_sign_in", diff --git a/app/src/lib/enterprise/queries.ts b/app/src/lib/enterprise/queries.ts index 8829e0e00..7e58b5331 100644 --- a/app/src/lib/enterprise/queries.ts +++ b/app/src/lib/enterprise/queries.ts @@ -141,3 +141,29 @@ export function actionRecordsQueryOptions() { }), }); } + +/** The capability switches as they apply to the person asking. */ +export type MyCapabilities = { + capabilities: Partial>; + /** False on a deployment running without enterprise controls, where the defaults stand. */ + enforced: boolean; +}; + +/** + * What this person may use, which is not an administrator's read. + * + * `/me` sits under the enterprise ADMIN prefix and is deliberately the one route there that any + * signed-in person may call: the switches decide what the app offers them, so the app has to be + * able to ask. Everything else behind that prefix requires an administrator. + */ +export function myCapabilitiesQueryOptions() { + return queryOptions({ + queryKey: ["enterprise", "me"] as const, + queryFn: async (): Promise => + ( + await client("/api/admin/enterprise/me", { + fallback: "Could not load what this deployment allows", + }) + ).json(), + }); +} diff --git a/app/tests/computer-tools-offered.test.ts b/app/tests/computer-tools-offered.test.ts new file mode 100644 index 000000000..b3fb95f18 --- /dev/null +++ b/app/tests/computer-tools-offered.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, test } from "bun:test"; +import { computerToolsOffered } from "@/lib/copilot/computer-available"; + +/** + * Which computer tools a Bot is given in this conversation. + * + * Two different questions, and the screen used to ask only the first. WHOSE computer it is decides + * whether this browser may drive it at all; the deployment's capability switches decide whether + * anybody may. A Bot offered a tool the second question refuses reaches for it, is refused, and the + * person watches a turn spent on an answer the deployment already knew — so the tool is not offered. + * + * Unresolved reads as allowed, in both halves: the server decides, and this only declines to offer + * what it would certainly refuse. + */ + +const mine = { visibility: "private", canManage: true } as const; +const somebodyElses = { visibility: "private", canManage: false } as const; +const everything = { + cloudBrowser: true, + cloudNetwork: true, + cloudComputer: true, +}; + +describe("whose computer it is", () => { + test("a Bot this person administers offers everything", () => { + expect( + computerToolsOffered({ profile: mine, capabilities: everything }), + ).toEqual({ browser: true, navigate: true, computer: true }); + }); + + test("a public Bot offers everything", () => { + expect( + computerToolsOffered({ + profile: { visibility: "public", canManage: false }, + capabilities: everything, + }), + ).toEqual({ browser: true, navigate: true, computer: true }); + }); + + test("a teammate's Team Bot offers nothing", () => { + // The computer carries its owner's signed-in browser; the server refuses every route on it. + expect( + computerToolsOffered({ + profile: somebodyElses, + capabilities: everything, + }), + ).toEqual({ browser: false, navigate: false, computer: false }); + }); + + test("a profile still loading offers everything", () => { + expect( + computerToolsOffered({ profile: undefined, capabilities: everything }), + ).toEqual({ browser: true, navigate: true, computer: true }); + }); +}); + +describe("what the deployment allows", () => { + test("browser use switched off withdraws the browser tools", () => { + expect( + computerToolsOffered({ + profile: mine, + capabilities: { ...everything, cloudBrowser: false }, + }), + ).toEqual({ browser: false, navigate: false, computer: true }); + }); + + test("network access switched off withdraws only opening pages", () => { + // Clicking and typing on a page already open are still the browser, and still allowed. + expect( + computerToolsOffered({ + profile: mine, + capabilities: { ...everything, cloudNetwork: false }, + }), + ).toEqual({ browser: true, navigate: false, computer: true }); + }); + + test("computer use switched off withdraws files and commands", () => { + expect( + computerToolsOffered({ + profile: mine, + capabilities: { ...everything, cloudComputer: false }, + }), + ).toEqual({ browser: true, navigate: true, computer: false }); + }); + + test("capabilities still loading offer everything", () => { + expect( + computerToolsOffered({ profile: mine, capabilities: undefined }), + ).toEqual({ browser: true, navigate: true, computer: true }); + }); +}); From e9846dd94f75f4a65d1e83a62b80ba5f60de531e Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 14:44:46 -0300 Subject: [PATCH 3/6] Offer an unconnected app in the conversation, not the name of a screen MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A Bot asked to use an app whose account the asker had not connected answered "Connect it in Settings and ask again" — a correct sentence, and a bad one to end on, in the middle of a conversation about an app the Bot held a tool for all along. The refusal now carries a connect prompt, declared once in shared and read from both sides for the reason shared/handoff-markers.ts sets out: a wording that lives in two files is a contract that drifts. The transcript turns it into a card naming the app, with a Connect button, and tool-activity keeps it out of the fold for the same reason a consent card is kept out — a question nobody can see is a conversation that has stopped without saying so. Connect opens the vendor's consent in a new tab rather than navigating this one: the trip is a full page load, so sending this tab would replace the conversation and land somebody on the connected-accounts page with their question behind them. The card watches this person's own connections while it waits and says connected only when the server lists one. The return trip is an unsigned redirect in a tab this one cannot read, so arriving back proves nothing. Four refusals carry it: never connected, brokered and user-OAuth alike, disconnected while a call was in flight, and access withdrawn. An app shared across the deployment deliberately does not — the person reading it cannot connect that account however clearly they are asked. --- .../components/channels/chat-transcript.tsx | 5 + .../components/channels/connect-app-card.tsx | 101 ++++++++++++++++ app/src/components/channels/tool-activity.tsx | 17 ++- app/tests/connect-app-card.test.tsx | 109 ++++++++++++++++++ app/tests/tool-activity.test.tsx | 13 +++ server/src/plugins/plugin-store/context.ts | 30 ++++- server/tests/helpers/refusals.ts | 10 +- server/tests/plugin-store.integration.test.ts | 44 ++++++- shared/connect-prompt.test.ts | 60 ++++++++++ shared/connect-prompt.ts | 66 +++++++++++ 10 files changed, 440 insertions(+), 15 deletions(-) create mode 100644 app/src/components/channels/connect-app-card.tsx create mode 100644 app/tests/connect-app-card.test.tsx create mode 100644 shared/connect-prompt.test.ts create mode 100644 shared/connect-prompt.ts diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index e890e2ec0..e4b3e0f84 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -59,6 +59,7 @@ import { toolResultFailed, } from "@/lib/plugins/tool-result"; import { readTeamBotConsent } from "@/lib/team-bots"; +import { readConnectPrompt } from "@openbot/shared/connect-prompt"; import { cn } from "@/lib/utils"; import { VOICE_CHAT_ACTIVITY, type VoiceChatEntry } from "@/lib/voice/archive"; import { @@ -72,6 +73,7 @@ import { toVisibleChatItems, } from "./chat-messages"; import type { QueuedMessage } from "./composer"; +import { ConnectAppCard } from "./connect-app-card"; import { TeamBotConsentCard } from "./team-bot-consent"; import { ToolRenderBoundary } from "./tool-boundary"; import { ToolLine } from "./tool-line"; @@ -1371,6 +1373,9 @@ function ServerToolLine({ name, result }: { name: string; result?: string }) { // A Team Bot asking to use this person's own account draws its consent card, not the sentence. const consent = refused ? readTeamBotConsent(body) : null; if (consent) return ; + // An app this person has not connected draws the connect button, not the name of a screen. + const connect = refused ? readConnectPrompt(body) : null; + if (connect) return ; return ( row.serverId === serverId); + + /* + * The grants a Bot holds are read per conversation, so the turn that follows has to be asked + * against a store that knows about the new account. Invalidated once, on the edge. + */ + const was = useRef(connected); + useEffect(() => { + if (connected && !was.current) { + setWaiting(false); + void queryClient.invalidateQueries(); + } + was.current = connected; + }, [connected]); + + const open = () => + connect.mutate(serverId, { + onSuccess: (authorizationUrl) => { + if (!authorizationUrl) return; + setWaiting(true); + window.open(authorizationUrl, "_blank", "noopener,noreferrer"); + }, + }); + + return ( +
+
+

Connect {title}

+

{message}

+
+ {connected ? ( +

+ {title} is connected. Ask again and the Bot can use it. +

+ ) : ( +
+ {/* The heading above already names the app; the button says what pressing it does. */} + + {waiting ? ( +

+ Finish in the tab that opened, then come back here. +

+ ) : null} + {connect.isError ? ( +

+ {connect.error instanceof Error + ? connect.error.message + : `${title} could not be connected.`} +

+ ) : null} +
+ )} +
+ ); +} diff --git a/app/src/components/channels/tool-activity.tsx b/app/src/components/channels/tool-activity.tsx index 5fc9e69d5..7e772e376 100644 --- a/app/src/components/channels/tool-activity.tsx +++ b/app/src/components/channels/tool-activity.tsx @@ -15,6 +15,7 @@ import { toolResultFailed, } from "@/lib/plugins/tool-result"; import { readTeamBotConsent } from "@/lib/team-bots"; +import { readConnectPrompt } from "@openbot/shared/connect-prompt"; import { researchRoleOfTool } from "@openbot/shared/research"; import { browserStepDetails, isBrowserStep } from "./browser-activity"; import type { VisibleChatItem } from "./chat-messages"; @@ -69,16 +70,22 @@ export function groupToolSteps( function foldable(item: ToolItem): boolean { if (isBrowserStep(item)) return true; if (!item.toolCall.function.name.startsWith("mcp__")) return false; - return !asksForConsent(item.result); + return !asksThePerson(item.result); } -function asksForConsent(result: string | undefined): boolean { +/** + * Whether this refusal is a question for the person rather than a line about what a Bot did. + * + * Both of these draw a card with something to press — a Team Bot asking to act as them, and an app + * they have not connected — and neither may sit behind a disclosure for the same reason: a question + * nobody can see is a conversation that has stopped without saying so. + */ +function asksThePerson(result: string | undefined): boolean { if (result === undefined) return false; const answer = asText(result); if (!answer.startsWith(REFUSAL_MARKER)) return false; - return ( - readTeamBotConsent(answer.slice(REFUSAL_MARKER.length).trim()) !== null - ); + const body = answer.slice(REFUSAL_MARKER.length).trim(); + return readTeamBotConsent(body) !== null || readConnectPrompt(body) !== null; } type StepKind = "search" | "fetch" | "browser" | "connector"; diff --git a/app/tests/connect-app-card.test.tsx b/app/tests/connect-app-card.test.tsx new file mode 100644 index 000000000..1da68ebc9 --- /dev/null +++ b/app/tests/connect-app-card.test.tsx @@ -0,0 +1,109 @@ +import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, fireEvent, render, waitFor } from "@testing-library/react"; +import type { ReactNode } from "react"; +import { ConnectAppCard } from "@/components/channels/connect-app-card"; +import { settleReactWork } from "./settle-react-work"; + +/** + * The card a Bot's refusal turns into when the app it needed is one this person can connect. + * + * What it must never do is navigate this tab. The consent screen belongs to the vendor and the trip + * is a full page load, so the conversation the person is reading would be replaced by it — and the + * question they asked would be behind them when they got back. + */ + +beforeAll(() => GlobalRegistrator.register()); +afterEach(cleanup); +afterAll(async () => { + await settleReactWork(); + GlobalRegistrator.unregister(); +}); + +const originalFetch = global.fetch; +afterEach(() => { + global.fetch = originalFetch; +}); + +function mounted(connections: { serverId: string; holder?: string }[] = []) { + const opened: string[] = []; + (window as unknown as { open: (url: string) => void }).open = (url) => { + opened.push(url); + }; + + global.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + if (url.includes("/api/plugins/connections")) { + return new Response(JSON.stringify({ connections, redirectUri: null }), { + headers: { "content-type": "application/json" }, + }); + } + if (url.includes("/connect") && init?.method === "POST") { + return new Response( + JSON.stringify({ authorizationUrl: "https://notion.test/consent" }), + { headers: { "content-type": "application/json" } }, + ); + } + return new Response("{}", { + headers: { "content-type": "application/json" }, + }); + }) as unknown as typeof fetch; + + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const Wrapper = ({ children }: { children: ReactNode }) => ( + {children} + ); + const view = render( + , + { wrapper: Wrapper }, + ); + return { opened, view }; +} + +test("asks for the app by name, in the words the Bot used", () => { + const card = mounted(); + + expect(card.view.getByText(/Connect Notion/)).toBeTruthy(); + expect( + card.view.getByText(/You have not connected your Notion account/), + ).toBeTruthy(); +}); + +test("sends the person to the vendor in another tab, never in this one", async () => { + const card = mounted(); + const before = window.location.href; + + fireEvent.click(card.view.getByRole("button", { name: /^Connect$/ })); + + await waitFor(() => + expect(card.opened).toEqual(["https://notion.test/consent"]), + ); + expect(window.location.href).toBe(before); + expect(card.view.getByText(/Finish in the tab that opened/)).toBeTruthy(); +}); + +test("says so once the account is actually listed, rather than once they return", async () => { + const card = mounted([{ serverId: "srv_notion", holder: "person" }]); + + await waitFor(() => + expect(card.view.getByText(/Notion is connected/)).toBeTruthy(), + ); + // No button to press any more: the next step is to ask the Bot again. + expect(card.view.queryByRole("button", { name: /^Connect$/ })).toBeNull(); +}); + +test("does not read a Shared app's deployment account as this person's", async () => { + const card = mounted([{ serverId: "srv_notion", holder: "deployment" }]); + + await waitFor(() => + expect(card.view.getByRole("button", { name: /^Connect$/ })).toBeTruthy(), + ); + expect(card.view.queryByText(/Notion is connected/)).toBeNull(); +}); diff --git a/app/tests/tool-activity.test.tsx b/app/tests/tool-activity.test.tsx index 16114b3d3..041ac9f50 100644 --- a/app/tests/tool-activity.test.tsx +++ b/app/tests/tool-activity.test.tsx @@ -81,6 +81,19 @@ test("a request to connect the person's own account is never folded away", () => expect(grouped.map((item) => item.id)).toContain("consent"); }); +test("a request to connect an app at all is never folded away either", () => { + const prompt = call( + "connect", + "mcp__notion__search", + {}, + JSON.stringify( + 'Refused. [connect-app]{"serverId":"srv_notion","title":"Notion"} You have not connected your Notion account. Connect it and ask again.', + ), + ); + const grouped = groupToolSteps([search("a"), prompt, search("b")]); + expect(grouped.map((item) => item.id)).toContain("connect"); +}); + test("the summary says what was done, never which provider searched", () => { const steps = [ search(), diff --git a/server/src/plugins/plugin-store/context.ts b/server/src/plugins/plugin-store/context.ts index 4d001db4f..b877d5bf4 100644 --- a/server/src/plugins/plugin-store/context.ts +++ b/server/src/plugins/plugin-store/context.ts @@ -1,3 +1,4 @@ +import { connectAppRefusal } from "@openbot/shared/connect-prompt"; import { and, asc, eq, inArray, isNull, sql } from "drizzle-orm"; import { recordAuditEvent } from "../../audit"; import { @@ -526,10 +527,19 @@ export function createStoreContext(options: PluginStoreOptions) { .where(accountRow(access.toolkit, resolved.ref)) .limit(1); if (!connected) { + /* + * Only the personal half carries a connect prompt. A Shared app's account belongs to the + * deployment, so the person reading this cannot connect it however clearly they are asked — + * a card offering them the button would be an invitation to a refusal. + */ throw new PluginRefusedError( resolved.ref.holder === "deployment" ? `${row.title} is shared across this deployment and no account is connected to it yet. Ask an administrator to connect it.` - : `You have not connected your ${row.title} account. Connect it in Settings and ask again.`, + : connectAppRefusal({ + serverId: row.id, + title: row.title, + message: `You have not connected your ${row.title} account. Connect it and ask again.`, + }), null, ); } @@ -602,7 +612,11 @@ export function createStoreContext(options: PluginStoreOptions) { if (!held) { throw new PluginRefusedError( - `You have not connected your ${entry.title} account. Connect it in Settings and ask again.`, + connectAppRefusal({ + serverId: row.id, + title: entry.title, + message: `You have not connected your ${entry.title} account. Connect it and ask again.`, + }), null, ); } @@ -801,7 +815,11 @@ export function createStoreContext(options: PluginStoreOptions) { // and connecting again is the thing to do. if (!current) { throw new PluginRefusedError( - `You have not connected your ${title} account. Connect it in Settings and ask again.`, + connectAppRefusal({ + serverId: row.id, + title, + message: `You have not connected your ${title} account. Connect it and ask again.`, + }), null, ); } @@ -821,7 +839,11 @@ export function createStoreContext(options: PluginStoreOptions) { */ if (!locked || locked.revokedAt) { throw new PluginRefusedError( - `Your ${title} access was withdrawn. Connect it again in Settings.`, + connectAppRefusal({ + serverId: row.id, + title, + message: `Your ${title} access was withdrawn. Connect it again.`, + }), null, ); } diff --git a/server/tests/helpers/refusals.ts b/server/tests/helpers/refusals.ts index b94e9ff30..907b3b01d 100644 --- a/server/tests/helpers/refusals.ts +++ b/server/tests/helpers/refusals.ts @@ -127,10 +127,16 @@ const TABLE = { /** `connectionFields`: a box whose answer could not be sent back, because it has no name. */ fieldNameMissing: () => /under the name nothing, which cannot be filled in here/, - /** Personal-mode guard: the person asking has not connected their own account for this app. */ + /** + * Personal-mode guard: the person asking has not connected their own account for this app. + * + * No longer "in Settings": this refusal carries a connect prompt (see + * `@openbot/shared/connect-prompt`), so the transcript draws the button rather than naming a + * screen somewhere else. The sentence is still the whole of what the model is told. + */ personalNotConnected: (app: string = "[^.]+") => new RegExp( - `You have not connected your ${app} account\\. Connect it in Settings and ask again\\.`, + `You have not connected your ${app} account\\. Connect it and ask again\\.`, ), /** Shared-mode guard: this deployment's shared account for the app has not been connected yet. */ sharedNotConnected: (app: string = "[^.]+") => diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index 12e46dd96..4950c8de7 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -11,6 +11,7 @@ import { randomUUID } from "node:crypto"; import { readdirSync, readFileSync } from "node:fs"; import { join } from "node:path"; import { MCPMock, type MCPToolDefinition } from "@copilotkit/aimock/mcp"; +import { readConnectPrompt } from "@openbot/shared/connect-prompt"; import type { ToolAnnotations } from "@modelcontextprotocol/sdk/types.js"; import { and, asc, eq, gte, inArray, sql } from "drizzle-orm"; import { createAuditStore } from "../src/audit"; @@ -4440,13 +4441,48 @@ test("a Composio call by somebody who has not connected the app is refused with botId: "bot_helper", actorId: "user_asker", }), - ).rejects.toThrow(/connect it in settings/i); + ).rejects.toThrow(/connect it and ask again/i); // Refused here rather than at Composio, so a person is told what to do instead of being shown // somebody else's error, and so no call is spent finding out. expect(reached).toEqual([]); }); +test("a refusal nobody but the asker can clear carries the app to connect", async () => { + /* + * THE CARD IS DRAWN FROM THIS STRING. The transcript turns this refusal into a Connect button + * (see `app/src/components/channels/connect-app-card.tsx`), and the only thing telling it which + * app to connect is the prompt inside the sentence. A refusal that stops carrying it degrades + * silently: the English still reads correctly and the button quietly stops being offered. + */ + const { store, database } = await freshStore(); + useComposioClient({ + listActions: async () => [], + execute: async () => vendorAnswered(), + }); + await seedComposioGmail(database, store, { connect: false }); + + const refusal = await store + .callTool({ + ref: "gmail/GMAIL_FETCH_EMAILS", + args: {}, + botId: "bot_helper", + actorId: "user_asker", + }) + .then( + () => null, + (error: unknown) => + error instanceof Error ? error.message : String(error), + ); + + expect(readConnectPrompt(refusal)).toEqual({ + serverId: "gmail", + title: "Gmail", + message: + "You have not connected your Gmail account. Connect it and ask again.", + }); +}); + /** * One person having connected the app is not the asking person having connected it. * @@ -4484,7 +4520,7 @@ test("a Composio call by somebody who has not connected the app is refused even botId: "bot_helper", actorId: "user_stranger", }), - ).rejects.toThrow(/connect it in settings/i); + ).rejects.toThrow(/connect it and ask again/i); // Never dialled, which is the half that matters: a call let through here is spent at the broker // in a stranger's name, and the person asking sees somebody else's mailbox or somebody else's @@ -4550,7 +4586,7 @@ test("a Composio call whose row id and url name different apps is refused", asyn botId: "bot_helper", actorId: "user_asker", }), - ).rejects.toThrow(/connect it in settings/i); + ).rejects.toThrow(/connect it and ask again/i); // The gate keys on the app the url names, because that is the app the transport dials. Keyed on // the row id, this call completed: it ran a Slack action against a Gmail connection, sent the @@ -4626,7 +4662,7 @@ test("an app whose accounts are somebody's still refuses without a row, and name botId: "bot_helper", actorId: "user_asker", }), - ).rejects.toThrow(/connect it in settings/i); + ).rejects.toThrow(/connect it and ask again/i); // Never dialled. The refusal is local so the person is told their own next step instead of shown // the broker's error about an account it cannot find, and no call is spent finding that out. diff --git a/shared/connect-prompt.test.ts b/shared/connect-prompt.test.ts new file mode 100644 index 000000000..630b76437 --- /dev/null +++ b/shared/connect-prompt.test.ts @@ -0,0 +1,60 @@ +import { describe, expect, test } from "bun:test"; +import { + CONNECT_APP_MARKER, + connectAppRefusal, + readConnectPrompt, +} from "./connect-prompt"; + +describe("a refusal asking somebody to connect an app", () => { + test("survives the round trip", () => { + const text = connectAppRefusal({ + serverId: "srv_notion", + title: "Notion", + message: "You have not connected your Notion account.", + }); + + expect(readConnectPrompt(text)).toEqual({ + serverId: "srv_notion", + title: "Notion", + message: "You have not connected your Notion account.", + }); + }); + + test("keeps the sentence readable for anything that cannot parse it", () => { + const text = connectAppRefusal({ + serverId: "srv_notion", + title: "Notion", + message: "You have not connected your Notion account.", + }); + + // The model is told this string. Whatever the marker does, the English has to survive. + expect(text.endsWith("You have not connected your Notion account.")).toBe( + true, + ); + expect(text.startsWith(CONNECT_APP_MARKER)).toBe(true); + }); +}); + +describe("text that is not one", () => { + test.each([ + ["an ordinary refusal", "That tool is not granted."], + ["nothing at all", ""], + ["a truncated marker", `${CONNECT_APP_MARKER}{"serverId":"srv"`], + ["JSON that is not an object", `${CONNECT_APP_MARKER}"srv"} hello`], + ["a missing title", `${CONNECT_APP_MARKER}{"serverId":"srv"} hello`], + [ + "a title of the wrong type", + `${CONNECT_APP_MARKER}{"serverId":"srv","title":7} hello`, + ], + [ + "an empty server id", + `${CONNECT_APP_MARKER}{"serverId":"","title":"Notion"} hello`, + ], + ])("reads as no prompt: %s", (_what, text) => { + expect(readConnectPrompt(text)).toBeNull(); + }); + + test("undefined reads as no prompt", () => { + expect(readConnectPrompt(undefined)).toBeNull(); + }); +}); diff --git a/shared/connect-prompt.ts b/shared/connect-prompt.ts new file mode 100644 index 000000000..499ea3278 --- /dev/null +++ b/shared/connect-prompt.ts @@ -0,0 +1,66 @@ +/** + * The refusal a person can act on: an app this call needed, that they have not connected. + * + * ONE DECLARATION, READ FROM BOTH SIDES, for the reason `./handoff-markers.ts` sets out at length. + * A tool that runs on the server reaches the transcript as text meant for a model, so a renderer + * can only tell this refusal from the others by its wording — and a wording that lives in two files + * is a contract that drifts. The server builds the sentence here and the transcript reads it here. + * + * WHY IT IS WORTH TELLING APART AT ALL. "Connect it in Settings and ask again" is a correct + * sentence and a bad one to end on: it describes a screen somewhere else, in the middle of a + * conversation the person is having now, about an app the Bot was holding a tool for all along. + * What they need is the thing itself, where they are reading — which is what the marker lets the + * transcript put there. + */ + +/** The first characters of the refusal, immediately followed by its JSON. */ +export const CONNECT_APP_MARKER = "[connect-app]"; + +export type ConnectPrompt = { + /** The plugin server to connect, as `POST /api/plugins/servers/:serverId/connect` names it. */ + serverId: string; + /** The app's own name, for a card that says what it is asking for. */ + title: string; + /** What the Bot would have said, kept intact for a reader with no card to draw. */ + message: string; +}; + +/** + * The refusal text a `PluginRefusedError` carries for an app the asker has not connected. + * + * The sentence stays in the string rather than being rebuilt by the card, because it is also what + * the MODEL is told, and because a transcript that cannot parse the marker still reads correctly. + */ +export function connectAppRefusal(prompt: ConnectPrompt): string { + const { serverId, title, message } = prompt; + return `${CONNECT_APP_MARKER}${JSON.stringify({ serverId, title })} ${message}`; +} + +/** + * The prompt inside a refusal, or null for any other text. + * + * Fails to null on anything it does not fully recognise — a truncated marker, JSON that is not an + * object, a field of the wrong type. A card drawn from a half-read refusal would offer to connect + * something nobody named. + */ +export function readConnectPrompt( + text: string | undefined | null, +): ConnectPrompt | null { + if (!text?.startsWith(CONNECT_APP_MARKER)) return null; + const rest = text.slice(CONNECT_APP_MARKER.length); + const end = rest.indexOf("}"); + if (end < 0) return null; + try { + const parsed: unknown = JSON.parse(rest.slice(0, end + 1)); + if (!parsed || typeof parsed !== "object") return null; + const { serverId, title } = parsed as { + serverId?: unknown; + title?: unknown; + }; + if (typeof serverId !== "string" || typeof title !== "string") return null; + if (!serverId || !title) return null; + return { serverId, title, message: rest.slice(end + 1).trim() }; + } catch { + return null; + } +} From 9e72a95891ca4eff71684ee60a0853bb6027d70f Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 14:44:49 -0300 Subject: [PATCH 4/6] Changelog for the Bot's screen and the connect card --- CHANGELOG.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2aa59af7d..123213f78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,38 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. `code_required` (below). A server and a computer must be upgraded together: an older computer has no `/sign-in/code`. +### The Bot's screen opens only when there is something to watch + +Asking a Bot a question it answered in words could still open its computer beside the conversation, +on a still screen, for an action that never happened. + +- **A call that never reaches the screen no longer opens it.** Reading, writing or listing a file, + and running a command, happen on the computer rather than on its display. Only work that happens + on the screen opens it. +- **A refused action takes its screen back.** The screen is announced before the action so a page + can be watched while it loads; an action a policy, a capability switch or a network rule refuses + never touches the browser, so the screen it opened closes again. A refusal does not close a screen + some earlier action in the same run legitimately opened, and an action that was permitted and + failed keeps its screen, because the page may have moved. +- **A Bot is no longer given computer tools this deployment would refuse.** Admin → Controls already + decided whether anybody may use a Bot's browser (`Cloud browser use`), its files and shell + (`Cloud computer use`) and the network (`Cloud network access`), and the boundary enforced all + three — but the Bot was still offered the tools, so it reached for one, was refused, and spent the + turn saying so. The tools a switch turns off are now withheld from the Bot in the first place. + +### An app you have not connected is offered where you are reading + +A Bot asked to use an app whose account the person had not connected answered with the name of a +screen somewhere else: "Connect it in Settings and ask again." The conversation now carries the +thing itself — a card naming the app, with a Connect button. + +- Pressing Connect opens the vendor's consent in a **new tab**, so the conversation is not replaced + by a page load and the question is still there afterwards. The card watches for the account to + appear and says so only once the server lists it, never merely because somebody came back. +- It covers an account that was never connected, one disconnected while a call was in flight, and + access that was withdrawn. An app shared across the deployment still says to ask an administrator, + since the person reading cannot connect it. + ### A reply's tool calls fold into one line A Bot that searched, read two pages and checked Drive before answering drew four lines above its From e4e23bdf1ddb1ac216089b6204b8fc47a9c35455 Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 15:04:06 -0300 Subject: [PATCH 5/6] Say when a connection's credential has been withdrawn A row is not access. Revoking a credential leaves mcp_user_credentials pointing at it, and connectionsFor read that table alone, so every caller asking "is this person connected" was answered "does this person have a row". Reconnecting settles nothing either: the upsert moves credential_id and updated_at and leaves connected_at where it was, so nothing visible changes when access comes back. The in-chat connect card is where that surfaced. Drawn over the "your access was withdrawn" refusal it said the app was connected and took away the button that is the only way to get access back. Reported rather than filtered, so a caller wanting every app somebody has ever connected still gets one and memory ingestion keeps the behaviour it had. The join is inner and total because credential_id is a real foreign key that does not cascade, which is also why there is no third state to report; a test pins that, since it is what the field's completeness rests on. --- app/src/lib/plugins/queries.ts | 14 ++++++ .../src/plugins/plugin-store/connections.ts | 38 +++++++++++++-- ...plugin-user-credential.integration.test.ts | 48 +++++++++++++++++++ 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/app/src/lib/plugins/queries.ts b/app/src/lib/plugins/queries.ts index 05b215df9..7151a234c 100644 --- a/app/src/lib/plugins/queries.ts +++ b/app/src/lib/plugins/queries.ts @@ -221,6 +221,20 @@ export const pluginKeys = { */ export type PluginConnection = { serverId: string; + /** + * Whether the credential behind this row has been withdrawn or deleted, present only on a HELD + * row — a brokered one keeps no secret here, so it has no equivalent question. + * + * A ROW IS NOT ACCESS. Revoking a credential leaves this row pointing at it, and reconnecting + * moves `credential_id` and `updated_at` without touching `connected_at`, so the presence of a + * row says nothing about whether the app can be used and nothing visible changes when it can be + * again. Anything drawing the word "connected" has to read this; anything listing the apps + * somebody has ever connected does not. + * + * Undefined on a brokered row and on any shape written before this existed, and the absence + * reads as not revoked. + */ + revoked?: boolean; /** What the vendor actually granted, which is not always what was asked for. Empty for brokered. */ scope: string; connectedAt: string; diff --git a/server/src/plugins/plugin-store/connections.ts b/server/src/plugins/plugin-store/connections.ts index ab748bd12..3ca6daa0d 100644 --- a/server/src/plugins/plugin-store/connections.ts +++ b/server/src/plugins/plugin-store/connections.ts @@ -182,16 +182,47 @@ export function connectionMethods(context: StoreContext) { oauthClientFor: storedOAuthClient, /** Which `user-oauth` servers this person has connected, for their own settings page. */ - async connectionsFor( - userId: string, - ): Promise<{ serverId: string; scope: string; connectedAt: string }[]> { + async connectionsFor(userId: string): Promise< + { + serverId: string; + scope: string; + connectedAt: string; + /** + * Whether the credential this row points at has been withdrawn. + * + * THE ROW OUTLIVES THE SECRET, and until this field existed the two were impossible to tell + * apart from outside. Revoking a credential sets `revoked_at` on the credential; it does not + * delete the row here, which goes on pointing at it — so "this person has a row for Notion" + * and "this person can use Notion" are different facts, and every reader of this list was + * getting the first while asking the second. A reconnection does not settle it either: the + * upsert moves `credential_id` and `updated_at` and leaves `connected_at` where it was, so + * nothing in the shape this used to return changes when access comes back. + * + * Reported rather than filtered. A caller that wants "connected" asks for a row that is not + * revoked; one that wants to draw every app somebody has ever connected still can, and + * memory ingestion's existence check keeps the behaviour it had. + */ + revoked: boolean; + }[] + > { const rows = await database .select({ serverId: mcpUserCredentials.serverId, scope: mcpUserCredentials.scope, connectedAt: mcpUserCredentials.connectedAt, + revokedAt: credentialRows.revokedAt, }) .from(mcpUserCredentials) + /* + * Inner, and total because of the foreign key: `credential_id` is a real reference that + * does not cascade, so a join row cannot outlive the credential it points at — a deletion + * is refused while the row is there. That is why there is no "the credential is gone" case + * to report here, and why a revoked credential is the only way this answer goes false. + */ + .innerJoin( + credentialRows, + eq(credentialRows.id, mcpUserCredentials.credentialId), + ) .where(eq(mcpUserCredentials.userId, userId)) .orderBy(asc(mcpUserCredentials.serverId)); @@ -199,6 +230,7 @@ export function connectionMethods(context: StoreContext) { serverId: row.serverId, scope: row.scope, connectedAt: iso(row.connectedAt) ?? "", + revoked: row.revokedAt !== null, })); }, diff --git a/server/tests/plugin-user-credential.integration.test.ts b/server/tests/plugin-user-credential.integration.test.ts index b425e64d8..63b2dc467 100644 --- a/server/tests/plugin-user-credential.integration.test.ts +++ b/server/tests/plugin-user-credential.integration.test.ts @@ -497,6 +497,54 @@ describe("a person who has connected", () => { * session — the session is the half they cannot use, and the token is the half this deployment is * still holding. For a per-person connector it is the first thing a customer asks about. */ +describe("a connection whose credential no longer works", () => { + /* + * A ROW IS NOT ACCESS, and the two used to be impossible to tell apart from outside. + * + * `connectionsFor` reads the join table, which revoking a credential does not touch. Every reader + * asking "is this person connected" therefore got "does this person have a row", and the in-chat + * connect card drew "connected" over the very refusal telling them their access was withdrawn — + * taking away the button that is the only way to get it back. + */ + test("is listed as revoked rather than as connected", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + + expect(await store.connectionsFor(askerId)).toMatchObject([ + { serverId, revoked: false }, + ]); + + await database + .update(credentials) + .set({ revokedAt: new Date() }) + .where(eq(credentials.id, credentialId)); + + expect(await store.connectionsFor(askerId)).toMatchObject([ + { serverId, revoked: true }, + ]); + }); + + test("cannot have its credential deleted out from under it", async () => { + /* + * What makes the join above total, and therefore what makes "revoked" the only way access can + * be gone. `credential_id` is a real foreign key that does not cascade, so there is no third + * state to report — the database refuses to create one. + */ + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + + // Awaited inside a real promise: a drizzle query builder is a thenable, not a Promise, and + // `rejects` given one asserts against the builder object rather than running the query. + await expect( + (async () => { + await database + .delete(credentials) + .where(eq(credentials.id, credentialId)); + })(), + ).rejects.toThrow(); + }); +}); + describe("retiring the credentials a person owns", () => { /* * Counted as a property, not as a number. From a9a0c554590a72dd0ab5e05de9f5aba67e23058d Mon Sep 17 00:00:00 2001 From: Guido Vizoso Date: Fri, 16 Oct 2026 15:04:13 -0300 Subject: [PATCH 6/6] Make Connect work for a blocked tab, a typed key, and a withdrawn account MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the card could press Connect and leave somebody with nothing. A browser only lets a page open a tab while a click is still recent — about five seconds in Chrome — and the tab was opened after the request that mints the authorization url came back. On a slow deployment the activation had expired, the tab was blocked, and the card said to finish in a tab that did not exist. The blank tab is now claimed on the click itself and navigated once the url arrives; noopener cannot be passed, since it makes window.open answer null and null is a handle nothing can navigate later, so the opener is severed on the handle instead. A tab blocked anyway leaves the url as a link to click. An app connected by typing a key has no page to visit: the route answers { fields } and the mutation unwraps nothing, so the button silently did nothing. The card now says so and links to the page with the form. And an account whose credential was withdrawn is no longer read as connected, now that the connections list says which is which. --- .../components/channels/connect-app-card.tsx | 79 +++++- app/tests/connect-app-card.test.tsx | 231 +++++++++++++++--- 2 files changed, 270 insertions(+), 40 deletions(-) diff --git a/app/src/components/channels/connect-app-card.tsx b/app/src/components/channels/connect-app-card.tsx index 6f2fa2fd1..89f712d54 100644 --- a/app/src/components/channels/connect-app-card.tsx +++ b/app/src/components/channels/connect-app-card.tsx @@ -1,4 +1,5 @@ import { useMutation, useQuery } from "@tanstack/react-query"; +import { Link } from "@tanstack/react-router"; import { useEffect, useRef, useState } from "react"; import { Button } from "@/components/ui/button"; import { connectAccountMutationOptions } from "@/lib/plugins/mutations"; @@ -31,15 +32,25 @@ export function ConnectAppCard({ message: string; }) { const [waiting, setWaiting] = useState(false); + /** This app takes a key somebody types, so there is no page to send them to. */ + const [needsKey, setNeedsKey] = useState(false); + /** The authorization url, when the browser refused to open the tab holding it. */ + const [blockedUrl, setBlockedUrl] = useState(null); const connect = useMutation(connectAccountMutationOptions()); const connections = useQuery({ ...connectionsQueryOptions(), // Only while a consent is open in the other tab; otherwise this is an ordinary cached read. refetchInterval: waiting ? 2_000 : false, }); + /* + * A ROW IS NOT ACCESS, which is the whole reason `revoked` is read here. Withdrawing somebody's + * access leaves their row in place pointing at a revoked credential, so a card drawn over the + * "your access was withdrawn" refusal would have said the app was connected and taken away the + * button that is the only way to get it back. + */ const connected = personalConnections( connections.data?.connections ?? [], - ).some((row) => row.serverId === serverId); + ).some((row) => row.serverId === serverId && row.revoked !== true); /* * The grants a Bot holds are read per conversation, so the turn that follows has to be asked @@ -54,14 +65,47 @@ export function ConnectAppCard({ was.current = connected; }, [connected]); - const open = () => + /** + * The tab, opened on the click itself rather than on the answer to it. + * + * A BROWSER ONLY LETS A PAGE OPEN A TAB WHILE A CLICK IS STILL RECENT. Chrome's transient + * activation lasts about five seconds, and the request that mints the authorization url has to + * finish first — so on a slow deployment `window.open` was called with the activation already + * expired, the tab was blocked, and the card cheerfully said to finish in a tab that did not + * exist. The blank tab is claimed first and navigated once the url arrives. + * + * `noopener` CANNOT BE PASSED HERE: it makes `window.open` answer null, and null is a handle + * nothing can navigate later. The opener is severed on the handle instead, which is the same + * guarantee by a different route. + */ + const claimTab = (): Window | null => { + const tab = window.open("", "_blank"); + if (tab) tab.opener = null; + return tab; + }; + + const open = () => { + const tab = claimTab(); connect.mutate(serverId, { onSuccess: (authorizationUrl) => { - if (!authorizationUrl) return; + /* + * No url means this app is connected by typing a key rather than by visiting a page — the + * route answers `{ fields }` and the mutation unwraps nothing. Saying so, with the page + * that has the form, beats a button that silently does nothing. + */ + if (!authorizationUrl) { + tab?.close(); + setNeedsKey(true); + return; + } setWaiting(true); - window.open(authorizationUrl, "_blank", "noopener,noreferrer"); + if (tab) tab.location.replace(authorizationUrl); + // The tab was blocked. The url is still good, so offer it as something to click. + else setBlockedUrl(authorizationUrl); }, + onError: () => tab?.close(), }); + }; return (
{title} is connected. Ask again and the Bot can use it.

+ ) : needsKey ? ( +

+ {title} is connected with a key you hold rather than by visiting a + page.{" "} + + Enter it on {title}'s page + + , then ask again. +

) : (
{/* The heading above already names the app; the button says what pressing it does. */} @@ -87,6 +144,20 @@ export function ConnectAppCard({ Finish in the tab that opened, then come back here.

) : null} + {blockedUrl ? ( +

+ This browser blocked the tab.{" "} + + Open {title}'s sign-in + + . +

+ ) : null} {connect.isError ? (

{connect.error instanceof Error diff --git a/app/tests/connect-app-card.test.tsx b/app/tests/connect-app-card.test.tsx index 1da68ebc9..72203f268 100644 --- a/app/tests/connect-app-card.test.tsx +++ b/app/tests/connect-app-card.test.tsx @@ -1,20 +1,42 @@ import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; import { GlobalRegistrator } from "@happy-dom/global-registrator"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { + createMemoryHistory, + createRootRoute, + createRoute, + createRouter, + Outlet, + RouterProvider, +} from "@tanstack/react-router"; import { cleanup, fireEvent, render, waitFor } from "@testing-library/react"; -import type { ReactNode } from "react"; import { ConnectAppCard } from "@/components/channels/connect-app-card"; import { settleReactWork } from "./settle-react-work"; /** * The card a Bot's refusal turns into when the app it needed is one this person can connect. * - * What it must never do is navigate this tab. The consent screen belongs to the vendor and the trip - * is a full page load, so the conversation the person is reading would be replaced by it — and the - * question they asked would be behind them when they got back. + * Three things it must never do, each of which it did: + * + * - navigate THIS tab. The consent screen belongs to the vendor and the trip is a full page load, + * so the conversation being read would be replaced by it and the question would be behind them. + * - say "connected" over a row whose credential has been withdrawn, which takes away the button + * that is the only way to get access back. + * - press Connect for an app that takes a typed key and then do nothing at all, because the route + * answers a form rather than a page to visit and there was no url to open. + * + * THE HARNESS IS THIS REPOSITORY'S: `GlobalRegistrator` in `beforeAll`/`afterAll`, `cleanup` in + * `afterEach`, queries off `render()`'s own return, and a `QueryClient` with `retry: false`. The + * router is real but minimal — the card links to a connector's settings page, and a `Link` has to + * resolve against a route tree that contains it. */ -beforeAll(() => GlobalRegistrator.register()); +// Captured inside `beforeAll`: there is no `window` until the registrator has made one. +let originalOpen: typeof window.open; +beforeAll(() => { + GlobalRegistrator.register(); + originalOpen = window.open; +}); afterEach(cleanup); afterAll(async () => { await settleReactWork(); @@ -24,53 +46,111 @@ afterAll(async () => { const originalFetch = global.fetch; afterEach(() => { global.fetch = originalFetch; + window.open = originalOpen; }); -function mounted(connections: { serverId: string; holder?: string }[] = []) { - const opened: string[] = []; - (window as unknown as { open: (url: string) => void }).open = (url) => { - opened.push(url); - }; +type Row = { serverId: string; holder?: string; revoked?: boolean }; + +function mounted( + options: { + connections?: Row[]; + /** What `POST /connect` answers: a page to visit, or a form to fill in. */ + answers?: "url" | "fields" | "failure"; + /** False stands in for a browser that refused the tab, which answers null. */ + popupsAllowed?: boolean; + } = {}, +) { + const { connections = [], answers = "url", popupsAllowed = true } = options; + + /** Where a claimed tab was pointed, which is the only evidence the vendor was reached. */ + const sentTo: string[] = []; + let closed = 0; + let claimed = 0; + window.open = ((url?: string) => { + claimed += 1; + if (url) sentTo.push(url); + if (!popupsAllowed) return null; + const tab = { + opener: {} as unknown, + location: { + replace: (next: string) => { + sentTo.push(next); + }, + }, + close: () => { + closed += 1; + }, + }; + return tab as unknown as Window; + }) as typeof window.open; global.fetch = (async (input: RequestInfo | URL, init?: RequestInit) => { const url = String(input); - if (url.includes("/api/plugins/connections")) { - return new Response(JSON.stringify({ connections, redirectUri: null }), { + const json = (body: unknown, status = 200) => + new Response(JSON.stringify(body), { + status, headers: { "content-type": "application/json" }, }); - } + if (url.includes("/api/plugins/connections")) + return json({ connections, redirectUri: null }); if (url.includes("/connect") && init?.method === "POST") { - return new Response( - JSON.stringify({ authorizationUrl: "https://notion.test/consent" }), - { headers: { "content-type": "application/json" } }, - ); + if (answers === "failure") + return json({ error: "That account could not be connected." }, 502); + // What a key-entry app answers: a form to fill in, and no page to visit. + if (answers === "fields") + return json({ fields: [{ name: "api_key", type: "string" }] }); + return json({ authorizationUrl: "https://notion.test/consent" }); } - return new Response("{}", { - headers: { "content-type": "application/json" }, - }); + return json({}); }) as unknown as typeof fetch; const client = new QueryClient({ defaultOptions: { queries: { retry: false } }, }); - const Wrapper = ({ children }: { children: ReactNode }) => ( - {children} - ); + const root = createRootRoute({ component: Outlet }); + const card = createRoute({ + getParentRoute: () => root, + path: "/", + component: () => ( + + ), + }); + // The destination the card links to for an app connected with a typed key. + const settings = createRoute({ + getParentRoute: () => root, + path: "/settings/connected-accounts/$key", + component: () => null, + }); + const router = createRouter({ + routeTree: root.addChildren([card, settings]), + history: createMemoryHistory({ initialEntries: ["/"] }), + }); const view = render( - , - { wrapper: Wrapper }, + + + , ); - return { opened, view }; + return { + view, + sentTo, + tabsClosed: () => closed, + tabsClaimed: () => claimed, + }; } -test("asks for the app by name, in the words the Bot used", () => { +const press = (card: ReturnType) => + fireEvent.click(card.view.getByRole("button", { name: /^Connect$/ })); + +test("asks for the app by name, in the words the Bot used", async () => { const card = mounted(); - expect(card.view.getByText(/Connect Notion/)).toBeTruthy(); + await waitFor(() => + expect(card.view.getByText(/Connect Notion/)).toBeTruthy(), + ); expect( card.view.getByText(/You have not connected your Notion account/), ).toBeTruthy(); @@ -79,18 +159,80 @@ test("asks for the app by name, in the words the Bot used", () => { test("sends the person to the vendor in another tab, never in this one", async () => { const card = mounted(); const before = window.location.href; + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); - fireEvent.click(card.view.getByRole("button", { name: /^Connect$/ })); + press(card); await waitFor(() => - expect(card.opened).toEqual(["https://notion.test/consent"]), + expect(card.sentTo).toContain("https://notion.test/consent"), ); expect(window.location.href).toBe(before); expect(card.view.getByText(/Finish in the tab that opened/)).toBeTruthy(); }); +test("claims the tab on the click itself, before the request that mints the url", async () => { + /* + * A browser only lets a page open a tab while a click is still recent — about five seconds in + * Chrome. Opening after the request came back put that window at the mercy of how slow the + * deployment was, and a blocked tab left the card telling somebody to finish in one that was + * never there. + */ + const card = mounted(); + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + // Claimed synchronously: the count has already moved before anything is awaited. + expect(card.tabsClaimed()).toBe(1); + await waitFor(() => + expect(card.sentTo).toContain("https://notion.test/consent"), + ); +}); + +test("offers the link when the browser blocked the tab anyway", async () => { + const card = mounted({ popupsAllowed: false }); + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + const link = await waitFor(() => + card.view.getByRole("link", { name: /Open Notion's sign-in/ }), + ); + expect(link.getAttribute("href")).toBe("https://notion.test/consent"); +}); + +test("says where to type a key for an app that has no page to visit", async () => { + const card = mounted({ answers: "fields" }); + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + const link = await waitFor(() => + card.view.getByRole("link", { name: /Enter it on Notion's page/ }), + ); + expect(link.getAttribute("href")).toBe( + "/settings/connected-accounts/srv_notion", + ); + // The blank tab claimed on the click is let go rather than left sitting on about:blank. + expect(card.tabsClosed()).toBe(1); +}); + +test("lets go of the claimed tab when connecting fails", async () => { + const card = mounted({ answers: "failure" }); + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + await waitFor(() => expect(card.tabsClosed()).toBe(1)); + // The tab was claimed and let go without ever being pointed at the vendor. + expect(card.sentTo).toEqual([]); + expect(card.view.getByText(/could not be connected/)).toBeTruthy(); +}); + test("says so once the account is actually listed, rather than once they return", async () => { - const card = mounted([{ serverId: "srv_notion", holder: "person" }]); + const card = mounted({ + connections: [{ serverId: "srv_notion", holder: "person" }], + }); await waitFor(() => expect(card.view.getByText(/Notion is connected/)).toBeTruthy(), @@ -99,8 +241,25 @@ test("says so once the account is actually listed, rather than once they return" expect(card.view.queryByRole("button", { name: /^Connect$/ })).toBeNull(); }); +test("does not read a withdrawn credential as a connected account", async () => { + /* + * The row outlives the secret. Drawn over the "your access was withdrawn" refusal, a card + * reading the row alone said the app was connected and removed the only way back. + */ + const card = mounted({ + connections: [{ serverId: "srv_notion", holder: "person", revoked: true }], + }); + + await waitFor(() => + expect(card.view.getByRole("button", { name: /^Connect$/ })).toBeTruthy(), + ); + expect(card.view.queryByText(/Notion is connected/)).toBeNull(); +}); + test("does not read a Shared app's deployment account as this person's", async () => { - const card = mounted([{ serverId: "srv_notion", holder: "deployment" }]); + const card = mounted({ + connections: [{ serverId: "srv_notion", holder: "deployment" }], + }); await waitFor(() => expect(card.view.getByRole("button", { name: /^Connect$/ })).toBeTruthy(),