diff --git a/CHANGELOG.md b/CHANGELOG.md index 85bcf0b573..5ae34840b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. - Migration `0062_action_policy_labels` adds a `labels` column to `action_policies`, defaulting to empty. Existing boundary rules keep working and show as "Custom rule" until somebody names them. +### An app card carries the task on once the app is ready + +When a Bot stopped because it needed an app, and the person then connected it in another tab or an +administrator turned it on for the Bot, the card said "Ask again". It now sends "Gmail is ready now. +Please carry on." for the person, once, and the Bot picks the task back up; a card opened after the +change offers Continue instead. An administrator looking at the card gets the fix itself, "Turn on +and give it to Sales" or "Give it to Sales", which grants the app's read-only actions with the same +checks and trail as approving a request, or a link to the app's page where one press can't do it. + ### Screens share one spacing, row and empty-state design Every configuration screen now takes its spacing from the page frame: 32px under the title, 48px diff --git a/app/src/components/channels/app-access-card.tsx b/app/src/components/channels/app-access-card.tsx index c41443819d..e0eff73e31 100644 --- a/app/src/components/channels/app-access-card.tsx +++ b/app/src/components/channels/app-access-card.tsx @@ -4,8 +4,10 @@ import { appAccessSentence, } from "@/components/plugins/app-access-action"; import { PluginLogo } from "@/components/plugins/plugin-logo"; +import { Button } from "@/components/ui/button"; import { type AppAccess, botAppQueryOptions } from "@/lib/app-access/queries"; import { useDeclaredBotId } from "@/lib/copilot/active-bot"; +import { type CardPlace, useCarryOn } from "@/lib/copilot/carry-on"; import type { ConnectPrompt } from "@openbot/shared/connect-prompt"; import { ConnectAppCard } from "./connect-app-card"; @@ -31,35 +33,63 @@ import { ConnectAppCard } from "./connect-app-card"; * `reconnect` or `shared` refusal over a live `ready` is drawn as the refusal says — unless the * account was connected after the refusal, which is the reconnection. Anything else live is newer * than the refusal and is drawn as it is. + * + * AND THEN THE TASK CARRIES ON. Once the app is ready — connected in the other tab, switched on and + * given by an admin — the card sends the conversation on by itself, or offers Continue where it was + * not watching when that happened. `place` is where the card sits in its conversation, without + * which (a card drawn outside one) it only says the app is ready. See `lib/copilot/carry-on.ts`. */ -export function AppAccessCard({ prompt }: { prompt: ConnectPrompt }) { +export function AppAccessCard({ + prompt, + place, +}: { + prompt: ConnectPrompt; + place?: CardPlace; +}) { const declared = useDeclaredBotId(); const agentId = prompt.agentId ?? declared; /* * A refusal from before refusals named their Bot, on a surface that declared none: there is no Bot * to ask about, so the card it was drawn as then is the card it is drawn as now. */ - if (!agentId) return ; - return ; + if (!agentId) return ; + return ; } function LiveAppAccessCard({ agentId, + place, prompt, }: { agentId: string; + place: CardPlace | undefined; prompt: ConnectPrompt; }) { - const live = useQuery(botAppQueryOptions(agentId, prompt.serverId)); + const live = useQuery({ + ...botAppQueryOptions(agentId, prompt.serverId), + /* + * Read again whenever this tab has the focus back. What changes the state usually happens in + * another tab — a consent screen, an admin's page for the app — and the person returning here + * is the moment the card should say what is true now. + */ + refetchOnWindowFocus: true, + }); + const access = live.data ? asRefused(live.data, prompt) : null; + const name = access?.displayName ?? prompt.title; + const { carried, offer } = useCarryOn({ + place, + ready: access ? access.state === "ready" : null, + // A refusal was blocked by definition; `show_app_access` says what it found. + blockedWhenAsked: prompt.reason !== "access" || prompt.blocked === true, + app: name, + }); /* * A read that failed falls back to what this card was before it read anything, where that was a * card at all: a person's own missing connection still gets its Connect button. The other reasons * had only ever been a sentence, and the sentence is what is left. */ if (live.error && (prompt.reason ?? "connect") === "connect") - return ; - const access = live.data ? asRefused(live.data, prompt) : null; - const name = access?.displayName ?? prompt.title; + return ; return (
- {access.state === "ready" && prompt.reason !== "access" - ? `${name} is ready now. Ask again and the Bot can use it.` - : refusedSentence(access, prompt)} + {access.state !== "ready" + ? refusedSentence(access, prompt) + : carried || offer + ? `${name} is ready now.` + : prompt.reason !== "access" + ? `${name} is ready now. Ask again and the Bot can use it.` + : refusedSentence(access, prompt)}

) : (

@@ -95,6 +129,12 @@ function LiveAppAccessCard({

+ ) : offer ? ( +
+ +
) : null}
); diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index b439b5c5d7..921a8b1738 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 type { CardPlace } from "@/lib/copilot/carry-on"; import { readConnectPrompt, SHOW_APP_ACCESS_TOOL, @@ -1301,12 +1302,15 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ name, args, result, + latest, }: { delay: number; toolCallId: string; name: string; args: string; result?: string; + /** Nothing said or queued since this call, and the Bot is not working. See `CardPlace`. */ + latest: boolean; }) { const renderToolCall = useRenderToolCall(); const toolCall = useMemo( @@ -1344,7 +1348,13 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ * What was called, shimmering until its result arrives, and then the server's own words * drawn the way a Bot's prose is drawn. */} - {drawn ?? } + {drawn ?? ( + + )} ); @@ -1361,9 +1371,12 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ export function ServerToolLine({ name, result, + place, }: { name: string; result?: string; + /** Where this call sits in the conversation, for an app card that may carry the task on. */ + place?: CardPlace; }) { const running = result === undefined; const { label, detail } = readToolName(name, running); @@ -1398,7 +1411,7 @@ export function ServerToolLine({ : name === SHOW_APP_ACCESS_TOOL ? readConnectPrompt(answer) : null; - if (card) return ; + if (card) return ; /* * THAT TOOL'S OTHER ANSWERS ARE FOR THE MODEL. An app nobody can name comes back as plain text * telling the Bot to say so plainly and not to browse to it — an instruction, which read by the @@ -1441,6 +1454,32 @@ export function isPersonSentMessage( return role === "user" && readFiring(text) === null; } +/** + * WHERE THE CONVERSATION HAS GOT TO, for an app card that may carry its task on once the app is + * ready (see `lib/copilot/carry-on.ts`): the index from which a row is still the latest thing, or + * past the end when none is. + * + * A card is the latest thing while nothing has been said to the Bot since it, nothing is queued, and + * the Bot is not working: past that, the conversation has moved on, and an older task is not picked + * up again underneath it. ANY USER TURN COUNTS, a routine firing included — unlike the scroll + * machinery's `isPersonSentMessage`. A firing is not the person speaking, but it is a new task the + * Bot has answered, and "please carry on" sent after it would land on the routine's work rather than + * on the task the card interrupted. Exported so it can be checked without mounting anything. + */ +export function latestFrom( + items: readonly TranscriptItem[], + { busy, queued }: { busy: boolean; queued: number }, +): number { + if (busy || queued > 0) return items.length; + return ( + items.findLastIndex( + (item) => + (item.kind === "text" && item.role === "user") || + item.kind === "attachments", + ) + 1 + ); +} + /** * The turn a row belongs to. * @@ -1602,6 +1641,9 @@ export function ChatTranscript({ )?.id ?? null; useSmoothSendScroll(viewportRef, newestUserMessageId); + /* Which tool calls are still the latest thing, for an app card: see `latestFrom`. */ + const latestIndex = latestFrom(items, { busy, queued: queued.length }); + /* One rule for both kinds of row — see `anchorRowIds`, which is where it is written down. */ const anchorRows = anchorRowIds(items); @@ -1723,6 +1765,7 @@ export function ChatTranscript({ = latestIndex} name={item.toolCall.function.name} result={item.result} toolCallId={item.toolCall.id} diff --git a/app/src/components/channels/connect-app-card.tsx b/app/src/components/channels/connect-app-card.tsx index d89372fa4b..241519654e 100644 --- a/app/src/components/channels/connect-app-card.tsx +++ b/app/src/components/channels/connect-app-card.tsx @@ -3,6 +3,7 @@ import { Link } from "@tanstack/react-router"; import { useEffect, useRef } from "react"; import { useConnectInNewTab } from "@/components/plugins/connect-in-new-tab"; import { Button } from "@/components/ui/button"; +import { type CardPlace, useCarryOn } from "@/lib/copilot/carry-on"; import { connectionsQueryOptions, personalConnections, @@ -26,15 +27,19 @@ import { queryClient } from "@/query-client"; * draws it from the app's live state for this person on that Bot. This one is drawn only where no * Bot can be named — a refusal written before refusals carried one, on a surface that declared none * — and it can only offer Connect, which is all such a refusal ever meant. + * + * Once connected it carries the conversation on, as `AppAccessCard` does: see `useCarryOn`. */ export function ConnectAppCard({ serverId, title, message, + place, }: { serverId: string; title: string; message: string; + place?: CardPlace | undefined; }) { const { open: openTab, @@ -59,6 +64,13 @@ export function ConnectAppCard({ const connected = personalConnections( connections.data?.connections ?? [], ).some((row) => row.serverId === serverId && row.revoked !== true); + // Every card drawn here is a refusal, so the app was blocked when the Bot asked. + const { carried, offer } = useCarryOn({ + place, + ready: connections.data ? connected : null, + blockedWhenAsked: true, + app: title, + }); /* * The grants a Bot holds are read per conversation, so the turn that follows has to be asked @@ -85,9 +97,18 @@ export function ConnectAppCard({

{message}

{connected ? ( -

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

+
+

+ {carried || offer + ? `${title} is connected.` + : `${title} is connected. Ask again and the Bot can use it.`} +

+ {offer ? ( + + ) : null} +
) : needsKey ? (

{title} needs a key from you.{" "} diff --git a/app/src/components/plugins/app-access-action.tsx b/app/src/components/plugins/app-access-action.tsx index 484bbfa3d3..42fe1cfbd5 100644 --- a/app/src/components/plugins/app-access-action.tsx +++ b/app/src/components/plugins/app-access-action.tsx @@ -1,13 +1,16 @@ -import { useMutation } from "@tanstack/react-query"; +import { useMutation, useQuery } from "@tanstack/react-query"; import { Link } from "@tanstack/react-router"; import { useEffect, useRef } from "react"; import { useConnectInNewTab } from "@/components/plugins/connect-in-new-tab"; import { Button } from "@/components/ui/button"; +import { agentQueryOptions } from "@/lib/agents/queries"; import { + approveBotAppMutationOptions, cancelAppAccessRequestMutationOptions, requestAppAccessMutationOptions, } from "@/lib/app-access/mutations"; import { type AppAccess, appAccessKeys } from "@/lib/app-access/queries"; +import { currentUserQueryOptions } from "@/lib/auth/queries"; import { queryClient } from "@/query-client"; /** @@ -92,7 +95,8 @@ function disabledSentence(access: AppAccess): string { * that already has a row (which the connect route refuses until it is disconnected), go to the * app's own page in Settings instead, where the form and Disconnect are. * - the three an administrator fixes — Request access, which files the request the server decides - * the kind of, and then Requested with a way to withdraw it. + * the kind of, and then Requested with a way to withdraw it. Pressed by an administrator, it would + * be a request to themselves, so they get the fix itself instead: see `AdminAction`. */ export function AppAccessAction({ agentId, @@ -101,15 +105,118 @@ export function AppAccessAction({ agentId: string; access: AppAccess; }) { + /* + * WHO IS LOOKING decides only which button is drawn, never what is allowed: the server answers + * the one press 403 for anybody else, and `SharedAppNotice` already reads the role the same way. + */ + const me = useQuery(currentUserQueryOptions()).data; if (access.state === "ready" || access.state === "shared_not_approved") return null; if (access.state === "reconnect" && access.accountHolder === "owner") return null; if (access.state === "not_connected" || access.state === "reconnect") return ; + if (me?.role === "admin") + return ; return ; } +/** + * The step only an administrator can take, offered to the administrator as the step itself. + * + * ONE PRESS WHERE ONE PRESS CAN DO IT. An app nobody switched on, or one this Bot was never given, + * is switched on and given here through the inbox's own approval (`approveBotAppMutationOptions`): + * the same checks, the read-only actions only, and the same trail. Anything that sends or writes is + * still chosen on the Bot's page. + * + * A LINK WHERE IT CANNOT. A broker with no key, a set-up somebody left unfinished and a team account + * nobody connected all need something only typed or consented to on the app's admin page. That page + * opens in a new tab so the conversation stays where it is, and the card reads the app again when + * this tab has the focus back. + */ +function AdminAction({ + agentId, + access, +}: { + agentId: string; + access: AppAccess; +}) { + const bot = useQuery(agentQueryOptions(agentId)).data?.name ?? "this Bot"; + const approve = useMutation(approveBotAppMutationOptions(queryClient)); + const name = access.displayName; + + const oneClick = + access.state === "not_granted" || + (access.state === "disabled" && + (access.unavailable === null || access.unavailable === "absent")); + if (oneClick) { + const disabled = access.state === "disabled"; + const label = disabled + ? `Turn on and give it to ${bot}` + : `Give it to ${bot}`; + // The card names the app beside the button; on its own, the button has to. + const spoken = disabled + ? `Turn on ${name} and give it to ${bot}` + : `Give ${name} to ${bot}`; + return ( +

+ {approve.error ? ( +

+ {approve.error.message} +

+ ) : null} + +
+ ); + } + + const page = + access.state === "disabled" && + access.unavailable === "composio_unconfigured" + ? access.provider === "arcade" + ? "arcade" + : "composio" + : access.key; + const [label, spoken] = + access.state !== "shared_not_connected" + ? ["Set it up", `Set up ${name}`] + : access.requestKind + ? ["Connect the team account", `Connect the team ${name} account`] + : ["Reconnect the team account", `Reconnect the team ${name} account`]; + return ( + + ); +} + /** The person's own account: connect it, or reconnect it. */ function ConnectAction({ access }: { access: AppAccess }) { const label = access.state === "reconnect" ? "Reconnect" : "Connect"; diff --git a/app/src/lib/app-access/mutations.ts b/app/src/lib/app-access/mutations.ts index 31131a5df2..f550bbd847 100644 --- a/app/src/lib/app-access/mutations.ts +++ b/app/src/lib/app-access/mutations.ts @@ -85,6 +85,32 @@ export function approveAppAccessRequestMutationOptions( }); } +/** + * An administrator's one press: file the request this app needs on this Bot and approve it in the + * same step, through the inbox's own approval — the app switched on and its read-only actions + * granted, with the trail two presses would leave. Answers with what changed, or the sentence for + * what is missing (an OAuth client, a broker key) when it cannot be done from here. + */ +export function approveBotAppMutationOptions(queryClient: QueryClient) { + return mutationOptions({ + mutationFn: async (variables: { + agentId: string; + app: string; + }): Promise<{ + request: AppAccessRequest; + enabled: boolean; + granted: string[]; + }> => { + const response = await client( + `/api/plugins/for/${encodeURIComponent(variables.agentId)}/apps/${encodeURIComponent(variables.app)}/approve`, + { method: "POST", fallback: "Couldn't turn that app on. Try again." }, + ); + return response.json(); + }, + onSettled: () => refresh(queryClient), + }); +} + /** Dismiss a request without changing anything else. */ export function dismissAppAccessRequestMutationOptions( queryClient: QueryClient, diff --git a/app/src/lib/copilot/carry-on.ts b/app/src/lib/copilot/carry-on.ts new file mode 100644 index 0000000000..f01fe4360d --- /dev/null +++ b/app/src/lib/copilot/carry-on.ts @@ -0,0 +1,119 @@ +import { useEffect, useRef, useState } from "react"; +import { useConversation } from "./conversation"; + +/** + * Carrying on with the task an app card interrupted, once the app is ready, without the person + * having to type their question again. + * + * WHY NOT THE HANDOFF RESUME. `handoff-resume.ts` answers a browser tool call that is still open: + * the turn is paused inside the call, and resuming hands that call its result. An app card is drawn + * from a call that has already answered — a refusal, or `show_app_access` — and the turn it was in + * has ended with the Bot saying what it needs. There is nothing open to answer. What carries the + * task on is simply the next turn, so that is what this sends: one short message, as the person, + * through the same `ask` a button inside any transcript card uses. + * + * A MESSAGE RATHER THAN A HIDDEN RUN, deliberately. It is what the person would have typed, it + * shows in the transcript so nobody wonders why the Bot started again, and it reaches the Bot with + * the whole conversation behind it, so the Bot picks up the task it had stopped on. A run with no + * new message would have the model guessing why it was woken. + * + * ONCE, AND ONLY WHILE NOTHING ELSE HAS HAPPENED. The card counts as the latest thing only while + * the person has said nothing since it, has nothing queued, and the Bot is not working (see + * `ChatTranscript`). A person who moved on is not dragged back to an older task. And it is sent + * once per card, across re-renders, remounts and other tabs on this browser, by a mark kept in + * `localStorage` under the tool call the card was drawn for, set before the message is sent. + * + * SEEN, OR OFFERED. A card that watched the app go from blocked to ready carries on by itself: the + * person was here, and the change is what they were waiting for. A card drawn already ready, in a + * conversation opened after the change (an admin approved overnight), cannot know the person still + * wants the task done, so it offers Continue and leaves it to them. + */ + +/** Where an app card sits in its conversation, as the transcript knows it. */ +export type CardPlace = { + /** The tool call the card was drawn for: what "only once" is counted against. */ + toolCallId: string; + /** Nothing said or queued since the card, and the Bot is not working. */ + latest: boolean; +}; + +const MARK = "openbot:carried-on:"; + +/** Marks kept here as well, for a browser that refuses `localStorage` (a private window, a policy). */ +const carriedHere = new Set(); + +function hasCarriedOn(toolCallId: string): boolean { + if (carriedHere.has(toolCallId)) return true; + try { + return window.localStorage.getItem(MARK + toolCallId) !== null; + } catch { + return false; + } +} + +/** Mark this card as carried on. False when something already had: this caller must not send. */ +function markCarriedOn(toolCallId: string): boolean { + if (hasCarriedOn(toolCallId)) return false; + carriedHere.add(toolCallId); + try { + window.localStorage.setItem(MARK + toolCallId, new Date().toISOString()); + } catch { + // The in-memory mark still holds for this tab. + } + return true; +} + +/** What is sent as the person when the task carries on. Short, and true whoever made it ready. */ +export function carryOnMessage(app: string): string { + return `${app} is ready now. Please carry on.`; +} + +/** + * Carry on when this card's app becomes ready, or offer to. + * + * `ready` is null until the card knows; only a known `false` counts as having seen the app blocked. + * `blockedWhenAsked` says whether the app was out of reach when the Bot asked — what lets a card + * drawn already ready offer Continue only where something changed since. Returns `carried` once + * this card has carried on (here or in another tab), and `offer` when a Continue is worth drawing. + */ +export function useCarryOn({ + place, + ready, + blockedWhenAsked, + app, +}: { + place: CardPlace | undefined; + ready: boolean | null; + blockedWhenAsked: boolean; + app: string; +}): { carried: boolean; offer: (() => void) | null } { + const ask = useConversation()?.ask; + const sawBlocked = useRef(false); + // Only re-renders this card once it has sent, so `carried` is read again. + const [, rerender] = useState(0); + useEffect(() => { + if (ready === false) sawBlocked.current = true; + }, [ready]); + + const toolCallId = place?.toolCallId; + const carried = toolCallId ? hasCarriedOn(toolCallId) : false; + const can = Boolean(ask && toolCallId && place?.latest && ready === true); + + const send = () => { + if (!ask || !toolCallId || !markCarriedOn(toolCallId)) return; + ask(carryOnMessage(app)); + rerender((count) => count + 1); + }; + const sendRef = useRef(send); + sendRef.current = send; + + useEffect(() => { + if (can && sawBlocked.current) sendRef.current(); + }, [can]); + + return { + carried, + offer: + can && !carried && !sawBlocked.current && blockedWhenAsked ? send : null, + }; +} diff --git a/app/tests/app-access-action.test.tsx b/app/tests/app-access-action.test.tsx index 1eb3e7a327..904d235ab3 100644 --- a/app/tests/app-access-action.test.tsx +++ b/app/tests/app-access-action.test.tsx @@ -56,6 +56,7 @@ type Sent = { method: string; url: string; body: unknown }; function mounted( access: AppAccess, answer?: { status: number; body: unknown }, + options: { admin?: boolean } = {}, ) { const sent: Sent[] = []; const opened: string[] = []; @@ -79,6 +80,10 @@ function mounted( url, body: init?.body ? JSON.parse(String(init.body)) : undefined, }); + if (options.admin && url === "/api/me") + return Response.json({ user: { id: "usr_admin", role: "admin" } }); + if (options.admin && url === "/api/agents/expenses") + return Response.json({ agent: { id: "expenses", name: "Expenses" } }); if (answer && method !== "GET") return Response.json(answer.body, { status: answer.status }); if (url.endsWith("/connect?returnTo=settings")) @@ -112,8 +117,13 @@ function mounted( path: "/connections/$key", component: () => null, }); + const admin = createRoute({ + getParentRoute: () => root, + path: "/admin/plugins/$key", + component: () => null, + }); const router = createRouter({ - routeTree: root.addChildren([page, settings]), + routeTree: root.addChildren([page, settings, admin]), history: createMemoryHistory({ initialEntries: ["/"] }), }); const view = render( @@ -442,3 +452,82 @@ test("the focus follows the press: Request access hands it to Withdraw, and With await view.findByRole("button", { name: "Request access to Gmail" }), ); }); + +test.each([ + [ + "disabled", + "Turn on and give it to Expenses", + "Turn on Gmail and give it to Expenses", + ], + ["not_granted", "Give it to Expenses", "Give Gmail to Expenses"], +] as const)( + "an admin looking at %s gets the fix itself, in one press, never a request to themselves", + async (state, label, spoken) => { + const { view, sent } = mounted( + appAccess({ + state, + unavailable: state === "disabled" ? "absent" : null, + requestKind: state === "disabled" ? "enable" : "grant", + }), + undefined, + { admin: true }, + ); + const button = await view.findByRole("button", { name: spoken }); + expect(button.textContent).toBe(label); + expect( + view.queryByRole("button", { name: "Request access to Gmail" }), + ).toBeNull(); + fireEvent.click(button); + await waitFor(() => + expect(sent.find((call) => call.method === "POST")).toStrictEqual({ + method: "POST", + url: "/api/plugins/for/expenses/apps/composio-gmail/approve", + body: undefined, + }), + ); + }, +); + +test("an admin's press that cannot be carried out says what is missing", async () => { + const { view } = mounted( + appAccess({ state: "not_granted", requestKind: "grant" }), + { status: 409, body: { error: "Gmail has no tools to give yet." } }, + { admin: true }, + ); + fireEvent.click( + await view.findByRole("button", { name: "Give Gmail to Expenses" }), + ); + expect((await view.findByRole("alert")).textContent).toBe( + "Gmail has no tools to give yet.", + ); +}); + +test.each([ + [ + { state: "shared_not_connected", requestKind: "connect_shared" }, + "Connect the team Gmail account", + "/admin/plugins/composio-gmail", + ], + [ + { state: "disabled", unavailable: "token_missing", requestKind: "enable" }, + "Set up Gmail", + "/admin/plugins/composio-gmail", + ], + [ + { + state: "disabled", + unavailable: "composio_unconfigured", + requestKind: "enable", + }, + "Set up Gmail", + "/admin/plugins/composio", + ], +] as const)( + "an admin gets the page that fixes what one press cannot, in a new tab", + async (overrides, name, href) => { + const { view } = mounted(appAccess(overrides), undefined, { admin: true }); + const link = await view.findByRole("button", { name }); + expect(link.getAttribute("href")).toBe(href); + expect(link.getAttribute("target")).toBe("_blank"); + }, +); diff --git a/app/tests/app-access-card.test.tsx b/app/tests/app-access-card.test.tsx index 5530127fac..975bd4da3b 100644 --- a/app/tests/app-access-card.test.tsx +++ b/app/tests/app-access-card.test.tsx @@ -1,4 +1,11 @@ -import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; +import { + afterAll, + afterEach, + beforeAll, + describe, + expect, + test, +} from "bun:test"; import { GlobalRegistrator } from "@happy-dom/global-registrator"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { @@ -9,12 +16,20 @@ import { Outlet, RouterProvider, } from "@tanstack/react-router"; -import { cleanup, render, waitFor } from "@testing-library/react"; +import { + act, + cleanup, + fireEvent, + render, + waitFor, +} from "@testing-library/react"; import type { ReactNode } from "react"; import { AppAccessCard } from "@/components/channels/app-access-card"; import { ServerToolLine } from "@/components/channels/chat-transcript"; import type { AppAccess } from "@/lib/app-access/queries"; import { ActiveBotProvider, useActiveBot } from "@/lib/copilot/active-bot"; +import { carryOnMessage } from "@/lib/copilot/carry-on"; +import { ConversationProvider } from "@/lib/copilot/conversation"; import { REFUSAL_MARKER } from "@/lib/plugins/tool-result"; import { type ConnectPrompt, @@ -83,11 +98,12 @@ function mount(children: ReactNode) { routeTree: root.addChildren([page, settings]), history: createMemoryHistory({ initialEntries: ["/"] }), }); - return render( + const view = render( , ); + return Object.assign(view, { client }); } const prompt = (overrides: Partial = {}): ConnectPrompt => ({ @@ -388,3 +404,142 @@ test("show_app_access about an app nobody can name draws the line, never its wor // Nothing behind a disclosure either: the line is not expandable. expect(view.container.querySelector("details")).toBeNull(); }); + +/* + * CARRYING ON. Once the app is ready the conversation goes on by itself — one message, as the + * person, through the conversation's `ask` — but only from the card that is still the latest thing, + * only once, and only where the card saw the change; a card drawn already ready offers Continue. + */ +describe("carrying the task on once the app is ready", () => { + let calls = 0; + const nextId = () => { + calls += 1; + return `call_${calls}_${Math.random().toString(36).slice(2)}`; + }; + + /** Answers the live read with whatever `state.live` is now, so a test can change it. */ + function servingNow(state: { live: AppAccess }) { + global.fetch = (async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/apps/")) return Response.json({ app: state.live }); + if (url.includes("/api/plugins/connections")) + return Response.json({ connections: [], redirectUri: null }); + return Response.json({}); + }) as unknown as typeof fetch; + } + + function inConversation( + children: ReactNode, + ): ReturnType & { asked: string[] } { + const asked: string[] = []; + const view = mount( + asked.push(text)}> + {children} + , + ); + return Object.assign(view, { asked }); + } + + test("a card that sees the app become ready carries on, once", async () => { + const state = { live: appAccess({ state: "not_connected" }) }; + servingNow(state); + const place = { toolCallId: nextId(), latest: true }; + const view = inConversation( + , + ); + expect((await card(view)).getAttribute("data-state")).toBe("not_connected"); + state.live = appAccess({ state: "ready" }); + await act(() => view.client.invalidateQueries()); + await waitFor(() => expect(view.asked).toEqual([carryOnMessage("Gmail")])); + expect((await card(view)).textContent).toContain("Gmail is ready now."); + expect(view.container.textContent).not.toContain("Ask again"); + // Read again and drawn again: still the one message. + await act(() => view.client.invalidateQueries()); + await settleReactWork(); + expect(view.asked).toHaveLength(1); + expect(view.queryByRole("button", { name: "Continue" })).toBeNull(); + }); + + test("a card the conversation has moved on from only says the app is ready", async () => { + const state = { live: appAccess({ state: "not_connected" }) }; + servingNow(state); + const view = inConversation( + , + ); + await card(view); + state.live = appAccess({ state: "ready" }); + await act(() => view.client.invalidateQueries()); + await waitFor(async () => + expect((await card(view)).getAttribute("data-state")).toBe("ready"), + ); + await settleReactWork(); + expect(view.asked).toEqual([]); + expect((await card(view)).textContent).toContain( + "Gmail is ready now. Ask again and the Bot can use it.", + ); + }); + + test("a card drawn already ready offers Continue, which carries on once", async () => { + servingNow({ live: appAccess({ state: "ready" }) }); + const place = { toolCallId: nextId(), latest: true }; + const view = inConversation( + , + ); + fireEvent.click(await view.findByRole("button", { name: "Continue" })); + expect(view.asked).toEqual([carryOnMessage("Gmail")]); + await waitFor(() => + expect(view.queryByRole("button", { name: "Continue" })).toBeNull(), + ); + cleanup(); + // The same card drawn again, here or in another tab, has nothing left to offer. + const again = inConversation( + , + ); + expect((await card(again)).textContent).toContain("Gmail is ready now."); + expect(again.queryByRole("button", { name: "Continue" })).toBeNull(); + expect(again.asked).toEqual([]); + }); + + test.each([ + [true, true], + [false, false], + [undefined, false], + ])( + "show_app_access drawn ready offers Continue only if the app was blocked when asked (blocked: %p)", + async (blocked, offered) => { + servingNow({ live: appAccess({ state: "ready" }) }); + const view = inConversation( + , + ); + await card(view); + await settleReactWork(); + expect(view.queryByRole("button", { name: "Continue" }) !== null).toBe( + offered, + ); + expect(view.asked).toEqual([]); + }, + ); + + test("with no conversation around it, the card only says the app is ready", async () => { + servingNow({ live: appAccess({ state: "ready" }) }); + const view = mount( + , + ); + expect((await card(view)).textContent).toContain( + "Ask again and the Bot can use it.", + ); + expect(view.queryByRole("button", { name: "Continue" })).toBeNull(); + }); +}); diff --git a/app/tests/app-access-mutations.test.ts b/app/tests/app-access-mutations.test.ts index 91f4a06ffd..30e0292890 100644 --- a/app/tests/app-access-mutations.test.ts +++ b/app/tests/app-access-mutations.test.ts @@ -2,6 +2,7 @@ import { afterEach, expect, test } from "bun:test"; import { MutationObserver, QueryClient } from "@tanstack/react-query"; import { approveAppAccessRequestMutationOptions, + approveBotAppMutationOptions, cancelAppAccessRequestMutationOptions, dismissAppAccessRequestMutationOptions, requestAppAccessMutationOptions, @@ -106,6 +107,22 @@ test("approving sends refs only when given", async () => { expect(sent[0]).not.toHaveProperty("body"); }); +test("an admin's one press names the Bot and the app in the path, and sends no refs", async () => { + const sent = answering(200, { request: {}, enabled: true, granted: [] }); + const { asked } = await run(approveBotAppMutationOptions, { + agentId: "bot 1", + app: "composio-gmail", + }); + expect(sent).toEqual([ + { + url: "/api/plugins/for/bot%201/apps/composio-gmail/approve", + method: "POST", + }, + ]); + expect(sent[0]).not.toHaveProperty("body"); + expect(asked).toEqual([{ queryKey: pluginKeys.all }]); +}); + test("a refused write still refreshes every plugin screen", async () => { for (const build of [ requestAppAccessMutationOptions, diff --git a/app/tests/connect-app-card.test.tsx b/app/tests/connect-app-card.test.tsx index 93e3fd5872..16d37fd6eb 100644 --- a/app/tests/connect-app-card.test.tsx +++ b/app/tests/connect-app-card.test.tsx @@ -11,6 +11,8 @@ import { } from "@tanstack/react-router"; import { cleanup, fireEvent, render, waitFor } from "@testing-library/react"; import { ConnectAppCard } from "@/components/channels/connect-app-card"; +import { carryOnMessage } from "@/lib/copilot/carry-on"; +import { ConversationProvider } from "@/lib/copilot/conversation"; import { settleReactWork } from "./settle-react-work"; /** @@ -61,6 +63,8 @@ function mounted( answers?: "url" | "fields" | "failure" | "connected"; /** False stands in for a browser that refused the tab, which answers null. */ popupsAllowed?: boolean; + /** Drawn inside a conversation, as the latest thing in it: what the card sends is kept here. */ + asked?: string[]; } = {}, ) { const { connections = [], answers = "url", popupsAllowed = true } = options; @@ -122,17 +126,30 @@ function mounted( const client = new QueryClient({ defaultOptions: { queries: { retry: false } }, }); + // One card, one tool call, however often the route draws it. + const toolCallId = `call_${Math.random().toString(36).slice(2)}`; const root = createRootRoute({ component: Outlet }); const card = createRoute({ getParentRoute: () => root, path: "/", - component: () => ( - - ), + component: () => { + const drawn = ( + + ); + const { asked } = options; + return asked ? ( + asked.push(text)}> + {drawn} + + ) : ( + drawn + ); + }, }); // The destination the card links to for an app connected with a typed key. const settings = createRoute({ @@ -299,3 +316,16 @@ test("does not read a Shared app's deployment account as this person's", async ( ); expect(card.view.queryByText(/Notion is connected/)).toBeNull(); }); + +test("once connected in front of the person, the conversation carries on by itself, once", async () => { + const asked: string[] = []; + const card = mounted({ answers: "connected", asked }); + await waitFor(() => card.view.getByRole("button", { name: /^Connect$/ })); + + press(card); + + await waitFor(() => expect(asked).toEqual([carryOnMessage("Notion")])); + expect(card.view.getByText("Notion is connected.")).toBeTruthy(); + await settleReactWork(); + expect(asked).toHaveLength(1); +}); diff --git a/app/tests/transcript-latest.test.ts b/app/tests/transcript-latest.test.ts new file mode 100644 index 0000000000..6718bc6bcc --- /dev/null +++ b/app/tests/transcript-latest.test.ts @@ -0,0 +1,75 @@ +import { describe, expect, test } from "bun:test"; +import { frameFiring } from "@openbot/shared/routine-firing"; +import type { VisibleChatItem } from "../src/components/channels/chat-messages"; +import { latestFrom } from "../src/components/channels/chat-transcript"; + +/** + * Which rows of a transcript are still the latest thing, the gate an app card passes before it + * carries its task on by itself (see `lib/copilot/carry-on.ts`). A row is latest only while nothing + * has been said to the Bot since it, nothing is queued, and the Bot is not working. + */ + +const said = (id: string, text = "find my invoices"): VisibleChatItem => ({ + kind: "text", + id, + role: "user", + text, +}); +const answered = (id: string): VisibleChatItem => ({ + kind: "text", + id, + role: "assistant", + text: "Gmail isn't connected yet.", +}); +const card = (id: string): VisibleChatItem => ({ + kind: "tool", + id, + toolCall: { + id, + type: "function", + function: { name: "show_app_access", arguments: "{}" }, + }, + result: "{}", +}); +const idle = { busy: false, queued: 0 }; + +describe("latestFrom", () => { + test("a card and the Bot's words after it are latest once the Bot is idle", () => { + const items = [said("u1"), card("c1"), answered("a1")]; + expect(latestFrom(items, idle)).toBe(1); + }); + + test("a card the person has spoken after is not", () => { + const items = [said("u1"), card("c1"), answered("a1"), said("u2")]; + expect(latestFrom(items, idle)).toBe(items.length); + }); + + test("a file sent with no caption moves the conversation on too", () => { + const items: VisibleChatItem[] = [ + said("u1"), + card("c1"), + { kind: "attachments", id: "u2:attachments", attachments: [] }, + ]; + expect(latestFrom(items, idle)).toBe(items.length); + }); + + test("a routine firing after the card moves it on, though nobody typed it", () => { + const items = [ + said("u1"), + card("c1"), + said("f1", frameFiring("Summarise the support queue.")), + answered("a2"), + ]; + expect(latestFrom(items, idle)).toBe(3); + }); + + test("nothing is latest while the Bot works or a message waits", () => { + const items = [said("u1"), card("c1")]; + expect(latestFrom(items, { busy: true, queued: 0 })).toBe(items.length); + expect(latestFrom(items, { busy: false, queued: 1 })).toBe(items.length); + }); + + test("a conversation the Bot started has every row latest", () => { + expect(latestFrom([card("c1")], idle)).toBe(0); + }); +}); diff --git a/server/src/plugins/app-access-tool.ts b/server/src/plugins/app-access-tool.ts index c9c4ec25ab..3c9a60fdf9 100644 --- a/server/src/plugins/app-access-tool.ts +++ b/server/src/plugins/app-access-tool.ts @@ -87,6 +87,8 @@ export function appAccessTools(options: { title: access.displayName, agentId: options.botId, reason: "access", + // Lets a card drawn later tell "ready since" from "ready all along". + blocked: access.state !== "ready", message: toldTheModel(access), }); }, diff --git a/server/src/plugins/route-groups/app-access.ts b/server/src/plugins/route-groups/app-access.ts index a3c2364ccb..bc6a782b9d 100644 --- a/server/src/plugins/route-groups/app-access.ts +++ b/server/src/plugins/route-groups/app-access.ts @@ -1,7 +1,11 @@ import type { Context } from "hono"; import { type AppVariables, requireAdmin } from "../../auth/guards"; import { BROKERED_PROFILE, type BrokerProvider } from "../access"; -import type { Unavailable } from "../app-access"; +import type { + AppAccess, + AppAccessRequestKind, + Unavailable, +} from "../app-access"; import type { CatalogueEntry } from "../catalogue"; import type { AppAccessRequestRecord, @@ -64,6 +68,7 @@ function unavailableSentence( reason: Unavailable, title: string, provider: BrokerProvider, + again = "approve this request again", ): string { switch (reason) { case "absent": @@ -71,14 +76,14 @@ function unavailableSentence( case "composio_unconfigured": { const { vendor, envVar } = BROKERED_PROFILE[provider]; const article = /^[aeiou]/i.test(vendor) ? "an" : "a"; - return `${title} is ${article} ${vendor} app, and ${envVar} isn't set. Set it, then approve this request again.`; + return `${title} is ${article} ${vendor} app, and ${envVar} isn't set. Set it, then ${again}.`; } case "scheme_unreadable": - return `${title} is turned on, but OpenBot can't read how it connects. Remove it and turn it on again under Plugins, then approve this request again.`; + return `${title} is turned on, but OpenBot can't read how it connects. Remove it and turn it on again under Plugins, then ${again}.`; case "oauth_client_missing": - return `${title} has no OAuth client yet. Register one on its page under Plugins, then approve this request again.`; + return `${title} has no OAuth client yet. Register one on its page under Plugins, then ${again}.`; case "token_missing": - return `${title} has no token yet. Add one on its page under Plugins, then approve this request again.`; + return `${title} has no token yet. Add one on its page under Plugins, then ${again}.`; } } @@ -152,6 +157,214 @@ export function mountAppAccessRoutes({ return context.json({ error: input.error }, input.status); }; + /** + * Carry out a request already claimed by this administrator: switch the app on, grant its tools to + * the Bot, or — for a team account, which only a consent screen can connect — confirm it has been + * connected. Answers with the response to send, and marks `done` once the change is carried out; + * one that was not leaves the claim for the caller to put back. + * + * SHARED BY THE INBOX'S APPROVE AND AN ADMINISTRATOR'S ONE PRESS ON A CARD, so the second can never + * grant more, check less or leave a thinner trail than the first. `direct` marks the trail row of + * the one press, whose request was filed and approved by the same person in the same breath. + */ + const carryOut = async ( + context: RouteContext, + request: AppAccessRequestRecord, + chosen: string[] | undefined, + done: { carriedOut: boolean }, + direct = false, + ): Promise => { + const deny = ( + reason: string, + error: string, + status: 400 | 404 | 409 | 502 | 503, + ) => + refused(context, { + app: request.appKey, + request: request.id, + bot: request.agentId, + kind: request.kind, + reason, + error, + status, + }); + + const inspect = () => + store.inspectApp({ + app: request.appKey, + agentId: request.agentId ?? "", + userId: null, + composioConfigured, + directory, + }); + + // The one press filed no request the admin could approve again: they press again instead. + const again = direct ? "try again" : "approve this request again"; + + let inspected = await inspect(); + if (!inspected) + return deny( + "no_such_app", + `${request.appTitle} isn't available anymore.`, + 409, + ); + + let enabled = false; + if (request.kind !== "connect_shared" && inspected.unavailable) { + const title = inspected.access.displayName; + if (inspected.unavailable !== "absent") + return deny( + inspected.unavailable, + unavailableSentence( + inspected.unavailable, + title, + activeProvider, + again, + ), + inspected.unavailable === "composio_unconfigured" ? 503 : 409, + ); + if (inspected.toolkit) { + if (!composio) + return deny( + "composio_unconfigured", + unavailableSentence( + "composio_unconfigured", + title, + activeProvider, + again, + ), + 503, + ); + const outcome = await enableComposioApp( + { store, composio, activeProvider, activeVendor }, + inspected.toolkit, + actorEmail(context), + ); + if (!outcome.ok) + return deny("enable_failed", outcome.error, outcome.status); + } else if (inspected.entry) { + const needed = adminInputFor(inspected.entry); + if (needed) + return deny( + "needs_admin_input", + `${title} needs ${needed}. Add it on its page under Plugins, then ${again}.`, + 409, + ); + try { + await store.addServer({ + key: inspected.entry.key, + by: actorEmail(context), + }); + } catch (error) { + if ( + error instanceof CatalogueEntryUnknownError || + error instanceof CustomServerRefusedError + ) + return deny("enable_failed", error.message, 400); + if (isDeploymentFault(error)) + return deny("enable_failed", deploymentFaultSentence(error), 409); + throw error; + } + } else { + return deny( + "absent", + unavailableSentence("absent", title, activeProvider), + 409, + ); + } + enabled = true; + inspected = await inspect(); + if (!inspected || inspected.unavailable) + return deny( + "enable_failed", + `${title} was turned on but still can't be used. Open its page under Plugins to see why.`, + 409, + ); + } + + let granted: string[] = []; + if ( + request.kind !== "connect_shared" && + request.agentId && + !inspected.access.granted + ) { + const agentId = request.agentId; + const serverId = inspected.answeringId; + const name = inspected.access.displayName; + if (!(await store.agentIsRegistered(agentId)) || !serverId) + return deny("no_such_bot", "That Bot is no longer here.", 409); + const tools = await store.appTools(serverId); + const advertised = tools.map((tool) => tool.ref); + const refs = + chosen ?? tools.filter((tool) => tool.read).map((tool) => tool.ref); + const outside = refs.filter((ref) => !advertised.includes(ref)); + if (outside.length > 0) + return deny( + "refs_outside_app", + `${outside.join(", ")} ${outside.length === 1 ? "is" : "are"} not among ${name}'s tools.`, + 400, + ); + if (advertised.length === 0) + return deny( + "no_tools", + `${name} has no tools to give yet. Refresh its tools on its page under Plugins, then ${again}.`, + 409, + ); + if (refs.length === 0) + return deny( + "no_read_only_tools", + `None of ${name}'s actions are marked read-only, so approving here gives this Bot nothing. Choose its actions on ${name}'s page under Plugins, then ${again}.`, + 409, + ); + for (const ref of refs) { + await store.grant("mcp", ref, agentId, actorEmail(context)); + } + const sharedDecision = await sharedGrantApproval( + { store, shared }, + refs[0] ?? "", + agentId, + undefined, + ); + if (sharedDecision !== "unreadable" && sharedDecision.approval && shared) + await shared.use.setApproval({ + botId: agentId, + serverId: sharedDecision.appId, + approval: sharedDecision.approval, + by: context.var.actor.id, + }); + granted = refs; + } + + if ( + request.kind === "connect_shared" && + inspected.access.account === "shared" && + !inspected.sharedUsable + ) + return deny( + "shared_not_connected", + `The shared ${inspected.access.displayName} account is not connected yet. Connect it on ${inspected.access.displayName}'s page under Plugins, then approve this request.`, + 409, + ); + + done.carriedOut = true; + const closed = request; + await store.recordAppAccessEvent({ + eventType: "app_access.approved", + targetId: closed.appKey, + actorUserId: context.var.actor.id, + payload: { + request: closed.id, + app: closed.appKey, + bot: closed.agentId, + kind: closed.kind, + enabled, + granted: granted.length, + ...(direct ? { direct: true } : {}), + }, + }); + return context.json({ request: closed, enabled, granted }); + }; + /** * Every work app this Bot holds, with what each one is for the person asking. * @@ -199,26 +412,21 @@ export function mountAppAccessRoutes({ }); /** - * Ask an administrator for access. The body names the Bot and the app; the kind is the server's. - * - * A SECOND PRESS IS THE FIRST REQUEST. One pending request per person, app, Bot and kind, so - * asking again answers with the one already waiting and `created: false`, and writes no second - * trail row. - * - * ONLY FOR AN APP SOMETHING VOUCHES FOR, AND ONLY SO MANY. Asking to enable a `composio-` id the - * directory does not list would put whatever the caller typed in front of an administrator as an - * app's name, for an approval that can only fail; it is answered as no such app, unless the Bot - * already holds a grant for it and the request is about putting back something it was given. And - * a person with {@link MAX_PENDING_REQUESTS} waiting is told to wait for those to be answered. + * The app a request would be about, or the refusal: the Bot and the app named, the Bot one this + * person may use, the app one something vouches for, and a state only an administrator can fix. + * What filing a request checks, and what an administrator's one press checks before it files and + * approves in the same breath — so neither can be pressed for something the other would refuse. */ - routes.post("/access-requests", requireUser, async (context) => { - const body = (await context.req.json().catch(() => null)) as { - agentId?: unknown; - app?: unknown; - } | null; - const agentId = - typeof body?.agentId === "string" ? body.agentId.trim() : ""; - const app = typeof body?.app === "string" ? body.app.trim() : ""; + const requestable = async ( + context: RouteContext, + rawAgentId: unknown, + rawApp: unknown, + ): Promise< + | Response + | { agentId: string; access: AppAccess; requestKind: AppAccessRequestKind } + > => { + const agentId = typeof rawAgentId === "string" ? rawAgentId.trim() : ""; + const app = typeof rawApp === "string" ? rawApp.trim() : ""; if (!agentId || !app) return context.json({ error: "Name the Bot and the app." }, 400); if (agentId.length > MAX_NAME || app.length > MAX_NAME) @@ -266,6 +474,30 @@ export function mountAppAccessRoutes({ : `${access.displayName} needs your own account, not an admin. Connect it yourself.`, status: 409, }); + return { agentId, access, requestKind: access.requestKind }; + }; + + /** + * Ask an administrator for access. The body names the Bot and the app; the kind is the server's. + * + * A SECOND PRESS IS THE FIRST REQUEST. One pending request per person, app, Bot and kind, so + * asking again answers with the one already waiting and `created: false`, and writes no second + * trail row. + * + * ONLY FOR AN APP SOMETHING VOUCHES FOR, AND ONLY SO MANY. Asking to enable a `composio-` id the + * directory does not list would put whatever the caller typed in front of an administrator as an + * app's name, for an approval that can only fail; it is answered as no such app, unless the Bot + * already holds a grant for it and the request is about putting back something it was given. And + * a person with {@link MAX_PENDING_REQUESTS} waiting is told to wait for those to be answered. + */ + routes.post("/access-requests", requireUser, async (context) => { + const body = (await context.req.json().catch(() => null)) as { + agentId?: unknown; + app?: unknown; + } | null; + const asked = await requestable(context, body?.agentId, body?.app); + if (asked instanceof Response) return asked; + const { agentId, access, requestKind } = asked; /* * The one already waiting is answered as itself before the cap is counted, so a person at the * cap pressing again on a request they filed is shown it, not refused. @@ -279,7 +511,7 @@ export function mountAppAccessRoutes({ app: access.key, bot: agentId, reason: "too_many_pending", - kind: access.requestKind, + kind: requestKind, error: `You already have ${MAX_PENDING_REQUESTS} requests waiting for an admin. Withdraw one, or wait for an answer, then ask again.`, status: 429, }); @@ -288,11 +520,98 @@ export function mountAppAccessRoutes({ agentId, appKey: access.key, appTitle: access.displayName, - kind: access.requestKind, + kind: requestKind, }); return context.json(filed, filed.created ? 201 : 200); }); + /** + * An administrator's one press on a card: file the request this app needs on this Bot, and + * approve it, as one step. Their own request, approved by themselves, so the trail reads exactly + * as the two presses would — `app_access.requested`, then `app_access.approved` marked `direct` — + * and nothing about it is a second way to grant. + * + * THE SAME CHECKS, THE SAME CHANGE. What may be asked for is {@link requestable}'s answer, and + * what approving does is {@link carryOut}'s; this route only joins them. No `refs`: the one press + * grants what the inbox's Approve grants without them, the app's read-only actions, and anything + * that sends or writes is still chosen action by action on the Bot's page. + * + * A PRESS THAT CANNOT BE CARRIED OUT LEAVES NOTHING WAITING. The request this press filed is + * withdrawn again, since an administrator's own inbox holding a request they could not carry out + * a moment ago is noise; the refusal's sentence says what is missing. A request of theirs that was + * already waiting is put back as it was. + */ + routes.post( + "/for/:agentId/apps/:app/approve", + requireUser, + async (context) => { + const forbidden = requireAdmin(context); + if (forbidden) return forbidden; + const asked = await requestable( + context, + context.req.param("agentId"), + context.req.param("app"), + ); + if (asked instanceof Response) return asked; + const { agentId, access, requestKind } = asked; + const by = context.var.actor.id; + const filed = await store.fileAppAccessRequest({ + requestedBy: by, + agentId, + appKey: access.key, + appTitle: access.displayName, + kind: requestKind, + }); + const claimed = await store.claimAppAccessRequest({ + id: filed.request.id, + by, + }); + if (!claimed) + return refused(context, { + app: access.key, + request: filed.request.id, + bot: agentId, + kind: requestKind, + reason: "already_resolved", + error: "That request has already been answered.", + status: 409, + }); + const done = { carriedOut: false }; + try { + return await carryOut(context, claimed, undefined, done, true); + } finally { + if (!done.carriedOut) { + await store.releaseAppAccessRequest({ id: claimed.id, by }); + /* + * Withdrawn with the row a withdrawal always writes, so the trail does not hold an + * `app_access.requested` that nothing ever answered. + */ + const withdrawn = filed.created + ? await store.closeAppAccessRequest({ + id: claimed.id, + status: "cancelled", + by, + requestedBy: by, + }) + : null; + if (withdrawn) + await store.recordAppAccessEvent({ + eventType: "app_access.cancelled", + targetId: withdrawn.appKey, + actorUserId: by, + payload: { + request: withdrawn.id, + app: withdrawn.appKey, + bot: withdrawn.agentId, + kind: withdrawn.kind, + direct: true, + }, + }); + } + } + }, + ); + /** The person's own pending requests, newest first. */ routes.get("/access-requests/mine", requireUser, async (context) => context.json({ @@ -392,193 +711,6 @@ export function mountAppAccessRoutes({ return context.json({ error: "Tools must be a list of refs." }, 400); const chosen = body?.refs as string[] | undefined; - let carriedOut = false; - const approve = async (request: AppAccessRequestRecord) => { - const deny = ( - reason: string, - error: string, - status: 400 | 404 | 409 | 502 | 503, - ) => - refused(context, { - app: request.appKey, - request: request.id, - bot: request.agentId, - kind: request.kind, - reason, - error, - status, - }); - - const inspect = () => - store.inspectApp({ - app: request.appKey, - agentId: request.agentId ?? "", - userId: null, - composioConfigured, - directory, - }); - - let inspected = await inspect(); - if (!inspected) - return deny( - "no_such_app", - `${request.appTitle} isn't available anymore.`, - 409, - ); - - let enabled = false; - if (request.kind !== "connect_shared" && inspected.unavailable) { - const title = inspected.access.displayName; - if (inspected.unavailable !== "absent") - return deny( - inspected.unavailable, - unavailableSentence(inspected.unavailable, title, activeProvider), - inspected.unavailable === "composio_unconfigured" ? 503 : 409, - ); - if (inspected.toolkit) { - if (!composio) - return deny( - "composio_unconfigured", - unavailableSentence( - "composio_unconfigured", - title, - activeProvider, - ), - 503, - ); - const outcome = await enableComposioApp( - { store, composio, activeProvider, activeVendor }, - inspected.toolkit, - actorEmail(context), - ); - if (!outcome.ok) - return deny("enable_failed", outcome.error, outcome.status); - } else if (inspected.entry) { - const needed = adminInputFor(inspected.entry); - if (needed) - return deny( - "needs_admin_input", - `${title} needs ${needed}. Add it on its page under Plugins, then approve this request again.`, - 409, - ); - try { - await store.addServer({ - key: inspected.entry.key, - by: actorEmail(context), - }); - } catch (error) { - if ( - error instanceof CatalogueEntryUnknownError || - error instanceof CustomServerRefusedError - ) - return deny("enable_failed", error.message, 400); - if (isDeploymentFault(error)) - return deny("enable_failed", deploymentFaultSentence(error), 409); - throw error; - } - } else { - return deny( - "absent", - unavailableSentence("absent", title, activeProvider), - 409, - ); - } - enabled = true; - inspected = await inspect(); - if (!inspected || inspected.unavailable) - return deny( - "enable_failed", - `${title} was turned on but still can't be used. Open its page under Plugins to see why.`, - 409, - ); - } - - let granted: string[] = []; - if ( - request.kind !== "connect_shared" && - request.agentId && - !inspected.access.granted - ) { - const agentId = request.agentId; - const serverId = inspected.answeringId; - const name = inspected.access.displayName; - if (!(await store.agentIsRegistered(agentId)) || !serverId) - return deny("no_such_bot", "That Bot is no longer here.", 409); - const tools = await store.appTools(serverId); - const advertised = tools.map((tool) => tool.ref); - const refs = - chosen ?? tools.filter((tool) => tool.read).map((tool) => tool.ref); - const outside = refs.filter((ref) => !advertised.includes(ref)); - if (outside.length > 0) - return deny( - "refs_outside_app", - `${outside.join(", ")} ${outside.length === 1 ? "is" : "are"} not among ${name}'s tools.`, - 400, - ); - if (advertised.length === 0) - return deny( - "no_tools", - `${name} has no tools to give yet. Refresh its tools on its page under Plugins, then approve this request again.`, - 409, - ); - if (refs.length === 0) - return deny( - "no_read_only_tools", - `None of ${name}'s actions are marked read-only, so approving here gives this Bot nothing. Choose its actions on ${name}'s page under Plugins, then approve this request again.`, - 409, - ); - for (const ref of refs) { - await store.grant("mcp", ref, agentId, actorEmail(context)); - } - const sharedDecision = await sharedGrantApproval( - { store, shared }, - refs[0] ?? "", - agentId, - undefined, - ); - if ( - sharedDecision !== "unreadable" && - sharedDecision.approval && - shared - ) - await shared.use.setApproval({ - botId: agentId, - serverId: sharedDecision.appId, - approval: sharedDecision.approval, - by: context.var.actor.id, - }); - granted = refs; - } - - if ( - request.kind === "connect_shared" && - inspected.access.account === "shared" && - !inspected.sharedUsable - ) - return deny( - "shared_not_connected", - `The shared ${inspected.access.displayName} account is not connected yet. Connect it on ${inspected.access.displayName}'s page under Plugins, then approve this request.`, - 409, - ); - - carriedOut = true; - const closed = request; - await store.recordAppAccessEvent({ - eventType: "app_access.approved", - targetId: closed.appKey, - actorUserId: context.var.actor.id, - payload: { - request: closed.id, - app: closed.appKey, - bot: closed.agentId, - kind: closed.kind, - enabled, - granted: granted.length, - }, - }); - return context.json({ request: closed, enabled, granted }); - }; - const claimed = await store.claimAppAccessRequest({ id, by: context.var.actor.id, @@ -595,10 +727,11 @@ export function mountAppAccessRoutes({ status: existing ? 409 : 404, }); } + const done = { carriedOut: false }; try { - return await approve(claimed); + return await carryOut(context, claimed, chosen, done); } finally { - if (!carriedOut) + if (!done.carriedOut) await store.releaseAppAccessRequest({ id, by: context.var.actor.id }); } }); diff --git a/server/tests/app-access-routes.integration.test.ts b/server/tests/app-access-routes.integration.test.ts index 1fccc556c9..5dd01e0e01 100644 --- a/server/tests/app-access-routes.integration.test.ts +++ b/server/tests/app-access-routes.integration.test.ts @@ -998,6 +998,120 @@ describe("answering a request", () => { }); }); +describe("an administrator's one press on a card", () => { + const press = (app: string, as = admin, role: "admin" | "user" = "admin") => + appAs(as, role).request( + `/api/plugins/for/${bot}/apps/${encodeURIComponent(app)}/approve`, + { method: "POST" }, + ); + + test("turns an app on and gives it to the Bot, read-only, with the trail two presses leave", async () => { + const response = await press("Dropbox"); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ + request: { + kind: "enable", + appKey: DROPBOX, + status: "approved", + requestedBy: admin, + resolvedBy: admin, + }, + enabled: true, + granted: [`${DROPBOX}/LIST_FILES`], + }); + expect(enabled).toEqual([slug("dropbox")]); + expect(eventsOf("app_access.requested")[0]?.payload).toMatchObject({ + app: DROPBOX, + kind: "enable", + }); + expect(eventsOf("app_access.approved")[0]?.payload).toMatchObject({ + app: DROPBOX, + kind: "enable", + enabled: true, + granted: 1, + direct: true, + }); + const { app } = await json<{ app: AppAccess }>( + await appAs(person).request(`/api/plugins/for/${bot}/apps/${DROPBOX}`), + ); + expect(app.state).toBe("not_connected"); + }); + + test("gives an app that is on to the Bot, and grants no send", async () => { + await seedApp({ + key: CALENDAR, + title: "Google Calendar", + tools: ["SEND", "LIST"], + effects: { LIST: "read", SEND: "write" }, + grant: false, + }); + const response = await press(CALENDAR); + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ + request: { kind: "grant", status: "approved" }, + enabled: false, + granted: [`${CALENDAR}/LIST`], + }); + }); + + test("only an administrator may press it, and nothing is filed for anybody else", async () => { + await seedApp({ key: CALENDAR, title: "Google Calendar", grant: false }); + expect((await press(CALENDAR, person, "user")).status).toBe(403); + const rows = await database + .select() + .from(appAccessRequests) + .where(eq(appAccessRequests.appKey, CALENDAR)); + expect(rows).toHaveLength(0); + }); + + test("a state with nothing for an administrator to do is refused like a request", async () => { + await seedApp({ key: GMAIL, title: "Gmail" }); + const response = await press(GMAIL); + expect(response.status).toBe(409); + expect(eventsOf("app_access.refused").at(-1)?.payload).toMatchObject({ + reason: "nothing_to_request:not_connected", + }); + }); + + test("a press that cannot be carried out leaves no request waiting", async () => { + const response = await press("google-drive"); + expect(response.status).toBe(409); + const { error } = await json<{ error: string }>(response); + expect(error).toContain("OAuth client"); + // There is no request left to approve again: the admin presses again. + expect(error).toEndWith("then try again."); + const rows = await database + .select({ status: appAccessRequests.status }) + .from(appAccessRequests) + .where(eq(appAccessRequests.requestedBy, admin)); + expect(rows.map((row) => row.status)).toEqual(["cancelled"]); + // The trail closes what it opened: filed, refused, then withdrawn. + const filed = eventsOf("app_access.requested")[0]?.payload as { + request?: string; + }; + expect(eventsOf("app_access.cancelled")[0]?.payload).toMatchObject({ + request: filed.request, + app: "google-drive", + direct: true, + }); + expect(eventsOf("app_access.approved")).toHaveLength(0); + }); + + test("a request of theirs already waiting is put back as it was", async () => { + await appAs(admin, "admin").request( + "/api/plugins/access-requests", + post({ agentId: bot, app: "google-drive" }), + ); + expect((await press("google-drive")).status).toBe(409); + const rows = await database + .select({ status: appAccessRequests.status }) + .from(appAccessRequests) + .where(eq(appAccessRequests.requestedBy, admin)); + expect(rows.map((row) => row.status)).toEqual(["pending"]); + expect(eventsOf("app_access.cancelled")).toHaveLength(0); + }); +}); + /* * THE SAME RULE ON AN ARCADE DEPLOYMENT. Which rows are brokered apps, which connections count and * which id an app nobody enabled would get all come from the store's broker, so a personal Arcade diff --git a/server/tests/app-access-tool.test.ts b/server/tests/app-access-tool.test.ts index 25e9c253c3..d3a31f7a10 100644 --- a/server/tests/app-access-tool.test.ts +++ b/server/tests/app-access-tool.test.ts @@ -96,6 +96,7 @@ describe("show_app_access", () => { title: "Gmail", agentId: "expenses", reason: "access", + blocked: state !== "ready", }); expect(prompt?.message).toMatch(said); expect(prompt?.message).toContain("Do not browse to its website"); diff --git a/shared/connect-prompt.test.ts b/shared/connect-prompt.test.ts index 336c53058a..9f06d0560a 100644 --- a/shared/connect-prompt.test.ts +++ b/shared/connect-prompt.test.ts @@ -69,6 +69,7 @@ describe("the Bot, the reason and the moment it carries", () => { agentId: "expenses", reason: "reconnect", at: "2026-10-20T12:00:00.000Z", + blocked: true, }); expect(readConnectPrompt(text)).toEqual({ serverId: "composio-gmail", @@ -77,6 +78,7 @@ describe("the Bot, the reason and the moment it carries", () => { agentId: "expenses", reason: "reconnect", at: "2026-10-20T12:00:00.000Z", + blocked: true, }); expect(connectAppRefusal({ serverId: "s", title: "T", message: "m" })).toBe( `${CONNECT_APP_MARKER}{"serverId":"s","title":"T"} m`, @@ -85,7 +87,7 @@ describe("the Bot, the reason and the moment it carries", () => { test("a field of the wrong shape is dropped, and the prompt still names its app", () => { const prompt = readConnectPrompt( - `${CONNECT_APP_MARKER}{"serverId":"composio-gmail","title":"Gmail","agentId":7,"reason":"grant me","at":"yesterday"} hello`, + `${CONNECT_APP_MARKER}{"serverId":"composio-gmail","title":"Gmail","agentId":7,"reason":"grant me","at":"yesterday","blocked":"yes"} hello`, ); expect(prompt).toEqual({ serverId: "composio-gmail", diff --git a/shared/connect-prompt.ts b/shared/connect-prompt.ts index 95bafbedb4..6f26154262 100644 --- a/shared/connect-prompt.ts +++ b/shared/connect-prompt.ts @@ -66,6 +66,13 @@ export type ConnectPrompt = { * and the card stops saying Reconnect. See `AppAccess.connectedAt`. */ at?: string; + /** + * True when the app could not be used at the moment the Bot asked: always so of a refusal, and + * written by `show_app_access` from the state it found. What lets a card that is drawn already + * `ready` tell "became ready since" — worth a Continue — from "was ready all along", which is not. + * Absent on a `show_app_access` answer from before it was written, which offers nothing. + */ + blocked?: boolean; }; /** @@ -84,13 +91,14 @@ export const SHOW_APP_ACCESS_TOOL = "show_app_access"; * 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, agentId, reason, at } = prompt; + const { serverId, title, message, agentId, reason, at, blocked } = prompt; return `${CONNECT_APP_MARKER}${JSON.stringify({ serverId, title, ...(agentId ? { agentId } : {}), ...(reason ? { reason } : {}), ...(at ? { at } : {}), + ...(blocked !== undefined ? { blocked } : {}), })} ${message}`; } @@ -125,12 +133,13 @@ export function readConnectPrompt( try { const parsed: unknown = JSON.parse(rest.slice(0, end + 1)); if (!parsed || typeof parsed !== "object") return null; - const { serverId, title, agentId, reason, at } = parsed as { + const { serverId, title, agentId, reason, at, blocked } = parsed as { serverId?: unknown; title?: unknown; agentId?: unknown; reason?: unknown; at?: unknown; + blocked?: unknown; }; if (typeof serverId !== "string" || typeof title !== "string") return null; if (!serverId || !title) return null; @@ -150,6 +159,7 @@ export function readConnectPrompt( ...(typeof at === "string" && !Number.isNaN(Date.parse(at)) ? { at } : {}), + ...(typeof blocked === "boolean" ? { blocked } : {}), }; } catch { return null;