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 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 ( (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 && row.revoked !== true); + + /* + * 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]); + + /** + * 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) => { + /* + * 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); + 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 ( +
+
+

Connect {title}

+

{message}

+
+ {connected ? ( +

+ {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. */} + + {waiting ? ( +

+ 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 + ? 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/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/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/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/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"]); + }); +}); 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 }); + }); +}); diff --git a/app/tests/connect-app-card.test.tsx b/app/tests/connect-app-card.test.tsx new file mode 100644 index 000000000..72203f268 --- /dev/null +++ b/app/tests/connect-app-card.test.tsx @@ -0,0 +1,268 @@ +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 { 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. + * + * 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. + */ + +// 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(); + GlobalRegistrator.unregister(); +}); + +const originalFetch = global.fetch; +afterEach(() => { + global.fetch = originalFetch; + window.open = originalOpen; +}); + +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); + 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") { + 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 json({}); + }) as unknown as typeof fetch; + + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + 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( + + + , + ); + return { + view, + sentTo, + tabsClosed: () => closed, + tabsClaimed: () => claimed, + }; +} + +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(); + + await waitFor(() => + 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; + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + await waitFor(() => + 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({ + connections: [{ 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 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({ + connections: [{ 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/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/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/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. 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; + } +}