From ab206320f7461b27d0ad790ca4b203c733862031 Mon Sep 17 00:00:00 2001 From: Jerel John Velarde Date: Mon, 26 Oct 2026 15:28:34 -0700 Subject: [PATCH] Fix the app UI findings from the release review M1: onboarding's local step said conversations stay on this computer. They live in CopilotKit Intelligence (its cloud unless self-hosted), every message goes to the model, and runtime telemetry is on by default. The step now says each of those, and how to turn telemetry off. M3: Disconnect on Gmail and Google Calendar now works. DELETE /servers/:id/connection takes user-oauth apps down a new store.disconnectHeld: it asks the vendor to revoke the refresh token at the catalogue's revokeUrl (RFC 7009), then revokes the vault row and deletes the connection in one transaction, and audits it. If the vendor can't be reached, nothing changes. If the vendor refuses, access still ends here and the page says to finish at Google. M4: CopilotKit's inspector and its popover are off unless VITE_COPILOTKIT_INSPECTOR=true. M6: page body line heights are ratios. The page title wraps between words and gets its own row at large text. The Tasks header wraps. The + and the icon buttons stay a fixed size, with a spacer that clears the +. Also: Escape on New channel closes only the innermost thing and asks before dropping typed fields. User-visible "OpenBot" becomes the product name. The roster step loses the approval demo and the brief demo shows today's date. Model and network failures are plain sentences that survive a reload while still current. The Chat pill says "Still working on it" after 20 quiet seconds. The checklist reads an idle Bot computer as Ready, and the official API base URLs are no longer "custom". --- .../src/components/agents/new-bot-sheet.tsx | 10 +- .../src/components/agents/proposed-bot.tsx | 3 +- .../src/components/channels/channel-chat.tsx | 39 ++- .../components/channels/conversation-view.tsx | 19 +- .../src/components/delivery/reaching-you.tsx | 3 +- .../app/src/components/duet/page-sheet.tsx | 18 +- openbot/app/src/components/duet/page-view.tsx | 14 +- .../app/src/components/gallery/activity.tsx | 12 +- .../app/src/components/gallery/preview.tsx | 5 +- openbot/app/src/components/memory/sources.tsx | 6 +- .../components/onboarding/capability-demo.tsx | 30 +- .../app/src/components/plugins/grant-list.tsx | 3 +- .../components/responsibilities/triggers.tsx | 8 +- .../src/components/routines/routines-list.tsx | 5 +- .../components/settings/linked-notice.test.ts | 3 +- .../src/components/settings/linked-notice.ts | 4 +- .../src/components/shell/channel-sheet.tsx | 118 ++++++- .../src/components/shell/chat-presence.tsx | 67 ++++ .../app/src/components/shell/tasks-active.tsx | 10 +- .../app/src/components/shell/tasks-screen.tsx | 29 +- .../app/src/lib/channels/run-error-memory.ts | 71 +++++ openbot/app/src/lib/channels/run-lifecycle.ts | 37 +++ openbot/app/src/lib/connections/rows.ts | 6 +- openbot/app/src/lib/copilot/provider.tsx | 19 ++ openbot/app/src/lib/copilot/stopped-turn.ts | 8 +- openbot/app/src/lib/plugins/mutations.ts | 39 +++ .../routes/_authed/_app/connections.$key.tsx | 57 +++- .../routes/_authed/_app/connections.index.tsx | 3 +- .../routes/_authed/admin/components/$name.tsx | 4 +- .../src/routes/_authed/admin/computers.tsx | 3 +- .../src/routes/_authed/admin/credentials.tsx | 3 +- .../app/src/routes/_authed/admin/learning.tsx | 14 +- .../src/routes/_authed/admin/plugins/$key.tsx | 6 +- openbot/app/src/routes/_authed/onboarding.tsx | 48 ++- openbot/app/tests/channel-sheet.test.tsx | 101 +++++- openbot/app/tests/chat-run-lifecycle.test.tsx | 42 +++ openbot/app/tests/copilot-inspector.test.ts | 22 ++ .../tests/held-account-disconnect.test.tsx | 246 +++++++++++++++ openbot/app/tests/learning-admin.test.tsx | 4 +- openbot/app/tests/onboarding-route.test.tsx | 22 +- openbot/app/tests/patient-step.test.tsx | 67 ++++ .../app/tests/plugin-mutation-refresh.test.ts | 16 + openbot/app/tests/run-error-memory.test.ts | 47 +++ openbot/app/tests/run-lifecycle.test.ts | 36 +++ openbot/server/src/audit.ts | 11 +- openbot/server/src/index.ts | 6 +- .../server/src/model-availability-check.ts | 48 ++- openbot/server/src/openduet/computer-check.ts | 18 +- openbot/server/src/openduet/setup.ts | 84 ++++- .../src/plugins/plugin-store/connections.ts | 135 +++++++- .../src/plugins/plugin-store/context.ts | 3 + .../server/src/plugins/plugin-store/model.ts | 96 ++++++ .../src/plugins/route-groups/connections.ts | 49 ++- openbot/server/src/plugins/store.ts | 3 + openbot/server/tests/assertion-shape.test.ts | 7 - .../tests/google-workspace-fake.test.ts | 27 +- .../tests/model-availability-check.test.ts | 80 +++++ openbot/server/tests/oauth-revoke.test.ts | 113 +++++++ openbot/server/tests/openduet-setup.test.ts | 94 +++++- openbot/server/tests/plugin-routes.test.ts | 44 ++- ...plugin-user-credential.integration.test.ts | 292 +++++++++++++++++- openbot/server/tests/typecheck-baseline.txt | 1 - 62 files changed, 2281 insertions(+), 157 deletions(-) create mode 100644 openbot/app/src/lib/channels/run-error-memory.ts create mode 100644 openbot/app/tests/copilot-inspector.test.ts create mode 100644 openbot/app/tests/held-account-disconnect.test.tsx create mode 100644 openbot/app/tests/patient-step.test.tsx create mode 100644 openbot/app/tests/run-error-memory.test.ts create mode 100644 openbot/server/tests/oauth-revoke.test.ts diff --git a/openbot/app/src/components/agents/new-bot-sheet.tsx b/openbot/app/src/components/agents/new-bot-sheet.tsx index 41fc269e..a8535a63 100644 --- a/openbot/app/src/components/agents/new-bot-sheet.tsx +++ b/openbot/app/src/components/agents/new-bot-sheet.tsx @@ -40,6 +40,7 @@ import { testAgentConnection, } from "@/lib/agents/queries"; import { isComposing } from "@/lib/composing"; +import { appConfig } from "@/lib/generated/application-config"; import { cn } from "@/lib/utils"; import { queryClient } from "@/query-client"; @@ -493,7 +494,7 @@ const VISIBILITY_OPTIONS: Array<{ { value: "public", title: "Public", - description: "Everyone in your OpenBot can find and use it.", + description: `Everyone in your ${appConfig.brand.productName} can find and use it.`, icon: IconUsers, }, ]; @@ -685,7 +686,7 @@ function RuntimeStep({ Run it here {builtInAvailable - ? "OpenBot runs it for you. Nothing to set up, and it can start right away." + ? `${appConfig.brand.productName} runs it for you. Nothing to set up, and it can start right away.` : "Not available yet. An admin has to turn this on first."} @@ -786,8 +787,9 @@ function EndpointStep({ /> ) : ( - Any agent that speaks AG-UI works. OpenBot connects to it, so it - has to be reachable from OpenBot's server. + Any agent that speaks AG-UI works. {appConfig.brand.productName}{" "} + connects to it, so it has to be reachable from{" "} + {appConfig.brand.productName}'s server. )} diff --git a/openbot/app/src/components/agents/proposed-bot.tsx b/openbot/app/src/components/agents/proposed-bot.tsx index 0b64b45f..77f4817c 100644 --- a/openbot/app/src/components/agents/proposed-bot.tsx +++ b/openbot/app/src/components/agents/proposed-bot.tsx @@ -10,6 +10,7 @@ import { type ProposedBot, wasCreated, } from "@/lib/agents/proposal"; +import { appConfig } from "@/lib/generated/application-config"; /** * The coworker a Bot has written, put in front of the person before it exists. @@ -137,7 +138,7 @@ export function ProposedBotCard({ action={Waiting on you} caption={ clashes - ? `There's already a ${clashes.title}, and it's ${clashes.ownership === "the deployment's" ? "built into OpenBot" : clashes.ownership}. Two Bots with the same name are hard to tell apart.` + ? `There's already a ${clashes.title}, and it's ${clashes.ownership === "the deployment's" ? `built into ${appConfig.brand.productName}` : clashes.ownership}. Two Bots with the same name are hard to tell apart.` : "Nothing is created until you press Create it." } title={titleFor(values.name, clashes)} diff --git a/openbot/app/src/components/channels/channel-chat.tsx b/openbot/app/src/components/channels/channel-chat.tsx index 18982499..30d01bba 100644 --- a/openbot/app/src/components/channels/channel-chat.tsx +++ b/openbot/app/src/components/channels/channel-chat.tsx @@ -45,6 +45,11 @@ import { type ChannelSummary, channelKeys, } from "@/lib/channels/queries"; +import { + forgetRunError, + recalledRunError, + rememberRunError, +} from "@/lib/channels/run-error-memory"; import { claimSeed, interruptsToAnswer, @@ -892,7 +897,25 @@ export function ChannelChat({ }); // Run failures arrive as events and are reported only for turns started in this mount. - const [runError, setRunError] = useState(null); + const [runError, setRunErrorState] = useState(null); + /** + * Every report goes through here, so the note kept for a reload is always the one on screen, kept + * with the conversation's last message so a reload shows it only while it is still the latest + * news (`lib/channels/run-error-memory.ts`). A new turn's `null` forgets it. + */ + const setRunError = useCallback( + (said: string | null) => { + setRunErrorState(said); + if (said === null) forgetRunError(channel.id); + else + rememberRunError( + channel.id, + said, + agentRef.current.messages.at(-1)?.id ?? null, + ); + }, + [channel.id], + ); const awaitingReply = useRef(false); /** * WHY THIS TURN ENDED WITHOUT AN ANSWER, KEPT WHERE `deliver` CAN STILL SEE IT — because the one @@ -1377,7 +1400,7 @@ export function ChannelChat({ }, }); return () => subscription?.unsubscribe(); - }, [agent, runtimeAgentId]); + }, [agent, runtimeAgentId, setRunError]); /** Stable reference for effects and component callbacks. */ const sayRef = useRef(say); @@ -1613,6 +1636,16 @@ export function ChannelChat({ ...(task?.queued ?? []).map((reply) => reply.messageId), ]); + /* + * The failed turn's sentence from before a reload, while nothing has happened since: read only + * once history is back and no turn is going, and only while the last message is the one it was + * about. See `lib/channels/run-error-memory.ts`. + */ + const recalledError = + runError === null && !restoring && turnsInFlight === 0 && !agent.isRunning + ? recalledRunError(channel.id, agent.messages.at(-1)?.id ?? null) + : null; + return ( // Activity renderers resolve their agent through this SDK context. It must match useAgent's // channel instance so an action continues this thread instead of looking for a default agent. @@ -1831,7 +1864,7 @@ export function ChannelChat({ stopped={ runError && THREAD_LOCKED.test(runError) ? STILL_FINISHING - : (runError ?? undefined) + : (runError ?? recalledError ?? undefined) } /> diff --git a/openbot/app/src/components/channels/conversation-view.tsx b/openbot/app/src/components/channels/conversation-view.tsx index 7e2c62c0..2b8d3799 100644 --- a/openbot/app/src/components/channels/conversation-view.tsx +++ b/openbot/app/src/components/channels/conversation-view.tsx @@ -29,6 +29,7 @@ import { import { PageSheetHost } from "@/components/duet/page-sheet"; import { stepFor, + usePatientStep, useReportChatPresence, } from "@/components/shell/chat-presence"; import { ChatSuggestionCard } from "@/components/shell/chat-suggestion"; @@ -222,12 +223,24 @@ export function ConversationView({ // The app shell's orb follows Chat: thinking for the turn, listening to the draft. Not while // history is restoring: joining the thread is a run of its own, and it is not an answer. const working = (busy || inFlight) && !restoring; + /* + * What moves when the turn does: another message, the last one growing as it is written, or a + * tool call added to it. The pill's quiet clock (`usePatientStep`) starts again on each. + */ + const last = messages.at(-1); + const progress = `${messages.length}:${JSON.stringify(last?.content ?? "").length}:${ + last && "toolCalls" in last ? (last.toolCalls?.length ?? 0) : 0 + }`; + const step = usePatientStep( + // Read off the messages on every render, as the transcript reads them, because the agent + // grows the same array in place. Only Chat's conversation has a pill to say it in. + inShell ? stepFor(messages, working, approvals) : null, + progress, + ); const presence = useReportChatPresence(inShell, { working, failed: Boolean(stopped), - // Read off the messages on every render, as the transcript reads them, because the agent - // grows the same array in place. Only Chat's conversation has a pill to say it in. - step: inShell ? stepFor(messages, working, approvals) : null, + step, }); /** diff --git a/openbot/app/src/components/delivery/reaching-you.tsx b/openbot/app/src/components/delivery/reaching-you.tsx index 4a6a7f00..d940c2c0 100644 --- a/openbot/app/src/components/delivery/reaching-you.tsx +++ b/openbot/app/src/components/delivery/reaching-you.tsx @@ -52,6 +52,7 @@ import { startChatLinkMutationOptions, startSmsMutationOptions, } from "@/lib/delivery"; +import { appConfig } from "@/lib/generated/application-config"; import { queryClient } from "@/query-client"; /** @@ -513,7 +514,7 @@ function ConnectDialog({ - Send this message to the OpenBot app in{" "} + Send this message to the {appConfig.brand.productName} app in{" "} {linked.platform === "teams" ? "Microsoft Teams" : "Slack"}{" "} within {Math.ceil((linked.expiresAt - Date.now()) / 60_000)}{" "} minutes. It links that account to this conversation and Bot. diff --git a/openbot/app/src/components/duet/page-sheet.tsx b/openbot/app/src/components/duet/page-sheet.tsx index 2db8755b..887880b1 100644 --- a/openbot/app/src/components/duet/page-sheet.tsx +++ b/openbot/app/src/components/duet/page-sheet.tsx @@ -136,7 +136,8 @@ export function PageSheet({
+ {/* + * THE TITLE GETS A ROW OF ITS OWN WHEN THE TEXT IS LARGE. Back and "…" are 44px each + * in rem, so at 200% text they took 176px of a 342px row between them, and the title + * was squeezed into a column a few letters wide. It broke anywhere to fit, one syllable + * per line ("Prep / note / s: / Hea / rth"). Below 20rem of room, which is 320px at the + * default size and so no phone at it, the title moves under the two discs and takes the + * whole width. It breaks only between words, and a title longer than two lines ends in + * an ellipsis, with the whole of it still the dialog's name. + */}
- + {title} ) : undefined } - caption={`Counted from OpenBot's audit trail, last ${data.days} days`} + caption={`Counted from ${appConfig.brand.productName}'s audit trail, last ${data.days} days`} title={title ?? "Bot activity"} >
    @@ -223,7 +227,7 @@ function RefusalList({ return (

    - OpenBot hasn't refused anything. + {appConfig.brand.productName} hasn't refused anything.

    ); @@ -246,7 +250,7 @@ function RefusalList({ ) : undefined } - caption="Read from OpenBot's audit trail" + caption={`Read from ${appConfig.brand.productName}'s audit trail`} title={title ?? "Recent refusals"} >
      diff --git a/openbot/app/src/components/gallery/preview.tsx b/openbot/app/src/components/gallery/preview.tsx index 8b55af7c..662c0951 100644 --- a/openbot/app/src/components/gallery/preview.tsx +++ b/openbot/app/src/components/gallery/preview.tsx @@ -1,4 +1,5 @@ import { GALLERY_COMPONENTS } from "@/lib/copilot/gallery-registry"; +import { appConfig } from "@/lib/generated/application-config"; /** * The answers a Bot can give, drawn rather than described. @@ -117,8 +118,8 @@ function PreviewOf({ name }: { name: string }) { */ return (

      - Shows OpenBot's own records. There's nothing to show until a Bot asks - for it. + Shows {appConfig.brand.productName}'s own records. There's nothing to + show until a Bot asks for it.

      ); } diff --git a/openbot/app/src/components/memory/sources.tsx b/openbot/app/src/components/memory/sources.tsx index a789a508..5072e178 100644 --- a/openbot/app/src/components/memory/sources.tsx +++ b/openbot/app/src/components/memory/sources.tsx @@ -42,6 +42,7 @@ import { import { Separator } from "@/components/ui/separator"; import { Switch } from "@/components/ui/switch"; import { agentListQueryOptions } from "@/lib/agents/queries"; +import { appConfig } from "@/lib/generated/application-config"; import { addMemorySourceMutationOptions, availableMemorySourcesQueryOptions, @@ -203,7 +204,8 @@ export function MemorySources({ agentId: fixedBot }: { agentId?: string }) { Add a source - OpenBot reads it now, then every 15 minutes while it's on. + {appConfig.brand.productName} reads it now, then every 15 minutes + while it's on. @@ -235,7 +237,7 @@ export function MemorySources({ agentId: fixedBot }: { agentId?: string }) { .filter(Boolean); else if (setting.schema.type === "object") throw new Error( - "This one has settings OpenBot can't show. Choose another.", + `This one has settings ${appConfig.brand.productName} can't show. Choose another.`, ); else values[setting.name] = value; } diff --git a/openbot/app/src/components/onboarding/capability-demo.tsx b/openbot/app/src/components/onboarding/capability-demo.tsx index d042e00a..27570092 100644 --- a/openbot/app/src/components/onboarding/capability-demo.tsx +++ b/openbot/app/src/components/onboarding/capability-demo.tsx @@ -8,12 +8,7 @@ import { TaskRow } from "@/components/duet/task-card"; import type { ApprovalRequest } from "@/lib/approvals"; import { IllustrationFrame } from "./illustration-frame"; -export type CapabilityDemoKind = - | "brief" - | "approval" - | "task" - | "search" - | "handoff"; +export type CapabilityDemoKind = "brief" | "approval" | "task" | "search"; export const DEMO_BEAT_MS = 2400; const APPROVAL: ApprovalRequest = { @@ -42,10 +37,23 @@ const LABELS: Record = { approval: "Example: the Bot asks before sending an email, with Refuse and Allow once", task: "Example: a task moves from Working to Ready for review", - handoff: - "Example: the Bot asks Allow once before sending, then the task becomes Ready for review", }; +/** + * The brief's date line: today, as the real brief writes its own (`morning-brief-tool.tsx`). + * + * It was a fixed "Friday, October 23", which was right on the day it was written and wrong, with + * a weekday that no longer matched, on every day after. The rows under it name a day ("Friday") + * and never a date, so today is the one date the card can always show truthfully. + */ +export function demoDate(now: Date = new Date()): string { + return now.toLocaleDateString(undefined, { + weekday: "long", + month: "long", + day: "numeric", + }); +} + /** Real cards, isolated from input. A hidden/offscreen demo stops its clock and orb. */ export function CapabilityDemo({ kind }: { kind: CapabilityDemoKind }) { const reduced = useReducedMotion() === true; @@ -81,8 +89,8 @@ export function CapabilityDemo({ kind }: { kind: CapabilityDemoKind }) { }, [reduced, visible]); const phase = reduced ? 2 : beat; const done = phase === 2; - const approval = kind === "approval" || (kind === "handoff" && !done); - const task = kind === "task" || (kind === "handoff" && done); + const approval = kind === "approval"; + const task = kind === "task"; const caption = approval ? done ? "Only after your OK" @@ -146,7 +154,7 @@ export function CapabilityDemo({ kind }: { kind: CapabilityDemoKind }) { ) : ( { if (authorizationUrl === null) { setNotice( - "OpenBot answered without a consent link, so nothing was authorized. Try again, or ask an admin if it keeps happening.", + `${appConfig.brand.productName} answered without a consent link, so nothing was authorized. Try again, or ask an admin if it keeps happening.`, ); return; } diff --git a/openbot/app/src/components/responsibilities/triggers.tsx b/openbot/app/src/components/responsibilities/triggers.tsx index a158a84a..95ebc1e8 100644 --- a/openbot/app/src/components/responsibilities/triggers.tsx +++ b/openbot/app/src/components/responsibilities/triggers.tsx @@ -46,6 +46,7 @@ import { } from "@/components/ui/select"; import { Separator } from "@/components/ui/separator"; import { Switch } from "@/components/ui/switch"; +import { appConfig } from "@/lib/generated/application-config"; import { createTriggerMutationOptions, type ResponsibilityRecord, @@ -357,8 +358,8 @@ function TriggerRow({
) : (

- Email isn't set up on OpenBot yet, so this address can't receive - mail. An admin has to turn it on. + Email isn't set up on {appConfig.brand.productName} yet, so this + address can't receive mail. An admin has to turn it on.

))} {generated && ( @@ -531,7 +532,8 @@ function NewTriggerDialog({ Add trigger - Something outside OpenBot that starts this responsibility. + Something outside {appConfig.brand.productName} that starts this + responsibility. diff --git a/openbot/app/src/components/routines/routines-list.tsx b/openbot/app/src/components/routines/routines-list.tsx index d1f5342e..6eb2a76c 100644 --- a/openbot/app/src/components/routines/routines-list.tsx +++ b/openbot/app/src/components/routines/routines-list.tsx @@ -34,6 +34,7 @@ import { } from "@/components/ui/item"; import { Separator } from "@/components/ui/separator"; import { Switch } from "@/components/ui/switch"; +import { appConfig } from "@/lib/generated/application-config"; import { relativeTime } from "@/lib/relative-time"; import { deleteRoutineMutationOptions, @@ -262,8 +263,8 @@ export function RoutinesList({ Routines aren't running. {" "} {sweep?.lastSweptAt - ? `OpenBot last checked them ${relativeTime(sweep.lastSweptAt)}. None will run until it starts again. Ask an admin if this keeps happening.` - : "OpenBot hasn't started running routines yet, so none of these will run. Ask an admin to set this up."} + ? `${appConfig.brand.productName} last checked them ${relativeTime(sweep.lastSweptAt)}. None will run until it starts again. Ask an admin if this keeps happening.` + : `${appConfig.brand.productName} hasn't started running routines yet, so none of these will run. Ask an admin to set this up.`}

) : null} diff --git a/openbot/app/src/components/settings/linked-notice.test.ts b/openbot/app/src/components/settings/linked-notice.test.ts index 6b17101e..cc950d61 100644 --- a/openbot/app/src/components/settings/linked-notice.test.ts +++ b/openbot/app/src/components/settings/linked-notice.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { linkedNotice, visibleLinkedNotice } from "./linked-notice"; +import { appConfig } from "@/lib/generated/application-config"; describe("linkedNotice", () => { test("reports a linked GitHub account", () => { @@ -19,7 +20,7 @@ describe("linkedNotice", () => { test("tells the person the GitHub account belongs to another user, as an alert", () => { expect(linkedNotice("github-taken")).toEqual({ tone: "error", - text: "That GitHub account is linked to someone else in OpenBot. Sign in to GitHub with another account and try again, or ask an admin.", + text: `That GitHub account is linked to someone else in ${appConfig.brand.productName}. Sign in to GitHub with another account and try again, or ask an admin.`, }); }); diff --git a/openbot/app/src/components/settings/linked-notice.ts b/openbot/app/src/components/settings/linked-notice.ts index ee447ad6..4e224330 100644 --- a/openbot/app/src/components/settings/linked-notice.ts +++ b/openbot/app/src/components/settings/linked-notice.ts @@ -1,3 +1,5 @@ +import { appConfig } from "@/lib/generated/application-config"; + export type LinkedNotice = { tone: "success" | "error"; text: string }; /** @@ -17,7 +19,7 @@ export function linkedNotice(value: string | undefined): LinkedNotice | null { if (value === "github-taken") { return { tone: "error", - text: "That GitHub account is linked to someone else in OpenBot. Sign in to GitHub with another account and try again, or ask an admin.", + text: `That GitHub account is linked to someone else in ${appConfig.brand.productName}. Sign in to GitHub with another account and try again, or ask an admin.`, }; } return null; diff --git a/openbot/app/src/components/shell/channel-sheet.tsx b/openbot/app/src/components/shell/channel-sheet.tsx index ecb7287e..08b8fb46 100644 --- a/openbot/app/src/components/shell/channel-sheet.tsx +++ b/openbot/app/src/components/shell/channel-sheet.tsx @@ -1,6 +1,13 @@ import { Dialog as DialogPrimitive } from "@base-ui/react/dialog"; import { useMutation } from "@tanstack/react-query"; -import { type ComponentProps, useId, useRef, useState } from "react"; +import { + type ComponentProps, + type RefObject, + useEffect, + useId, + useRef, + useState, +} from "react"; import { CHANNEL_COLOUR_LABELS, CHANNEL_GLYPHS, @@ -98,6 +105,10 @@ export function ChannelSheet({ const [name, setName] = useState(channel?.name ?? ""); const [purpose, setPurpose] = useState(channel?.purpose ?? ""); const [picking, setPicking] = useState(false); + /** Escape or a tap outside met typed fields, so the sheet asks before dropping them. */ + const [confirming, setConfirming] = useState(false); + const changeIcon = useRef(null); + const keepEditing = useRef(null); const nameField = useRef(null); const purposeField = useRef(null); const create = useMutation(createDuetChannelMutationOptions(queryClient)); @@ -110,6 +121,42 @@ export function ChannelSheet({ const nameId = useId(); const purposeId = useId(); const title = editing ? "Edit channel" : "New channel"; + /* + * Whether there is anything typed or chosen that leaving would lose: for a new channel, a name + * or a purpose; for one being edited, anything that differs from what it is now. + */ + const dirty = channel + ? name.trim() !== channel.name || + purpose.trim() !== (channel.purpose ?? "") || + icon !== channel.icon || + colour !== channel.colour + : name.trim() !== "" || purpose.trim() !== ""; + + /** + * ESCAPE, AND A TAP OUTSIDE, CLOSE THE INNERMOST THING AND NEVER DROP WHAT WAS TYPED. + * + * The sheet used to close on either, whatever was open inside it: Escape pressed to fold the icon + * picker threw away the name and the purpose with it, without a word. Now the first press folds + * the picker (and puts focus back on Change icon), and a sheet with something typed in it asks + * "Discard this channel?" rather than closing; Escape again answers Keep editing. Only a sheet + * with nothing to lose closes straight away. Cancel is unchanged: it is the person saying discard. + */ + const dismiss = () => { + if (confirming) { + setConfirming(false); + return; + } + if (picking) { + setPicking(false); + changeIcon.current?.focus(); + return; + } + if (dirty) { + setConfirming(true); + return; + } + onClose(); + }; const save = () => { if (!ready || busy) return; @@ -131,8 +178,16 @@ export function ChannelSheet({ return ( { - if (!next && !busy) onClose(); + onOpenChange={(next, details) => { + if (next || busy) return; + if ( + details.reason === "escape-key" || + details.reason === "outside-press" + ) { + dismiss(); + return; + } + onClose(); }} open > @@ -190,6 +245,17 @@ export function ChannelSheet({ {error.message}

) : null} + {confirming ? ( + { + setConfirming(false); + nameField.current?.focus(); + }} + /> + ) : null}
@@ -205,6 +271,7 @@ export function ChannelSheet({ aria-expanded={picking} className="h-[34px] min-w-0 text-[15px]" onClick={() => setPicking((open) => !open)} + ref={changeIcon} > Change icon @@ -344,3 +411,48 @@ export function ChannelSheet({ ); } + +/** + * The one question before typed fields are dropped: under the top bar, where the sheet's own error + * line goes, with Keep editing first and focused, so a second Escape or a stray Return keeps them. + */ +function ConfirmDiscard({ + editing, + keepRef, + onKeep, + onDiscard, +}: { + editing: boolean; + keepRef: RefObject; + onKeep: () => void; + onDiscard: () => void; +}) { + useEffect(() => { + keepRef.current?.focus(); + }, [keepRef]); + return ( +
+

+ {editing ? "Discard your changes?" : "Discard this channel?"} +

+
+ + Keep editing + + + Discard + +
+
+ ); +} diff --git a/openbot/app/src/components/shell/chat-presence.tsx b/openbot/app/src/components/shell/chat-presence.tsx index 83e95293..26106cb7 100644 --- a/openbot/app/src/components/shell/chat-presence.tsx +++ b/openbot/app/src/components/shell/chat-presence.tsx @@ -180,6 +180,73 @@ export function stepFor( return running ?? THINKING; } +/** What the pill says once the Bot has been thinking with nothing to show for a while. */ +export const STILL_WORKING = "Still working on it"; +/** And once that while has become long. */ +export const TAKING_LONGER = "Taking longer than usual"; +/** How long a quiet "Thinking it through" lasts before the pill says it is still working. */ +export const STILL_WORKING_MS = 20_000; +/** How long before it says this is taking longer than usual. */ +export const TAKING_LONGER_MS = 60_000; + +/** + * The pill's phrase for a wait that has gone quiet: "Thinking it through", then "Still working on + * it" after {@link STILL_WORKING_MS} with nothing new, then "Taking longer than usual" after + * {@link TAKING_LONGER_MS}. Every other phrase is passed through: a named step already says what is + * happening, and a turn that is writing is not quiet. + */ +export function patientStep( + step: string | null, + quietForMs: number, +): string | null { + if (step !== THINKING) return step; + if (quietForMs >= TAKING_LONGER_MS) return TAKING_LONGER; + if (quietForMs >= STILL_WORKING_MS) return STILL_WORKING; + return step; +} + +/** + * {@link patientStep}, timed from the last sign of progress. + * + * WHY THE APP SAYS SOMETHING EARLY, AND THE SERVER DOES NOT. A model that hangs held the pill on + * "Thinking it through" for more than two minutes, until the request itself timed out and the turn + * failed. The server's stall guard (`server/src/channels/stall-guard.ts`) watches remote Bots only, + * and ending a slow turn sooner is not obviously right: a long tool call or a slow model may still + * answer. So nothing is ended here. The pill only stops implying that all is well: after 20 quiet + * seconds it says the Bot is still working, after a minute that this is taking longer than usual. + * + * `progress` is anything that changes when the turn moves (the transcript's length, the last + * message's size), so writing, a new step or a tool result starts the quiet clock again. + */ +export function usePatientStep( + step: string | null, + progress: string, +): string | null { + const moved = `${step ?? ""}\u0000${progress}`; + const [quiet, setQuiet] = useState({ since: Date.now(), now: Date.now() }); + const [movedAt, setMovedAt] = useState(moved); + if (movedAt !== moved) { + // Reset during render, so the frame that moved never shows a stale "Still working on it". + setMovedAt(moved); + const now = Date.now(); + setQuiet({ since: now, now }); + } + const thinking = step === THINKING; + useEffect(() => { + if (!thinking) return; + const timers = [STILL_WORKING_MS, TAKING_LONGER_MS].map((ms) => + setTimeout( + () => setQuiet((current) => ({ ...current, now: Date.now() })), + Math.max(0, quiet.since + ms - Date.now()), + ), + ); + return () => { + for (const timer of timers) clearTimeout(timer); + }; + }, [thinking, quiet.since]); + return patientStep(step, quiet.now - quiet.since); +} + /** * Whether the server says Chat's conversation has a turn running, and that conversation's thread, * from the busy flag it puts on the conversation list. That flag covers runs this screen never diff --git a/openbot/app/src/components/shell/tasks-active.tsx b/openbot/app/src/components/shell/tasks-active.tsx index 77052b09..02dc4e8d 100644 --- a/openbot/app/src/components/shell/tasks-active.tsx +++ b/openbot/app/src/components/shell/tasks-active.tsx @@ -281,14 +281,18 @@ function Group({ }) { const resolved = group.id === "resolved"; const stamp = updated ? ( - + Updated {agoShort(group.updatedAt, now)} ) : null; return (
-
-

+ {/* + * Wraps at large text: the heading keeps its words on one line, and "Updated …" drops under + * it on the right, rather than squeezing "Needs input 1" into a column a word wide. + */} +
+

{resolved ? (

- {/* Room for the last row to scroll clear of the +. */} - {bot ?
: null} + {/* + * Room for the last row to scroll clear of the +: its 56px and its 14px lift, less the 32px the + * list already ends with, and a little air. Pixels, as the button is. + */} + {bot ?
: null} {adding && bot ? ( -
+
@@ -181,7 +195,7 @@ function TasksSwitch({ Up next
-
+
= [ + [ + /rate.?limit|too many requests|\boverloaded\b|\b(429|529)\b/i, + "The AI model is busy right now. Try again in a minute.", + ], + [ + /invalid x-api-key|incorrect api key|invalid_api_key|authentication_error|api key (is )?(invalid|not valid)/i, + "The AI model didn't accept this install's key. An admin needs to update it.", + ], + [ + /\b(operation|request) (has )?timed out\b|\bETIMEDOUT\b|aborted due to timeout/i, + "That took too long, so it was stopped. Try again.", + ], + [ + /Failed after \d+ attempts|Cannot connect to API/i, + "Couldn't reach the AI model. Check your internet connection, then try again.", + ], +]; + /** * The sentence to show for a turn that ended badly, never a raw error. * * Most reasons are written for people: the stall guard's sentence, a Bot's refusal. Some are not: a * transport error with a status line and a JSON body, or the SDK naming a thread or a tool call by * id. Those say nothing a person can act on and leak ids, so they become one plain sentence. + * + * A MODEL THAT FAILED IS SAID AS ONE, IN THE PERSON'S TERMS. The AI SDK's own text reached the + * screen as it was: "Failed after 3 attempts. Last error: Unable to connect. Is the computer able to + * access the url?" ("the computer" is Bun's, and in this product it means a Bot's computer, which had + * nothing to do with it), and "The operation timed out." Each is now matched by its signature and + * becomes a line that says what happened and what to do (`MODEL_FAILURES`). A bare connection error + * with no model signature is the Bot's own endpoint, and says so. */ export function sayableReason(reason: string): string { + for (const [signature, sentence] of MODEL_FAILURES) { + if (signature.test(reason)) return sentence; + } const raw = /\b[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\b/i.test( reason, @@ -353,6 +387,9 @@ export function sayableReason(reason: string): string { /pending interrupt|toolu_|\bThread\b.*\bis locked\b/i.test(reason) || /^(TypeError: )?(Failed to fetch|NetworkError when attempting to fetch resource\.?|Load failed|network error)$/i.test( reason, + ) || + /Is the computer able to access the url|\b(ECONNREFUSED|ECONNRESET|ENOTFOUND|EAI_AGAIN)\b|socket hang up|^fetch failed$/i.test( + reason, ); return raw ? "Couldn't reach the Bot. Try again." : reason; } diff --git a/openbot/app/src/lib/connections/rows.ts b/openbot/app/src/lib/connections/rows.ts index 6e22ac30..4f3dbf51 100644 --- a/openbot/app/src/lib/connections/rows.ts +++ b/openbot/app/src/lib/connections/rows.ts @@ -1,3 +1,4 @@ +import { appConfig } from "@/lib/generated/application-config"; import type { IdentityProviders, LinkedAccount } from "@/lib/identity/queries"; import { brokeredProvenance } from "@/lib/plugins/provider-vocabulary"; import type { CatalogueItem, PluginServer } from "@/lib/plugins/queries"; @@ -51,9 +52,8 @@ export function isBrokeredGithub( } export const IDENTITY_SUMMARIES = { - github: - "Lets OpenBot recognise you on GitHub, so a repository's events can start your Bots' work.", - slack: "Lets OpenBot recognise you when you message it from Slack.", + github: `Lets ${appConfig.brand.productName} recognise you on GitHub, so a repository's events can start your Bots' work.`, + slack: `Lets ${appConfig.brand.productName} recognise you when you message it from Slack.`, } as const; const IDENTITY_TITLES = { github: "GitHub", slack: "Slack" } as const; diff --git a/openbot/app/src/lib/copilot/provider.tsx b/openbot/app/src/lib/copilot/provider.tsx index 42c4029a..85e042fb 100644 --- a/openbot/app/src/lib/copilot/provider.tsx +++ b/openbot/app/src/lib/copilot/provider.tsx @@ -20,6 +20,24 @@ import { ReplyChipsTool } from "./reply-chips-tool"; import { SandboxedTools } from "./sandboxed-tools"; import { SkillTools } from "./skill-tools"; +/** + * Whether CopilotKit's developer inspector is drawn over the app. Off unless a developer asks for it + * with `VITE_COPILOTKIT_INSPECTOR=true` when starting the app. + * + * OFF BY DEFAULT BECAUSE THE SDK'S DEFAULT IS WRONG FOR THIS PRODUCT. The SDK turns the inspector on + * for every development build served from localhost, and the README's own path (`start.sh`) serves + * the app through Vite in development mode. So everybody who followed the README got a floating + * button over the tab bar, plus the SDK's own promotional popover over the header and the page + * titles. The button also sat on the New channel sheet's Create and on a page's "…". That is a + * developer's tool drawn over a person's product. `enableInspector={false}` is the SDK's documented + * switch (`CopilotKitProviderProps.enableInspector`, react-core 1.77), and it removes both. + */ +export function inspectorEnabled( + environment: Record = import.meta.env, +): boolean { + return environment.VITE_COPILOTKIT_INSPECTOR === "true"; +} + /** * The CopilotKit client, wrapped once for the whole authenticated app. * @@ -41,6 +59,7 @@ export function CopilotProvider({ children }: { children: ReactNode }) { { const subscription = agent.subscribe?.({ onRunInitialized: () => setStopped(null), - onRunErrorEvent: ({ event }) => setStopped(stoppedReason(event?.message)), - onRunFailed: ({ error }) => setStopped(stoppedReason(error)), + // Through `sayableReason`, as the channel does, so a raw SDK or network error is never shown. + onRunErrorEvent: ({ event }) => + setStopped(sayableReason(stoppedReason(event?.message))), + onRunFailed: ({ error }) => + setStopped(sayableReason(stoppedReason(error))), }); return () => subscription?.unsubscribe(); }, [agent]); diff --git a/openbot/app/src/lib/plugins/mutations.ts b/openbot/app/src/lib/plugins/mutations.ts index 7063d605..10c2a141 100644 --- a/openbot/app/src/lib/plugins/mutations.ts +++ b/openbot/app/src/lib/plugins/mutations.ts @@ -490,6 +490,45 @@ export function disconnectBrokeredMutationOptions(queryClient: QueryClient) { }); } +/** What ending an account this deployment holds said: whether it ended, and whether the vendor confirmed it. */ +export type HeldDisconnect = { disconnected: boolean; vendorRevoked: boolean }; + +/** + * End the signed-in person's own account for an app this deployment holds the token for (Gmail and + * Google Calendar, Drive, Notion): the same route as a brokered disconnect, which the server sends + * down its own path for these. + * + * Answers with the body, unlike the brokered one, because the page has two different things to say. + * The server asks the vendor to withdraw the grant before ending it here, and `vendorRevoked: false` + * is a vendor that did not confirm it, after which the person still has a step to take there. A + * vendor that could not be reached is a refusal with the server's sentence, and nothing was changed. + * + * Refreshes the plugin screens either way, as every write here does: a refusal is not evidence that + * nothing changed (see `app/tests/plugin-mutation-refresh.test.ts`). + */ +export function disconnectHeldMutationOptions(queryClient: QueryClient) { + return mutationOptions({ + mutationFn: async (serverId: string): Promise => { + const response = await client( + `/api/plugins/servers/${encodeURIComponent(serverId)}/connection`, + { + method: "DELETE", + fallback: "Couldn't disconnect that account. Try again.", + }, + ); + const body = (await response.json().catch(() => null)) as { + disconnected?: unknown; + vendorRevoked?: unknown; + } | null; + return { + disconnected: body?.disconnected === true, + vendorRevoked: body?.vendorRevoked === true, + }; + }, + onSettled: () => invalidatePlugins(queryClient), + }); +} + export function removeSkillMutationOptions(queryClient: QueryClient) { return mutationOptions({ mutationFn: async (slug: string) => { diff --git a/openbot/app/src/routes/_authed/_app/connections.$key.tsx b/openbot/app/src/routes/_authed/_app/connections.$key.tsx index 928b100c..f07a41ab 100644 --- a/openbot/app/src/routes/_authed/_app/connections.$key.tsx +++ b/openbot/app/src/routes/_authed/_app/connections.$key.tsx @@ -1,5 +1,5 @@ import { IconArrowUpRight, IconChevronDown } from "@tabler/icons-react"; -import { useMutation, useQuery } from "@tanstack/react-query"; +import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { createFileRoute, useParams } from "@tanstack/react-router"; import { useEffect, useState } from "react"; import { IdentityConnectionPage } from "@/components/connections/identity-connection-page"; @@ -30,7 +30,11 @@ import { } from "@/components/ui/item"; import { Separator } from "@/components/ui/separator"; import { currentUserQueryOptions } from "@/lib/auth/queries"; -import { connectAccountMutationOptions } from "@/lib/plugins/mutations"; +import { appConfig } from "@/lib/generated/application-config"; +import { + connectAccountMutationOptions, + disconnectHeldMutationOptions, +} from "@/lib/plugins/mutations"; import { brokeredProvenance, disconnectSiblingsNotice, @@ -45,6 +49,9 @@ import { } from "@/lib/plugins/queries"; import { searchTextEntry } from "@/lib/search-text"; +/** The product's own name, for the sentences a person reads here. */ +const PRODUCT = appConfig.brand.productName; + /** * Whether this server is reached through Composio AS THIS PERSON. * @@ -174,6 +181,10 @@ function AppConnectionPage() { const plugins = useQuery(pluginsPageQueryOptions()); const connections = useQuery(connectionsQueryOptions()); const [notice, setNotice] = useState(null); + /** What a disconnect from this page ended, said once it has. Not an error, so not `notice`. */ + const [done, setDone] = useState(null); + const queryClient = useQueryClient(); + const disconnect = useMutation(disconnectHeldMutationOptions(queryClient)); const connect = useMutation({ ...connectAccountMutationOptions(), @@ -517,7 +528,7 @@ function AppConnectionPage() { {entry ? "Bots use one shared account for this app, the same for everybody." - : "OpenBot has no app by that name."} + : `${PRODUCT} has no app by that name.`} ); @@ -534,6 +545,11 @@ function AppConnectionPage() { {notice}

) : null} + {done ? ( +

+ {done} +

+ ) : null} {/* One decision, so no heading: it would only repeat the row's own title. */} @@ -585,23 +601,32 @@ function AppConnectionPage() { */} + disabled={disconnect.isPending} + onClick={() => { /* - * NOT BUILT YET, and it says so rather than appearing to work. + * ENDS IT, AT THE VENDOR FIRST. The server asks the vendor to withdraw the + * grant, then revokes this deployment's copy and deletes the connection + * (`disconnectHeld`). It used to say it could not disconnect here, because + * none of those three acts existed for an account held in the vault. * - * Withdrawing is three acts — revoke at the vendor, revoke the vault - * credential, delete the row — and none exist. An item that closed the menu - * and changed nothing would report that access had been withdrawn when it - * had not, which is the one outcome worse than not offering it. + * What it says after depends on the vendor's answer, because the person's + * next step does: nothing, or remove the app at the vendor as well. */ - setNotice( - `You can't disconnect here yet. To stop access now, remove OpenBot in your ${entry.vendor} account's third-party access settings.`, - ) - } - className="whitespace-nowrap" + setNotice(null); + setDone(null); + disconnect.mutate(key, { + onSuccess: (answer) => + setDone( + answer.vendorRevoked || !answer.disconnected + ? `Disconnected. ${PRODUCT} can't use your ${entry.vendor} account any more.` + : `Disconnected here. ${entry.vendor} didn't confirm it, so also remove ${PRODUCT} in your ${entry.vendor} account's third-party access settings.`, + ), + onError: (thrown: Error) => setNotice(thrown.message), + }); + }} variant="destructive" > - Disconnect your {entry.title} account + {disconnect.isPending ? "Disconnecting…" : "Disconnect"} @@ -631,7 +656,7 @@ function AppConnectionPage() { {connection ? ( diff --git a/openbot/app/src/routes/_authed/_app/connections.index.tsx b/openbot/app/src/routes/_authed/_app/connections.index.tsx index 26778366..36c14f7b 100644 --- a/openbot/app/src/routes/_authed/_app/connections.index.tsx +++ b/openbot/app/src/routes/_authed/_app/connections.index.tsx @@ -34,6 +34,7 @@ import { ItemTitle, } from "@/components/ui/item"; import { type ConnectionRow, connectionRows } from "@/lib/connections/rows"; +import { appConfig } from "@/lib/generated/application-config"; import { identityProvidersQueryOptions, linkedAccountsQueryOptions, @@ -130,7 +131,7 @@ export const AUTHORIZATION_NOTICES: ReadonlyMap = new Map([ ], [ "vendor-unavailable", - "The app didn't answer when OpenBot tried to finish that authorization, so nothing was recorded. Try the link again in a moment.", + `The app didn't answer when ${appConfig.brand.productName} tried to finish that authorization, so nothing was recorded. Try the link again in a moment.`, ], [ "not-your-authorization", diff --git a/openbot/app/src/routes/_authed/admin/components/$name.tsx b/openbot/app/src/routes/_authed/admin/components/$name.tsx index 56512916..31dec5e2 100644 --- a/openbot/app/src/routes/_authed/admin/components/$name.tsx +++ b/openbot/app/src/routes/_authed/admin/components/$name.tsx @@ -60,6 +60,7 @@ import { dataFunctionsQueryOptions, } from "@/lib/components/queries"; import { RENDERABLE_NAMES } from "@/lib/copilot/gallery-registry"; +import { appConfig } from "@/lib/generated/application-config"; import { queryClient } from "@/query-client"; /** @@ -326,7 +327,8 @@ function ComponentDetail({ Not in this build - OpenBot can't draw it. The settings below have no effect. + {appConfig.brand.productName} can't draw it. The settings + below have no effect. diff --git a/openbot/app/src/routes/_authed/admin/computers.tsx b/openbot/app/src/routes/_authed/admin/computers.tsx index fe314eee..502fc005 100644 --- a/openbot/app/src/routes/_authed/admin/computers.tsx +++ b/openbot/app/src/routes/_authed/admin/computers.tsx @@ -37,6 +37,7 @@ import { type HostAccessPendingOperation, } from "@/lib/computers/host-access"; import { computerFleetQueryOptions } from "@/lib/computers/queries"; +import { appConfig } from "@/lib/generated/application-config"; export const Route = createFileRoute("/_authed/admin/computers")({ component: ComputersPage, @@ -359,7 +360,7 @@ function HostFoldersSection({ return ( {problem ? ( diff --git a/openbot/app/src/routes/_authed/admin/credentials.tsx b/openbot/app/src/routes/_authed/admin/credentials.tsx index 49bd6fd9..41435a07 100644 --- a/openbot/app/src/routes/_authed/admin/credentials.tsx +++ b/openbot/app/src/routes/_authed/admin/credentials.tsx @@ -55,6 +55,7 @@ import { type CredentialStatus, credentialListQueryOptions, } from "@/lib/credentials/queries"; +import { appConfig } from "@/lib/generated/application-config"; export const Route = createFileRoute("/_authed/admin/credentials")({ component: CredentialsPage, @@ -95,7 +96,7 @@ function CredentialsPage() { Add credential } - description="Keys and tokens for this deployment. OpenBot never shows a secret after it is saved." + description={`Keys and tokens for this deployment. ${appConfig.brand.productName} never shows a secret after it is saved.`} title="Credentials" > {/* diff --git a/openbot/app/src/routes/_authed/admin/learning.tsx b/openbot/app/src/routes/_authed/admin/learning.tsx index c5094d63..eef49249 100644 --- a/openbot/app/src/routes/_authed/admin/learning.tsx +++ b/openbot/app/src/routes/_authed/admin/learning.tsx @@ -5,6 +5,7 @@ import { PageSection, PageShell } from "@/components/layout/page-shell"; import { Button } from "@/components/ui/button"; import { Input } from "@/components/ui/input"; import { agentListQueryOptions } from "@/lib/agents/queries"; +import { appConfig } from "@/lib/generated/application-config"; import { createLearningContainer, learningAutomaticQueryOptions, @@ -35,13 +36,13 @@ function automaticMessage( automatic.state === "assigned" && settings.defaultTarget?.containerId === automatic.containerId ) - return `OpenBot assigned this Intelligence project's "${automatic.containerId}" container automatically. You can change it here.`; + return `${appConfig.brand.productName} assigned this Intelligence project's "${automatic.containerId}" container automatically. You can change it here.`; if (automatic.state === "absent") return `This Intelligence project has no "${automatic.containerId}" Learning container yet. Nothing is collected.`; if (automatic.state === "not_entitled") return "This Intelligence project's plan doesn't include Learning. No container was assigned."; if (automatic.state === "unavailable") - return "Couldn't reach Intelligence to find this project's Learning container. OpenBot will try again."; + return `Couldn't reach Intelligence to find this project's Learning container. ${appConfig.brand.productName} will try again.`; return undefined; } @@ -197,7 +198,7 @@ export function LearningPage() { > {settingsQuery.isPending && (

Loading Learning settings…

@@ -479,9 +480,10 @@ export function LearningPage() {

Managing Learning needs your Intelligence account and project - access. OpenBot keeps project credentials on the server. Turning - Learning off here keeps settings and conversation assignments. - Schedules and publishing stay in Intelligence. + access. {appConfig.brand.productName} keeps project credentials on + the server. Turning Learning off here keeps settings and + conversation assignments. Schedules and publishing stay in + Intelligence.

diff --git a/openbot/app/src/routes/_authed/admin/plugins/$key.tsx b/openbot/app/src/routes/_authed/admin/plugins/$key.tsx index 4d780e42..7bde1947 100644 --- a/openbot/app/src/routes/_authed/admin/plugins/$key.tsx +++ b/openbot/app/src/routes/_authed/admin/plugins/$key.tsx @@ -45,6 +45,7 @@ import { Switch } from "@/components/ui/switch"; import { grantRoster } from "@/lib/agents/grant-roster"; import { agentListQueryOptions } from "@/lib/agents/queries"; import { storeMcpToken } from "@/lib/credentials/mutations"; +import { appConfig } from "@/lib/generated/application-config"; import { addCuratedServerMutationOptions, connectAccountMutationOptions, @@ -321,7 +322,7 @@ function RouteComponent() { */ if (authorizationUrl === null) { setError( - "OpenBot got no consent link back, so nothing was connected. Try again, or check this connector's setup if it keeps happening.", + `${appConfig.brand.productName} got no consent link back, so nothing was connected. Try again, or check this connector's setup if it keeps happening.`, ); return; } @@ -980,7 +981,8 @@ function RouteComponent() {

Add this to the client's authorised redirect URIs at the vendor, exactly as written. One wrong character fails there, - with an error that doesn't mention OpenBot. + with an error that doesn't mention{" "} + {appConfig.brand.productName}.

)} {!plugins.data?.redirectUri ? ( diff --git a/openbot/app/src/routes/_authed/onboarding.tsx b/openbot/app/src/routes/_authed/onboarding.tsx index 50f2ed16..b4e0661c 100644 --- a/openbot/app/src/routes/_authed/onboarding.tsx +++ b/openbot/app/src/routes/_authed/onboarding.tsx @@ -239,15 +239,20 @@ function RosterStep() { })), ]; + /* + * THE ROSTER IS THIS STEP'S PICTURE, AND THE ONLY ONE. It used to sit under an animated approval + * card (the hand-off demo), which filled a phone's screen by itself: the step about picking a Bot + * opened on Refuse and Allow once, with the Bots to pick from cut off below the fold. Asking + * before acting has its own step, so here the roster is drawn first and whole. + */ return (

Pick a Bot or make your own

- `${a.name} (${a.example ? "example" : "available"})`).join(", ")}, and one of your own`} > {roster.map((a) => { @@ -292,20 +297,43 @@ function RosterStep() { * the product backlog). None of it is said to an organization's own people, who are past all three. */ +/** + * Where an evaluator's data is kept, said as the server actually keeps it. + * + * IT USED TO SAY CONVERSATIONS STAYED ON THIS COMPUTER, AND THEY DO NOT. The server refuses to start + * without CopilotKit Intelligence (`INTELLIGENCE_API_URL` and `INTELLIGENCE_API_KEY`, see + * `.env.example` and `docs/telemetry.md`), because Intelligence owns durable threads and memory. Its + * address defaults to CopilotKit's managed service, and only somebody who runs Intelligence + * themselves points it elsewhere. So each sentence below is one thing this install verifiably does: + * + * - Bots and settings are rows in this install's own Postgres (`agents`, preferences, rules). + * "This install's database", not "this computer": `DATABASE_URL` decides where that is, and + * nothing here reads it. Files and connected accounts are left out on purpose. A file's bytes are + * in that database, but what a message carries about it also reaches the thread and the model; + * and an app connected through Composio or Arcade is held by that service, not here. + * - Conversations and memory are in Intelligence, named, with where it runs by default. + * - Every message goes to the model provider this install was set up with. + * - The CopilotKit runtime sends usage events to CopilotKit unless `DO_NOT_TRACK=1` or + * `COPILOTKIT_TELEMETRY_DISABLED=true` is set (`server/src/telemetry.ts`). Naming the variable is + * fine here: this step is shown only to a single-user install, whose one reader runs the server. + * + * Nothing about sign-in: activating a local install signs you in to CopilotKit, which is its own + * question and not this step's. + */ function LocalStep() { return (

- It runs on your machine + Where your data lives

- {/* - * "Stored", and the model named, because "your data never leaves" would be untrue: every turn - * goes to the model provider this install was set up with. What stays is everything kept. No - * word about sign-in either: activating a local install signs you in to CopilotKit. - */}

- Your conversations, files and Bots are stored on this computer. Only - what you send to your model leaves it. + Your Bots and settings stay in this install's database. Conversations + and memory are kept in CopilotKit Intelligence, in CopilotKit's cloud + unless you host it yourself. What you ask goes to your AI model. +

+

+ CopilotKit also receives usage events. Set DO_NOT_TRACK=1 to turn them + off.

diff --git a/openbot/app/tests/channel-sheet.test.tsx b/openbot/app/tests/channel-sheet.test.tsx index e881fd2d..de907b8b 100644 --- a/openbot/app/tests/channel-sheet.test.tsx +++ b/openbot/app/tests/channel-sheet.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 { QueryClientProvider } from "@tanstack/react-query"; import { cleanup, render, waitFor, within } from "@testing-library/react"; @@ -255,3 +262,95 @@ test("editing a channel starts from it, saves only what changed, and can archive path: "/api/openduet/channels/c-kiln/archive", }); }); + +describe("Escape closes only the innermost thing, and never drops what was typed", () => { + test("with the icon picker open, Escape folds the picker and keeps the sheet and its fields", async () => { + serving(() => Response.json({})); + const user = userEvent.setup({ document }); + let closed = 0; + const { view } = draw({ onClose: () => closed++ }); + const dialog = await view.findByRole("dialog", { name: "New channel" }); + await user.type(within(dialog).getByLabelText("Name"), "Kiln move"); + const change = within(dialog).getByRole("button", { name: "Change icon" }); + await user.click(change); + await user.click( + within(within(dialog).getByRole("group", { name: "Icon" })).getByRole( + "radio", + { name: "Home" }, + ), + ); + + await user.keyboard("{Escape}"); + + expect(closed).toBe(0); + expect(change.getAttribute("aria-expanded")).toBe("false"); + expect(document.activeElement).toBe(change); + expect( + (within(dialog).getByLabelText("Name") as HTMLInputElement).value, + ).toBe("Kiln move"); + expect(preview(dialog)?.getAttribute("data-channel-icon")).toBe("home"); + }); + + test("with something typed, Escape asks first; Keep editing keeps it, Discard closes", async () => { + serving(() => Response.json({})); + const user = userEvent.setup({ document }); + let closed = 0; + const { view } = draw({ onClose: () => closed++ }); + const dialog = await view.findByRole("dialog", { name: "New channel" }); + await user.type( + within(dialog).getByLabelText("Purpose"), + "Moving the kiln by March.", + ); + + await user.keyboard("{Escape}"); + expect(closed).toBe(0); + expect(within(dialog).getByText("Discard this channel?")).toBeTruthy(); + // Keep editing is focused, so a second Escape or a Return keeps the draft. + const keep = within(dialog).getByRole("button", { name: "Keep editing" }); + expect(document.activeElement).toBe(keep); + + await user.keyboard("{Escape}"); + expect(closed).toBe(0); + expect(within(dialog).queryByText("Discard this channel?")).toBeNull(); + expect( + (within(dialog).getByLabelText("Purpose") as HTMLTextAreaElement).value, + ).toBe("Moving the kiln by March."); + + await user.keyboard("{Escape}"); + await user.click(within(dialog).getByRole("button", { name: "Discard" })); + expect(closed).toBe(1); + }); + + test("with nothing typed, Escape simply closes", async () => { + serving(() => Response.json({})); + const user = userEvent.setup({ document }); + let closed = 0; + const { view } = draw({ onClose: () => closed++ }); + await view.findByRole("dialog", { name: "New channel" }); + + await user.keyboard("{Escape}"); + expect(closed).toBe(1); + }); + + test("editing asks about changes only once something differs", async () => { + serving(() => Response.json({})); + const user = userEvent.setup({ document }); + let closed = 0; + const { view } = draw({ channel: kiln, onClose: () => closed++ }); + const dialog = await view.findByRole("dialog", { name: "Edit channel" }); + + await user.click(within(dialog).getByLabelText("Name")); + await user.keyboard("{Escape}"); + expect(closed).toBe(1); + + cleanup(); + const again = draw({ channel: kiln, onClose: () => closed++ }); + const edited = await again.view.findByRole("dialog", { + name: "Edit channel", + }); + await user.type(within(edited).getByLabelText("Name"), " soon"); + await user.keyboard("{Escape}"); + expect(within(edited).getByText("Discard your changes?")).toBeTruthy(); + expect(closed).toBe(1); + }); +}); diff --git a/openbot/app/tests/chat-run-lifecycle.test.tsx b/openbot/app/tests/chat-run-lifecycle.test.tsx index 44c8a20b..6444494e 100644 --- a/openbot/app/tests/chat-run-lifecycle.test.tsx +++ b/openbot/app/tests/chat-run-lifecycle.test.tsx @@ -705,6 +705,48 @@ test.each([ 15_000, ); +test("a model that fails mid-turn is said plainly, and still said after a reload until something newer arrives", async () => { + const PLAIN = + "Couldn't reach the AI model. Check your internet connection, then try again."; + const channel = freshChannel(); + reset(); + quiet(); + onRun = (input) => + sse([ + { type: "RUN_STARTED", threadId: input.threadId, runId: input.runId }, + { + type: "RUN_ERROR", + message: + "Failed after 3 attempts. Last error: Cannot connect to API: Unable to connect. Is the computer able to access the url?", + }, + ]); + const view = render(tree(channel)); + await waitFor(() => expect(connectCalls).toBe(1)); + await type(view, "What's on today?"); + expect(await view.findByText(PLAIN)).toBeTruthy(); + expect(view.container.textContent).not.toContain("Is the computer able"); + const sent = runRequests[0]!.messages.at(-1)!; + + // A reload: the thread's stored transcript still ends on the message that failed. + cleanup(); + reset({ stored: [sent] }); + const reloaded = render(tree(channel)); + expect(await reloaded.findByText(PLAIN)).toBeTruthy(); + + // Once a reply has arrived since, from anywhere, the sentence is old news and is not shown. + cleanup(); + reset({ + stored: [ + sent, + { id: "reply-later", role: "assistant", content: "Here's your day." }, + ], + }); + const later = render(tree(channel)); + expect(await later.findByText("Here's your day.")).toBeTruthy(); + expect(later.queryByText(PLAIN)).toBeNull(); + window.sessionStorage.clear(); +}, 15_000); + test.each(["network failure", "thread conflict"])( "a failed frontend-tool continuation (%s) restores the draft after the intermediate run finishes", async (failure) => { diff --git a/openbot/app/tests/copilot-inspector.test.ts b/openbot/app/tests/copilot-inspector.test.ts new file mode 100644 index 00000000..67058d42 --- /dev/null +++ b/openbot/app/tests/copilot-inspector.test.ts @@ -0,0 +1,22 @@ +import { expect, test } from "bun:test"; +import { inspectorEnabled } from "@/lib/copilot/provider"; + +/** + * CopilotKit's developer inspector is off unless a developer asks for it. + * + * The SDK turns it on for every development build on localhost, which is exactly how the README's + * `start.sh` serves the app, so without this everybody saw a floating inspector button and the SDK's + * promotional popover over the header, the New channel sheet and a page's actions. + */ + +test("the inspector is off when nothing asks for it", () => { + expect(inspectorEnabled({})).toBe(false); + expect(inspectorEnabled({ DEV: true, MODE: "development" })).toBe(false); +}); + +test("only an explicit VITE_COPILOTKIT_INSPECTOR=true turns it on", () => { + expect(inspectorEnabled({ VITE_COPILOTKIT_INSPECTOR: "true" })).toBe(true); + for (const value of ["1", "yes", "TRUE", "", "false"]) { + expect(inspectorEnabled({ VITE_COPILOTKIT_INSPECTOR: value })).toBe(false); + } +}); diff --git a/openbot/app/tests/held-account-disconnect.test.tsx b/openbot/app/tests/held-account-disconnect.test.tsx new file mode 100644 index 00000000..d3aa3c99 --- /dev/null +++ b/openbot/app/tests/held-account-disconnect.test.tsx @@ -0,0 +1,246 @@ +import { + afterAll, + afterEach, + beforeAll, + beforeEach, + 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, render, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { + PluginConnections, + PluginServer, + PluginsPage, +} from "@/lib/plugins/queries"; +import { Route as ConnectedAccountRoute } from "@/routes/_authed/_app/connections.$key"; +import { settleReactWork } from "./settle-react-work"; + +/** + * Disconnecting Gmail and Google Calendar, an account this deployment holds the token for. + * + * The page used to offer "Disconnect" and then say it couldn't, leaving the token in place. It now + * ends the account through the server, which asks Google first, and says what happened: done, or + * done here with one more step at Google when Google did not confirm it. + * + * The harness is `composio-unconfigured-account.test.tsx`'s: the exported `Route` is captured and + * restored around its `.update()`. + */ + +beforeAll(() => GlobalRegistrator.register({ url: "http://localhost:3010" })); +afterEach(cleanup); +afterAll(async () => { + await settleReactWork(); + GlobalRegistrator.unregister(); +}); + +const originalFetch = global.fetch; +let connected: boolean; +let deletes: string[]; +let answer: () => Response; + +beforeEach(() => { + connected = true; + deletes = []; + answer = () => Response.json({ disconnected: true, vendorRevoked: true }); + global.fetch = Object.assign( + async (input: RequestInfo | URL, init?: RequestInit) => { + const path = String(input); + if (path === "/api/plugins") return Response.json(page); + if (path === "/api/plugins/connections") + return Response.json(connectionsNow()); + if ( + path === "/api/plugins/servers/google-workspace/connection" && + init?.method === "DELETE" + ) { + deletes.push(path); + const response = answer(); + if (response.ok) connected = false; + return response; + } + return new Response(null, { status: 404 }); + }, + { preconnect: originalFetch.preconnect }, + ); +}); + +afterEach(() => { + global.fetch = originalFetch; +}); + +const workspace = { + id: "google-workspace", + title: "Gmail and Google Calendar", + summary: "Mail and calendar of whoever is asking.", + vendor: "Google", + url: "https://www.googleapis.com/", + provenance: "first-party", + authScheme: null, + accountMode: "personal", + tools: [], +} as unknown as PluginServer; + +const page: PluginsPage = { + catalogue: [ + { + key: "google-workspace", + title: "Gmail and Google Calendar", + vendor: "Google", + summary: "Mail and calendar of whoever is asking.", + docsUrl: "https://developers.google.com", + auth: "user-oauth", + perInstance: false, + }, + ], + servers: [workspace], + skills: [], + botsMayCallBack: true, + redirectUri: "https://app.example/api/plugins/oauth/callback", + composioConfigured: false, + authorizesPerAction: false, +}; + +function connectionsNow(): PluginConnections { + return { + connections: connected + ? [ + { + serverId: "google-workspace", + scope: "https://www.googleapis.com/auth/gmail.readonly", + connectedAt: "2026-10-20T09:00:00Z", + revoked: false, + holder: "person", + }, + ] + : [], + redirectUri: page.redirectUri, + }; +} + +type Updatable = { update: (options: unknown) => unknown }; +const pristine = { + ...ConnectedAccountRoute, + options: { ...ConnectedAccountRoute.options }, +} as Record; +afterEach(() => { + for (const key of Object.keys(ConnectedAccountRoute)) { + if (!(key in pristine)) { + delete (ConnectedAccountRoute as unknown as Record)[key]; + } + } + Object.assign(ConnectedAccountRoute, pristine); +}); + +function renderAccount() { + const client = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + const rootRoute = createRootRoute({ component: Outlet }); + const authedRoute = createRoute({ + id: "/_authed", + getParentRoute: () => rootRoute, + component: Outlet, + }); + const appRoute = createRoute({ + id: "/_app", + getParentRoute: () => authedRoute, + component: Outlet, + }); + const connectionsRoute = createRoute({ + path: "/connections", + getParentRoute: () => appRoute, + component: Outlet, + }); + const indexRoute = createRoute({ + path: "/", + getParentRoute: () => connectionsRoute, + component: () => null, + }); + const account = (ConnectedAccountRoute as unknown as Updatable).update({ + id: "/$key", + path: "/$key", + getParentRoute: () => connectionsRoute, + }); + const router = createRouter({ + routeTree: rootRoute.addChildren([ + authedRoute.addChildren([ + appRoute.addChildren([ + connectionsRoute.addChildren([indexRoute, account as never]), + ]), + ]), + ]), + history: createMemoryHistory({ + initialEntries: ["/connections/google-workspace"], + }), + }); + return render( + + + , + ); +} + +async function pressDisconnect(view: ReturnType) { + const user = userEvent.setup({ document }); + await user.click(await view.findByRole("button", { name: /Connected/ })); + await user.click(await view.findByRole("menuitem", { name: "Disconnect" })); +} + +test("Disconnect ends the account, says so, and the row offers Connect again", async () => { + const view = renderAccount(); + await pressDisconnect(view); + + expect( + await view.findByText( + "Disconnected. OpenDuet can't use your Google account any more.", + ), + ).toBeTruthy(); + expect(deletes).toEqual(["/api/plugins/servers/google-workspace/connection"]); + await waitFor(() => + expect(view.getByRole("button", { name: "Connect" })).toBeTruthy(), + ); + // The old "can't disconnect here yet" sentence, and the old product name, are gone. + expect(view.queryByText(/can't disconnect here/)).toBeNull(); + expect(document.body.textContent).not.toContain("OpenBot"); +}); + +test("when Google did not confirm it, the person is told the one step left", async () => { + answer = () => Response.json({ disconnected: true, vendorRevoked: false }); + const view = renderAccount(); + await pressDisconnect(view); + + expect( + await view.findByText( + "Disconnected here. Google didn't confirm it, so also remove OpenDuet in your Google account's third-party access settings.", + ), + ).toBeTruthy(); +}); + +test("when Google could not be reached, the server's sentence is shown and the account stays", async () => { + answer = () => + Response.json( + { + error: + "Couldn't reach Google, so nothing was disconnected. Try again in a moment.", + }, + { status: 502 }, + ); + const view = renderAccount(); + await pressDisconnect(view); + + const alert = await view.findByRole("alert"); + expect(alert.textContent).toBe( + "Couldn't reach Google, so nothing was disconnected. Try again in a moment.", + ); + expect(view.getByRole("button", { name: /Connected/ })).toBeTruthy(); +}); diff --git a/openbot/app/tests/learning-admin.test.tsx b/openbot/app/tests/learning-admin.test.tsx index 645ae083..35b72513 100644 --- a/openbot/app/tests/learning-admin.test.tsx +++ b/openbot/app/tests/learning-admin.test.tsx @@ -152,7 +152,7 @@ test.each([ [ { enabled: true, defaultTarget: { containerId: "openbot" }, agents: {} }, { state: "assigned", containerId: "openbot" }, - 'OpenBot assigned this Intelligence project\'s "openbot" container automatically', + 'OpenDuet assigned this Intelligence project\'s "openbot" container automatically', ], [ { enabled: true, defaultTarget: null, agents: {} }, @@ -182,7 +182,7 @@ test("a deployment that chose its own container hears nothing about automatic as await waitFor(() => expect(view.queryByText("Loading Learning settings…")).toBeNull(), ); - expect(view.queryByText("OpenBot assigned", { exact: false })).toBeNull(); + expect(view.queryByText("OpenDuet assigned", { exact: false })).toBeNull(); expect( view.queryByText("Learning container yet", { exact: false }), ).toBeNull(); diff --git a/openbot/app/tests/onboarding-route.test.tsx b/openbot/app/tests/onboarding-route.test.tsx index 75df5ded..02706f4f 100644 --- a/openbot/app/tests/onboarding-route.test.tsx +++ b/openbot/app/tests/onboarding-route.test.tsx @@ -58,6 +58,8 @@ const originalFetch = globalThis.fetch; let saved: number; let saves: number[]; let completed: boolean; +/** Whether the server says this is somebody trying OpenDuet alone, which adds three steps. */ +let localEvaluation: boolean; beforeEach(() => { // Another file's screens may have cached the signed-in person in the app's one query client. @@ -65,6 +67,7 @@ beforeEach(() => { saved = 0; saves = []; completed = false; + localEvaluation = false; (globalThis as { Notification?: unknown }).Notification = { permission: "default", requestPermission: async () => "default", @@ -106,7 +109,7 @@ beforeEach(() => { }); } if (path === "/api/capabilities") - return Response.json({ localEvaluation: false }); + return Response.json({ localEvaluation }); if (path.startsWith("/api/agents")) return Response.json({ agents: [BOT] }); if (path === "/api/plugins/for/openduet/apps/google-workspace") @@ -187,6 +190,23 @@ test("arriving without a step writes the first one into the address, in place", expect(view.history.length).toBe(1); }); +test("the local install's step says where its data really lives", async () => { + localEvaluation = true; + const view = open("/onboarding?step=local"); + + await heading(view, "Where your data lives"); + const page = document.body.textContent ?? ""; + // Conversations live in Intelligence, which is CopilotKit's cloud unless self-hosted, and every + // message reaches the model: neither is "on this computer". + expect(page).toContain( + "Conversations and memory are kept in CopilotKit Intelligence", + ); + expect(page).toContain("What you ask goes to your AI model."); + expect(page).toContain("Set DO_NOT_TRACK=1 to turn them off."); + expect(page).not.toContain("stored on this computer"); + expect(page).not.toContain("Only what you send to your model leaves it"); +}); + test("Next adds a step to the address and to history, and saves it", async () => { const view = open("/onboarding?step=welcome"); await heading(view, "Welcome to OpenDuet"); diff --git a/openbot/app/tests/patient-step.test.tsx b/openbot/app/tests/patient-step.test.tsx new file mode 100644 index 00000000..13ad5da2 --- /dev/null +++ b/openbot/app/tests/patient-step.test.tsx @@ -0,0 +1,67 @@ +import { afterAll, afterEach, beforeAll, expect, jest, test } from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { act, cleanup, renderHook } from "@testing-library/react"; +import { + patientStep, + STILL_WORKING, + STILL_WORKING_MS, + TAKING_LONGER, + TAKING_LONGER_MS, + THINKING, + usePatientStep, +} from "@/components/shell/chat-presence"; +import { settleReactWork } from "./settle-react-work"; + +/** + * A Bot that thinks with nothing to show for a while gets a gentle word in the pill, rather than + * "Thinking it through" for two minutes and then a failure. Nothing is ended: only what the pill + * says changes, and any sign of progress puts it back. + */ + +beforeAll(() => GlobalRegistrator.register({ url: "http://localhost:3010" })); +afterEach(() => { + cleanup(); + jest.useRealTimers(); +}); +afterAll(async () => { + await settleReactWork(); + GlobalRegistrator.unregister(); +}); + +test("a quiet think becomes still working, then taking longer than usual", () => { + expect(patientStep(THINKING, 0)).toBe(THINKING); + expect(patientStep(THINKING, STILL_WORKING_MS - 1)).toBe(THINKING); + expect(patientStep(THINKING, STILL_WORKING_MS)).toBe(STILL_WORKING); + expect(patientStep(THINKING, TAKING_LONGER_MS)).toBe(TAKING_LONGER); +}); + +test("a named step and no step pass through untouched", () => { + expect(patientStep("Searching your email", TAKING_LONGER_MS * 2)).toBe( + "Searching your email", + ); + expect(patientStep(null, TAKING_LONGER_MS * 2)).toBeNull(); +}); + +test("the pill moves on with time, and progress starts the quiet clock again", () => { + jest.useFakeTimers(); + const view = renderHook( + ({ step, progress }: { step: string | null; progress: string }) => + usePatientStep(step, progress), + { initialProps: { step: THINKING, progress: "1:0" } }, + ); + expect(view.result.current).toBe(THINKING); + + act(() => { + jest.advanceTimersByTime(STILL_WORKING_MS); + }); + expect(view.result.current).toBe(STILL_WORKING); + + act(() => { + jest.advanceTimersByTime(TAKING_LONGER_MS - STILL_WORKING_MS); + }); + expect(view.result.current).toBe(TAKING_LONGER); + + // The answer starts arriving: the turn is not quiet any more. + view.rerender({ step: THINKING, progress: "2:14" }); + expect(view.result.current).toBe(THINKING); +}); diff --git a/openbot/app/tests/plugin-mutation-refresh.test.ts b/openbot/app/tests/plugin-mutation-refresh.test.ts index 3dd72fd7..0b14a6de 100644 --- a/openbot/app/tests/plugin-mutation-refresh.test.ts +++ b/openbot/app/tests/plugin-mutation-refresh.test.ts @@ -18,6 +18,7 @@ import { connectAccountMutationOptions, connectBrokeredWithFieldsMutationOptions, disconnectBrokeredMutationOptions, + disconnectHeldMutationOptions, enableComposioAppMutationOptions, recheckBrokeredConnectionMutationOptions, refreshPluginServerMutationOptions, @@ -268,6 +269,15 @@ const REFUSALS: { factory: disconnectBrokeredMutationOptions, variables: "gmail", }, + { + name: "disconnecting an account this deployment holds", + route: "DELETE /api/plugins/servers/:id/connection", + status: 502, + message: + "Couldn't reach Google, so nothing was disconnected. Try again in a moment.", + factory: disconnectHeldMutationOptions, + variables: "google-workspace", + }, { name: "handing over a typed key", route: "POST /api/plugins/servers/:id/connect", @@ -525,6 +535,12 @@ const ENCODED_REQUESTS: { build: disconnectBrokeredMutationOptions, variables: HOSTILE_ID, }, + { + name: "disconnecting an account this deployment holds", + url: `/api/plugins/servers/${ENCODED_ID}/connection`, + build: disconnectHeldMutationOptions, + variables: HOSTILE_ID, + }, { name: "asking an app what it wants typed in", url: `/api/plugins/servers/${ENCODED_ID}/connect`, diff --git a/openbot/app/tests/run-error-memory.test.ts b/openbot/app/tests/run-error-memory.test.ts new file mode 100644 index 00000000..f36e8fd6 --- /dev/null +++ b/openbot/app/tests/run-error-memory.test.ts @@ -0,0 +1,47 @@ +import { afterAll, afterEach, beforeAll, expect, test } from "bun:test"; +import { GlobalRegistrator } from "@happy-dom/global-registrator"; +import { + forgetRunError, + recalledRunError, + rememberRunError, +} from "@/lib/channels/run-error-memory"; + +/** + * Why a conversation's last turn failed survives a reload in this tab, and only while nothing has + * happened in the conversation since. + */ + +beforeAll(() => GlobalRegistrator.register({ url: "http://localhost:3010" })); +afterEach(() => window.sessionStorage.clear()); +afterAll(() => GlobalRegistrator.unregister()); + +const SAID = + "Couldn't reach the AI model. Check your internet connection, then try again."; + +test("a failed turn's sentence comes back while its message is still the last one", () => { + rememberRunError("channel-1", SAID, "message-7"); + expect(recalledRunError("channel-1", "message-7")).toBe(SAID); + // Another conversation has nothing to recall. + expect(recalledRunError("channel-2", "message-7")).toBeNull(); +}); + +test("anything newer in the conversation makes it old news", () => { + rememberRunError("channel-1", SAID, "message-7"); + expect(recalledRunError("channel-1", "message-8")).toBeNull(); +}); + +test("a new turn forgets it", () => { + rememberRunError("channel-1", SAID, "message-7"); + forgetRunError("channel-1"); + expect(recalledRunError("channel-1", "message-7")).toBeNull(); +}); + +test("something unreadable in storage is no sentence at all", () => { + window.sessionStorage.setItem("openduet.run-error.channel-1", "{not json"); + expect(recalledRunError("channel-1", "message-7")).toBeNull(); + window.sessionStorage.setItem( + "openduet.run-error.channel-1", + JSON.stringify({ said: 42, after: "message-7" }), + ); + expect(recalledRunError("channel-1", "message-7")).toBeNull(); +}); diff --git a/openbot/app/tests/run-lifecycle.test.ts b/openbot/app/tests/run-lifecycle.test.ts index 275c0c51..a534deb1 100644 --- a/openbot/app/tests/run-lifecycle.test.ts +++ b/openbot/app/tests/run-lifecycle.test.ts @@ -145,6 +145,42 @@ describe("sayableReason", () => { const said = "The Bot stopped responding, so this turn was ended."; expect(sayableReason(said)).toBe(said); }); + + test("a model that could not be reached is said plainly, never in the SDK's words", () => { + // What the release review saw, word for word: "the computer" is Bun's, not a Bot's computer. + const seen = + "Failed after 3 attempts. Last error: Cannot connect to API: Unable to connect. Is the computer able to access the url?"; + expect(sayableReason(seen)).toBe( + "Couldn't reach the AI model. Check your internet connection, then try again.", + ); + expect(sayableReason(seen)).not.toContain("computer"); + }); + + test("each kind of model failure gets its own next step", () => { + expect(sayableReason("The operation timed out.")).toBe( + "That took too long, so it was stopped. Try again.", + ); + // The retry wrapper names the cause last, and the cause is what is said. + expect( + sayableReason("Failed after 3 attempts. Last error: Overloaded"), + ).toBe("The AI model is busy right now. Try again in a minute."); + expect(sayableReason("429 Too Many Requests: rate limit reached")).toBe( + "The AI model is busy right now. Try again in a minute.", + ); + expect(sayableReason("401 invalid x-api-key")).toBe( + "The AI model didn't accept this install's key. An admin needs to update it.", + ); + }); + + test("a bare connection error with no model in it is the Bot's endpoint", () => { + for (const raw of [ + "Unable to connect. Is the computer able to access the url?", + "connect ECONNREFUSED 127.0.0.1:4210", + "fetch failed", + ]) { + expect(sayableReason(raw)).toBe("Couldn't reach the Bot. Try again."); + } + }); }); describe("joinOrFollow", () => { diff --git a/openbot/server/src/audit.ts b/openbot/server/src/audit.ts index 441c2f59..dcf863b4 100644 --- a/openbot/server/src/audit.ts +++ b/openbot/server/src/audit.ts @@ -225,9 +225,14 @@ export const auditEventTypes = [ * administrator removing them, because those are the same effect and very different events. * * `vendorRevocationRequested` says whether the grant at the vendor was asked to be withdrawn as - * well. It is false for every credential this deployment holds in its own vault: removing - * somebody stops us holding a usable secret, and the grant at Google outlives it until it is - * revoked there, which nothing on that path asks for. It is true for a brokered account whose + * well. It is false when an administrator removes somebody who held a credential in this + * deployment's own vault: that stops us holding a usable secret, and the grant at Google outlives + * it until it is revoked there, which nothing on that path asks for. When a person disconnects + * their own vault-held account (`disconnectHeld`), the vendor's revocation endpoint is asked + * first, and the field is true only when the vendor confirmed it or said the token was already + * dead; `vendorAnswer` beside it is the one word for what the vendor said (`revoked`, + * `already_gone`, `refused`, or `not_asked` for a credential already revoked here), never its + * text. It is true for a brokered account whose * withdrawal Composio accepted — accepted rather than completed, because the upstream revocation * runs as a background job with no supported way to poll it, which is why the field is named for * the ask. Recorded rather than glossed, because a row that implied otherwise would be worse diff --git a/openbot/server/src/index.ts b/openbot/server/src/index.ts index c4000b99..5fe9e251 100644 --- a/openbot/server/src/index.ts +++ b/openbot/server/src/index.ts @@ -2834,4 +2834,8 @@ for (const signal of ["SIGINT", "SIGTERM"] as const) { }); } -console.info(`OpenBot server listening on http://127.0.0.1:${port}`); +// The product's own name, from the tenant package, because this is the line a person reads when +// the server comes up. Nothing parses it. +console.info( + `${tenantPackage.productName} server listening on http://127.0.0.1:${port}`, +); diff --git a/openbot/server/src/model-availability-check.ts b/openbot/server/src/model-availability-check.ts index 3aed32a6..54549176 100644 --- a/openbot/server/src/model-availability-check.ts +++ b/openbot/server/src/model-availability-check.ts @@ -88,6 +88,50 @@ export function modelCheckOutcome(input: { }; } +/** Each provider's own API origin, the one {@link ENDPOINTS} asks. */ +const OFFICIAL_HOST: Record = { + openai: "api.openai.com", + anthropic: "api.anthropic.com", +}; + +/** + * Whether a base URL sends this provider's calls somewhere other than the provider itself. + * + * THE PROVIDER'S OWN ADDRESS, WRITTEN OUT, IS NOT CUSTOM. A base URL set to + * `https://api.anthropic.com` (with or without `/v1`, which `normalizeModelBaseUrls` in `copilot.ts` + * adds anyway) still reaches Anthropic, so the model lookup below answers for it exactly as it does + * with nothing set. Treating any set value as custom skipped the check, and the setup checklist then + * said "It uses a custom endpoint, which OpenDuet can't check" about the official API. + * + * Exact on everything that could move the call: https only, the provider's own host, no port other + * than the default, no credentials, query or fragment, and a path of nothing or one version segment. + * Anything else is a gateway or a proxy, which the provider's lookup cannot vouch for. + */ +export function isCustomModelBase( + provider: Provider, + base: string | undefined, +): boolean { + const value = base?.trim(); + if (!value) return false; + let url: URL; + try { + url = new URL(value); + } catch { + return true; + } + const path = url.pathname.replace(/\/+$/, ""); + return !( + url.protocol === "https:" && + url.hostname === OFFICIAL_HOST[provider] && + url.port === "" && + url.username === "" && + url.password === "" && + url.search === "" && + url.hash === "" && + (path === "" || path === "/v1") + ); +} + /** Ask once and log what was found. Resolves either way and never throws. */ export async function reportModelAvailability(options: { provider: Provider; @@ -103,11 +147,11 @@ export async function reportModelAvailability(options: { const request = options.fetch ?? fetch; const { provider, model } = options; - const customBase = + const base = provider === "anthropic" ? environment.ANTHROPIC_BASE_URL : environment.OPENAI_BASE_URL; - if (customBase?.trim()) return { kind: "skipped" }; + if (isCustomModelBase(provider, base)) return { kind: "skipped" }; let outcome: ModelCheckOutcome; try { diff --git a/openbot/server/src/openduet/computer-check.ts b/openbot/server/src/openduet/computer-check.ts index 942eb8e4..4111d40d 100644 --- a/openbot/server/src/openduet/computer-check.ts +++ b/openbot/server/src/openduet/computer-check.ts @@ -4,11 +4,19 @@ import { } from "../computer/policy-network"; import type { ComputerProvider } from "../computer/provider"; import type { WebBotAuth } from "../web-bot-auth"; -import type { SetupStatus } from "./setup"; +import type { ComputerVerdict } from "./setup"; /** * Only running computers, without waking any. Each list, policy read and push has five seconds; * a healthy fleet's total time scales with its size. Serial delivery bounds the active work to one. + * + * NO COMPUTER RUNNING IS `idle`, NOT `not_checked`. Computers start the first time a Bot needs one, + * so a fresh install that configured them correctly has none running. The list itself is the check: + * it is an authenticated call to the computer service, so an answer means the address and the token + * both work, and a wrong one throws (which the checklist reads as Needs setup). Reporting that as + * "not checked" put the row's fix, "Set AGENT_COMPUTER_URL and COMPUTER_TOKEN, then restart", in + * front of somebody who had already set both. There is no policy to deliver to a computer that is + * not running; one that starts gets its policy then. */ export function createComputerCheck(input: { provider?: Pick; @@ -18,8 +26,8 @@ export function createComputerCheck(input: { token?: string; webBotAuth?: WebBotAuth; fetchImpl?: typeof fetch; -}): () => Promise { - let inFlight: Promise | undefined; +}): () => Promise { + let inFlight: Promise | undefined; let operation: Promise = Promise.resolve(); const bounded = (work: Promise): Promise => { operation = work; @@ -35,13 +43,13 @@ export function createComputerCheck(input: { }), ]).finally(() => clearTimeout(timer)); }; - const check = async (): Promise => { + const check = async (): Promise => { if (!input.provider) return "needs_setup"; const computers = await bounded(input.provider.list()); const running = computers.filter( (computer) => computer.status === "running" && computer.url, ); - if (running.length === 0) return "not_checked"; + if (running.length === 0) return "idle"; const controls = input.controls(); if (!controls) return "not_checked"; for (const computer of running) { diff --git a/openbot/server/src/openduet/setup.ts b/openbot/server/src/openduet/setup.ts index 3b827bab..163730e3 100644 --- a/openbot/server/src/openduet/setup.ts +++ b/openbot/server/src/openduet/setup.ts @@ -82,6 +82,14 @@ export type SetupFix = */ export type SetupStatus = "ready" | "needs_setup" | "not_checked"; +/** + * What the Bot computer check found, which has one answer more than a row's status: `idle`, the + * computer service answered and no Bot's computer is running yet. That is how every correctly set + * up install starts, because a computer starts the first time a Bot needs one, so the row reads + * Ready and says so. See `createComputerCheck`. + */ +export type ComputerVerdict = SetupStatus | "idle"; + export type SetupRow = { id: SetupRowId; /** A deployment row is the admin's to set up; a person row is the reader's own. */ @@ -156,7 +164,8 @@ export type SetupFacts = { primaryBot: string; /** A Bot computer is configured. Absent, its routes are not mounted at all. */ computer: boolean; - computerStatus?: SetupStatus; + /** What the computer check last found, when one is configured. Absent, nobody has asked. */ + computerStatus?: ComputerVerdict; /** A broker for apps beyond the ones an admin adds, and which one this deployment would use. */ apps: { configured: boolean; provider: BrokerProvider }; /** The OpenTag pairing that carries Slack and Microsoft Teams. */ @@ -256,16 +265,7 @@ export function setupRows( id: "computer", title: "Bot computer", required: false, - status: facts.computer - ? (facts.computerStatus ?? "not_checked") - : "needs_setup", - readyDetail: "Bots can browse the web and work with files.", - gapDetail: "Without it, Bots can't browse the web or work with files.", - fix: { - kind: "env", - names: ["AGENT_COMPUTER_URL", "COMPUTER_TOKEN"], - docsUrl: `${DOCS}/configuration.md#computer-and-supervisor`, - }, + ...computerRow(facts), }), deployment({ id: "apps", @@ -292,6 +292,64 @@ export function setupRows( ]; } +/** + * The Bot computer row, from whether one is configured and what the check last found. + * + * EACH STATE SAYS ITS OWN THING, because they ask different people for different acts. Not + * configured is the admin's to set, and names the variables. Configured and answering, with no Bot's + * computer running yet, is how a correct install starts, so it reads Ready and says when one starts. + * Configured but not checked has nothing to fix yet. Configured and failing is the address or the + * token, so it names those. It used to name the variables in every state but Ready, which told + * somebody who had already set both to set them. + */ +function computerRow(facts: SetupFacts): { + status: SetupStatus; + readyDetail: string; + gapDetail: string; + fix: SetupFix | null; +} { + const variables: SetupFix = { + kind: "env", + names: ["AGENT_COMPUTER_URL", "COMPUTER_TOKEN"], + docsUrl: `${DOCS}/configuration.md#computer-and-supervisor`, + }; + const ready = "Bots can browse the web and work with files."; + if (!facts.computer) + return { + status: "needs_setup", + readyDetail: ready, + gapDetail: "Without it, Bots can't browse the web or work with files.", + fix: variables, + }; + switch (facts.computerStatus ?? "not_checked") { + case "ready": + return { status: "ready", readyDetail: ready, gapDetail: "", fix: null }; + case "idle": + return { + status: "ready", + readyDetail: `${ready} Each Bot's computer starts the first time it's needed.`, + gapDetail: "", + fix: null, + }; + case "not_checked": + return { + status: "not_checked", + readyDetail: ready, + gapDetail: + "Set up, not checked yet. Each Bot's computer starts the first time it's needed.", + fix: null, + }; + case "needs_setup": + return { + status: "needs_setup", + readyDetail: ready, + gapDetail: + "Set up, but the computer service didn't answer. Check its address and token.", + fix: variables, + }; + } +} + const INTELLIGENCE_VARIABLES = [ "INTELLIGENCE_API_URL", "INTELLIGENCE_GATEWAY_WS_URL", @@ -613,7 +671,7 @@ export function createSetupSources(input: { primaryBot: string; computer: boolean; /** Authenticated reachability and policy delivery, with per-operation deadlines and no wake. */ - computerCheck?: () => Promise; + computerCheck?: () => Promise; apps: { configured: boolean; provider: BrokerProvider }; chat: boolean; }): SetupSources { @@ -629,7 +687,7 @@ export function createSetupSources(input: { intelligenceFact, ); - const askComputer = async (): Promise => { + const askComputer = async (): Promise => { if (!input.computer) return "needs_setup"; if (!input.computerCheck) return "not_checked"; try { diff --git a/openbot/server/src/plugins/plugin-store/connections.ts b/openbot/server/src/plugins/plugin-store/connections.ts index 68d09778..fda8750c 100644 --- a/openbot/server/src/plugins/plugin-store/connections.ts +++ b/openbot/server/src/plugins/plugin-store/connections.ts @@ -1,5 +1,6 @@ import { and, asc, eq, inArray, isNull, sql } from "drizzle-orm"; import { createAuditStore, recordAuditEvent } from "../../audit"; +import { decryptSecret } from "../../credentials"; import type { Database } from "../../db/client"; import { brokeredConnections, @@ -35,6 +36,8 @@ import { type PendingAuthorization, PluginRefusedError, pendingAuthorizationFromRow, + type VendorRevocation, + VendorRevocationUnreachableError, } from "./model"; /** OAuth clients and a person's connections, including every brokered (Composio) connection flow. */ @@ -44,6 +47,8 @@ export function connectionMethods(context: StoreContext) { database, auditStore, credentials, + encryptionKey, + revokeAtVendor, registerClient, broker, accountRow, @@ -3133,6 +3138,133 @@ export function connectionMethods(context: StoreContext) { return { cleared }; }, + /** + * End one person's own account for a `user-oauth` app this deployment holds the refresh token + * for (Gmail and Google Calendar, Drive, Notion): at the vendor first, then here. + * + * WHAT IT DOES, IN ORDER, UNDER ONE ROW LOCK. + * 1. Lock the person's connection row and the vault row it points at, so a tool call spending + * the same token waits, and one queued behind this finds the connection gone + * (`connectionTokenFor` reads both after taking the same lock). + * 2. Ask the vendor to withdraw the refresh token at the catalogue's own `revokeUrl` (RFC 7009), + * with the deployment's OAuth client. + * 3. Revoke the vault row and delete the connection row, in the same transaction. + * 4. Write `mcp.account_disconnected`, reason `self`, saying what the vendor answered. + * + * VENDOR BEFORE LOCAL, AND AN UNREACHABLE VENDOR CHANGES NOTHING. The refresh token is the only + * thing that can withdraw the grant at Google, and it is in this row. Revoking our copy first + * would leave a live grant on somebody's mailbox that nothing here can reach any more, the + * failure `disconnectBrokered` is ordered against too. So a vendor that does not answer (a + * timeout, a 5xx, a 429) throws {@link VendorRevocationUnreachableError}, the transaction rolls + * back, and pressing Disconnect again is the recovery. + * + * A VENDOR THAT ANSWERS NO STILL GETS A DISCONNECT HERE. A refusal is an answer, and asking + * again would get the same one, so holding the person's access open until the vendor changes its + * mind would make Disconnect impossible. Our copy is revoked and the row goes, and the answer + * comes back as `vendorRevoked: false` so the page can tell the person to finish at the vendor. + * A token the vendor already calls invalid is `already_gone`: nothing is left to withdraw. + * + * THE ACCESS TOKEN NEEDS NOTHING. Nothing caches one (see `connectionTokenFor`): every call + * trades the refresh token for a fresh access token and throws it away, so once the row is gone + * no call can mint another. + * + * ONLY THE ASKER'S OWN ROW, by `(serverId, userId)` from the session: the route passes the + * signed-in person and nothing from the request names whose account this is. A person with no + * row, including on a Shared app whose account is the deployment's, gets `disconnected: false` + * and nothing is touched. + */ + async disconnectHeld(input: { + serverId: string; + userId: string; + by: string; + }): Promise<{ disconnected: boolean; vendorRevoked: boolean }> { + if (!input.userId) return { disconnected: false, vendorRevoked: false }; + const { entry } = await requireServer(input.serverId); + if (entry?.auth.kind !== "user-oauth") { + throw new PluginRefusedError( + "That app doesn't connect this way.", + null, + ); + } + const { revokeUrl } = entry.auth; + const vendor = entry.vendor; + /* + * The client, read before the transaction rather than inside it, as `connectionTokenFor` + * reads it: it is per server, not what the lock protects, and reading it inside would open a + * second pooled connection while this one holds row locks. None held is still an ask. + */ + const client = await storedOAuthClient(input.serverId); + const owned = and( + eq(mcpUserCredentials.serverId, input.serverId), + eq(mcpUserCredentials.userId, input.userId), + ); + + const answer = await database.transaction(async (transaction) => { + const [row] = await transaction + .select({ credentialId: mcpUserCredentials.credentialId }) + .from(mcpUserCredentials) + .where(owned) + .for("update"); + if (!row) return null; + const [credential] = await transaction + .select({ + encryptedValue: credentialRows.encryptedValue, + revokedAt: credentialRows.revokedAt, + }) + .from(credentialRows) + .where(eq(credentialRows.id, row.credentialId)) + .for("update"); + + /* + * A credential already revoked here (the vendor answered `invalid_grant` on a call, which + * revokes it) has nothing live to withdraw, so the vendor is not asked; the row still goes. + */ + let said: VendorRevocation | "not_asked" = "not_asked"; + if (credential && credential.revokedAt === null) { + said = await revokeAtVendor({ + revokeUrl, + client, + refreshToken: await decryptSecret( + encryptionKey, + credential.encryptedValue, + ), + }); + if (said === "unreachable") { + throw new VendorRevocationUnreachableError(vendor); + } + await credentials.revoke(row.credentialId, transaction); + } + await transaction.delete(mcpUserCredentials).where(owned); + return said; + }); + if (answer === null) return { disconnected: false, vendorRevoked: false }; + + const vendorRevoked = answer === "revoked" || answer === "already_gone"; + await recordAuditEvent(auditStore, { + eventType: "mcp.account_disconnected", + targetType: "mcp_server", + targetId: input.serverId, + payload: { + actor: input.by, + server: input.serverId, + owner: input.userId, + reason: "self", + /* + * True only when the vendor confirmed it, or said the token was already dead; see the + * event's own note in `audit.ts`. Spelled as a literal per branch, never the answer + * passed through: what the trail may claim is decided once, by `vendorRevocationClaim`, + * for every brokered writer (`sole-definition.test.ts`), and a held account has no + * broker to ask it of. `vendorAnswer` is the one word for what the vendor said. + */ + ...(vendorRevoked + ? { vendorRevocationRequested: true } + : { vendorRevocationRequested: false }), + vendorAnswer: answer, + }, + }); + return { disconnected: true, vendorRevoked }; + }, + /** * Retire every connector credential belonging to one person. * @@ -3160,7 +3292,8 @@ export function connectionMethods(context: StoreContext) { * Sweeping the vault alone therefore left that gate passing for somebody who had been removed. * * NOT VENDOR-SIDE REVOCATION FOR THE VAULT HALF. That needs the OAuth client and the vendor's - * revoke endpoint, and it belongs with disconnect. Those rows are the half that stops us + * revoke endpoint, and it belongs with disconnect, which asks for it ({@link disconnectHeld}). + * Offboarding does not, yet. Those rows are the half that stops us * holding the secret; the grant at Google outlives it until somebody revokes it there. Said * plainly rather than implied, because the difference matters to whoever has to answer for it. * diff --git a/openbot/server/src/plugins/plugin-store/context.ts b/openbot/server/src/plugins/plugin-store/context.ts index 6f42bf0e..60f0fd19 100644 --- a/openbot/server/src/plugins/plugin-store/context.ts +++ b/openbot/server/src/plugins/plugin-store/context.ts @@ -51,6 +51,7 @@ import { type PluginKind, PluginRefusedError, type PluginStoreOptions, + revokeRefreshTokenOverHttp, type StoredClient, TokenRefusedError, type Transaction, @@ -71,6 +72,7 @@ export function createStoreContext(options: PluginStoreOptions) { const injectedVendor = options.callVendor; const exchangeRefreshToken = options.exchangeRefreshToken ?? exchangeRefreshTokenOverHttp; + const revokeAtVendor = options.revokeAtVendor ?? revokeRefreshTokenOverHttp; const registerClient = options.registerClient ?? registerDynamicClient; // No default, unlike the seams above: there is no real implementation in this tree to fall back // to, and a deployment with no Composio key is supposed to have no broker. See `./broker`. @@ -2046,6 +2048,7 @@ export function createStoreContext(options: PluginStoreOptions) { encryptionKey, injectedVendor, exchangeRefreshToken, + revokeAtVendor, registerClient, broker, exchangeChains, diff --git a/openbot/server/src/plugins/plugin-store/model.ts b/openbot/server/src/plugins/plugin-store/model.ts index b961c471..7027abba 100644 --- a/openbot/server/src/plugins/plugin-store/model.ts +++ b/openbot/server/src/plugins/plugin-store/model.ts @@ -1028,6 +1028,93 @@ export async function exchangeRefreshTokenOverHttp(input: { /** How long a vendor's token endpoint gets. Shorter than a call: it is one round trip, or nothing. */ export const TOKEN_TIMEOUT_MS = 10_000; +/** + * What a vendor said when asked to withdraw a refresh token (RFC 7009), as one word. + * + * - `revoked`: a 2xx. The vendor ended the grant. + * - `already_gone`: a 400 whose `error` is `invalid_token` or `invalid_grant`. The vendor no longer + * honours this token, so there was nothing left to withdraw, and the grant is as dead as a revoke + * would have left it. Google answers this way for a token that was already revoked or expired. + * - `refused`: any other answer the vendor gave on purpose, a 4xx or a redirect. Nothing was + * withdrawn, and asking the same way again would get the same answer. + * - `unreachable`: no answer, a 5xx or a 429. Asking again may well work. + */ +export type VendorRevocation = + | "revoked" + | "already_gone" + | "refused" + | "unreachable"; + +/** + * Ask the vendor to withdraw a refresh token, the way RFC 7009 says: a form POST to the catalogue's + * own `revokeUrl` naming the token, with the client authenticating as it does at the token endpoint. + * + * THE SAME CLIENT AUTHENTICATION AS {@link exchangeRefreshTokenOverHttp}, in the body. RFC 7009 has a + * confidential client authenticate to revoke, and a vendor such as Notion refuses a revocation that + * names no client. Google documents only `token` and accepts the request either way. No client is + * a client that cannot be named, not a reason to skip the ask: the token alone is still sent. + * + * NEVER THROWS, AND NEVER CARRIES THE VENDOR'S WORDS OUT. Every outcome is one of four words, so + * nothing the vendor wrote, and nothing of the token, can reach an audit row or a screen from here. + * + * Exported for its own tests. The store reaches it as the default `revokeAtVendor`. + */ +export async function revokeRefreshTokenOverHttp(input: { + revokeUrl: string; + client: OAuthClient | null; + refreshToken: string; +}): Promise { + const params = new URLSearchParams({ + token: input.refreshToken, + token_type_hint: "refresh_token", + }); + if (input.client) { + params.set("client_id", input.client.clientId); + if (input.client.clientSecret) { + params.set("client_secret", input.client.clientSecret); + } + } + let response: Response; + try { + response = await fetch(input.revokeUrl, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: params, + // A redirect is a refusal, not a detour: this request carries somebody's refresh token and + // the deployment's client secret, and following a 3xx would hand both to the address it named. + redirect: "manual", + signal: AbortSignal.timeout(TOKEN_TIMEOUT_MS), + }); + } catch { + return "unreachable"; + } + if (response.ok) return "revoked"; + if (response.status >= 500 || response.status === 429) return "unreachable"; + if (response.status === 400) { + const body = (await response.json().catch(() => null)) as { + error?: unknown; + } | null; + if (body?.error === "invalid_token" || body?.error === INVALID_GRANT) { + return "already_gone"; + } + } + return "refused"; +} + +/** + * The vendor could not be asked to withdraw somebody's access, so nothing here was changed. + * + * Its own class so the route can say that in the person's words, and so a disconnect that cannot + * reach the vendor is told apart from one the vendor refused: the first is worth pressing again, + * the second is not. + */ +export class VendorRevocationUnreachableError extends Error { + constructor(readonly vendor: string) { + super(`Couldn't reach ${vendor} to end access.`); + this.name = "VendorRevocationUnreachableError"; + } +} + /** * The deployment's OAuth client for one vendor, as it is held in the vault. * @@ -1192,6 +1279,15 @@ export type PluginStoreOptions = { client: OAuthClient; refreshToken: string; }) => Promise; + /** + * Withdrawing a refresh token at the vendor (RFC 7009), for a person disconnecting their own + * account. Defaults to a real HTTP request; see {@link revokeRefreshTokenOverHttp}. + */ + revokeAtVendor?: (input: { + revokeUrl: string; + client: OAuthClient | null; + refreshToken: string; + }) => Promise; /** RFC 7591 self-registration, for entries whose clientRegistration is dynamic. */ registerClient?: (input: { registrationUrl: string; diff --git a/openbot/server/src/plugins/route-groups/connections.ts b/openbot/server/src/plugins/route-groups/connections.ts index d7588819..c003d094 100644 --- a/openbot/server/src/plugins/route-groups/connections.ts +++ b/openbot/server/src/plugins/route-groups/connections.ts @@ -28,6 +28,7 @@ import { CatalogueEntryUnknownError, type OAuthClient, PluginRefusedError, + VendorRevocationUnreachableError, } from "../store"; import { type AuthorizationRefusal, @@ -1214,7 +1215,53 @@ export function mountConnectionRoutes({ * refused to anybody but an administrator. */ routes.delete("/servers/:id/connection", requireUser, async (context) => { - const resolved = await brokeredAccountFor(context, context.req.param("id")); + const serverId = context.req.param("id"); + /* + * AN ACCOUNT THIS DEPLOYMENT HOLDS ITSELF, BEFORE THE BROKERED PATH BELOW EVER SEES IT. + * + * A `user-oauth` app (Gmail and Google Calendar, Drive, Notion) is reached with a refresh token + * in this deployment's own vault, not through a broker, so `brokeredAccountFor` answers it with + * "That app doesn't connect this way." That is why its page used to offer Disconnect and then + * say it couldn't: there was no route that could end it. `disconnectHeld` is that route's work, + * and its docblock says the order and why. + * + * WHOSE ACCOUNT IS THE SESSION'S, as on the brokered path: the store is handed the signed-in + * person and nothing from the request, so this DELETE can only ever end its caller's own grant. + * Not admin-gated, for the same reason connecting is not: it is the person's own account. + */ + const address = await store.serverAddress(serverId); + const entry = catalogueEntry(serverId); + if ( + address !== undefined && + appOfUrl(address.url) === null && + entry?.auth.kind === "user-oauth" + ) { + try { + return context.json( + await store.disconnectHeld({ + serverId, + userId: context.var.actor.id, + by: context.var.actor.id, + }), + ); + } catch (error) { + /* + * The vendor could not be asked, so nothing was changed and pressing again is the recovery. + * 502 because it is the vendor, not this deployment or the person, that did not answer. + */ + if (error instanceof VendorRevocationUnreachableError) { + return context.json( + { + error: `Couldn't reach ${error.vendor}, so nothing was disconnected. Try again in a moment.`, + }, + 502, + ); + } + throw error; + } + } + + const resolved = await brokeredAccountFor(context, serverId); if (resolved.refusal) { return context.json( { error: resolved.refusal.error }, diff --git a/openbot/server/src/plugins/store.ts b/openbot/server/src/plugins/store.ts index b06db612..6370861e 100644 --- a/openbot/server/src/plugins/store.ts +++ b/openbot/server/src/plugins/store.ts @@ -24,6 +24,7 @@ export type { SkillActor, SkillRecord, ToolRecord, + VendorRevocation, WithdrawnGrant, } from "./plugin-store/model"; export { @@ -38,9 +39,11 @@ export { PluginInvariantError, PluginRefusedError, refFromToolName, + revokeRefreshTokenOverHttp, TokenRefusedError, toolNameFor, unlistedAdvertisedTools, + VendorRevocationUnreachableError, } from "./plugin-store/model"; export function createPluginStore(options: PluginStoreOptions) { diff --git a/openbot/server/tests/assertion-shape.test.ts b/openbot/server/tests/assertion-shape.test.ts index ed261f55..e9d8aba7 100644 --- a/openbot/server/tests/assertion-shape.test.ts +++ b/openbot/server/tests/assertion-shape.test.ts @@ -581,13 +581,6 @@ const ALLOW: Allowed[] = [ finding: "NEW (round 2 did not find this)", why: 'expect( stampSignIn(broken, "anybody", SIGNED_IN_AT), ).resolves.toBeUndefined();', }, - { - file: "server/tests/plugin-user-credential.integration.test.ts", - line: 369, - shape: "a", - finding: "round 2 — plugin-user-credential.integration.test.ts:361", - why: 'describe("a person who has not connected", () => { test("is refused, and told to connect rather than told it b', - }, { file: "app/tests/bot-setup.test.tsx", line: 178, diff --git a/openbot/server/tests/google-workspace-fake.test.ts b/openbot/server/tests/google-workspace-fake.test.ts index cbdf8f79..965fef5f 100644 --- a/openbot/server/tests/google-workspace-fake.test.ts +++ b/openbot/server/tests/google-workspace-fake.test.ts @@ -21,7 +21,10 @@ import { createVerifier, redeemAuthorizationCode, } from "../src/plugins/oauth"; -import { exchangeRefreshTokenOverHttp } from "../src/plugins/plugin-store/model"; +import { + exchangeRefreshTokenOverHttp, + revokeRefreshTokenOverHttp, +} from "../src/plugins/plugin-store/model"; /** * The Gmail and Calendar adapter against the fake Google, over a real socket. @@ -187,6 +190,28 @@ describe("the fake's own OAuth", () => { expect(after.text).toContain("(401)"); }); + test("Disconnect's revocation, sent as the store sends it, ends the grant", async () => { + const { refresh_token } = await signIn(fake.url); + + expect( + await revokeRefreshTokenOverHttp({ + revokeUrl: `${fake.url}/revoke`, + client: { clientId: "test-client", clientSecret: "test-secret" }, + refreshToken: refresh_token, + }), + ).toBe("revoked"); + + const renewed = await fetch(`${fake.url}/token`, { + method: "POST", + body: new URLSearchParams({ + grant_type: "refresh_token", + refresh_token, + client_id: "test-client", + }), + }); + expect(renewed.status).toBe(400); + }); + test("an API call without a valid token is a 401, which the adapter says how to fix", async () => { const result = await callTool( { url: fake.url, token: "fake-access-made-up" }, diff --git a/openbot/server/tests/model-availability-check.test.ts b/openbot/server/tests/model-availability-check.test.ts index b97e6214..3a78da2f 100644 --- a/openbot/server/tests/model-availability-check.test.ts +++ b/openbot/server/tests/model-availability-check.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from "bun:test"; import { + isCustomModelBase, modelCheckOutcome, reportModelAvailability, } from "../src/model-availability-check"; @@ -136,6 +137,44 @@ describe("asking at boot", () => { expect(calls).toHaveLength(0); }); + test("asks Anthropic when the base URL is Anthropic's own API, written out", async () => { + for (const base of [ + "https://api.anthropic.com", + "https://api.anthropic.com/", + // What `normalizeModelBaseUrls` turns the line above into before this runs. + "https://api.anthropic.com/v1", + " https://api.anthropic.com/v1/ ", + ]) { + const { fetcher, calls } = fakeFetch(200); + const outcome = await reportModelAvailability({ + provider: "anthropic", + model: "claude-sonnet-4-5", + apiKey: async () => KEY, + environment: { ANTHROPIC_BASE_URL: base }, + fetch: fetcher, + log: () => {}, + }); + expect(outcome).toEqual({ kind: "available" }); + expect(calls[0]?.url).toBe( + "https://api.anthropic.com/v1/models/claude-sonnet-4-5", + ); + } + }); + + test("asks OpenAI when the base URL is OpenAI's own API, written out", async () => { + const { fetcher, calls } = fakeFetch(200); + const outcome = await reportModelAvailability({ + provider: "openai", + model: "gpt-5.6-terra", + apiKey: async () => KEY, + environment: { OPENAI_BASE_URL: "https://api.openai.com/v1" }, + fetch: fetcher, + log: () => {}, + }); + expect(outcome).toEqual({ kind: "available" }); + expect(calls).toHaveLength(1); + }); + test("says nothing while no key is configured yet", async () => { const { fetcher, calls } = fakeFetch(200); const lines: string[] = []; @@ -170,3 +209,44 @@ describe("asking at boot", () => { expect(JSON.parse(lines[0] ?? "{}").reason).toContain("ENOTFOUND"); }); }); + +describe("which base URLs are custom", () => { + test("unset and blank are the provider itself", () => { + expect(isCustomModelBase("anthropic", undefined)).toBe(false); + expect(isCustomModelBase("openai", " ")).toBe(false); + }); + + test("the provider's own API, with or without its version, is not custom", () => { + expect(isCustomModelBase("anthropic", "https://api.anthropic.com")).toBe( + false, + ); + expect( + isCustomModelBase("anthropic", "https://api.anthropic.com/v1/"), + ).toBe(false); + expect(isCustomModelBase("openai", "https://api.openai.com/v1")).toBe( + false, + ); + // The default port, written out, is the same address. + expect(isCustomModelBase("openai", "https://api.openai.com:443/v1")).toBe( + false, + ); + }); + + test("anything that could send the call elsewhere is custom", () => { + for (const [provider, base] of [ + ["anthropic", "http://api.anthropic.com"], + ["anthropic", "https://api.anthropic.com.evil.example"], + ["anthropic", "https://api.anthropic.com:8443/v1"], + ["anthropic", "https://user:pass@api.anthropic.com"], + ["anthropic", "https://api.anthropic.com/proxy/v1"], + ["anthropic", "https://api.anthropic.com/v1?via=gateway"], + // Another provider's address is a custom endpoint for this one. + ["anthropic", "https://api.openai.com/v1"], + ["openai", "https://gateway.internal/v1"], + ["openai", "http://localhost:11434/v1"], + ["openai", "not a url"], + ] as const) { + expect(isCustomModelBase(provider, base)).toBe(true); + } + }); +}); diff --git a/openbot/server/tests/oauth-revoke.test.ts b/openbot/server/tests/oauth-revoke.test.ts new file mode 100644 index 00000000..91267568 --- /dev/null +++ b/openbot/server/tests/oauth-revoke.test.ts @@ -0,0 +1,113 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { revokeRefreshTokenOverHttp } from "../src/plugins/store"; + +/** + * Withdrawing a person's refresh token at the vendor (RFC 7009), which is what Disconnect asks + * before anything here is removed. Every outcome is one of four words, so the store can decide what + * to keep and the trail can say what happened without carrying the vendor's text or the token. + */ + +const realFetch = globalThis.fetch; +afterEach(() => { + globalThis.fetch = realFetch; +}); + +type Seen = { + url: string; + method: string | undefined; + redirect: RequestRedirect | undefined; + body: URLSearchParams; +}; + +function vendorAnswers(answer: () => Response | Promise): Seen[] { + const seen: Seen[] = []; + globalThis.fetch = (async (url: unknown, init?: RequestInit) => { + seen.push({ + url: String(url), + method: init?.method, + redirect: init?.redirect, + body: new URLSearchParams(String(init?.body)), + }); + return answer(); + }) as unknown as typeof fetch; + return seen; +} + +const ask = (client = { clientId: "c-1", clientSecret: "s-1" }) => + revokeRefreshTokenOverHttp({ + revokeUrl: "https://oauth2.googleapis.com/revoke", + client, + refreshToken: "rt-1", + }); + +describe("asking the vendor to withdraw a refresh token", () => { + test("posts the token, as this client, to the catalogue's address only", async () => { + const seen = vendorAnswers(() => new Response(null, { status: 200 })); + + expect(await ask()).toBe("revoked"); + + expect(seen).toHaveLength(1); + expect(seen[0]?.url).toBe("https://oauth2.googleapis.com/revoke"); + expect(seen[0]?.method).toBe("POST"); + // The request carries a secret, so a redirect is never followed. + expect(seen[0]?.redirect).toBe("manual"); + expect(Object.fromEntries(seen[0]?.body ?? [])).toEqual({ + token: "rt-1", + token_type_hint: "refresh_token", + client_id: "c-1", + client_secret: "s-1", + }); + }); + + test("a public client sends no empty secret, and no client still asks with the token", async () => { + const seen = vendorAnswers(() => new Response(null, { status: 200 })); + + await ask({ clientId: "public", clientSecret: "" }); + await revokeRefreshTokenOverHttp({ + revokeUrl: "https://vendor.example/revoke", + client: null, + refreshToken: "rt-2", + }); + + expect(seen[0]?.body.has("client_secret")).toBe(false); + expect(seen[0]?.body.get("client_id")).toBe("public"); + expect(Object.fromEntries(seen[1]?.body ?? [])).toEqual({ + token: "rt-2", + token_type_hint: "refresh_token", + }); + }); + + test("a token the vendor already calls invalid is already gone", async () => { + for (const error of ["invalid_token", "invalid_grant"]) { + vendorAnswers(() => Response.json({ error }, { status: 400 })); + expect(await ask()).toBe("already_gone"); + } + }); + + test("any other answer on purpose is a refusal, a redirect included", async () => { + for (const answer of [ + () => Response.json({ error: "invalid_client" }, { status: 401 }), + () => Response.json({ error: "invalid_request" }, { status: 400 }), + () => new Response("nope", { status: 400 }), + () => + new Response(null, { + status: 302, + headers: { location: "https://elsewhere.example/revoke" }, + }), + ]) { + vendorAnswers(answer); + expect(await ask()).toBe("refused"); + } + }); + + test("no answer, a server error or a rate limit is unreachable, and nothing throws", async () => { + for (const answer of [ + () => new Response(null, { status: 503 }), + () => new Response(null, { status: 429 }), + () => Promise.reject(new TypeError("fetch failed")), + ]) { + vendorAnswers(answer); + expect(await ask()).toBe("unreachable"); + } + }); +}); diff --git a/openbot/server/tests/openduet-setup.test.ts b/openbot/server/tests/openduet-setup.test.ts index 6fd5846e..82d30cad 100644 --- a/openbot/server/tests/openduet-setup.test.ts +++ b/openbot/server/tests/openduet-setup.test.ts @@ -5,6 +5,7 @@ import type { CredentialStatus } from "../src/credentials"; import type { ModelCheckOutcome } from "../src/model-availability-check"; import { createComputerCheck } from "../src/openduet/computer-check"; import { + type ComputerVerdict, createSetupSources, intelligenceFact, modelKeyOf, @@ -590,7 +591,7 @@ function appWith(input: { modelCheck?: () => ModelCheckOutcome; entitlements?: () => Promise; computer?: boolean; - computerCheck?: () => Promise<"ready" | "needs_setup" | "not_checked">; + computerCheck?: () => Promise; apps?: boolean; chat?: boolean; signedIn?: boolean; @@ -1147,3 +1148,94 @@ test("a configured computer without a reachability verdict is not yet checked", ).status, ).toBe("not_checked"); }); + +describe("the Bot computer row", () => { + const computerOf = (computerStatus: ComputerVerdict | undefined) => { + const { computerStatus: _, ...facts } = READY; + return row( + setupRows( + computerStatus === undefined ? facts : { ...facts, computerStatus }, + { productName: PRODUCT, audience: "admin" }, + ), + "computer", + ); + }; + + test("configured with no Bot's computer running yet reads Ready, and says when one starts", () => { + const computer = computerOf("idle"); + expect(computer.status).toBe("ready"); + expect(computer.detail).toBe( + "Bots can browse the web and work with files. Each Bot's computer starts the first time it's needed.", + ); + expect(computer.fix).toBeNull(); + }); + + test("configured and not checked never tells somebody to set what they already set", () => { + const computer = computerOf(undefined); + expect(computer.status).toBe("not_checked"); + expect(computer.fix).toBeNull(); + expect(computer.detail).not.toContain("Without it"); + }); + + test("configured and failing names the address and token to check", () => { + const computer = computerOf("needs_setup"); + expect(computer.status).toBe("needs_setup"); + expect(computer.detail).toBe( + "Set up, but the computer service didn't answer. Check its address and token.", + ); + expect(computer.fix).toMatchObject({ + kind: "env", + names: ["AGENT_COMPUTER_URL", "COMPUTER_TOKEN"], + }); + }); + + test("not configured still names the variables", () => { + const computer = row( + setupRows(UNSET, { productName: PRODUCT, audience: "admin" }), + "computer", + ); + expect(computer.status).toBe("needs_setup"); + expect(computer.detail).toBe( + "Without it, Bots can't browse the web or work with files.", + ); + }); + + test("a computer service that answers with nothing running is idle, and wakes nothing", async () => { + let pushes = 0; + const check = createComputerCheck({ + provider: { + list: async () => [ + { botId: "asleep", status: "stopped", url: "http://asleep.local" }, + ], + }, + controls: () => ({ + policyForBot: async () => ({ mode: "allow_all", rules: [] }), + }), + fetchImpl: Object.assign( + async () => { + pushes += 1; + return new Response(null, { status: 204 }); + }, + { preconnect() {} }, + ), + }); + expect(await check()).toBe("idle"); + expect(pushes).toBe(0); + }); + + test("a fresh install's checklist reads the idle computer as Ready", async () => { + const app = appWith({ + environment: {}, + who: "admin", + computer: true, + computerCheck: createComputerCheck({ + provider: { list: async () => [] }, + controls: () => undefined, + }), + }); + const computer = row((await checklistFrom(app)).checklist.rows, "computer"); + expect(computer.status).toBe("ready"); + expect(computer.fix).toBeNull(); + expect(computer.detail).toContain("starts the first time it's needed"); + }); +}); diff --git a/openbot/server/tests/plugin-routes.test.ts b/openbot/server/tests/plugin-routes.test.ts index 94bdb0e2..d65ada0a 100644 --- a/openbot/server/tests/plugin-routes.test.ts +++ b/openbot/server/tests/plugin-routes.test.ts @@ -1631,6 +1631,12 @@ function brokeredApp( by: string; reason: string; }> = []; + /** Every account this deployment holds itself that the route asked the store to end. */ + const heldDisconnected: Array<{ + serverId: string; + userId: string; + by: string; + }> = []; /** Every time the route went and asked Composio what an app wants typed in. */ const asked: Array<{ toolkit: string; authScheme: string }> = []; /** @@ -1884,6 +1890,14 @@ function brokeredApp( if (deployment.storeThrows) throw deployment.storeThrows; return { vendorRevocationRequested: true }; }, + disconnectHeld: async (input: { + serverId: string; + userId: string; + by: string; + }) => { + heldDisconnected.push(input); + return { disconnected: true, vendorRevoked: true }; + }, }); const app = createApp( @@ -1933,6 +1947,7 @@ function brokeredApp( confirmed, rechecked, disconnected, + heldDisconnected, asked, submitted, /** @@ -3031,26 +3046,39 @@ describe("confirming and ending a brokered connection", () => { // `notion` is a row this deployment really has. What is wrong is not that it is missing but // that its connection does not live at Composio, and the sentence says so rather than talking // about a broker setting or an app nobody has heard of. - const { confirmed, disconnected, confirm, disconnect } = brokeredApp(); + const { confirmed, disconnected, confirm } = brokeredApp(); const confirmResponse = await confirm({ serverId: "notion" }); - const disconnectResponse = await disconnect({ serverId: "notion" }); expect(confirmResponse.status).toBe(400); - expect(disconnectResponse.status).toBe(400); expect((await confirmResponse.json()).error).toBe( "That app doesn't connect this way.", ); - expect((await disconnectResponse.json()).error).toBe( - "That app doesn't connect this way.", - ); // And the store was left alone: a confirm here would write a row for an app whose connection - // is not Composio's to answer about, and a disconnect would ask the broker to revoke a grant - // it never issued. + // is not Composio's to answer about. expect(confirmed).toEqual([]); expect(disconnected).toEqual([]); }); + test("disconnecting an app this deployment holds the token for ends the asker's own account, never through the broker", async () => { + // `notion` is `user-oauth`: its refresh token is in this deployment's vault, so Disconnect is + // `disconnectHeld`'s, for the signed-in person and nobody the request names. + const { disconnected, heldDisconnected, disconnect } = brokeredApp(); + + const response = await disconnect({ serverId: "notion" }); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + disconnected: true, + vendorRevoked: true, + }); + expect(heldDisconnected).toEqual([ + { serverId: "notion", userId: ADMIN.id, by: ADMIN.id }, + ]); + // A broker that never issued this grant is not asked to revoke it. + expect(disconnected).toEqual([]); + }); + test("a deployment with no broker is told which setting to set", async () => { // The same answer the directory and connect give, for the same reason: nobody was asked, and // the remedy is one environment variable long. Answering "not connected" instead would draw a diff --git a/openbot/server/tests/plugin-user-credential.integration.test.ts b/openbot/server/tests/plugin-user-credential.integration.test.ts index 988515fa..ba9f005d 100644 --- a/openbot/server/tests/plugin-user-credential.integration.test.ts +++ b/openbot/server/tests/plugin-user-credential.integration.test.ts @@ -1,12 +1,14 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { randomUUID } from "node:crypto"; -import { and, eq, inArray, isNull } from "drizzle-orm"; +import { and, desc, eq, inArray, isNull, sql } from "drizzle-orm"; +import { createApp } from "../src/app"; import { createAuditStore } from "../src/audit"; import type { ActionPolicy } from "../src/computer/policy"; import { encryptSecret } from "../src/credentials"; import { createDatabase } from "../src/db/client"; import { agents, + auditEvents, credentials, mcpServers, mcpTools, @@ -14,8 +16,15 @@ import { pluginGrants, users, } from "../src/db/schema"; -import { createPluginStore, PluginRefusedError } from "../src/plugins/store"; +import { + createPluginStore, + PluginRefusedError, + type VendorRevocation, + VendorRevocationUnreachableError, +} from "../src/plugins/store"; +import { loadConfig } from "../src/config"; import { TEST_POOL, testDatabaseUrl } from "./support/database"; +import { testEnvironment } from "./support/environment"; /** * Whose credential a call to a `user-oauth` server goes out with. @@ -63,6 +72,14 @@ const decrypted: string[] = []; /** Every refresh token handed to the vendor's token endpoint, in order. */ const exchanged: string[] = []; +/** Every revocation the vendor was asked for, and what it answers the next one with. */ +const revocations: { + revokeUrl: string; + client: { clientId: string; clientSecret: string } | null; + refreshToken: string; +}[] = []; +let vendorAnswer: VendorRevocation = "revoked"; + /** The deployment's OAuth client, as the connect flow will eventually write it. */ const CLIENT = { clientId: "client-id", clientSecret: "client-secret" }; @@ -116,6 +133,16 @@ const store = createPluginStore({ updateSecret: async () => { throw new Error("this suite writes credentials directly"); }, + // Nor does anything here replace or look up a credential through the vault: loud, as above. + rotate: async () => { + throw new Error("this suite writes credentials directly"); + }, + isLive: async () => { + throw new Error("this suite reads credentials directly"); + }, + findLiveByKey: async () => { + throw new Error("this suite reads credentials directly"); + }, /* * A real revocation, against this suite's own rows. * @@ -126,8 +153,13 @@ const store = createPluginStore({ * * `create` still throws, because connections are still written directly. */ - revoke: async (id) => { - const [row] = await database + revoke: async (id, executor = database) => { + /* + * On the caller's transaction when it passes one, as the real vault does. A disconnect holds + * this row `FOR UPDATE` while it revokes it, so a revoke on a second connection would wait for + * a lock only its own caller can release. + */ + const [row] = await executor .update(credentials) .set({ revokedAt: new Date() }) .where(eq(credentials.id, id)) @@ -155,6 +187,14 @@ const store = createPluginStore({ exchanged.push(refreshToken); return { accessToken: accessTokenFrom(refreshToken) }; }, + /* + * Stands in for Google's revocation endpoint: records what it was asked to withdraw, with which + * client and where, and answers what the test set in `vendorAnswer`. + */ + revokeAtVendor: async (request) => { + revocations.push(request); + return vendorAnswer; + }, }); /** @@ -363,10 +403,10 @@ afterAll(async () => { }); describe("a person who has not connected", () => { - test("is refused, and told to connect rather than told it broke", () => { + test("is refused, and told to connect rather than told it broke", async () => { // A refusal, not an error. Nothing is wrong: they simply have not granted access yet, and the - // sentence they get should be one they can act on. - expect( + // sentence they get should be one they can act on. Awaited, so the assertion is made at all. + await expect( store.callTool({ ref, args: {}, botId, actorId: askerId }), ).rejects.toThrow(PluginRefusedError); }); @@ -649,3 +689,241 @@ describe("retiring the credentials a person owns", () => { ).toBe(0); }); }); + +describe("disconnecting your own account", () => { + const liveCredential = async (credentialId: string) => { + const [row] = await database + .select({ revokedAt: credentials.revokedAt }) + .from(credentials) + .where(eq(credentials.id, credentialId)); + return row !== undefined && row.revokedAt === null; + }; + const disconnect = (userId: string) => + store.disconnectHeld({ serverId, userId, by: userId }); + + test("asks the vendor to withdraw it, then ends it here, and a call after is refused", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + revocations.length = 0; + vendorAnswer = "revoked"; + + expect(await disconnect(askerId)).toEqual({ + disconnected: true, + vendorRevoked: true, + }); + + // The vendor was asked about this person's token, at the catalogue's own address, as this + // deployment's client. + expect(revocations).toEqual([ + { + revokeUrl: "https://oauth2.googleapis.com/revoke", + client: CLIENT, + refreshToken: askerRefreshToken, + }, + ]); + expect(await liveCredential(credentialId)).toBe(false); + expect( + (await store.connectionsFor(askerId)).filter( + (row) => row.serverId === serverId, + ), + ).toEqual([]); + await expect( + store.callTool({ ref, args: {}, botId, actorId: askerId }), + ).rejects.toBeInstanceOf(PluginRefusedError); + + // The trail says who, whose, why, and the vendor's answer as one word. Never the token. + const [row] = await database + .select({ payload: auditEvents.payload }) + .from(auditEvents) + .where( + and( + eq(auditEvents.eventType, "mcp.account_disconnected"), + eq(auditEvents.targetId, serverId), + sql`${auditEvents.payload}->>'owner' = ${askerId}`, + ), + ) + .orderBy(desc(auditEvents.createdAt)) + .limit(1); + expect(row?.payload).toMatchObject({ + actor: askerId, + owner: askerId, + reason: "self", + vendorRevocationRequested: true, + vendorAnswer: "revoked", + }); + expect(JSON.stringify(row?.payload)).not.toContain(askerRefreshToken); + }); + + test("a vendor that cannot be reached changes nothing, so pressing again is the recovery", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + vendorAnswer = "unreachable"; + + await expect(disconnect(askerId)).rejects.toBeInstanceOf( + VendorRevocationUnreachableError, + ); + expect(await liveCredential(credentialId)).toBe(true); + expect( + (await store.connectionsFor(askerId)).some( + (row) => row.serverId === serverId && !row.revoked, + ), + ).toBe(true); + + vendorAnswer = "revoked"; + expect((await disconnect(askerId)).disconnected).toBe(true); + expect(await liveCredential(credentialId)).toBe(false); + }); + + test("a vendor that refuses still ends it here, and says the vendor did not confirm", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + vendorAnswer = "refused"; + + expect(await disconnect(askerId)).toEqual({ + disconnected: true, + vendorRevoked: false, + }); + expect(await liveCredential(credentialId)).toBe(false); + }); + + test("a token the vendor already calls invalid counts as withdrawn", async () => { + await registerClient(); + await connect(askerId, askerRefreshToken); + vendorAnswer = "already_gone"; + + expect(await disconnect(askerId)).toEqual({ + disconnected: true, + vendorRevoked: true, + }); + }); + + test("ends only the asker's own account", async () => { + await registerClient(); + await connect(askerId, askerRefreshToken); + const othersCredential = await connect(otherId, otherRefreshToken); + revocations.length = 0; + vendorAnswer = "revoked"; + + await disconnect(askerId); + + expect(revocations.map((request) => request.refreshToken)).toEqual([ + askerRefreshToken, + ]); + expect(await liveCredential(othersCredential)).toBe(true); + exchanged.length = 0; + await store.callTool({ ref, args: {}, botId, actorId: otherId }); + expect(exchanged).toEqual([otherRefreshToken]); + }); + + test("somebody with nothing connected is told so, and the vendor is not asked", async () => { + await registerClient(); + await connect(askerId, askerRefreshToken); + await disconnect(askerId); + revocations.length = 0; + + expect(await disconnect(askerId)).toEqual({ + disconnected: false, + vendorRevoked: false, + }); + // The anonymous actor owns nothing either. + expect(await disconnect("")).toEqual({ + disconnected: false, + vendorRevoked: false, + }); + expect(revocations).toEqual([]); + }); + + test("a credential already revoked here is not sent to the vendor, and the row still goes", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + await database + .update(credentials) + .set({ revokedAt: new Date() }) + .where(eq(credentials.id, credentialId)); + revocations.length = 0; + + expect(await disconnect(askerId)).toEqual({ + disconnected: true, + vendorRevoked: false, + }); + expect(revocations).toEqual([]); + expect( + (await store.connectionsFor(askerId)).filter( + (row) => row.serverId === serverId, + ), + ).toEqual([]); + }); +}); + +describe("DELETE /api/plugins/servers/:id/connection for an account held here", () => { + /** The real route through `createApp`, signed in as `who`, over this suite's store. */ + function appAs(who: string) { + const args = new Array(createApp.length) as unknown as Parameters< + typeof createApp + >; + args[0] = loadConfig( + testEnvironment({ OPENBOT_APP_URL: "https://app.example" }), + ); + args[1] = { + handler: () => new Response(null, { status: 204 }), + api: { + getSession: async () => ({ + user: { id: who, email: `${who}@openbot.test`, name: "Person" }, + }), + }, + } as never; + args[2] = { rolesForUser: async () => ["user"] } as never; + args[12] = createAuditStore(database); + args[14] = store; + return createApp(...args); + } + const press = (who: string) => + appAs(who).request( + `https://app.example/api/plugins/servers/${serverId}/connection`, + { method: "DELETE" }, + ); + + test("ends the signed-in person's own account, and only theirs", async () => { + await registerClient(); + await connect(askerId, askerRefreshToken); + const othersCredential = await connect(otherId, otherRefreshToken); + revocations.length = 0; + vendorAnswer = "revoked"; + + const response = await press(askerId); + + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + disconnected: true, + vendorRevoked: true, + }); + expect(revocations.map((request) => request.refreshToken)).toEqual([ + askerRefreshToken, + ]); + const [other] = await database + .select({ revokedAt: credentials.revokedAt }) + .from(credentials) + .where(eq(credentials.id, othersCredential)); + expect(other?.revokedAt).toBeNull(); + }); + + test("an unreachable vendor is a 502 in the person's words, and nothing changes", async () => { + await registerClient(); + const credentialId = await connect(askerId, askerRefreshToken); + vendorAnswer = "unreachable"; + + const response = await press(askerId); + + expect(response.status).toBe(502); + expect(await response.json()).toEqual({ + error: + "Couldn't reach Google, so nothing was disconnected. Try again in a moment.", + }); + const [credential] = await database + .select({ revokedAt: credentials.revokedAt }) + .from(credentials) + .where(eq(credentials.id, credentialId)); + expect(credential?.revokedAt).toBeNull(); + vendorAnswer = "revoked"; + }); +}); diff --git a/openbot/server/tests/typecheck-baseline.txt b/openbot/server/tests/typecheck-baseline.txt index 6b8a0929..7caedc6b 100644 --- a/openbot/server/tests/typecheck-baseline.txt +++ b/openbot/server/tests/typecheck-baseline.txt @@ -195,7 +195,6 @@ server/tests/plugin-store.integration.test.ts: error TS2345: Argument of type '{ server/tests/plugin-store.integration.test.ts: error TS2345: Argument of type '{ provenance: string; url: string; }' is not assignable to parameter of type '{ provenance: string; url: string; authScheme: string | null; accountMode?: string | null | undefined; }'. server/tests/plugin-store.integration.test.ts: error TS2345: Argument of type '{ slug: string; title: string; by: string; }' is not assignable to parameter of type '{ slug: string; title: string; logo?: string | null | undefined; by: string; connection: BrokerConnection; }'. server/tests/plugin-store.integration.test.ts: error TS2556: A spread argument must either have a tuple type or be passed to a rest parameter. -server/tests/plugin-user-credential.integration.test.ts: error TS2322: Type '{ readSecret: (id: string) => Promise<{ encryptedValue: string; revokedAt: Date | null; }>; create: () => Promise; updateSecret: () => Promise; revoke: (id: string) => Promise<...>; }' is not assignable to type 'CredentialSecretReader & CredentialStore'. server/tests/policy-dry-run.test.ts: error TS2739: Type '{ id: string; actorUserId: null; eventType: string; targetType: string; targetId: string; createdAt: string; payload: Record; }' is missing the following properties from type 'AuditEvent': initiatorKind, initiatorId server/tests/remote-approval-context.test.ts: error TS2345: Argument of type '(input: RunAgentInput) => Observable<{ type: string; threadId: string; runId: string; } | { type: string; snapshot: { real: string; }; } | { type: string; toolCallId: string; toolCallName: string; parentMessageId: string; } | { ...; } | { ...; } | { ...; } | { ...; }>' is not assignable to parameter of type '(input: RunAgentInput) => Observable'. server/tests/remote-bot-handoff.integration.test.ts: error TS2322: Type 'Promise' is not assignable to type 'Promise'.