diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7fa0f51f..2383a48c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -85,6 +85,9 @@ jobs: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + # The new-values-key check compares this chart against the last released one, or against + # main where the chart has not shipped yet. A shallow clone has neither to compare with. + fetch-depth: 0 - uses: azure/setup-helm@9bc31f4ebc9c6b171d7bfbaa5d006ae7abdb4310 # v5.0.1 with: version: v3.19.0 @@ -156,6 +159,15 @@ jobs: --set networkPolicy.enabled=true \ --set computers.extraEnv[0].name=EGRESS_PROXY_DEFAULT \ --set-string computers.extraEnv[0].value=http://proxy.internal:3128 + # And that a values key this chart did not used to have still renders when it is absent. + # + # `helm upgrade --reuse-values` takes the previous release's computed values rather than + # merging the new chart's defaults, so a key added by the release being installed is missing on + # every deployment that already exists. Unguarded that is a nil dereference that fails the + # whole render, or an empty scalar Kubernetes reads as unset. Both shipped: one was found in + # review, the other by a live upgrade after the first had been fixed one key over. + - name: A new values key can be absent + run: bun scripts/check-new-values-keys.ts charts/openbot/ci/${{ matrix.target }}-values.yaml test: name: tests diff --git a/app/src/components/agents/orb/agent-orb.tsx b/app/src/components/agents/orb/agent-orb.tsx index 1857e41f..9fc2c7f3 100644 --- a/app/src/components/agents/orb/agent-orb.tsx +++ b/app/src/components/agents/orb/agent-orb.tsx @@ -1,4 +1,3 @@ -import { cn } from "@/lib/utils"; import { type MotionStyle, motion, @@ -7,6 +6,7 @@ import { useReducedMotion, useTransform, } from "motion/react"; +import { cn } from "@/lib/utils"; import { type AIAmplitude, type AIState, diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index f5044c1c..548bc600 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -1,18 +1,15 @@ import type { Message } from "@ag-ui/core"; -import { IconBox } from "@tabler/icons-react"; import { useRenderToolCall } from "@copilotkit/react-core/v2"; +import { IconBox } from "@tabler/icons-react"; import { motion, useReducedMotion } from "motion/react"; import { memo, useEffect, useMemo, useRef } from "react"; import { Streamdown } from "streamdown"; -import { markdownComponents } from "@/lib/markdown"; -import { EASE_OUT, ENTRANCE_SECONDS } from "@/lib/motion"; import { Bubble, BubbleContent } from "@/components/ui/bubble"; import { MessageContent, MessageFooter, Message as MessageRow, } from "@/components/ui/message"; -import { Skeleton } from "@/components/ui/skeleton"; import { MessageScroller, MessageScrollerButton, @@ -22,12 +19,15 @@ import { MessageScrollerViewport, useMessageScroller, } from "@/components/ui/message-scroller"; -import { toVisibleChatItems } from "./chat-messages"; -import { asText, forDisplay, REFUSAL_MARKER } from "@/lib/plugins/tool-result"; +import { Skeleton } from "@/components/ui/skeleton"; +import { markdownComponents } from "@/lib/markdown"; +import { EASE_OUT, ENTRANCE_SECONDS } from "@/lib/motion"; import { readToolName } from "@/lib/plugins/tool-name"; +import { asText, forDisplay, REFUSAL_MARKER } from "@/lib/plugins/tool-result"; +import { toVisibleChatItems } from "./chat-messages"; import type { QueuedMessage } from "./composer"; -import { ToolLine } from "./tool-line"; import { ToolRenderBoundary } from "./tool-boundary"; +import { ToolLine } from "./tool-line"; type ChatTranscriptProps = { busy?: boolean; diff --git a/app/src/components/layout/page-shell.tsx b/app/src/components/layout/page-shell.tsx index 377fa01d..3663a667 100644 --- a/app/src/components/layout/page-shell.tsx +++ b/app/src/components/layout/page-shell.tsx @@ -1,9 +1,8 @@ +import { IconChevronLeft } from "@tabler/icons-react"; +import { Link, type LinkProps } from "@tanstack/react-router"; import type * as React from "react"; - import { cn } from "@/lib/utils"; -import { Link, type LinkProps } from "@tanstack/react-router"; import { Button } from "../ui/button"; -import { IconChevronLeft } from "@tabler/icons-react"; /** * The frame every configuration screen sits in. diff --git a/app/src/lib/channels/mutations.ts b/app/src/lib/channels/mutations.ts index 2d10c4e4..af83550c 100644 --- a/app/src/lib/channels/mutations.ts +++ b/app/src/lib/channels/mutations.ts @@ -1,6 +1,6 @@ import { - mutationOptions, type InfiniteData, + mutationOptions, type QueryClient, } from "@tanstack/react-query"; import { client, tryClient } from "@/lib/client"; diff --git a/app/src/lib/copilot/escalation-tool.tsx b/app/src/lib/copilot/escalation-tool.tsx new file mode 100644 index 00000000..98ae338d --- /dev/null +++ b/app/src/lib/copilot/escalation-tool.tsx @@ -0,0 +1,61 @@ +import { useRenderTool } from "@copilotkit/react-core/v2"; +import { z } from "zod"; +import { ToolLine } from "@/components/channels/tool-line"; +import { PUT_TO } from "@/lib/copilot/markers"; +import { saidItWentAhead } from "@/lib/plugins/tool-result"; + +/** + * How a Bot stopping to ask a person reads in the transcript. + * + * RENDER ONLY, for the same reason as the handoff beside it: `ask_person` runs on the server, where + * the route and the audit row are. What this adds is that the choice is legible. A Bot which decided + * it could not settle something on its own, and said so rather than guessing, has done the right + * thing; drawn as a raw `ask_person` call with its arguments as JSON it reads as a malfunction. + */ +const parameters = z.object({ + question: z.string().optional(), + why: z.string().optional(), +}); + +/** + * Whether the question reached anybody. + * + * Decoded first, because a server-side tool's result arrives as a JSON-encoded string and a prefix + * matched against the raw value never matches: that mistake drew every successful handoff as + * Blocked. A route that could not reach a person is the case worth drawing differently, because the + * Bot has stopped and nobody has been asked. + */ +function reached(result: unknown): boolean { + return saidItWentAhead(result, PUT_TO); +} + +export function EscalationTool() { + useRenderTool({ + name: "ask_person", + parameters, + render: ({ parameters: given, result, status }) => { + const running = status !== "complete" && result === undefined; + return ( + +
+ {given?.question ?

{given.question}

: null} + {/* + * Why it stopped, which is the half a person is owed. "I need a decision only you can + * make" and "I could not find the answer" look the same from the outside and are not. + */} + {given?.why ? ( +

{given.why}

+ ) : null} +
+ + ); + }, + }); + + return null; +} diff --git a/app/src/lib/copilot/handoff-tool.tsx b/app/src/lib/copilot/handoff-tool.tsx new file mode 100644 index 00000000..daac5cac --- /dev/null +++ b/app/src/lib/copilot/handoff-tool.tsx @@ -0,0 +1,79 @@ +import { useRenderTool } from "@copilotkit/react-core/v2"; +import { z } from "zod"; +import { ToolLine } from "@/components/channels/tool-line"; +import { HANDED_OVER } from "@/lib/copilot/markers"; +import { saidItWentAhead } from "@/lib/plugins/tool-result"; + +/** + * How a Bot handing work to another Bot reads in the transcript. + * + * RENDER ONLY. `message_bot` runs on the server, where the grant, the caps and the audit row are, so + * nothing here registers a tool or decides anything. What it registers is a line, because a hop that + * happens off-screen is the thing the issue asks to avoid: a conversation that quietly fans out to + * four Bots and bills for all of them should say so while it is doing it. + * + * Without this the call still appears, as a generic tool call named `message_bot` with its arguments + * as JSON. That is technically visible and practically not: the point is that a person can see their + * Bot bringing in another one and read what it asked for. + */ +const parameters = z.object({ + bot: z.string().optional(), + task: z.string().optional(), + constraints: z.string().optional(), + expecting: z.string().optional(), +}); + +/** + * Whether the deployment refused the hop. + * + * The result is a sentence the Bot can say either way, because a refusal mid-run is an answer rather + * than an exception. The transcript still has to tell the two apart: one is a Bot bringing in help, + * the other is a boundary holding, and drawing them the same way would make a working cap look like + * a working handoff. + */ +function refused(result: unknown): boolean { + return !saidItWentAhead(result, HANDED_OVER); +} + +export function HandoffTool() { + useRenderTool({ + name: "message_bot", + parameters, + render: ({ parameters: given, result, status }) => { + const asked = given?.bot?.trim(); + const running = status !== "complete" && result === undefined; + return ( + + {/* + * The parts, kept as parts. The asking model was made to name them so the receiving one + * need not infer them, and a person reading the conversation gets the same benefit: what + * was asked, what bounded it, and what was wanted back. + */} +
+ {given?.task ?

{given.task}

: null} + {given?.constraints ? ( +

+ Constraints: {given.constraints} +

+ ) : null} + {given?.expecting ? ( +

+ Wanted back: {given.expecting} +

+ ) : null} + {typeof result === "string" ? ( +

{result}

+ ) : null} +
+ + ); + }, + }); + + return null; +} diff --git a/app/src/lib/copilot/markers.ts b/app/src/lib/copilot/markers.ts new file mode 100644 index 00000000..696d0ee7 --- /dev/null +++ b/app/src/lib/copilot/markers.ts @@ -0,0 +1,8 @@ +/** + * The two marker phrases, re-exported from the one place they are declared. + * + * `shared/` is where the server reads them from too, so a rewording changes both sides at once. This + * file exists so the browser code keeps importing through `@/`, and so the path to `shared/` is + * written down once rather than in every renderer. + */ +export { HANDED_OVER, PUT_TO } from "../../../../shared/handoff-markers"; diff --git a/app/src/lib/copilot/provider.tsx b/app/src/lib/copilot/provider.tsx index 1d58b744..14e0b196 100644 --- a/app/src/lib/copilot/provider.tsx +++ b/app/src/lib/copilot/provider.tsx @@ -2,7 +2,9 @@ import { CopilotKitProvider } from "@copilotkit/react-core/v2"; import type { ReactNode } from "react"; import { ActiveBotProvider } from "./active-bot"; import { ComputerTools } from "./computer-tools"; +import { EscalationTool } from "./escalation-tool"; import { GalleryTools } from "./gallery-tools"; +import { HandoffTool } from "./handoff-tool"; import { SandboxedTools } from "./sandboxed-tools"; /** @@ -25,6 +27,12 @@ export function CopilotProvider({ children }: { children: ReactNode }) { {/* Computer tools target the Bot declared by the mounted surface. */} + {/* + Draws a Bot bringing in another Bot. Registers no tool: `message_bot` runs on the server, + where the grant and the caps are. A hop that happens off-screen is the thing to avoid. + */} + + {/* Gallery tools are registered once; their handlers re-read the active Bot to avoid shadowing renderers. */} {/* Browser-authored components use the same component grants as the compiled gallery. */} diff --git a/app/src/lib/plugins/tool-result.ts b/app/src/lib/plugins/tool-result.ts index 12495668..ce2a57b4 100644 --- a/app/src/lib/plugins/tool-result.ts +++ b/app/src/lib/plugins/tool-result.ts @@ -81,3 +81,22 @@ export function forDisplay(text: string): string { return `\`\`\`json\n${JSON.stringify(parsed, null, 2)}\n\`\`\``; } + +/** + * Whether a server-side tool's result begins with the phrase that means it went ahead. + * + * TWO CALLERS AND ONE RULE, because they had two. A tool that runs on the server reaches the + * transcript as text meant for a model, so the only thing the renderer can read an outcome out of is + * the wording — and the wording arrives JSON-encoded, which is why this decodes before it matches. + * + * The awkward case is a result that is neither a string nor absent. `message_bot` treated that as + * success and `ask_person` treated it as a refusal, for the same situation, and the handoff's own + * comments say which way round is worse: a boundary that held drawn as a Bot getting on with it. + * So anything unrecognisable is not success. Absent is left alone, because a call still running has + * no result yet and the caller decides that from its status. + */ +export function saidItWentAhead(result: unknown, marker: string): boolean { + if (result === undefined) return true; + if (typeof result !== "string") return false; + return asText(result).startsWith(marker); +} diff --git a/app/src/routes/_authed/_app/agents/index.tsx b/app/src/routes/_authed/_app/agents/index.tsx index 436b7769..118ea7f4 100644 --- a/app/src/routes/_authed/_app/agents/index.tsx +++ b/app/src/routes/_authed/_app/agents/index.tsx @@ -3,10 +3,10 @@ import { useQuery } from "@tanstack/react-query"; import { createFileRoute, Link } from "@tanstack/react-router"; import { z } from "zod"; import { AgentCard } from "@/components/agents/agent-card"; -import { StaggerItem } from "@/components/layout/stagger"; import { AgentProfile as AgentProfileDetail } from "@/components/agents/agent-profile"; import { NewAgent } from "@/components/agents/new-agent"; import { DetailPanel } from "@/components/layout/detail-panel"; +import { StaggerItem } from "@/components/layout/stagger"; import { Button } from "@/components/ui/button"; import { Empty, EmptyHeader, EmptyTitle } from "@/components/ui/empty"; import { agentListQueryOptions } from "@/lib/agents/queries"; diff --git a/app/src/routes/_authed/_app/skills.tsx b/app/src/routes/_authed/_app/skills.tsx index 410d39ac..bb9dd1aa 100644 --- a/app/src/routes/_authed/_app/skills.tsx +++ b/app/src/routes/_authed/_app/skills.tsx @@ -1,8 +1,8 @@ +import { IconDots, IconPlus } from "@tabler/icons-react"; import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query"; import { createFileRoute, Link } from "@tanstack/react-router"; import { useState } from "react"; import { z } from "zod"; -import { IconPlus } from "@tabler/icons-react"; import { DetailPanel } from "@/components/layout/detail-panel"; import { PageRows, @@ -13,10 +13,14 @@ import { StaggerItem } from "@/components/layout/stagger"; import { EditSkill } from "@/components/skills/edit-skill"; import { NewSkill } from "@/components/skills/new-skill"; import { Button } from "@/components/ui/button"; +import { + DropdownMenu, + DropdownMenuContent, + DropdownMenuGroup, + DropdownMenuItem, + DropdownMenuTrigger, +} from "@/components/ui/dropdown-menu"; import { Empty, EmptyHeader, EmptyTitle } from "@/components/ui/empty"; -import { currentUserQueryOptions } from "@/lib/auth/queries"; -import { removeSkillMutationOptions } from "@/lib/plugins/mutations"; -import { pluginsPageQueryOptions } from "@/lib/plugins/queries"; import { Item, ItemActions, @@ -24,15 +28,10 @@ import { ItemDescription, ItemTitle, } from "@/components/ui/item"; -import { - DropdownMenu, - DropdownMenuContent, - DropdownMenuGroup, - DropdownMenuItem, - DropdownMenuTrigger, -} from "@/components/ui/dropdown-menu"; -import { IconDots } from "@tabler/icons-react"; import { Separator } from "@/components/ui/separator"; +import { currentUserQueryOptions } from "@/lib/auth/queries"; +import { removeSkillMutationOptions } from "@/lib/plugins/mutations"; +import { pluginsPageQueryOptions } from "@/lib/plugins/queries"; /** * Personal `/` skills. They are instructions, not capabilities, and can only be granted to Bots the diff --git a/app/src/routes/_authed/admin/boundaries.tsx b/app/src/routes/_authed/admin/boundaries.tsx index d8bff38c..8efa6945 100644 --- a/app/src/routes/_authed/admin/boundaries.tsx +++ b/app/src/routes/_authed/admin/boundaries.tsx @@ -2,6 +2,8 @@ import { useMutation, useQuery } from "@tanstack/react-query"; import { createFileRoute, Link } from "@tanstack/react-router"; import { useState } from "react"; import { PageSection, PageShell } from "@/components/layout/page-shell"; +import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; import { saveActionPolicyMutationOptions } from "@/lib/computers/mutations"; import { type ActionPolicy, @@ -11,8 +13,6 @@ import { type PolicyMode, } from "@/lib/computers/queries"; import { queryClient } from "@/query-client"; -import { Button } from "@/components/ui/button"; -import { Input } from "@/components/ui/input"; /** * CEL computer-action boundary editor. Rules are shown as the gateway evaluates them, and denied diff --git a/app/src/routes/_authed/settings/connected-accounts/$key.tsx b/app/src/routes/_authed/settings/connected-accounts/$key.tsx index c59dfcdf..19f8b1c9 100644 --- a/app/src/routes/_authed/settings/connected-accounts/$key.tsx +++ b/app/src/routes/_authed/settings/connected-accounts/$key.tsx @@ -8,13 +8,6 @@ import { PageSection, PageShell, } from "@/components/layout/page-shell"; -import { - Item, - ItemActions, - ItemContent, - ItemDescription, - ItemTitle, -} from "@/components/ui/item"; import { Button } from "@/components/ui/button"; import { DropdownMenu, @@ -22,6 +15,13 @@ import { DropdownMenuItem, DropdownMenuTrigger, } from "@/components/ui/dropdown-menu"; +import { + Item, + ItemActions, + ItemContent, + ItemDescription, + ItemTitle, +} from "@/components/ui/item"; import { Separator } from "@/components/ui/separator"; import { connectAccountMutationOptions } from "@/lib/plugins/mutations"; import { diff --git a/app/tests/channel-menu-mutations.test.ts b/app/tests/channel-menu-mutations.test.ts index b0cd4791..a3404f93 100644 --- a/app/tests/channel-menu-mutations.test.ts +++ b/app/tests/channel-menu-mutations.test.ts @@ -1,11 +1,11 @@ import { afterEach, expect, test } from "bun:test"; -import { QueryClient, type InfiniteData } from "@tanstack/react-query"; +import { type InfiniteData, QueryClient } from "@tanstack/react-query"; import { deleteChannelMutationOptions, markChannelReadMutationOptions, setChannelPinnedMutationOptions, } from "../src/lib/channels/mutations"; -import { channelKeys, type ChannelPage } from "../src/lib/channels/queries"; +import { type ChannelPage, channelKeys } from "../src/lib/channels/queries"; const realFetch = globalThis.fetch; diff --git a/app/tests/theme-preference.test.ts b/app/tests/theme-preference.test.ts index 1e807e2e..ead9b15b 100644 --- a/app/tests/theme-preference.test.ts +++ b/app/tests/theme-preference.test.ts @@ -1,5 +1,5 @@ -import { readFileSync } from "node:fs"; import { describe, expect, test } from "bun:test"; +import { readFileSync } from "node:fs"; import { applyDarkTheme, parseStoredDarkTheme, diff --git a/app/tests/tool-result.test.ts b/app/tests/tool-result.test.ts index 92bb14cc..a700f206 100644 --- a/app/tests/tool-result.test.ts +++ b/app/tests/tool-result.test.ts @@ -1,5 +1,10 @@ import { describe, expect, test } from "bun:test"; -import { asText, forDisplay } from "../src/lib/plugins/tool-result"; +import { HANDED_OVER, PUT_TO } from "../src/lib/copilot/markers"; +import { + asText, + forDisplay, + saidItWentAhead, +} from "../src/lib/plugins/tool-result"; /** * What a tool actually said, recovered from how the transcript carries it. @@ -47,3 +52,70 @@ describe("reading a tool's answer", () => { ); }); }); + +/** + * The two lines a hop draws. + * + * Both read an outcome out of the tool's own prose, which is what a server-side tool leaves + * available, and both read it through `asText` for the reason above: matched against the raw value + * the prefix never matches, and every accepted hop was drawn as Blocked. + */ +describe("telling an accepted hop from a refused one", () => { + /* + * Read from the server's own source, not retyped. + * + * These markers cross a network: the server writes the sentence and the transcript reads its first + * words. Nothing coupled the two ends, so a rewording on the server left every accepted hop drawn + * as Blocked with the whole suite green — which is the bug both renderers' comments recount. The + * test imports the browser's copies and asserts they still match the server's. + */ + const handedOver = HANDED_OVER; + const putTo = PUT_TO; + + test("an accepted handoff is not a refusal, encoded or not", () => { + const said = `${handedOver}Knowledge. It will answer in its own conversation.`; + expect(asText(JSON.stringify(said)).startsWith(handedOver)).toBe(true); + expect(asText(said).startsWith(handedOver)).toBe(true); + }); + + test("a cap refusing a hop does not start with the marker", () => { + const said = + "This turn has already asked 3 Bots, which is as many as this deployment allows."; + expect(asText(JSON.stringify(said)).startsWith(handedOver)).toBe(false); + }); + + test("a question that reached somebody is not drawn as one that did not", () => { + const said = `${putTo}the person in this conversation. Ask it in your own words now.`; + expect(asText(JSON.stringify(said)).startsWith(putTo)).toBe(true); + }); + + test("a route that reached nobody does not start with the marker", () => { + const said = "The on-call rota is not configured."; + expect(asText(JSON.stringify(said)).startsWith(putTo)).toBe(false); + }); +}); + +/** + * The two ends of a phrase that crosses a network. + * + * The server writes the sentence; the transcript reads its first words to decide whether to draw a + * hop or a boundary. One declaration in `shared/handoff-markers.ts` means the two cannot disagree + * about the phrase, so what is left to hold is how the transcript READS a result — which is what + * this block does. That the sentences still begin with these markers is asserted where the sentences + * are written: `server/tests/agent-handoff-tool.test.ts` and `agent-escalation.test.ts`. + */ +describe("the markers the server and the transcript both use", () => { + /* + * A result that is neither a string nor absent used to mean success to one renderer and a refusal + * to the other, for the same situation. Anything unrecognisable is not success: a boundary that + * held drawn as a Bot getting on with it is the worse of the two mistakes. + */ + test("an unrecognisable result is never drawn as success", () => { + expect(saidItWentAhead({ some: "object" }, HANDED_OVER)).toBe(false); + expect(saidItWentAhead(42, PUT_TO)).toBe(false); + }); + + test("a result that has not arrived yet is left to the caller's status", () => { + expect(saidItWentAhead(undefined, HANDED_OVER)).toBe(true); + }); +}); diff --git a/app/tests/transcript-messages.test.ts b/app/tests/transcript-messages.test.ts index 3e7fe695..0fab2ede 100644 --- a/app/tests/transcript-messages.test.ts +++ b/app/tests/transcript-messages.test.ts @@ -1,5 +1,5 @@ -import type { Message } from "@ag-ui/core"; import { describe, expect, test } from "bun:test"; +import type { Message } from "@ag-ui/core"; import { seedMessage, stashFirstMessage, diff --git a/app/tsconfig.json b/app/tsconfig.json index 6bf334e8..8775fe29 100644 --- a/app/tsconfig.json +++ b/app/tsconfig.json @@ -8,5 +8,5 @@ "jsx": "react-jsx", "lib": ["ES2024", "DOM", "DOM.Iterable"] }, - "include": ["src", "vite.config.ts"] + "include": ["src", "vite.config.ts", "../shared/handoff-markers.ts"] } diff --git a/bun.lock b/bun.lock index 2911131c..1ac087cf 100644 --- a/bun.lock +++ b/bun.lock @@ -5,7 +5,7 @@ "": { "name": "openbot", "devDependencies": { - "@biomejs/biome": "^2.3.8", + "@biomejs/biome": "2.5.10", "@copilotkit/aimock": "1.39.0", "@types/bun": "^1.3.3", "roughjs": "^4.6.6", diff --git a/charts/openbot/templates/_helpers.tpl b/charts/openbot/templates/_helpers.tpl index bc75d2d8..059653a6 100644 --- a/charts/openbot/templates/_helpers.tpl +++ b/charts/openbot/templates/_helpers.tpl @@ -174,6 +174,34 @@ and in whatever holds the release, which is not where `KEY_ENCRYPTION_KEY` belon - name: COMPUTER_SANDBOX_TEMPLATE_FILE value: /etc/openbot/sandbox-template.json {{- end }} +{{- /* + How far one Bot may hand work to another. + + Always set, so a deployment that has switched this off says so rather than relying on the image's + default staying what it is today. + + ABSENT AND ZERO ARE DIFFERENT, which is why this is not `| default`. Sprig's `default` substitutes + whenever a value is EMPTY, and zero is empty: `--set config.handoff.maxDepth=0` rendered `"1"` and + silently switched the capability back on for a deployment that had switched it off. A guard that + defeats the off switch is worse than the nil dereference it was added for. `kindIs "invalid"` asks + the question actually being asked, which is whether anybody said anything at all. + + PARENTHESISED, because `config.handoff` is a key this chart did not have before. + `helm upgrade --reuse-values` takes the previous release's computed values instead of merging the + new chart's defaults, so on every existing deployment this map is simply absent. Reached with a + bare `.Values.config.handoff.maxDepth` that is a nil dereference, and it fails the WHOLE render: + this helper is included by the server deployment, so the upgrade does not lose the handoff + feature, it does not install at all. +*/}} +{{- $handoff := .Values.config.handoff | default dict -}} +{{- $maxDepth := 1 -}} +{{- if not (kindIs "invalid" $handoff.maxDepth) -}}{{- $maxDepth = $handoff.maxDepth -}}{{- end -}} +{{- $maxPerRun := 3 -}} +{{- if not (kindIs "invalid" $handoff.maxPerRun) -}}{{- $maxPerRun = $handoff.maxPerRun -}}{{- end }} +- name: BOT_HANDOFF_MAX_DEPTH + value: {{ $maxDepth | quote }} +- name: BOT_HANDOFF_MAX_PER_RUN + value: {{ $maxPerRun | quote }} - name: INTELLIGENCE_API_URL value: {{ .Values.config.intelligence.apiUrl | quote }} - name: INTELLIGENCE_GATEWAY_WS_URL @@ -276,7 +304,7 @@ and in whatever holds the release, which is not where `KEY_ENCRYPTION_KEY` belon override whatever `extraEnv` set, which turns the escape hatch into a trap for the one variable someone would need it for. */}} -{{- if .Values.routines.enabled }} +{{- if (.Values.routines).enabled }} - name: WORKER_SHARED_SECRET valueFrom: secretKeyRef: @@ -404,3 +432,4 @@ than anything that names the cause. {{- define "openbot.automountToken" -}} {{- or .Values.serviceAccount.automountServiceAccountToken (eq .Values.computers.mode "sandbox") -}} {{- end -}} + diff --git a/charts/openbot/templates/computer/culler-cronjob.yaml b/charts/openbot/templates/computer/culler-cronjob.yaml index 40f469f6..997d9e21 100644 --- a/charts/openbot/templates/computer/culler-cronjob.yaml +++ b/charts/openbot/templates/computer/culler-cronjob.yaml @@ -28,6 +28,31 @@ spec: jobTemplate: spec: backoffLimit: 1 + {{- /* + A CEILING ON ONE SWEEP, because `Forbid` above turns a hung one into a permanent stop. + + Without this a sweep that never returns — a wedged API-server call, a database connection + that hangs rather than refuses — holds the concurrency lock for ever. Kubernetes will not + start the next one, so culling simply ceases: no error, no restart, no alert, and the first + sign is a cloud bill for a fleet of browsers nobody has used in a fortnight. That is exactly + the failure this feature exists to prevent, arriving through the mechanism meant to prevent + overlapping runs. + + Comfortably longer than a real sweep, which claims twenty computers and suspends them. + + Defaulted through `kindIs "invalid"` rather than `| default`, because sprig substitutes on + EMPTY and zero is empty — the same trap that silently defeated the handoff off-switch one + file over. Here it is defaulted because this key is newer than the culler around it. Under + `helm upgrade --reuse-values` an existing release carries `culler` without it, so the + template still renders and emits an empty scalar: null, which Kubernetes reads as unset. The + ceiling described above would then silently not exist, on exactly the deployments that have + been running long enough to have a wedged sweep. + */}} + {{- $deadline := 600 -}} + {{- if not (kindIs "invalid" .Values.computers.sandbox.culler.activeDeadlineSeconds) -}} + {{- $deadline = .Values.computers.sandbox.culler.activeDeadlineSeconds -}} + {{- end }} + activeDeadlineSeconds: {{ $deadline }} template: metadata: labels: diff --git a/charts/openbot/templates/routines/cronjob.yaml b/charts/openbot/templates/routines/cronjob.yaml index 386e9ea3..4cd48e4b 100644 --- a/charts/openbot/templates/routines/cronjob.yaml +++ b/charts/openbot/templates/routines/cronjob.yaml @@ -1,4 +1,4 @@ -{{- if .Values.routines.enabled }} +{{- if (.Values.routines).enabled }} {{- $component := "routines" -}} {{/* Firing the routines a Bot was scheduled to run. @@ -18,7 +18,14 @@ metadata: labels: {{ include "openbot.componentLabels" (dict "root" . "component" $component) | indent 4 }} spec: - schedule: {{ .Values.routines.schedule | quote }} + {{- $schedule := "*/5 * * * *" -}} + {{- if not (kindIs "invalid" (.Values.routines).schedule) -}} + {{- $schedule = (.Values.routines).schedule -}} + {{- end }} + # The fallback matches values.yaml, and is only reached on an upgrade that reuses values from + # before this key existed. It said `* * * * *` for one commit, which is five times more often than + # anything documents. + schedule: {{ $schedule | quote }} concurrencyPolicy: Forbid successfulJobsHistoryLimit: 1 failedJobsHistoryLimit: 3 diff --git a/charts/openbot/templates/secret.yaml b/charts/openbot/templates/secret.yaml index 8b7ecf8b..4e9e60f2 100644 --- a/charts/openbot/templates/secret.yaml +++ b/charts/openbot/templates/secret.yaml @@ -52,7 +52,7 @@ stringData: deployment turn routines on with no secret and find out at 03:05 that every firing gets a 401. `required` fails at `helm install` instead. */}} - {{- if .Values.routines.enabled }} + {{- if (.Values.routines).enabled }} worker-shared-secret: {{ required "secrets.workerSharedSecret is required when routines.enabled. Generate one with: openssl rand -base64 32" .Values.secrets.workerSharedSecret | quote }} {{- end }} {{- end }} diff --git a/charts/openbot/templates/validation.yaml b/charts/openbot/templates/validation.yaml index 5b569eca..c0e33367 100644 --- a/charts/openbot/templates/validation.yaml +++ b/charts/openbot/templates/validation.yaml @@ -287,7 +287,7 @@ This template renders nothing. `managed-agent-token` and `better-auth-secret` above: the value is not readable at template time, but the list of keys is, and a store that never mentions this key cannot be holding one. */}} -{{- if and .Values.routines.enabled .Values.externalSecrets.enabled }} +{{- if and (.Values.routines).enabled .Values.externalSecrets.enabled }} {{- $named := list }} {{- range .Values.externalSecrets.data }}{{- $named = append $named .secretKey }}{{- end }} {{- if not (has "worker-shared-secret" $named) }} diff --git a/charts/openbot/values.yaml b/charts/openbot/values.yaml index fe47886c..7b0efe5d 100644 --- a/charts/openbot/values.yaml +++ b/charts/openbot/values.yaml @@ -148,6 +148,19 @@ config: apiUrl: "" gatewayWsUrl: "" # The two secret halves live under `secrets` below, never here. + # How far one Bot may hand work to another. + # + # A Bot addressing another is a grant an administrator makes, and these are the ceilings on what + # that grant can cost. Both are deliberately mean: a hop is a whole agent turn at the other end, + # several Bots asked in one turn cost several full runs rather than a fraction each, and with + # `computers.mode: sandbox` a hop to a Bot whose browser is asleep also pays a pod resume. A chatty + # Bot fanning out to four others wakes four machines. + # + # `maxDepth: 0` switches the capability off entirely: no Bot is offered the tool and the delivery + # loop does not run. + handoff: + maxDepth: 1 + maxPerRun: 3 # Your own Bot, over AG-UI. # # OpenBot is a shell for somebody else's agent, so this is the seam that matters: point it at a @@ -234,6 +247,9 @@ computers: # is not the same question, so the culler below asks this one. idleAfter: 30m culler: + # A ceiling on one sweep. `concurrencyPolicy: Forbid` means a sweep that hangs holds the lock + # for ever and culling stops silently, which is the failure this feature exists to prevent. + activeDeadlineSeconds: 600 enabled: true schedule: "*/5 * * * *" diff --git a/docs/architecture.md b/docs/architecture.md index 5a2c9403..46b4e2de 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -143,6 +143,99 @@ Governance: The shipped component data functions read the audit trail: `botActivity` and `recentRefusals`. +## One Bot handing work to another + +A Bot can address another Bot, and the addressed one answers for itself rather than the first +relaying text on its behalf. + +`message_bot` is offered beside a Bot's granted tools, so which Bots may reach which is an ordinary +grant: `plugin_grants` with a `bot` kind. A Bot granted nobody is offered nothing. + +What it takes is typed. The asking model names the task, anything that bounds it and what a good +answer looks like, rather than writing a paragraph. Free text is the commonest way a handoff goes +quietly wrong: the receiving Bot infers the intent, guesses the constraints, and when it guesses +wrong it does not fail, it answers something else confidently. + +Four things are decided by the deployment and never by the model: + +- **Who is being addressed**, resolved against the roster the asking person may see. A Bot must not + reach a Bot its person cannot, or this is a way around agent visibility. A Bot that does not exist + and one that is not theirs to see are refused in the same words, so this cannot enumerate the + roster. +- **Where the answer lands**, from the signed run assertion. Otherwise a Bot could drop a turn into a + conversation it was never part of. +- **Who is asking**, stamped from the row this deployment wrote. A Bot able to write its own + attribution could claim to be another one. +- **How deep the chain is**, also from the assertion, which is what stops A asking B asking C asking + A for ever. + +The second Bot runs as the same person, with its own role and its own grants, so it sees what that +person may see and no more. + +**The answer lands in that Bot's own conversation with the person.** Not the conversation that asked, +and this is a property of the platform rather than a choice: an Intelligence thread is owned by +exactly one agent. So the conversation that asked says where the work went, and the one that answers +moves to the top of the roster with an unread mark. The person gets both halves. + +What the answering conversation keeps is one line saying who asked and what for, not the envelope. +Those are two texts with two readers: the model needs the task, the constraints and the shape of a +good answer, while a person scrolling needs to know why that Bot suddenly spoke. The asking +conversation's history is read by the addressed Bot as context and is not repeated into the +transcript. + +**A hop that fails for good is said out loud.** When one runs out of attempts, the asking Bot is sent +back into the conversation the person is watching to say plainly that nothing came back. Otherwise a +question handed on and never answered is indistinguishable from a slow one, and the conversation just +stops. + +**A hop is claimed work, not a callback.** It is a row on the same queue the idle-computer culler +uses: the Bot being addressed is very unlikely to be on the pod that addressed it, and a hop held in +memory is lost the moment either is rescheduled. Every replica sweeps for hops and the queue decides +which gets which. The lease is renewed for as long as the run takes, because a run is minutes and a +lapsed lease hands the same hop to a second replica. + +`BOT_HANDOFF_MAX_DEPTH` and `BOT_HANDOFF_MAX_PER_RUN` are the ceilings, and both refuse rather than +truncate. They are not polish: a hop is a whole agent turn at the other end, several Bots asked in one +turn cost several full runs, and where each Bot has its own computer a fan-out wakes a machine per +Bot. `BOT_HANDOFF_MAX_DEPTH=0` switches the capability off, and then no Bot is offered the tool and +the delivery loop does not run. + +Every outcome is in the audit trail: offered, refused with which cap or missing grant stopped it, +delivered, failed, and retried. The refused row is the one that matters most, because a hop that +happened is visible in the transcript and one that was refused is invisible everywhere else. + +### Asking a person + +`ask_person` sits beside `message_bot` and competes with it for the same decision. A Bot that needs +judgement it does not have should stop and ask rather than guess or hand the question sideways to a +Bot that cannot settle it either; a model with no named way to stop takes one of the two it has. + +It is offered to every run this deployment builds, whether or not that Bot has been granted anybody. +Reaching a second Bot spends a model call, may wake a computer and can fan out; asking the person +already in the conversation costs nothing and cannot be aimed anywhere they cannot see. A deployment +able to switch off the safe exit and keep the expensive one would be backwards. + +Both tools are for Bots that run here. A Bot at its own endpoint runs its own loop and is handed +descriptions of the tools it may call back for, and the callback path executes MCP refs only, so +neither `message_bot` nor `ask_person` can reach it. + +It is the Bot **doing the asking** that has to run here. Being handed work is not the same as being +able to hand it on, so the target of a grant may perfectly well live at its own endpoint. A grant +whose *grantee* is remote is refused rather than stored, so an administrator finds out at the point +of granting rather than from a Bot that never hands anything on. + +That is a real limit rather than a detail, and it is worth being plain about which Bots it leaves +out: **a Bot created through the UI is a remote one**, because creating a coworker here means +pointing it at an AG-UI endpoint. Only Bots a tenant package declares as built-in run in this +process. So on a deployment with no package, nothing can be granted `message_bot` at all, and the +screens say nothing about why. + +Who "a person" is, is a seam. This template answers the person in the conversation, which is the only +answer a template can give honestly; a company has an on-call rota or a duty desk, and that is a +route the deployment hands in rather than a channel post written into the tool. `agent.escalated` +records the question and why it needed a person; `agent.escalation_failed` records one that reached +nobody, which is the row worth finding later. + ## MCP and skills MCP servers and skills share the plugin grant table, but they have different ownership rules. diff --git a/docs/configuration.md b/docs/configuration.md index baf69eef..1593ca5d 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -193,6 +193,19 @@ where `` is `google`, `microsoft` or `okta`. `OPENBOT_APP_URL` is where the callback sends the person afterwards. It is a separate setting because the app and the API are separate addresses: locally the app is Vite on `3010` and the API is `3001`, so a relative redirect would land on the API, which serves no pages. A deployment serving both from one origin can leave it unset. +## One Bot handing work to another + +| Variable | Meaning | +| -------------------------- | ------------------------------------------------------------------------------------------- | +| `BOT_HANDOFF_MAX_DEPTH` | How many Bots deep a chain may go. `0` switches the capability off entirely. Default `1`. | +| `BOT_HANDOFF_MAX_PER_RUN` | How many other Bots one run may address. Default `3`. | + +Both refuse rather than truncate, and both are refused at start-up if they are not whole numbers of +zero or more: a deployment that typed `two` and silently got the default would believe it had set a +cap. + +Which Bots may address which is a grant, not a variable. It is made per Bot like any other grant. + ## Computer and supervisor | Variable | Meaning | diff --git a/scripts/check-new-values-keys.ts b/scripts/check-new-values-keys.ts new file mode 100644 index 00000000..678db3f3 --- /dev/null +++ b/scripts/check-new-values-keys.ts @@ -0,0 +1,285 @@ +/** + * Every values key this release adds has to survive being absent. + * + * `helm upgrade --reuse-values` takes the previous release's computed values rather than merging the + * new chart's defaults, so a key introduced by the release being installed is simply missing on every + * deployment that already exists. Reached unguarded that is a nil dereference, and because the + * helpers are included by the server deployment it fails the WHOLE render: the upgrade does not lose + * the new feature, it does not install. Emitted unguarded it writes an empty scalar, which is null, + * which Kubernetes reads as unset — a value that silently stops applying on exactly the deployments + * old enough to need it. + * + * Both shipped. `config.handoff` was found in review; `routines` was found by this script's absence, + * on a live upgrade, after the same fault had been fixed one key over. So the list of keys to check + * is not a list anybody maintains: it is whatever this release added that the last one did not. + * + * bun scripts/check-new-values-keys.ts [--since v0.0.4] + */ +import { parse } from "yaml"; + +const [valuesFile, ...rest] = process.argv.slice(2); +if (!valuesFile) { + console.error( + "Usage: bun scripts/check-new-values-keys.ts [--since ]", + ); + process.exit(2); +} +const sinceFlag = rest.indexOf("--since"); +const since = sinceFlag === -1 ? await lastReleaseTag() : rest[sinceFlag + 1]; + +/** + * What an existing deployment would already have in its stored values. + * + * The newest release whose tree actually contains the chart, because that is the oldest thing + * somebody could be upgrading FROM. The chart has not been released yet, so today that is nothing + * and this falls back to `origin/main`: a key this branch adds on top of what is already merged. + * Once the chart ships, the tag becomes the honest baseline on its own. + */ +async function lastReleaseTag(): Promise { + const tags = await run(["git", "tag", "--list", "v*", "--sort=-v:refname"]); + for (const tag of tags + .split("\n") + .map((line) => line.trim()) + .filter(Boolean)) { + const has = Bun.spawnSync( + ["git", "cat-file", "-e", `${tag}:charts/openbot/values.yaml`], + { stdout: "pipe", stderr: "pipe" }, + ); + if (has.exitCode === 0) return tag; + } + return "origin/main"; +} + +async function run(command: string[]): Promise { + const result = Bun.spawnSync(command, { stdout: "pipe", stderr: "pipe" }); + if (result.exitCode !== 0) { + throw new Error( + `${command.join(" ")} failed: ${new TextDecoder().decode(result.stderr)}`, + ); + } + return new TextDecoder().decode(result.stdout); +} + +/** Every path through a values map, as Helm's `--set` would name it. */ +function paths(value: unknown, prefix = ""): string[] { + if (value === null || typeof value !== "object" || Array.isArray(value)) { + return prefix ? [prefix] : []; + } + const here = prefix ? [prefix] : []; + return here.concat( + Object.entries(value as Record).flatMap(([key, child]) => + paths(child, prefix ? `${prefix}.${key}` : key), + ), + ); +} + +const before = new Set( + paths( + parse(await run(["git", "show", `${since}:charts/openbot/values.yaml`])), + ), +); +const now = paths(parse(await Bun.file("charts/openbot/values.yaml").text())); +/* + * A key whose parent is also new is covered by nulling the parent, and nulling both is the same + * test twice. The parent is the harsher of the two, because that is what --reuse-values actually + * leaves absent. + */ +/** + * The parent of `a.b` is `a`; a top-level key has none. + * + * `slice(0, lastIndexOf("."))` looks right and is not: `lastIndexOf` answers -1 for a dotless key, + * and `slice(0, -1)` chops the last character. `routines` became `routine`, which is in nobody's + * value file, so every NEW TOP-LEVEL KEY was filtered out of the check — precisely the case that + * caused this script to be written. + */ +function parentOf(path: string): string | null { + const cut = path.lastIndexOf("."); + return cut === -1 ? null : path.slice(0, cut); +} + +const added = now + .filter((path) => !before.has(path)) + .filter((path) => { + const parent = parentOf(path); + // A key whose parent is also new is covered by nulling the parent, which is the harsher test. + return parent === null || before.has(parent); + }); + +if (added.length === 0) { + console.log(`No values keys added since ${since}.`); +} else { + console.log(`Keys added since ${since}: ${added.join(", ")}`); +} + +/** Render, and say what came out. */ +function render(extra: string[]): { ok: boolean; out: string; err: string } { + const result = Bun.spawnSync( + [ + "helm", + "template", + "ci", + "charts/openbot", + "--values", + valuesFile, + "--set-string", + `secrets.keyEncryptionKey=${btoa("0".repeat(32))}`, + "--api-versions", + "agents.x-k8s.io/v1beta1/Sandbox", + "--api-versions", + "extensions.agents.x-k8s.io/v1beta1/SandboxTemplate", + ...extra, + ], + { stdout: "pipe", stderr: "pipe" }, + ); + return { + ok: result.exitCode === 0, + out: new TextDecoder().decode(result.stdout), + err: new TextDecoder().decode(result.stderr), + }; +} + +/** + * Keys rendered with nothing after them. + * + * An empty scalar is null, which Kubernetes reads as unset rather than as the chart's default. But + * `key:` with nothing after it is also how YAML opens a nested mapping or a block sequence, so the + * test is whether anything belongs UNDER it, not what the line looks like on its own. + */ +function emptyKeys(rendered: string): Set { + const lines = rendered.split("\n"); + const indentOf = (line: string) => line.length - line.trimStart().length; + const found = new Set(); + lines.forEach((line, index) => { + if (!/^\s*[A-Za-z][A-Za-z0-9_.-]*:\s*$/.test(line)) return; + for (let next = index + 1; next < lines.length; next += 1) { + const candidate = lines[next] ?? ""; + if (candidate.trim() === "") continue; + if (indentOf(candidate) > indentOf(line)) return; + // A block sequence may sit at the same indentation as the key it belongs to. + if ( + indentOf(candidate) === indentOf(line) && + candidate.trimStart().startsWith("- ") + ) { + return; + } + found.add(line.trim()); + return; + } + found.add(line.trim()); + }); + return found; +} + +/* + * What this chart renders empty ANYWAY, so only what a missing key causes is reported. + * + * The bundled PostgreSQL subchart emits an empty `annotations:` of its own on some targets. Flagging + * that would train whoever reads this to ignore it, which is the same as not having the check. + */ +const baseline = render([]); +if (!baseline.ok) { + console.error( + `::error::The chart does not render with ${valuesFile} at all.`, + ); + console.error(baseline.err.trim().split("\n").slice(-3).join(" ")); + process.exit(1); +} +const alreadyEmpty = emptyKeys(baseline.out); + +let bad = 0; +for (const path of added) { + const attempt = render(["--set", `${path}=null`]); + if (!attempt.ok) { + const why = attempt.err.trim().split("\n").slice(-3).join(" "); + console.error( + `::error::Rendering without ${path} failed, which is what --reuse-values does to it. ${why}`, + ); + bad += 1; + continue; + } + const caused = [...emptyKeys(attempt.out)].filter( + (key) => !alreadyEmpty.has(key), + ); + if (caused.length > 0) { + console.error( + `::error::Rendering without ${path} left a key with an empty value: ${caused[0]}`, + ); + bad += 1; + continue; + } + console.log(`renders without ${path}`); +} + +/* + * And that a value of ZERO is rendered as zero, WHETHER OR NOT ANY KEY IS NEW. + * + * This is not about upgrades, so it does not belong under the added-keys check: once `config.handoff` + * ships in a tag it stops being new, and an assertion that stopped running with it would let a + * `| default 1` come back unnoticed. + * + * `| default` substitutes on empty, and in Go templates zero IS empty, so a guard added for the + * absent case silently rewrote `maxDepth: 0` to `1` — switching a capability back on for a + * deployment that had switched it off. A nil-guard that defeats an off switch is worse than the nil + * dereference it was added for, and it renders perfectly, so nothing above would have caught it. + */ +const offSwitches: Array<{ path: string; variable: string }> = [ + { path: "config.handoff.maxDepth", variable: "BOT_HANDOFF_MAX_DEPTH" }, + { path: "config.handoff.maxPerRun", variable: "BOT_HANDOFF_MAX_PER_RUN" }, +]; + +/** The value rendered onto a named env var, or undefined if it is not there. */ +function renderedValue(out: string, variable: string): string | undefined { + const lines = out.split("\n"); + const at = lines.findIndex((line) => line.includes(`name: ${variable}`)); + if (at === -1) return undefined; + return lines[at + 1] + ?.trim() + .replace(/^value:\s*/, "") + .replace(/"/g, ""); +} + +/* + * And that the TEMPLATE's own fallback is the number values.yaml documents. + * + * Reached only on an upgrade from before the key existed, which is exactly when nobody is looking. + * Asserted here rather than in the suite that checks values.yaml against the code, because that one + * runs in a job with no Helm — and a test that shells out to a binary which is not there returns + * undefined rather than failing. + */ +const chartValues = parse( + await Bun.file("charts/openbot/values.yaml").text(), +) as { config?: { handoff?: Record } }; +const absent = render(["--set", "config.handoff=null"]); +for (const { path, variable } of offSwitches) { + const leaf = path.slice(path.lastIndexOf(".") + 1); + const documented = chartValues.config?.handoff?.[leaf]; + const got = absent.ok ? renderedValue(absent.out, variable) : undefined; + if (got !== String(documented)) { + console.error( + `::error::With config.handoff absent, ${variable} rendered ${got ?? "nothing"} but values.yaml documents ${documented}.`, + ); + bad += 1; + } else { + console.log(`${variable} falls back to ${got}, as values.yaml says`); + } +} +for (const { path, variable } of offSwitches) { + const attempt = render(["--set", `${path}=0`]); + if (!attempt.ok) { + console.error(`::error::The chart failed to render with ${path}=0.`); + bad += 1; + continue; + } + const lines = attempt.out.split("\n"); + const at = lines.findIndex((line) => line.includes(`name: ${variable}`)); + const value = at === -1 ? undefined : lines[at + 1]?.trim(); + if (value !== 'value: "0"') { + console.error( + `::error::Setting ${path}=0 rendered ${value ?? "nothing"} rather than value: "0". A zero is an off switch, not an absent value.`, + ); + bad += 1; + continue; + } + console.log(`${path}=0 stays zero`); +} +process.exit(bad === 0 ? 0 : 1); diff --git a/server/scripts/fire-routines.ts b/server/scripts/fire-routines.ts index cbfb0bb4..0cd5c8c9 100644 --- a/server/scripts/fire-routines.ts +++ b/server/scripts/fire-routines.ts @@ -17,9 +17,9 @@ import { loadConfig } from "../src/config"; import { createDatabase } from "../src/db/client"; import { createRoutineStore } from "../src/routines/store"; import { - ROUTINE_FIRE_KIND, dispatchClaimedRoutines, offerDueRoutines, + ROUTINE_FIRE_KIND, } from "../src/routines/sweep"; import { createWorkQueue } from "../src/work/queue"; diff --git a/server/src/agents/callback-token.ts b/server/src/agents/callback-token.ts index 7106a843..717d0442 100644 --- a/server/src/agents/callback-token.ts +++ b/server/src/agents/callback-token.ts @@ -78,6 +78,30 @@ export type RunAssertion = { actorId: string; /** The run itself, so a trail can tie a tool call to the answer it informed. */ runId: string; + /** + * The conversation this run belongs to. + * + * HERE RATHER THAN IN THE TOOL CALL, because a Bot handing work to another has to say where the + * answer goes, and letting the model name it would let one Bot drop a turn into a conversation it + * was never part of. This is the deployment's own statement of which thread the run is in, signed + * with the rest. + * + * Optional because an assertion minted before this existed still reads, and a run with no thread + * simply cannot hand work on. + */ + threadId?: string; + /** + * How many Bots deep this run already is. Absent means it began with a person, which is zero. + * + * IT TRAVELS HERE BECAUSE IT HAS TO CROSS A PROCESS. A Bot handing work to another Bot is A to B to + * C, three runs on up to three pods, and a counter held in a variable stops applying the moment the + * second hop lands somewhere else. That is also the moment a loop starts costing real money, so the + * cap would go quiet exactly when it was needed. This is signed by the deployment and already + * crosses every boundary the run does, which makes it the one place a Bot cannot edit it. + * + * Optional on the way in so an assertion minted before this existed still reads, and read as zero. + */ + depth?: number; }; type SignedRun = RunAssertion & { exp: number }; @@ -94,7 +118,11 @@ export function mintRunAssertion( encryptionKey: string, now: number = Date.now(), ): string { - const payload: SignedRun = { ...run, exp: now + RUN_TTL_MS }; + const payload: SignedRun = { + ...run, + depth: run.depth ?? 0, + exp: now + RUN_TTL_MS, + }; const value = Buffer.from(JSON.stringify(payload)).toString("base64url"); return sign(value, encryptionKey, RUN_LABEL); } @@ -132,6 +160,21 @@ export function readRunAssertion( botId: payload.botId, actorId: payload.actorId, runId: payload.runId, + ...(typeof payload.threadId === "string" && payload.threadId + ? { threadId: payload.threadId } + : {}), + /* + * A depth that is not a whole number at least zero is not a depth. Read as zero rather than + * refused, because the assertion's signature has already been checked: this is a field that + * predates the handoff feature being absent, not a caller lying, and the cap that consumes it + * refuses on the way out anyway. + */ + depth: + typeof payload.depth === "number" && + Number.isInteger(payload.depth) && + payload.depth >= 0 + ? payload.depth + : 0, }; } catch { return null; diff --git a/server/src/agents/escalation.ts b/server/src/agents/escalation.ts new file mode 100644 index 00000000..8a00e104 --- /dev/null +++ b/server/src/agents/escalation.ts @@ -0,0 +1,147 @@ +/** + * Asking a person, as a first-class answer. + * + * A Bot that needs judgement has three things it can do: guess, ask another Bot, or ask the person. + * Only the first two were ever offered, and a model with no named way to stop will take one of them: + * it guesses confidently, or it hands the work sideways to a Bot that cannot settle it either and + * spends a run finding that out. The caps in `handoff.ts` then become the only exit from a chain + * that should never have started. + * + * So this is a tool, sitting beside the one for handing work to another Bot and competing with it + * for the same decision. It ends the Bot's turn by putting the question to whoever this deployment + * says stands behind the work, and it says who that was, so the Bot can tell the person what it has + * done rather than falling silent. + * + * WHO "A PERSON" IS, IS A SEAM. In this template it is the person in the conversation, which is the + * only answer a template can give honestly. A company running this has a different one: an on-call + * rota, a duty desk, a queue somebody works through in the morning. That is a route this deployment + * hands in, not a channel post written into the tool. + */ + +import { z } from "zod"; +import { PUT_TO } from "../../../shared/handoff-markers"; +import { type AuditStore, recordAuditEvent } from "../audit"; +import type { GrantedTool } from "../plugins/tools"; +import type { RunAssertion } from "./callback-token"; + +/** What the model is offered. One name, so a transcript can find every escalation by searching. */ +export const ESCALATE_TOOL = "ask_person"; + +/** + * Where a question for a person goes. + * + * Returns who was reached, in words a Bot can say out loud: "the person in this conversation", "the + * on-call engineer". It is the sentence the model repeats, so it is written for the person reading + * the transcript rather than for a log. + * + * A route that cannot reach anybody should say so rather than throw. A Bot mid-run with a person + * waiting gets nothing from an exception: the run ends with nothing said, which reads as the Bot + * ignoring them. + */ +export type EscalationRoute = (input: { + actorId: string; + botId: string; + threadId?: string; + runId: string; + question: string; + why?: string; +}) => Promise<{ reached: string } | { refusal: string }>; + +/** + * The route this template ships with: the person who is already here. + * + * It sends nothing anywhere, and that is the whole point. The Bot is in a conversation with the + * person who asked; the honest thing is for it to put the question to them in its own next sentence, + * which is a thing it can already do and was not doing. What this adds is that the model now has a + * named way to choose it, and that the choice is on the record. + */ +export const askTheirOwnPerson: EscalationRoute = async () => ({ + reached: "the person in this conversation", +}); + +const parameters = z.object({ + question: z + .string() + .describe("The question you need a person to answer, in one sentence"), + why: z + .string() + .optional() + .describe( + "Why this needs a person rather than you: what you cannot settle on your own", + ), +}); + +/** + * The tool, for any run at all. + * + * NOT GATED ON A GRANT, unlike handing work to another Bot. Reaching a second Bot spends a model + * call, may wake a computer and can fan out; asking the person who is already in the conversation + * costs nothing and cannot be aimed anywhere they cannot see. Making it a grant would mean a + * deployment could switch off the safe exit and leave the expensive one, which is backwards. + */ +export function escalationTool(options: { + /** The run doing the asking, as this deployment signed it. */ + from: RunAssertion; + route: EscalationRoute; + auditStore?: AuditStore; +}): GrantedTool { + const { from, route, auditStore } = options; + + return { + name: ESCALATE_TOOL, + ref: `bot/${ESCALATE_TOOL}`, + description: + "Put a question to a person when the work needs judgement you do not have: a decision only " + + "they can make, a fact only they know, permission you do not hold. Prefer this to guessing, " + + "and prefer it to asking another Bot when no other Bot could settle it either. Say what you " + + "need and why, then stop and wait for their answer.", + parameters, + execute: async (args: unknown) => { + const parsed = parameters.safeParse(args); + if (!parsed.success) { + return "That was not put to anybody: say what you need a person to answer."; + } + + const outcome = await route({ + actorId: from.actorId, + botId: from.botId, + ...(from.threadId ? { threadId: from.threadId } : {}), + runId: from.runId, + question: parsed.data.question, + ...(parsed.data.why ? { why: parsed.data.why } : {}), + }); + + /* + * Recorded either way. An escalation that could not be delivered is the one worth finding + * later: the Bot stopped, the person was never asked, and without a row nothing says so. + */ + if (auditStore) { + await recordAuditEvent(auditStore, { + eventType: + "reached" in outcome + ? "agent.escalated" + : "agent.escalation_failed", + targetType: "agent", + targetId: from.botId, + ...(from.actorId ? { actorUserId: from.actorId } : {}), + payload: { + bot: from.botId, + run: from.runId, + question: parsed.data.question, + ...(parsed.data.why ? { why: parsed.data.why } : {}), + ...("reached" in outcome + ? { reached: outcome.reached } + : { reason: outcome.refusal }), + }, + }); + } + + return "reached" in outcome + ? `${PUT_TO}${outcome.reached}. Ask it in your own words now, plainly, and stop there: do not answer it yourself and do not hand it to another Bot.` + : outcome.refusal; + }, + }; +} + +/** Re-exported so callers of this module do not need to know where it is declared. */ +export { PUT_TO }; diff --git a/server/src/agents/handoff-delivery.ts b/server/src/agents/handoff-delivery.ts new file mode 100644 index 00000000..fdf07502 --- /dev/null +++ b/server/src/agents/handoff-delivery.ts @@ -0,0 +1,462 @@ +/** + * Running the Bot that was addressed, and letting its answer land in the conversation. + * + * The delivery half of a hop. `handoff-runner.ts` decides which hop and holds the lease; this knows + * how to turn one into a turn. + * + * THROUGH THE PLATFORM'S OWN RUNNER, not by calling the agent and writing the result somewhere. The + * runner is what persists a turn to a thread, so an answer delivered this way is the same kind of + * object as one a person's run produced: it appears in the transcript, it is in the history the next + * run reads, and it survives whichever pod produced it. Calling `agent.run` directly would produce + * an answer nothing had recorded, which is the failure nobody can debug: the first Bot says it handed + * the work over, the second says it answered, and no row anywhere agrees. + */ +import type { AbstractAgent, BaseEvent } from "@ag-ui/client"; +import type { Observable } from "rxjs"; +import type { HandoffDelivery } from "./handoff-runner"; +import { textOf } from "./message-text"; + +/** Whatever runs an agent against a thread and records what it did. */ +export type ThreadRunner = { + run: (request: { + threadId: string; + agent: AbstractAgent; + input: unknown; + /** What the conversation keeps, when that is not the whole of what the model was sent. */ + persistedInputMessages?: readonly unknown[]; + }) => Observable; +}; + +/** + * The conversation's run lock. + * + * ONE RUN AT A TIME PER CONVERSATION, taken before anything is streamed. The platform hands the lock + * out through an ordinary authenticated call and hands back the token that proves it: a run that + * skips this and starts streaming is refused, because it is claiming to be a run nobody was told + * about. That is what every delivery did before this existed, and the refusal read like a platform + * limitation rather than a missing step. + * + * Taken with `NX`, so a conversation somebody else is already running in refuses rather than queues. + * That is the right answer and the hop simply waits its turn: it is released back to the queue and + * tried again, which is a wait rather than a failure. + */ +export type ThreadLock = { + /** + * The run id the platform issued, or null when somebody else is running in this conversation. + * + * ITS OWN ID, NOT THE ONE ASKED FOR. That id is what the gateway checks every streamed event + * against, so a run that used the local one would be claiming to be a run nobody was told about. + */ + acquire: (input: { + threadId: string; + runId: string; + userId: string; + agentId: string; + }) => Promise<{ runId: string } | null>; + /** Keep it while the addressed Bot works. The lock expires on its own otherwise. */ + renew: (input: { threadId: string; runId: string }) => Promise; + /** Give it back, so the next run does not wait out the whole expiry. */ + release: (input: { threadId: string; runId: string }) => Promise; +}; + +export function createHandoffDelivery(options: { + /** + * The addressed Bot, built for the person whose conversation this is. + * + * Built per hop and for that person, because a Bot's tools are resolved against their grants: the + * second Bot runs as the same person, with its own role and its own grants, and must see what they + * may see and no more. + */ + agentFor: (input: { + actorId: string; + botId: string; + }) => Promise; + /** + * The conversation so far, so the addressed Bot is not answering out of context. + * + * PASSED THROUGH UNTOUCHED, which is why its shape is the reader's rather than named here. The + * platform holds a thread's messages in its own type and takes them back in the same one; sitting + * in the middle with a stricter type would mean inventing a conversion between two shapes that + * already agree, and a conversion is a place to lose a message. + */ + history: (input: { + threadId: string; + actorId: string; + }) => Promise; + runner: ThreadRunner; + lock: ThreadLock; + /** + * Where the addressed Bot answers: a conversation of its own with the same person. + * + * NOT THE CONVERSATION THAT ASKED, and this is a property of the platform rather than a choice. An + * Intelligence thread is owned by exactly one agent: `assertThreadAgentOwnership` refuses any other + * one, and the managed-channel path that relaxes USER ownership still enforces agent ownership. A + * second Bot answering inside the first Bot's thread is not something this platform can express + * today, whatever the caller does. + * + * So the answer lands where that Bot can speak, and the conversation that asked says where it went. + * The person gets both halves; they are two conversations rather than one, which is the honest + * shape of what actually happened. + */ + answerIn: (input: { + actorId: string; + botId: string; + }) => Promise<{ threadId: string; channelId?: string }>; + /** + * Tell the roster this conversation moved. + * + * A HOP HAS NOBODY WATCHING, which is exactly why this is needed here. A conversation's place in + * the list and the line under its name are written by the browser when somebody's own run + * finishes; a hop finishes on a server with no browser attached, so without this the answer lands + * in a conversation that still says it was last used yesterday and sits where it was. The person + * is never told, and the whole point of a hop is that they find out. + */ + announce?: (input: { + actorId: string; + channelId: string; + agentId: string; + text: string; + }) => Promise; + newRunId: () => string; + /** + * How long one delivery may take before it is given up on. + * + * A HOP MUST BE BOUNDED, because nothing else bounds it. The addressed Bot's run is an ordinary + * agent turn: a model that stops mid-stream, a tool waiting on something that never arrives, a + * browser that never loads the page. On a person's own run there is somebody watching who can + * reload the page; a hop has nobody, and an unbounded one holds the conversation's lock, holds its + * place on the queue and leaves the person waiting on an answer that is never coming, with the + * conversation it was asked in locked against them for as long as the process lives. + */ + deadlineMs?: number; +}): HandoffDelivery { + const { + agentFor, + history, + runner, + lock, + answerIn, + announce, + newRunId, + deadlineMs = DEFAULT_DELIVERY_DEADLINE_MS, + } = options; + + return { + async deliver({ work, message, shown, assertion }) { + const agent = await agentFor({ + actorId: work.actorId, + botId: work.toBotId, + }); + if (!agent) { + /* + * Thrown rather than swallowed, so the hop is released and tried again. A Bot that cannot be + * built right now is usually a Bot whose endpoint is briefly unreachable or whose row is + * mid-edit, and both of those come back. + */ + throw new Error(`${work.toBotId} could not be built for this run`); + } + + /* + * What the addressed Bot actually did, kept so a hop that failed can say so. + * + * A hop has nobody watching it. When one goes wrong the only question worth answering first is + * how far it got: a Bot that said twenty things and stopped is a stalled model, and one that + * said nothing at all never reached its model. Those are different faults with different + * fixes, and without this they are the same silence. + * + * The runner publishes events to the platform rather than through the observable it returns, + * so the count has to be taken at the agent. Patched onto the instance, which is built fresh + * for this one delivery, rather than wrapped: the runner reads the agent's own fields and + * calls its methods, and a stand-in that proxies them is a second thing to keep in step. + */ + const seen = { count: 0, last: "" }; + const runAgent = + typeof agent.runAgent === "function" + ? agent.runAgent.bind(agent) + : undefined; + if (runAgent) + (agent as { runAgent: unknown }).runAgent = ( + input: unknown, + config?: { onEvent?: (emitted: unknown) => void }, + ) => + runAgent( + input as never, + { + ...(config ?? {}), + onEvent: (emitted: { event?: { type?: unknown } }) => { + seen.count += 1; + seen.last = String(emitted?.event?.type ?? ""); + config?.onEvent?.(emitted); + }, + } as never, + ); + + /* + * The conversation this answer belongs in. + * + * Named on the hop for the one kind that goes backwards: telling the asking Bot, where the + * person is watching, that the Bot it asked never came back. Every other hop lands in the + * addressed Bot's own conversation, because a thread has exactly one agent. + */ + const where: { threadId: string; channelId?: string } = work.answerIn + ? { threadId: work.answerIn } + : await answerIn({ actorId: work.actorId, botId: work.toBotId }); + + /* + * The conversation's lock, before a single event is streamed. + * + * The platform's run id is the one it hands back, not the one asked for: it is the identity the + * gateway will check every streamed event against, so using the local one would be claiming to + * be a run that does not exist. + */ + const held = await lock.acquire({ + threadId: where.threadId, + runId: newRunId(), + userId: work.actorId, + agentId: work.toBotId, + }); + if (!held) { + /* + * Somebody else is running in this conversation. Thrown so the hop goes back on the queue + * and is tried again: a person mid-question, or the Bot that asked still finishing its own + * sentence, is a wait rather than a failure. + */ + throw new Error( + `${where.threadId} is busy with another run; the hop will be tried again`, + ); + } + + const runId = held.runId; + + /* + * THE CONVERSATION GOES ON THE AGENT, NOT IN THE RUN. + * + * `runAgent` takes `runId`, `tools`, `context` and `forwardedProps` and nothing else: AG-UI + * keeps the messages and the thread on the agent itself, and builds the run's input from them. + * A `messages` array passed as a parameter is silently ignored, which is the worst shape a + * mistake can take. Nothing failed. The addressed Bot ran, against an empty conversation, and + * answered "how can I help?" to a question it had never been shown, in a transcript that + * displayed the question directly above the answer. + */ + const asked = [ + /* + * The conversation that ASKED, not the one it is answering in. The addressed Bot is joining + * something already in progress and has to have read it; its own conversation is new and + * empty, and reading that would tell it nothing. + */ + ...conversationOnly( + await history({ threadId: work.threadId, actorId: work.actorId }), + ), + { id: `handoff-${runId}`, role: "user", content: message }, + ]; + agent.threadId = where.threadId; + // The platform's own message type rather than AG-UI's, which is what `history` returns: the + // two agree where it matters, and converting between them is a place to lose a message. + agent.setMessages(asked as Parameters[0]); + /* + * Renewed while the addressed Bot works, because the lock expires on its own. A run is minutes + * and the platform's window is short; a lock that lapses mid-answer lets a second run into the + * conversation, which is the thing it exists to prevent. + */ + const heartbeat = setInterval(() => { + void lock.renew({ threadId: where.threadId, runId }).catch(() => {}); + }, LOCK_RENEW_EVERY_MS); + + try { + await settled( + runner.run({ + threadId: where.threadId, + agent, + /* + * What the conversation KEEPS, which is not what the model was sent. + * + * The runner persists whatever it is given here, and given nothing it persists the whole + * prompt: the asking conversation's history repeated into a second conversation, and a + * paragraph of instructions to a model sitting in a bubble that looks like something the + * person typed. What belongs in a transcript is the one line saying why this Bot spoke. + */ + persistedInputMessages: shown + ? [{ id: `handoff-${runId}`, role: "user", content: shown }] + : [], + /* + * NOTHING IS PASSED FOR THE CONNECTION, and that is load-bearing. + * + * The lock hands back a join token as well as a run id, and it reads like the thing to + * present here. It is not: it is what a BROWSER presents to join a conversation and + * watch it, and the runner's socket is a different connection with its own credential. + * Handing it in overrides that credential, the socket is refused, and because the runner + * treats a socket that will not connect as something to keep retrying rather than as a + * failed run, nothing is ever emitted and nothing ever completes. The hop hangs, in + * total silence, until the deadline below ends it. + * + * What makes this run legitimate is the lock itself: the gateway compares the run id on + * every event to the one the lock holds. Taking the lock is the whole of the ceremony. + */ + input: { + threadId: where.threadId, + runId, + /* + * The same conversation the agent was given, so the run's own record of what it was + * asked agrees with what it read. + */ + messages: asked, + tools: [], + context: [], + state: {}, + /* + * The deployment's own statement of what this run is, carrying how deep the chain has + * gone. It is what stops the addressed Bot handing the work on for ever, and it is + * signed, so the Bot cannot edit its own depth on the way past. + */ + forwardedProps: { openbotRun: assertion }, + }, + }), + deadlineMs, + () => + `${work.toBotId} did not finish within ${Math.round(deadlineMs / 1000)}s ${ + seen.count === 0 + ? "and never reached its model" + : `after ${seen.count} events, the last ${seen.last}` + }`, + ); + /* + * Only once the run is on record. A conversation lifted to the top of somebody's list for an + * answer that then failed is worse than one that did not move: they open it and find + * nothing, and nothing says why. + */ + if (announce && where.channelId && shown) { + await announce({ + actorId: work.actorId, + channelId: where.channelId, + agentId: work.toBotId, + text: shown, + }).catch(() => { + // The turn happened. A roster that has not caught up is worth less than a hop reported + // as failed and run a second time. + }); + } + } finally { + clearInterval(heartbeat); + /* + * Given back whatever happened. Left held, the conversation is unusable by anybody until the + * lock expires: the person cannot ask a follow-up and the next hop is refused, which turns + * one failed delivery into a conversation that has stopped working. + */ + /* + * The conversation the lock was taken on, which is the one being answered in and NOT the one + * that asked. Releasing the asking conversation's lock instead leaves this one held until it + * lapses: the person cannot type in it and the next hop to the same Bot is refused, while a + * lock somebody else may be holding on the asking side is dropped from under them. + */ + await lock.release({ threadId: where.threadId, runId }).catch(() => {}); + } + }, + }; +} + +/** + * The conversation, as a person would read it, with the asking Bot's tool traffic left out. + * + * A THREAD'S STORED HISTORY IS NOT A VALID PROMPT ON ITS OWN. What the platform keeps is what a + * person is shown: the messages, and the results of the tools that ran. It does not keep the + * assistant message that made a tool call, so the result is stored as a `tool` message whose + * `toolCallId` matches nothing in the thread. Sent to a model as-is that is a malformed request, and + * a hop delivered it every time: the asking Bot's own call to hand the work on is always the last + * thing to have run, so the poison was in the history of every conversation that had asked. + * + * It is the right message to leave out on its own terms, too. The addressed Bot is being brought + * into a conversation, not into another Bot's workings: those calls name tools it does not have, + * carry arguments it was never meant to read, and say nothing about what the person wants. What + * carries across a hop is what was said. + */ +function conversationOnly(messages: readonly unknown[]): readonly unknown[] { + return messages.filter((message) => { + if (typeof message !== "object" || message === null) return false; + const { role, content } = message as { role?: unknown; content?: unknown }; + if (role !== "user" && role !== "assistant") return false; + // An assistant message with nothing in it is a tool call and nothing else. Keeping it would put + // back the half of the pair that has no counterpart, which is the failure being fixed. + return textOf(content).length > 0; + }); +} + +/** + * How often the conversation's lock is refreshed while a Bot is working. + * + * Comfortably inside the platform's window, because a renewal that lands after it has lapsed is not + * a renewal: the conversation is already free and something else may be in it. + */ +const LOCK_RENEW_EVERY_MS = 30_000; + +/** + * How long one hop may run for by default. + * + * Long enough for a real answer and short enough to be a wait rather than a hang. A Bot that reads a + * corpus, drives a browser and writes a paragraph is minutes, not seconds, so a tight bound would + * cut off working deliveries; but a person who has been told their question was handed on will not + * wait a quarter of an hour to be told it was not, and the conversation stays locked for every + * second of it. + */ +const DEFAULT_DELIVERY_DEADLINE_MS = 5 * 60_000; + +/** + * Wait for the run to be over, and fail if it failed. + * + * A RUN_ERROR has to reject, or the hop is finished and never retried while nothing was ever said in + * the conversation. The stream completing without one is a turn that happened, whatever the Bot + * decided to say: "I could not find that" is an answer, and asking again would spend another model + * call on the same non-answer. + */ +function settled( + events: Observable, + deadlineMs: number, + /** Written when the deadline passes, so it can say how far the run had got by then. */ + timedOut: () => string, +): Promise { + return new Promise((resolve, reject) => { + let failure: Error | undefined; + let done = false; + /* + * Declared before the subscription rather than closed over it, because an observable is entitled + * to finish inside `subscribe` itself: a stream that is already complete calls back before the + * call that started it has returned, and a `const subscription` would not exist yet. + */ + let subscription: { unsubscribe: () => void } | undefined; + /* + * Unsubscribed on the way out, not merely abandoned. The subscription is what holds the run's + * socket open, so a delivery that walked away from a stalled one would leak a connection per + * attempt and go on paying for a run nobody is reading. + */ + const finish = (settle: () => void) => { + if (done) return; + done = true; + clearTimeout(timer); + subscription?.unsubscribe(); + settle(); + }; + const timer = setTimeout(() => { + finish(() => reject(new Error(timedOut()))); + }, deadlineMs); + subscription = events.subscribe({ + next: (event) => { + // Compared as a string rather than through the enum: `@ag-ui/client` re-exports the types + // this file needs and not that value, and adding a second AG-UI package for one constant + // would be a dependency to keep in step for no gain. + if (event.type === "RUN_ERROR") { + failure = new Error( + (event as { message?: string }).message ?? + "the run ended in an error", + ); + } + }, + error: (error: unknown) => + finish(() => + reject(error instanceof Error ? error : new Error(String(error))), + ), + complete: () => finish(() => (failure ? reject(failure) : resolve())), + }); + // The stream that finished inside `subscribe`: `finish` had nothing to unsubscribe from at the + // time, and the subscription it could not reach is this one. + if (done) subscription.unsubscribe(); + }); +} diff --git a/server/src/agents/handoff-runner.ts b/server/src/agents/handoff-runner.ts new file mode 100644 index 00000000..8d57fec3 --- /dev/null +++ b/server/src/agents/handoff-runner.ts @@ -0,0 +1,495 @@ +/** + * Delivering a hop: running the Bot that was addressed, and putting its answer in the conversation. + * + * The other half of `handoff.ts`. Deciding happens inside somebody's run and has to be quick and + * fail closed; delivering is a whole agent turn against a model, and it has to survive the pod it + * started on. So the two are separated by the queue rather than by a function call. + * + * CLAIMED, NOT ASSIGNED. Any replica may take any hop, which is what makes this work on a cluster + * where the Bot being addressed is very unlikely to be on the pod that addressed it. The lease is + * renewed for as long as the run takes, because a run is minutes and a lease that lapses mid-answer + * hands the same hop to a second replica and bills for it twice. + */ +import { type AuditStore, recordAuditEvent } from "../audit"; +import { DEFAULT_MAX_ATTEMPTS, type WorkQueue } from "../work/queue"; +import { HANDOFF_KIND } from "./handoff"; + +/** What a hop carries, as `handoff.ts` wrote it. */ +export type HandoffWork = { + fromBotId: string; + toBotId: string; + actorId: string; + threadId: string; + runId: string; + depth: number; + task: string; + constraints?: string; + expecting?: string; + /** The asking Bot's display name, for the line a person reads. Absent falls back to its id. */ + fromName?: string; + /** The addressed Bot's display name, for the same reason. */ + toName?: string; + /** + * Where the answer belongs, when it is not the addressed Bot's own conversation. + * + * Set on the one kind of hop that goes backwards: telling the asking Bot, in the conversation the + * person is actually watching, that the Bot it asked never answered. That conversation belongs to + * the asking Bot, which is why it can speak in it at all. + */ + answerIn?: string; +}; + +export type HandoffDelivery = { + /** + * Run the addressed Bot against the conversation, and resolve when its turn is on record. + * + * Rejecting means the hop did not happen and is worth another go. Resolving means it did, whatever + * the Bot said: a Bot that answers "I could not find that" has answered, and retrying would ask it + * the same question again and bill for the same non-answer. + */ + deliver: (input: { + work: HandoffWork; + /** The message the addressed Bot sees, already attributed by the deployment. */ + message: string; + /** + * The one line of it that belongs in the transcript, if any. + * + * TWO TEXTS, because they have two readers. The model needs the envelope: who is asking, the + * task, its constraints, what a good answer looks like, and an instruction about who to write + * for. A person scrolling their conversation with the addressed Bot needs to know why it + * suddenly said something, in one sentence. Persisting the envelope puts a paragraph of + * machine instructions in their transcript, in a bubble that looks like something they wrote. + * + * Absent means nothing is kept, which is right for a Bot going back to its own conversation to + * report a failure: what it says already explains why it spoke, and the instruction that made it + * speak is addressed to a model. + */ + shown?: string; + /** The signed statement of the run it is starting, carrying its depth. */ + assertion: string; + }) => Promise; +}; + +export type HandoffRunReport = { + delivered: string[]; + skipped: { key: string; reason: string }[]; +}; + +/** + * How often a claim is refreshed while a hop is being delivered. + * + * Comfortably inside the lease, because a renewal that lands after it has lapsed is not a renewal: + * the item has already gone to somebody else, and this one is now the second replica running it. + */ +const RENEW_EVERY_MS = 20_000; + +/** + * How long a hop that is over is kept before it is dropped. + * + * Long past the point where its key still has to stop a duplicate — that is the asking run's own + * lifetime, minutes — and short enough that this table holds about a day of work rather than all of + * it. Both the finished ones and the ones that ran out of attempts: the second is a terminal state + * somebody can query, and a day is long enough to query it in. + */ +const REAP_OLDER_THAN_MS = 24 * 60 * 60 * 1_000; + +export function createHandoffRunner(options: { + queue: WorkQueue; + delivery: HandoffDelivery; + /** Who this replica is, for the lease. */ + owner: string; + /** How the deployment signs what the addressed Bot's run is. */ + sign: (work: HandoffWork) => string; + auditStore: AuditStore; + /** How long a claim lasts before anything may take it back. */ + leaseMs?: number; + /** How many hops one sweep will take. */ + limit?: number; + /** After how many tries a hop is given up on. Told to `claim` as well as gating the notice. */ + maxAttempts?: number; + /** + * How often a claim is refreshed. Comfortably inside the lease. + * + * Injectable so the thing it protects against can be driven in a test in milliseconds rather than + * in minutes. What it protects against is a batch whose tail expires while its head is delivering, + * which is a matter of one duration outrunning another and does not care about the scale. + */ + renewEveryMs?: number; +}) { + const { + queue, + delivery, + owner, + sign, + auditStore, + leaseMs = 60_000, + limit = 5, + maxAttempts = DEFAULT_MAX_ATTEMPTS, + renewEveryMs = RENEW_EVERY_MS, + } = options; + + /** + * Put the failure in front of the person, by running the Bot that asked in the conversation they + * are watching. + * + * THROUGH THE SAME QUEUE, not by writing a line somewhere. The asking Bot is the only thing that + * can speak in that conversation, and what the person needs is a sentence in its voice saying who + * it asked and that nothing came back. A row written past the Bot would be a message from nobody. + * + * Marked with `answerIn`, which is also what stops this recursing: a notice that fails is not + * itself worth a notice, and the check above skips any hop that carries one. + */ + const tell = (work: HandoffWork, key: string, reason: string) => + queue.offer({ + kind: HANDOFF_KIND, + /* + * OUTSIDE THE RUN'S OWN PREFIX, and carrying the failed hop's key. + * + * Outside, because the fan-out cap counts every row whose key starts with `${runId}:` and a + * notice is not one of the Bots this run asked for. A hop that failed for good while the run + * was still going would otherwise spend a third of a three-Bot budget on the message saying + * so, and the run's next legitimate ask would be refused with "this turn has already asked 3 + * Bots" after asking two. + * + * Carrying the hop's key, because one run may legally ask the same Bot two different things. + * Keyed on the Bot alone both notices are the same work to `offer`, the second is dropped on + * conflict, and the person hears about one of their two lost questions with the other's + * reason — for a whole day, until `reap` drops the row that is blocking it. + */ + key: `notice:${key}`, + payload: { + fromBotId: work.toBotId, + toBotId: work.fromBotId, + actorId: work.actorId, + threadId: work.threadId, + runId: work.runId, + depth: work.depth, + answerIn: work.threadId, + task: `You asked ${work.toBotId} to help with this and it never answered: ${forThePerson(reason)}. Tell the person plainly that it did not come back, say what you had asked it for, and offer what you can do yourself.`, + } as unknown as Record, + }); + + return { + /** + * Drop hops that are over, long after they were. + * + * NOTHING ELSE REAPS THIS KIND. A finished hop is kept rather than deleted, because a key that + * is still there is what makes `offer` idempotent and stops a retried delivery running the other + * Bot twice. Kept for ever, though, the table only grows — and the fan-out cap counts rows under + * a run's prefix with a `LIKE` that no index serves, so every offer pays for every hop the + * deployment has ever made. + * + * The window is what keeps both true at once. Idempotency only has to hold while the asking run + * could still offer the same hop again, which is minutes; a day is far past that and still short + * enough that the table reflects roughly a day's work. + */ + async reap(): Promise { + return queue.purge({ + kind: HANDOFF_KIND, + olderThanMs: REAP_OLDER_THAN_MS, + maxAttempts, + }); + }, + + /** Deliver whatever this replica can claim. */ + async sweep(): Promise { + const claimed = await queue.claim({ + kind: HANDOFF_KIND, + owner, + leaseMs, + limit, + /* + * The same ceiling the notice below is gated on, because two different ceilings is two + * different ideas of when a hop is over. + * + * `claim` stops serving a row at its own cutoff. Set higher here than there and the row is + * never handed out again, `attempts` never reaches this number, and the notice that exists + * to stop a person waiting for ever is never sent: the silent stop, arriving through the + * feature built to prevent it. Set lower and the person is told it failed for good while + * the queue keeps handing it out, so the Bot may answer after they were told it would not. + */ + maxAttempts, + }); + const report: HandoffRunReport = { delivered: [], skipped: [] }; + + /* + * EVERY CLAIMED HOP IS RENEWED, not just the one being delivered. + * + * A claim leases the whole batch from one moment and this loop delivers them one at a time, so + * a heartbeat started per item leaves the rest of the batch on a lease that is quietly running + * out while the first delivery runs. A delivery is minutes and the lease is one, so the tail of + * every batch expired, was claimed by another replica, and was delivered twice: two model + * calls, two answers in the person's conversation, and both replicas reporting success. + * + * Reproduced against a real PostgreSQL with two replicas, which is the only way this shows up: + * the item in flight is fine, and the ones waiting behind it are not. + */ + const ours = new Set(claimed.map((item) => item.key)); + const heartbeat = setInterval(() => { + for (const key of ours) { + void queue + .renew({ kind: HANDOFF_KIND, key, owner, leaseMs }) + .then((kept) => { + // False means it went to somebody else. Dropped rather than renewed again, so the + // loop below knows not to spend a model call on work it no longer holds. + if (!kept) ours.delete(key); + }) + .catch(() => {}); + } + }, renewEveryMs); + + try { + for (const item of claimed) { + const work = item.payload as unknown as HandoffWork; + if (!work?.toBotId || !work.threadId) { + /* + * A hop nothing can be done with. Finished rather than released, because releasing it puts + * the same unusable row back on the queue for ever. + */ + await queue.finish({ kind: HANDOFF_KIND, key: item.key, owner }); + report.skipped.push({ key: item.key, reason: "not a hop" }); + continue; + } + + /* + * A hop that has already been tried is not a fresh one, and the difference matters here more + * than anywhere else this queue is used: a first attempt has certainly not run the other + * Bot, while a second may already have run it, spent a model call and posted an answer + * before its owner died. Recorded rather than guessed at, so somebody reading the trail can + * tell a duplicate answer from a mystery. + */ + if (item.attempts > 1) { + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_retried", + targetType: "agent", + targetId: work.toBotId, + ...(work.actorId ? { actorUserId: work.actorId } : {}), + payload: { + from: work.fromBotId, + to: work.toBotId, + run: work.runId, + attempt: item.attempts, + note: "A previous attempt may already have run this Bot.", + }, + }); + } + + /* + * How long the hop took, recorded either way. A hop is a run nobody is watching, so the + * trail is the only place its duration is visible: "delivered in 4s" and "delivered in 4m" + * are the same row otherwise, and the second is what a person waiting was actually shown. + */ + const startedAt = Date.now(); + /* + * Still ours, ASKED OF THE DATABASE, immediately before a model call rather than after it. + * + * Consulting the heartbeat's own set would only catch a renewal that had been attempted + * and refused. A process paused long enough for the lease to lapse never attempted one, so + * its set still says the hop is his, and he delivers it on top of whoever has since taken + * it. The renewal is the question and the answer at once, and it puts a fresh lease under + * the delivery about to start, which is the moment one is most needed. + * + * Running a hop that is no longer ours is the expensive half of a duplicate: a whole agent + * turn, billed, ending in a second answer in somebody's conversation. + */ + const stillOurs = await queue.renew({ + kind: HANDOFF_KIND, + key: item.key, + owner, + leaseMs, + }); + if (!stillOurs) { + ours.delete(item.key); + report.skipped.push({ + key: item.key, + reason: "the lease went elsewhere", + }); + continue; + } + + try { + const shown = summarise(work); + await delivery.deliver({ + work, + message: attribute(work), + ...(shown ? { shown } : {}), + assertion: sign(work), + }); + const kept = await queue.finish({ + kind: HANDOFF_KIND, + key: item.key, + owner, + }); + ours.delete(item.key); + /* + * `finish` answering false means the lease went elsewhere while this ran, so another + * replica may have delivered the same hop. The turn happened either way and the trail has + * to say so; what it must not say is that this replica finished the work, because it did + * not, and a person reading two similar answers would have nothing to tell a duplicate + * from a mystery. + */ + if (!kept) { + report.skipped.push({ + key: item.key, + reason: "delivered, but the lease had gone elsewhere", + }); + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_retried", + targetType: "agent", + targetId: work.toBotId, + ...(work.actorId ? { actorUserId: work.actorId } : {}), + payload: { + from: work.fromBotId, + to: work.toBotId, + run: work.runId, + attempt: item.attempts, + note: "This replica delivered a hop whose lease had already gone elsewhere. Another may have delivered it too.", + }, + }); + continue; + } + report.delivered.push(work.toBotId); + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_delivered", + targetType: "agent", + targetId: work.toBotId, + ...(work.actorId ? { actorUserId: work.actorId } : {}), + payload: { + from: work.fromBotId, + to: work.toBotId, + run: work.runId, + depth: work.depth, + ms: Date.now() - startedAt, + }, + }); + } catch (error) { + const reason = + error instanceof Error ? error.message : "could not be delivered"; + /* + * The last try, so the person is told rather than left waiting. + * + * Enqueued before the release, because the release is what makes this attempt the last + * one: after it the row will never be claimed again and nothing else will ever look at + * this hop. A person who was told their question had been handed on, and then hears + * nothing for ever, has no way to tell a slow Bot from a broken one. + */ + if (item.attempts >= maxAttempts && !work.answerIn) { + await tell(work, item.key, reason).catch((failure) => { + // A notice that cannot be queued must not take the release with it: leaving the row + // claimed would be worse than a hop nobody was told about. + console.warn( + "Could not queue the notice for a hop that failed for good.", + failure, + ); + }); + } + /* + * Released and pushed out rather than dropped. The work still wants doing, and whatever + * refused it once will probably refuse it again in the next second. + */ + await queue.release({ + kind: HANDOFF_KIND, + key: item.key, + owner, + delayMs: 60_000, + reason, + }); + ours.delete(item.key); + report.skipped.push({ key: item.key, reason }); + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_failed", + targetType: "agent", + targetId: work.toBotId, + ...(work.actorId ? { actorUserId: work.actorId } : {}), + payload: { + from: work.fromBotId, + to: work.toBotId, + run: work.runId, + attempt: item.attempts, + reason, + ms: Date.now() - startedAt, + }, + }); + } + } + } finally { + clearInterval(heartbeat); + } + + return report; + }, + }; +} + +/** + * The same failure, in words that can be said out loud. + * + * The reason on a failed hop is whatever threw, and one of the things that throws is the platform + * client, whose message is `Intelligence platform error 409: {"error":{...}}` — a response body, + * verbatim. That reason is interpolated into the notice a Bot then paraphrases to a person, so an + * internal error envelope ends up in somebody's chat. The trail keeps the whole thing; the sentence + * gets the shape of the problem. + */ +function forThePerson(reason: string): string { + const platform = reason.match(/^Intelligence platform error (\d{3})\b/); + if (platform) { + return `the platform answered ${platform[1]} (the full response is in the trail)`; + } + return reason; +} + +/** + * What the addressed Bot is shown. + * + * WHO IS ASKING IS STAMPED HERE, from the row this deployment wrote, and never taken from anything a + * model produced. A Bot able to write its own attribution is a Bot able to claim to be another one, + * and the whole point of naming the sender is that the answer can be trusted to say who wanted it. + * + * The parts stay parts. The asking model was made to name the task, its constraints and what a good + * answer looks like precisely so the receiving one does not have to infer them out of a paragraph, + * and flattening them back into prose here would throw that away at the last step. + */ +function attribute(work: HandoffWork): string { + /* + * A notice is not a request for help, and must not read as one. + * + * This one goes to the Bot that ASKED, in the conversation it is already in, and its whole content + * is what became of the hop. Dressed in the wording below it would tell a Bot that the Bot it + * asked has now asked it for something, which is the beginning of a loop rather than the end of + * one. + */ + if (work.answerIn) { + return `${work.task}\n\nSay this in your own words to the person in this conversation, in a sentence or two. Do not hand it to another Bot.`; + } + const lines = [ + `${work.fromBotId} has asked you to help with this, on behalf of the person in this conversation.`, + "", + `Task: ${work.task}`, + ]; + if (work.constraints) lines.push(`Constraints: ${work.constraints}`); + if (work.expecting) + lines.push(`What a good answer looks like: ${work.expecting}`); + lines.push( + "", + "Answer in this conversation as yourself. The person can see it, so write it for them rather than for the Bot that asked.", + ); + return lines.join("\n"); +} + +/** + * The same hop, in one line, for the person who will scroll past it. + * + * They did not send this and it is not addressed to them: their conversation with one Bot has a + * message in it because a different Bot asked for something. So it says exactly that, and leaves the + * constraints and the shape-of-answer notes out. Those are instructions to a model, and reading + * somebody else's instructions to a model is how a transcript stops being a conversation. + */ +function summarise(work: HandoffWork): string | null { + /* + * Nothing, for a Bot going back to its own conversation to say a hop failed. Its own sentence is + * the whole message; the text that prompted it is an instruction to a model, and shown here it + * appears as something the person typed and then had read back to them. + */ + if (work.answerIn) return null; + return `${work.fromName ?? work.fromBotId} asked ${work.toName ?? work.toBotId} for this on your behalf: ${work.task}`; +} diff --git a/server/src/agents/handoff-tool.ts b/server/src/agents/handoff-tool.ts new file mode 100644 index 00000000..1679a40a --- /dev/null +++ b/server/src/agents/handoff-tool.ts @@ -0,0 +1,129 @@ +/** + * The tool one Bot uses to hand work to another. + * + * Offered beside a Bot's granted tools rather than through a new transport, so which Bots may reach + * which other Bots is an ordinary grant an administrator makes. A Bot with no such grant is offered + * nothing and cannot address anybody, which is the correct default. + * + * WHAT IT TAKES IS TYPED, and that is the one place this departs from the obvious build. The natural + * shape is `message_bot(target, message)` and free text is the commonest way a multi-agent system + * goes quietly wrong: the receiving Bot infers the intent, re-derives the constraints and guesses + * what shape of answer was wanted, and when it guesses wrong it does not fail, it returns something + * else confidently. Naming the parts costs the asking model a little effort and removes most of that. + */ + +import { z } from "zod"; +import { HANDED_OVER } from "../../../shared/handoff-markers"; +import type { GrantedTool } from "../plugins/tools"; +import type { RunAssertion } from "./callback-token"; +import type { HandoffDesk } from "./handoff"; + +/** What the model is offered. One name, so a transcript can find every hop by searching for it. */ +export const HANDOFF_TOOL = "message_bot"; + +const parameters = z.object({ + bot: z + .string() + .describe( + "The name of the Bot to hand this to, as it appears in the roster", + ), + task: z + .string() + .describe("What you are asking that Bot to do, in a sentence or two"), + constraints: z + .string() + .optional() + .describe( + "Anything that bounds the work: a date range, a system to look in, a rule it must not break", + ), + expecting: z + .string() + .optional() + .describe( + "What a good answer looks like coming back: a list, a number, a recommendation with reasons", + ), +}); + +/** + * The tool, for a run that is allowed to have it. + * + * Returns nothing when this deployment has switched handoff off, so a Bot in that deployment is not + * offered a tool whose every call would be refused. A model offered a tool it may never use spends + * attention on it and tells the person it tried. + */ +export function handoffTool(options: { + desk: HandoffDesk; + /** The run doing the asking, as this deployment signed it. */ + from: RunAssertion; + /** Whether this Bot has been granted anybody at all. */ + hasSomebodyToAsk: boolean; + maxDepth: number; + /** How many Bots one run may address. Zero switches it off as surely as a depth of zero. */ + maxPerRun: number; +}): GrantedTool | null { + const { desk, from, hasSomebodyToAsk, maxDepth, maxPerRun } = options; + /* + * Both zeros mean the same thing, and both have to be checked here. + * + * A run allowed to go no Bots deep and a run allowed to address no Bots are the same deployment + * decision from two directions, and only one of them was closing the door. With a fan-out cap of + * zero the tool was still offered, every call was refused by the desk, and the model spent + * attention on it and told the person it had tried and failed, which reads as the deployment being + * broken rather than as it being switched off. + */ + if (maxDepth <= 0 || maxPerRun <= 0 || !hasSomebodyToAsk) return null; + /* + * Not offered to a run that is already as deep as this deployment allows. + * + * The desk refuses it anyway, so this is about what the model is shown rather than about the + * boundary. A Bot at the cap that can see the tool will reach for it, be told no, and often tell + * the person it tried and failed, which reads as the deployment being broken rather than as it + * working. + */ + if ((from.depth ?? 0) >= maxDepth) return null; + + return { + name: HANDOFF_TOOL, + ref: `bot/${HANDOFF_TOOL}`, + description: + "Hand a piece of work to another Bot in this workspace and let it answer for itself. " + + "Use this when the work needs a role you do not have. The other Bot answers in its own " + + "conversation with this person, so do not wait for it or repeat what it will say: tell them " + + "who you have asked and what for. If the work is yours to do, do it, and if it needs a " + + "person's judgement rather than another Bot's, ask the person instead.", + parameters, + execute: async (args: unknown) => { + const parsed = parameters.safeParse(args); + if (!parsed.success) { + return "That handoff was not sent: name the Bot and say what you are asking it to do."; + } + const outcome = await desk.send({ + from, + target: parsed.data.bot, + envelope: { + task: parsed.data.task, + ...(parsed.data.constraints + ? { constraints: parsed.data.constraints } + : {}), + ...(parsed.data.expecting + ? { expecting: parsed.data.expecting } + : {}), + }, + }); + + /* + * A refusal comes back as a sentence, not an exception. + * + * The asking Bot is mid-run with a person waiting. A throw ends the run with nothing said, + * which reads to the person as the Bot ignoring them; the refusal is in the audit trail either + * way, and the model is owed something it can say out loud. + */ + return outcome.ok + ? `${HANDED_OVER}${outcome.toName}. It will answer in its own conversation with this person, so tell them you have asked it and what for, and do not answer on its behalf.` + : outcome.refusal; + }, + }; +} + +/** Re-exported so callers of this module do not need to know where it is declared. */ +export { HANDED_OVER }; diff --git a/server/src/agents/handoff.ts b/server/src/agents/handoff.ts new file mode 100644 index 00000000..ccb1c75e --- /dev/null +++ b/server/src/agents/handoff.ts @@ -0,0 +1,399 @@ +/** + * One Bot handing work to another. + * + * A person can put several Bots in a channel and address them with `@`. What they could not do is + * let one Bot bring in another: every hop went through a person, who read the answer, decided who + * should see it next, and pasted it across. + * + * THIS IS THE PART THAT DECIDES, not the part that delivers. It resolves who is being addressed, + * refuses when it should, writes the row that says what happened, and puts a durable hop on the + * queue. What claims that hop and runs the other Bot is `handoff-runner.ts`, and the split is + * deliberate: deciding happens inside somebody's run and must be fast and fail closed, while + * delivering is a whole agent turn that has to survive the pod it started on. + * + * EVERY REFUSAL IS AN ANSWER, NOT AN ERROR. The asking Bot is mid-run with a person waiting, so a + * refusal comes back as a sentence it can say. A thrown error ends the run with nothing said, which + * reads to the person as the Bot ignoring them. + */ +import { createHash } from "node:crypto"; +import { type AuditStore, recordAuditEvent } from "../audit"; +import type { WorkQueue } from "../work/queue"; +import type { RunAssertion } from "./callback-token"; +import type { AgentProfileStore } from "./profile-store"; +import type { AgentActor } from "./profile-types"; + +/** The kind of work a hop is, on the shared queue. */ +export const HANDOFF_KIND = "bot.message"; + +/** The kind of grant that lets one Bot address another. */ +export const HANDOFF_GRANT = "bot"; + +/** + * What one Bot sends another. + * + * TYPED FIELDS, NOT A PARAGRAPH, and this is the one decision here taken against the obvious build. + * The natural shape is `message_bot(target, message)` and it is what the issue proposed. Free text is + * the commonest way a multi-agent system goes quietly wrong: the receiving Bot has to infer the + * intent, re-derive the constraints and guess what shape of answer was wanted, and when it guesses + * wrong it does not fail, it confidently returns something else. Naming the parts costs the asking + * model a little more effort and removes most of that. + */ +export type HandoffEnvelope = { + /** What the other Bot is being asked to do. */ + task: string; + /** Anything that bounds it: a date range, a system, a rule it must not break. */ + constraints?: string; + /** What good looks like coming back: a list, a number, a recommendation with reasons. */ + expecting?: string; +}; + +/** How far this may go, in numbers a deployment chooses rather than constants. */ +export type HandoffCaps = { + /** How many Bots deep a chain may go. Zero means one Bot may never address another. */ + maxDepth: number; + /** How many other Bots one run may address. */ + maxPerRun: number; +}; + +export type HandoffOutcome = + | { ok: true; to: string; toName: string } + | { ok: false; refusal: string }; + +export type HandoffDesk = { + send: (input: { + /** + * The run doing the asking, as this deployment signed it. + * + * Where the answer goes comes from here too. A Bot naming its own thread would be a Bot able to + * drop a turn into a conversation it was never part of. + */ + from: RunAssertion; + /** The Bot being addressed, as the model named it. */ + target: string; + envelope: HandoffEnvelope; + }) => Promise; +}; + +export function createHandoffDesk(options: { + queue: WorkQueue; + profiles: AgentProfileStore; + /** Whether the asking Bot has been granted the Bot it is addressing. Read per hop, never cached. */ + mayAddress: (fromBotId: string, toBotId: string) => Promise; + /** + * Who the person is, as the roster is decided for them. Null when that cannot be established. + * + * A seam rather than a hardcoded `role: "user"`, because an administrator sees Bots a user does + * not: assumed, an administrator's hop to a Bot they can see and chat with was refused as "no + * such Bot". Resolved per hop, so a role granted or taken away a minute ago counts. + * + * NULL RATHER THAN A THROW, because everything in this module answers with a sentence. A role + * revoked mid-run, or a database that blinked, would otherwise end the run with nothing said at + * all — the failure the file's own opening paragraph is about, arriving through a seam added to + * fix something else. `mayAddress` beside it catches for exactly this reason. + */ + actorFor: (userId: string) => Promise; + auditStore: AuditStore; + caps: HandoffCaps; +}): HandoffDesk { + const { queue, profiles, mayAddress, actorFor, auditStore, caps } = options; + + /** Said once, so the trail carries the same words the Bot was given. */ + async function refuse( + from: RunAssertion, + target: string, + reason: string, + refusal: string, + ): Promise { + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_refused", + targetType: "agent", + targetId: from.botId, + ...(from.actorId ? { actorUserId: from.actorId } : {}), + payload: { + from: from.botId, + // As the model named it, capped: untrusted input, kept because "who did it reach for" is the + // useful half of the question. + target: target.slice(0, 120), + run: from.runId, + depth: from.depth ?? 0, + reason, + }, + }); + return { ok: false, refusal }; + } + + return { + async send({ from, target, envelope }) { + const task = envelope.task?.trim() ?? ""; + if (!task) { + return refuse( + from, + target, + "no_task", + "Nothing was sent: a handoff has to say what the other Bot is being asked to do.", + ); + } + + if (!from.threadId) { + return refuse( + from, + target, + "no_thread", + "This run is not in a conversation, so there is nowhere for another Bot's answer to land.", + ); + } + + /* + * The depth cap first, because it is the one that stops a loop. + * + * A asks B asks C asks A is the obvious failure and it spends real money going round. The count + * arrives in the signed assertion, so it is the deployment's number rather than anything the + * model can edit, and it is already correct on whichever pod this run landed on. + */ + const depth = from.depth ?? 0; + if (depth >= caps.maxDepth) { + return refuse( + from, + target, + "depth_cap", + caps.maxDepth === 0 + ? "This deployment does not let one Bot hand work to another." + : `This is already ${depth} ${depth === 1 ? "Bot" : "Bots"} deep, which is as far as this deployment allows. Answer with what you have, or ask the person.`, + ); + } + + /* + * The fan-out cap is enforced by the offer below rather than checked here. + * + * Checking first and offering second is a cap that holds only while nothing else is offering, + * and the case it has to hold in is precisely the opposite one: a model asked to do several + * things emits several tool calls in one turn, they run at once, and each reads a count taken + * before any of the others had written. Five calls passed a cap of three, every time, on a + * single pod. So the count and the write are one step, in the queue. See `atMost`. + */ + + /* + * Resolved against the roster the ASKING PERSON may see, never taken from the model. + * + * A Bot must not be able to reach a Bot its person cannot, or this becomes a way around agent + * visibility: the model would name anything and the deployment would go and find it. + * + * THE ROLE IS ASKED FOR, NOT ASSUMED. Which coworkers exist is decided per person, and an + * administrator sees Bots a user does not. Hardcoded to `user`, an administrator's own hop to + * a Bot they can see and chat with in the UI was refused as "no such Bot" — the same failure + * `index.ts` warns about for a routine's owner, one file over. + */ + const actor = await actorFor(from.actorId); + if (!actor) { + return refuse( + from, + target, + "no_actor", + "Who you are asking on behalf of could not be confirmed just now, so this was not sent. Try again, or ask the person.", + ); + } + const roster = await profiles.list(actor); + const wanted = target.trim().toLowerCase(); + /* + * An id is exact and a name is not, so an id wins outright. + * + * `agents.name` has no unique constraint and duplicating a Bot deliberately makes a second one + * with the same name, so a person can be looking at two Bots called Knowledge. Taking whichever + * sorted first would send the work to a Bot nobody meant — and the grant check runs after this, + * so with only the other twin granted a perfectly legitimate hop is refused as "not granted". + * Neither failure says a word about there having been two. + */ + const byId = roster.find( + (candidate) => candidate.id.toLowerCase() === wanted, + ); + const byName = roster.filter( + (candidate) => candidate.name.toLowerCase() === wanted, + ); + const reachable = byName.filter( + (candidate) => !candidate.hidden && candidate.deletedAt === null, + ); + if (!byId && reachable.length > 1) { + /* + * Named rather than guessed at. The ids are the escape hatch this refusal is pointing at, + * and they are all Bots this person can already see, so naming them tells the model nothing + * the roster did not. + */ + return refuse( + from, + target, + "ambiguous_bot", + `More than one Bot is called "${target.trim().slice(0, 60)}": ${reachable + .map((candidate) => candidate.id) + .join(", ")}. Ask again using the one you mean.`, + ); + } + // `reachable`, not `byName`: the same list the ambiguity check one line above counted. The + // roster already filters hidden and deleted today, so these agree — but a fallback that could + // disagree with the check guarding it is one refactor away from being wrong. + const found = byId ?? reachable[0]; + + /* + * The same answer whether it does not exist or is not theirs to see. + * + * Two different sentences here would let a Bot enumerate the deployment's roster by asking for + * names and reading which refusal came back. + */ + if (!found || found.hidden || found.deletedAt !== null) { + return refuse( + from, + target, + "no_such_bot", + `There is no Bot called "${target.trim().slice(0, 60)}" that you can reach.`, + ); + } + + if (found.id === from.botId) { + return refuse( + from, + target, + "self", + "A Bot cannot hand work to itself. Do it, or ask the person.", + ); + } + + // Read per hop and never held, so revoking a grant applies to the next hop rather than after a + // restart. + if (!(await mayAddress(from.botId, found.id))) { + return refuse( + from, + target, + "not_granted", + `You have not been given ${found.name} to hand work to. An administrator grants that.`, + ); + } + + /* + * The key is what stops this happening twice. + * + * `offer` is idempotent on it, and that is the only thing between a retried delivery and a + * second run of the receiving Bot. So it is derived from the run and the contents of the + * envelope rather than from a fresh id: the same request, sent twice in one run, is one hop. + * That is the honest reading of a model repeating itself, and the alternative is at-least-once + * with no ceiling. + * + * THE RUN IS HASHED, NOT INTERPOLATED, because `runId` arrives on the request and is a plain + * string this deployment never constrains. Written in raw it decides both halves of the key: + * a run calling itself `notice` gave the fan-out prefix `notice:`, which is what every failure + * notice in the deployment is keyed under, so one turn's budget of three was spent by other + * people's dead hops. Hashing removes every character a caller chooses from the prefix while + * keeping it stable for the run, which is all the cap needs. + */ + const runPrefix = `hop:${createHash("sha256") + .update(`${from.actorId}\u0000${from.runId}`) + .digest("hex") + .slice(0, 32)}:`; + const key = `${runPrefix}${createHash("sha256") + .update( + JSON.stringify([ + found.id, + task, + envelope.constraints ?? "", + envelope.expecting ?? "", + ]), + ) + .digest("hex") + .slice(0, 32)}`; + + const offered = await queue.offer({ + kind: HANDOFF_KIND, + key, + /* + * Counted from the rows rather than from a variable, because a run whose hops land on + * several pods is exactly what this exists to bound: every hop this run has offered is a row + * under its own prefix, so the rows are the count. + */ + atMost: { keyPrefix: runPrefix, max: caps.maxPerRun }, + payload: { + fromBotId: from.botId, + toBotId: found.id, + actorId: from.actorId, + threadId: from.threadId, + runId: from.runId, + /* + * One deeper than the run that asked. The receiving Bot's own assertion is minted from + * this, so the cap keeps counting across every pod the chain touches. + */ + depth: depth + 1, + /* + * The asking Bot's display name, resolved here against the same roster the target was. + * + * The delivery writes one line of this into the addressed Bot's conversation, and a person + * reading it should see "General Assistant" rather than `general-assistant`. Resolved on + * this side because this is the side holding the roster; the delivery runs minutes later + * on another replica and would have to fetch it again. + */ + ...(roster.find((profile) => profile.id === from.botId)?.name + ? { + fromName: roster.find((profile) => profile.id === from.botId) + ?.name, + } + : {}), + toName: found.name, + task, + ...(envelope.constraints + ? { constraints: envelope.constraints } + : {}), + ...(envelope.expecting ? { expecting: envelope.expecting } : {}), + }, + }); + + if (offered === "refused") { + return refuse( + from, + target, + "fanout_cap", + `This turn has already asked ${caps.maxPerRun} ${caps.maxPerRun === 1 ? "Bot" : "Bots"}, which is as many as this deployment allows. Answer with what you have, or ask the person.`, + ); + } + + /* + * The same ask again, which is not a second ask. + * + * `offer` is idempotent on the key, so a model repeating itself inside one run leaves one hop + * — which is the intent. What must not happen is telling it "handed over" a second time: the + * row it names may already have been delivered and finished, in which case nothing is queued + * and nobody is going to run it, and the Bot has just promised the person an answer twice. Said + * plainly instead, and not audited as a new hop, because it is not one. + */ + if (offered === "already") { + /* + * Recorded like every other refusal, and worded without claiming when. + * + * "In this turn" was a guess: the key is per run, and the run id arrives on the request, so + * a caller reusing one makes that sentence false. What is certainly true is that this exact + * ask already exists — it may be queued, it may have been delivered and finished. Either + * way it is not a new hop and saying "handed over" would promise a second answer. + */ + return refuse( + from, + target, + "duplicate", + `You have already asked ${found.name} exactly this. Wait for that answer rather than asking again.`, + ); + } + + await recordAuditEvent(auditStore, { + eventType: "agent.handoff_offered", + targetType: "agent", + targetId: found.id, + ...(from.actorId ? { actorUserId: from.actorId } : {}), + payload: { + from: from.botId, + to: found.id, + run: from.runId, + depth: depth + 1, + // What was asked, so the trail says what one Bot sent another rather than merely that it + // did. The task is the Bot's own words about the work, not a person's private content. + task: task.slice(0, 500), + }, + }); + + return { ok: true, to: found.id, toName: found.name }; + }, + }; +} diff --git a/server/src/agents/message-text.ts b/server/src/agents/message-text.ts new file mode 100644 index 00000000..1c7c4acc --- /dev/null +++ b/server/src/agents/message-text.ts @@ -0,0 +1,31 @@ +/** + * What a message says, whichever shape it says it in. + * + * A MESSAGE IS NOT ALWAYS A STRING. AG-UI's user message takes `string | InputContent[]`, and the + * platform types a thread message's content as unknown "structured AG-UI content". Nothing in this + * deployment writes an array yet, which is exactly why a `typeof content === "string"` test looks + * complete: the day attachments ship, every message carrying one silently stops counting wherever + * that test is made. + * + * Two places were making it — the tool selector, reading the message it is choosing tools for, and a + * hop, deciding what of a conversation to carry across. They are the same question and are answered + * here once. + * + * Only text is taken. A part this does not understand contributes nothing rather than being guessed + * at, and is dropped before joining so an image between two sentences does not leave a double space + * in the middle of the one thing the caller reads. + */ +export function textOf(content: unknown): string { + if (typeof content === "string") return content.trim(); + if (!Array.isArray(content)) return ""; + return content + .map((part) => { + if (typeof part === "string") return part; + if (typeof part !== "object" || part === null) return ""; + const { text } = part as { text?: unknown }; + return typeof text === "string" ? text : ""; + }) + .filter((part) => part !== "") + .join(" ") + .trim(); +} diff --git a/server/src/app.ts b/server/src/app.ts index 5a13148c..c802dcff 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -30,9 +30,9 @@ import type { SandboxedStore } from "./components/sandboxed"; import { createSandboxedRoutes } from "./components/sandboxed-routes"; import type { ComponentStore } from "./components/store"; import type { ComputerGateway } from "./computer/gateway"; +import type { PageFrameStore } from "./computer/page-frames"; import type { PolicyStore } from "./computer/policy-store"; import { createComputerRoutes } from "./computer/routes"; -import type { PageFrameStore } from "./computer/page-frames"; import { configuredAuthProviders, type DeploymentConfig } from "./config"; import type { CredentialAdminService, CredentialInput } from "./credentials"; import { createIntelligenceClient } from "./intelligence-client"; @@ -40,10 +40,10 @@ import type { PeopleStore } from "./people/store"; import { createPluginRoutes } from "./plugins/routes"; import type { PluginStore } from "./plugins/store"; import { REFUSAL_MARKER } from "./plugins/tools"; +import { createRoutineRoutes, type RoutineStore } from "./routines/routes"; +import type { RoutineRunner } from "./routines/runner"; import type { IntentRouter } from "./routing/classify"; import { createRoutingRoutes } from "./routing/routes"; -import type { RoutineRunner } from "./routines/runner"; -import { createRoutineRoutes, type RoutineStore } from "./routines/routes"; import type { PackageStatusReader } from "./tenant-package"; /** diff --git a/server/src/audit.ts b/server/src/audit.ts index c69fdc77..c92f89fd 100644 --- a/server/src/audit.ts +++ b/server/src/audit.ts @@ -337,6 +337,44 @@ export const auditEventTypes = [ "bot.deleted", "bot.callback_token_issued", "bot.callback_token_revoked", + + /* + * One Bot handing work to another. + * + * BOTH OUTCOMES, and the refused one is the more important of the pair. A hop that happened is + * visible in the transcript anyway; a hop that was refused is invisible everywhere else, and + * "why did this Bot not ask the specialist" is a question somebody asks about an answer that came + * back thin. The refusal row names which cap or which missing grant stopped it. + * + * `agent.handoff_offered` is written when the hop is accepted and made durable, not when the other + * Bot answers. The two are minutes apart on a busy cluster, and a trail that only recorded + * completion would be silent about work that was accepted and then lost. + */ + "agent.handoff_offered", + "agent.handoff_refused", + /* + * And what became of one, which is a different question from whether it was accepted. + * + * `delivered` is the other Bot's turn being on record. `failed` is a hop that will be tried again. + * `retried` is the one worth its own name: a hop on its second attempt may already have run that + * Bot, spent a model call and posted an answer before its owner died, so a person looking at two + * similar answers can tell a duplicate from a mystery. + */ + "agent.handoff_delivered", + "agent.handoff_failed", + "agent.handoff_retried", + /* + * A Bot asking a person instead. + * + * The counterpart to the rows above, and the one that says a chain stopped on purpose. Without it + * a Bot that correctly refused to guess looks identical to one that ran out of things to try: both + * end in a sentence to the person and neither leaves a trace of the decision. + * + * `agent.escalation_failed` is a question that reached nobody. It is the row worth finding later: + * the Bot stopped, the person was never asked, and nothing else anywhere says so. + */ + "agent.escalated", + "agent.escalation_failed", /* * A worker's bearer secret did not check out at `/internal/routines/run`, and every routine this * deployment has stopped firing until somebody notices. diff --git a/server/src/channels/routes.ts b/server/src/channels/routes.ts index 0d10a13a..afdc9ea2 100644 --- a/server/src/channels/routes.ts +++ b/server/src/channels/routes.ts @@ -144,8 +144,23 @@ const ROSTER_ORDER = [ desc(channels.id), ]; +/** The transaction `create` and `direct` share, as the driver hands it to a callback. */ +type ChannelTransaction = Parameters[0]>[0]; + export type ChannelStore = { create(actor: AgentActor, agentIds: string[]): Promise; + /** + * The one conversation this person has with this Bot alone, made if they have not had one yet. + * + * FOUND BEFORE IT IS MADE, because the callers that want it are called more than once for the + * same pair. A hop delivered to a Bot is retried when the delivery fails, and creating here would + * leave a fresh empty conversation behind for every attempt: the person would open the roster to + * five Knowledge channels, four of them empty, and no way to tell which one holds the answer. + * + * The one it finds is the one the person already talks to that Bot in, which is also where they + * would look for the answer. + */ + direct(actor: AgentActor, agentId: string): Promise; get(actor: AgentActor, channelId: string): Promise; list(actor: AgentActor, query?: ChannelQuery): Promise; /** Pin or unpin the caller's own membership. Throws ChannelNotFoundError for a non-member. */ @@ -201,62 +216,138 @@ export function createChannelStore( profileStore: AgentProfileStore, threadIdentity: ThreadIdentity, ): ChannelStore { - return { + /** + * Making a channel, on a transaction the caller already holds. + * + * Extracted so `direct` can find-or-create inside ONE transaction. Two of those arriving together + * for the same person and Bot each found nothing and each made a conversation, so that person had + * two Knowledge channels holding two threads, with their answers split between them. Reproduced + * against a real PostgreSQL: it needs no cluster, only two hops delivered at once, which is what a + * Bot asking for several things in one turn produces. + */ + const makeChannel = async ( + transaction: ChannelTransaction, + actor: AgentActor, + agentIds: string[], + ): Promise => { + // Validated on this transaction, not through `profileStore.get`: the read has to share + // the connection this transaction already holds, and has to hold the profile so an agent + // cannot be deleted between passing the check and being linked to the new channel. + // + // Locks are taken in agent-ID order. Two channels selecting the same pair of agents in + // opposite orders would otherwise be able to deadlock against each other. + const profilesById = new Map(); + for (const agentId of [...agentIds].sort()) { + const profile = await profileStore.getWithin(transaction, actor, agentId); + if (!profile) throw new AgentNotFoundError(agentId); + profilesById.set(agentId, profile); + } + + const id = `channel_${crypto.randomUUID()}`; + // Minted rather than a bare random id, so the thread says which deployment it belongs to + // in a project that may hold more than one. See thread-identity.ts. + const threadId = threadIdentity.mint(); + // Named from the caller's ordering, which is the order the channel presents its agents in. + const name = channelName( + agentIds.map((agentId) => { + const profile = profilesById.get(agentId); + if (!profile) throw new AgentNotFoundError(agentId); + return profile.name; + }), + ); + + await transaction.insert(channels).values({ + id, + name, + description: PRIVATE_AGENT_CHANNEL_DESCRIPTION, + }); + await transaction.insert(channelMemberships).values({ + channelId: id, + userId: actor.id, + }); + await transaction + .insert(channelAgents) + .values(agentIds.map((agentId) => ({ channelId: id, agentId }))); + await transaction.insert(intelligenceChannelMappings).values({ + userId: actor.id, + channelId: id, + threadId, + }); + + return { id, name, agentIds, threadId, active: true }; + }; + + const store: ChannelStore = { create(actor, agentIds) { return database.transaction( - async (transaction) => { - // Validated on this transaction, not through `profileStore.get`: the read has to share - // the connection this transaction already holds, and has to hold the profile so an agent - // cannot be deleted between passing the check and being linked to the new channel. - // - // Locks are taken in agent-ID order. Two channels selecting the same pair of agents in - // opposite orders would otherwise be able to deadlock against each other. - const profilesById = new Map(); - for (const agentId of [...agentIds].sort()) { - const profile = await profileStore.getWithin( - transaction, - actor, - agentId, - ); - if (!profile) throw new AgentNotFoundError(agentId); - profilesById.set(agentId, profile); - } + async (transaction) => makeChannel(transaction, actor, agentIds), + { isolationLevel: "read committed" }, + ); + }, - const id = `channel_${crypto.randomUUID()}`; - // Minted rather than a bare random id, so the thread says which deployment it belongs to - // in a project that may hold more than one. See thread-identity.ts. - const threadId = threadIdentity.mint(); - // Named from the caller's ordering, which is the order the channel presents its agents in. - const name = channelName( - agentIds.map((agentId) => { - const profile = profilesById.get(agentId); - if (!profile) throw new AgentNotFoundError(agentId); - return profile.name; - }), + async direct(actor, agentId) { + const found = await database.transaction( + async (transaction) => { + /* + * ONE AT A TIME PER PERSON AND BOT, across every replica. + * + * Looking and then making is not find-or-create: two hops delivered at the same moment + * each saw nothing and each made a conversation, and that person ended up with two + * Knowledge channels holding two threads, with the answers split between them. A Bot + * asking for several things in one turn produces exactly that, so it needs no cluster and + * no unusual timing. + * + * An advisory lock rather than a unique constraint, because what has to be unique is not a + * column: it is "this person's channel whose whole roster is this one Bot", which is a + * count over another table. The lock is held for the transaction and taken on the pair, so + * nothing else on the channel table waits behind it. + */ + await transaction.execute( + sql`select pg_advisory_xact_lock(hashtext(${`channel:direct:${actor.id}:${agentId}`}))`, ); + const [existing] = await transaction + .select({ id: channels.id }) + .from(channels) + .innerJoin( + channelMemberships, + and( + eq(channelMemberships.channelId, channels.id), + eq(channelMemberships.userId, actor.id), + ), + ) + .innerJoin( + channelAgents, + and( + eq(channelAgents.channelId, channels.id), + eq(channelAgents.agentId, agentId), + ), + ) + /* + * A channel of this person's whose whole roster is this one Bot. The count is what makes + * it "alone": a channel holding this Bot and another one would match an agent test on + * its own, and delivering into it would put the answer in front of a Bot nobody asked. + */ + .where( + and( + isNull(channels.deletedAt), + sql`(select count(*) from ${channelAgents} where ${channelAgents.channelId} = ${channels.id}) = 1`, + ), + ) + .orderBy(...ROSTER_ORDER) + .limit(1); - await transaction.insert(channels).values({ - id, - name, - description: PRIVATE_AGENT_CHANNEL_DESCRIPTION, - }); - await transaction.insert(channelMemberships).values({ - channelId: id, - userId: actor.id, - }); - await transaction - .insert(channelAgents) - .values(agentIds.map((agentId) => ({ channelId: id, agentId }))); - await transaction.insert(intelligenceChannelMappings).values({ - userId: actor.id, - channelId: id, - threadId, - }); - - return { id, name, agentIds, threadId, active: true }; + return existing + ? existing.id + : await makeChannel(transaction, actor, [agentId]); }, { isolationLevel: "read committed" }, ); + + if (typeof found !== "string") return found; + const channel = await store.get(actor, found); + // Null only if it was deleted between the two reads, which is a reason to make a new one + // rather than to fail: the caller asked for a conversation, not for that row. + return channel ?? store.create(actor, [agentId]); }, async get(actor, channelId) { @@ -666,6 +757,7 @@ export function createChannelStore( ); }, }; + return store; } export class ChannelNotFoundError extends Error { diff --git a/server/src/channels/thread-routes.ts b/server/src/channels/thread-routes.ts index 889374c3..2e5b6cde 100644 --- a/server/src/channels/thread-routes.ts +++ b/server/src/channels/thread-routes.ts @@ -1,5 +1,5 @@ -import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; import type { AppVariables } from "../auth/guards"; import type { ThreadIdentity } from "./thread-identity"; diff --git a/server/src/computer/routes.ts b/server/src/computer/routes.ts index e57cdd0c..21b24632 100644 --- a/server/src/computer/routes.ts +++ b/server/src/computer/routes.ts @@ -1,6 +1,7 @@ import type { Context, MiddlewareHandler } from "hono"; import { Hono } from "hono"; import type { BotAccessCheck } from "../agents/profile-policy"; +import type { AuditReader } from "../audit"; import type { AppVariables } from "../auth/guards"; import { requireAdmin } from "../auth/guards"; import { DEPLOYMENT_ROUTES } from "./deployment-routes"; @@ -17,9 +18,8 @@ import { WorkspaceRequestError, } from "./gateway"; import type { PageFrameStore } from "./page-frames"; -import type { AuditReader } from "../audit"; -import { type PolicyStore, parseActionPolicy } from "./policy-store"; import { dryRunAgainstHistory, REPLAYABLE_EVENT_TYPES } from "./policy-dry-run"; +import { type PolicyStore, parseActionPolicy } from "./policy-store"; /** * The Bot computer's surface, behind the same session guard as every other API route. diff --git a/server/src/config.ts b/server/src/config.ts index ba419afe..dbe95e5d 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -113,6 +113,24 @@ export type ManagedAgentConfig = { token: string; }; +/** + * How far one Bot handing work to another may go. + * + * NUMBERS A DEPLOYMENT CHOOSES, not constants. A small team and a company running this across + * departments want different answers, and neither should have to edit code to get one. + * + * Both defaults are deliberately mean. A hop costs a whole agent turn at the other end, fan-out + * shapes cost several times a single run because each Bot spends its own full budget, and on a + * cluster a hop to a Bot whose computer is asleep also pays a pod resume. One level of delegation is + * what most systems allow by default, and a deployment that wants more can say so. + */ +export type HandoffCaps = { + /** How many Bots deep a chain may go. `0` switches the whole capability off. */ + maxDepth: number; + /** How many other Bots one run may address. */ + maxPerRun: number; +}; + export type DeploymentConfig = { databaseUrl: string; keyEncryptionKey: string; @@ -215,6 +233,8 @@ export type DeploymentConfig = { * mounted and failing: a capability that is not configured should be missing, not broken. */ computer?: ComputerConfig; + /** How far one Bot handing work to another may go. */ + handoff: HandoffCaps; /** * The secret a Bot presents when it calls a tool back through this server. * @@ -239,6 +259,30 @@ export type DeploymentConfig = { type Environment = Record; +/** + * The caps, read from the environment, refusing anything that is not a whole number at least zero. + * + * Refused rather than coerced. A cap is a safety number, and a deployment that typed `two` and got + * the default would believe it had set one: the failure has to be at start-up where somebody is + * looking, not at the first loop. + */ +function handoffCaps(environment: Environment): HandoffCaps { + const read = (name: string, fallback: number): number => { + const raw = optional(environment, name); + if (raw === undefined) return fallback; + const value = Number(raw); + if (!Number.isInteger(value) || value < 0) { + throw new Error(`${name} must be a whole number of zero or more`); + } + return value; + }; + return { + // One level of delegation, which is what most systems allow before anybody asks for more. + maxDepth: read("BOT_HANDOFF_MAX_DEPTH", 1), + maxPerRun: read("BOT_HANDOFF_MAX_PER_RUN", 3), + }; +} + function required(environment: Environment, name: string): string { const value = environment[name]?.trim(); if (!value) { @@ -800,6 +844,7 @@ export function loadConfig( ? { appDistDir: optional(environment, "APP_DIST_DIR") as string } : {}), computer: computerConfig(environment), + handoff: handoffCaps(environment), ...(optional(environment, "AGENT_TOOL_TOKEN") ? { agentToolToken: optional(environment, "AGENT_TOOL_TOKEN") as string } : {}), diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 3527c69a..891c53ee 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -304,6 +304,8 @@ export async function buildAgents( * address a run finally reaches are only the same address while nobody redirects. */ agentFetch?: AgentFetch, + /** How a run gets its tool for handing work on. Absent means no Bot is offered one. */ + handoff?: HandoffForRun, ): Promise> { const vendors = await loadVendors().catch(() => [] as readonly string[]); return Object.fromEntries( @@ -321,6 +323,7 @@ export async function buildAgents( vendors, selection, agentFetch, + handoff, ), ]), ), @@ -338,6 +341,7 @@ async function buildAgent( connectedVendors: readonly string[] = [], selection?: ToolSelection, agentFetch?: AgentFetch, + handoff?: HandoffForRun, ): Promise { if (agent.type === "unavailable") { return new UnavailableAgent(agent); @@ -386,6 +390,17 @@ async function buildAgent( * `.use()` middleware is applied by `runAgent`, not by `run`, so an outer agent delegating to * `remote.run(input)` skips it: the endpoint would get a run with no standing role, no holdings * message, no tools and no signed assertion, and every one of those failures is silent. + * + * WHICH IS ALSO WHY A REMOTE BOT IS OFFERED NEITHER `message_bot` NOR `ask_person`. Both are + * executed here, by the wrapper below, against this deployment's grants and caps. A Bot at an + * endpoint runs its own loop and is handed descriptions of tools it may call back for, and the + * callback path executes MCP refs only — so a described `message_bot` would be a tool it could + * announce and never invoke. Granting one is refused at the door rather than stored dead: see + * `enablementRefusal` in plugins/routes.ts. + * + * Making this work is a feature rather than a fix: the callback would have to carry a run + * assertion the endpoint cannot forge, and execute a hop on its behalf. Worth doing; not done + * here, and worth knowing it is missing rather than assuming it is not. */ return remoteAgentWithStandingRole( agent, @@ -416,20 +431,48 @@ async function buildAgent( ); const whole = withTools(granted); - if (!narrowing) return whole; + if (!narrowing && !handoff) return whole; - return new RunSelectedAgent( + return new RunBuiltAgent( { agentId: agent.id, description: agent.name }, whole, async (input) => { - const offered = await offeredFor(input); - // Nothing narrowed means nothing to rebuild, and reusing the agent already built for this - // request keeps that path allocation-for-allocation what it was. - return offered.length === granted.length ? whole : withTools(offered); + const offered = narrowing ? await offeredFor(input) : granted; + /* + * The tool for handing work to another Bot is made per run, not per request. + * + * It has to know which run is asking: how deep the chain already is, and which conversation an + * answer belongs in. Both live on the run rather than on the request, and both have to be this + * deployment's own statement rather than anything the model can edit. A request is earlier + * than a run and knows neither. + */ + const passing = (await handoff?.(agent.id, input)) ?? []; + const tools = passing.length > 0 ? [...offered, ...passing] : offered; + // Nothing added and nothing narrowed means nothing to rebuild, and reusing the agent already + // built for this request keeps that path allocation-for-allocation what it was. + return tools.length === granted.length && passing.length === 0 + ? whole + : withTools(tools); }, ); } +/** + * The tools a run gets for reaching past itself: handing work to another Bot, and asking a person. + * + * Given the Bot and the run, because the answers depend on both: which Bots this one has been + * granted, and how deep the chain it is already part of has gone. Empty means this run reaches + * nobody, which is the right shape for a deployment with the capability switched off. + * + * The two arrive together because a model chooses between them. Offering the way to hand work + * sideways without the way to stop and ask leaves the model one exit from a decision it cannot make, + * and it takes it: it asks a Bot that cannot settle the question either. + */ +export type HandoffForRun = ( + botId: string, + input: RunAgentInput, +) => Promise; + /** * How a deployment narrows a Bot's tools to the ones a run is about. Absent means it does not. * @@ -596,7 +639,7 @@ function remoteAgentWithStandingRole( * one shared secret. */ ...(signRun - ? { openbotRun: signRun(agent.id, input.runId) } + ? { openbotRun: signRun(agent.id, input.runId, input.threadId) } : /* * Absent means this deployment cannot sign, so the agent is given nothing to hand back * and its tool calls will be refused. That is the right direction to fail: a Bot that @@ -625,7 +668,7 @@ function remoteAgentWithStandingRole( /** * An agent whose tools are decided when the run starts, because that is the first moment anybody - * knows what the run is about. + * knows what the run is about, and who is asking on whose behalf. * * WHY A WRAPPER AND NOT A NARROWER `loadTools`. Tools are resolved per request, and a request is * earlier than a run: at that point there is a Bot and a person and no message, so there is nothing @@ -638,7 +681,7 @@ function remoteAgentWithStandingRole( * The deferral is per subscription, so a retried run reselects rather than reusing a decision made * for a message that is no longer the last one. */ -class RunSelectedAgent extends AbstractAgent { +class RunBuiltAgent extends AbstractAgent { /** * The agent this run turned into, once there is one. * @@ -692,8 +735,8 @@ class RunSelectedAgent extends AbstractAgent { * (`agents[agentId].clone()`), which means the omission is not a corner case: without this, the * first message anybody sends fails on a `build` that is not a function. */ - clone(): RunSelectedAgent { - const cloned = super.clone() as RunSelectedAgent; + clone(): RunBuiltAgent { + const cloned = super.clone() as RunBuiltAgent; cloned.whole = this.whole; cloned.build = this.build; // Deliberately not the inner agent. A clone is a new run, and inheriting the last run's agent @@ -734,13 +777,32 @@ export async function resolveRuntimeAgents( loadVendors?: () => Promise, selection?: ToolSelection, agentFetch?: AgentFetch, + /** How a run gets its tool for handing work on. Absent means no Bot is offered one. */ + handoff?: HandoffForRun, + /** + * Build only this one, when the caller wants only this one. + * + * A hop delivery and a routine's turn each want a single Bot, and both were resolving the whole + * roster to reach it: every registered Bot constructed, and a granted-tools query for each, with + * all but one thrown away. On a hop that is paid again on every retry. The roster is still LOADED + * in full, because which Bots exist for this person is what decides whether the one asked for is + * theirs to see at all; what narrows is what gets built. + */ + onlyBotId?: string, ): Promise> { - const registered = await loadAgents(); - if (registered.length === 0) { + const all = await loadAgents(); + if (all.length === 0) { throw new Error( "No agents are registered. Add one to the tenant package or the agents table.", ); } + const registered = + onlyBotId === undefined + ? all + : all.filter((agent) => agent.id === onlyBotId); + // Not an error: a caller asking for a Bot this person cannot see gets an empty result and decides + // what that means, exactly as it would have from a roster that did not contain it. + if (registered.length === 0) return {}; const apiKey = registered.some((agent) => agent.type === "built_in") ? await resolveModelApiKey() @@ -756,6 +818,7 @@ export async function resolveRuntimeAgents( loadVendors, selection, agentFetch, + handoff, ); } @@ -769,7 +832,12 @@ export type LoadToolsForBot = (botId: string) => Promise; * configuration and this one never holds a secret. Shaped like `LoadToolsForBot` on purpose: both are * per-actor facts resolved once per request and asked per Bot. */ -export type SignRun = (botId: string, runId: string) => string; +export type SignRun = ( + botId: string, + runId: string, + /** Which conversation, so a Bot handing work on cannot choose where the answer lands. */ + threadId: string, +) => string; /** Who is asking. Agent visibility is decided per person, so a run has to know this first. */ export type IdentifyActor = (request: Request) => Promise; @@ -814,6 +882,13 @@ export function createRequestAgents( selectionForActor?: (actorId: string) => ToolSelection, /** The fetch remote agents are dialled with. See {@link buildAgents}. */ agentFetch?: AgentFetch, + /** + * How a run gets its tool for handing work to another Bot, resolved for whoever is asking. + * + * Per actor for the same reason the tools are: which Bots may be reached is decided against the + * roster that person can see, so a Bot must never be able to address one they cannot. + */ + handoffForActor?: (actorId: string) => HandoffForRun, ) { return async ({ request }: { request: Request }) => { const actor = await identifyActor(request); @@ -828,6 +903,7 @@ export function createRequestAgents( loadVendors, selectionForActor?.(actor.id), agentFetch, + handoffForActor?.(actor.id), ); }; } @@ -908,6 +984,14 @@ class IntelligenceKnowingANewThread extends CopilotKitIntelligence { } } +/** + * How long a conversation's lock is held before it lapses on its own. + * + * Matches the platform's own default rather than picking a number: this is renewed while a Bot works, + * so what it really sets is how long a conversation stays stuck after a process dies mid-run. + */ +const THREAD_LOCK_TTL_SECONDS = 120; + export function mountCopilotRuntime( config: DeploymentConfig, model: RuntimeModel, @@ -928,9 +1012,64 @@ export function mountCopilotRuntime( selectionForActor?: (actorId: string) => ToolSelection, /** The fetch remote agents are dialled with. See {@link buildAgents}. */ agentFetch?: AgentFetch, + /** How a run gets its tool for handing work on. Absent means no Bot is offered one. */ + handoffForActor?: (actorId: string) => HandoffForRun, ) { const { intelligence } = config.runtime; + /** + * The same Bot a person's run would get, built without a request. + * + * Handed out from here rather than assembled again elsewhere, because "built exactly the way a + * person's run builds it" is a property worth guaranteeing structurally. A hop delivering to a Bot + * assembled by parallel wiring would drift the first time one of these arguments changed, and the + * drift would be invisible: the Bot would run, and quietly hold different tools or a different + * role from the one the person talks to. + */ + const agentFor = async (input: { + /** + * The person, WITH THEIR ROLE, rather than an id this rebuilds a role for. + * + * An administrator sees Bots a user does not. Assumed to be a user here while the desk resolved + * the real role, the two disagreed in the worst direction: the desk accepted an administrator's + * hop to a Bot only they can see, the model was told it had been handed over, and then every + * delivery attempt failed to build that Bot and the person was told it never answered. A + * refusal that failed closed became a lie that failed slowly. + */ + actor: AgentActor; + botId: string; + }): Promise => { + const { actor } = input; + const agents = await resolveRuntimeAgents( + () => loadAgents(actor), + model, + resolveModelApiKey, + stallGuard, + loadToolsForActor?.(actor.id), + signRunForActor?.(actor.id), + config.computer ? COMPUTER_GUIDANCE : undefined, + loadVendors, + selectionForActor?.(actor.id), + agentFetch, + handoffForActor?.(actor.id), + // Only the Bot this hop is for. The roster is still read in full, so a Bot this person cannot + // see is still absent; what this skips is constructing the other Bots and asking the database + // what each of them was granted, on every delivery and again on every retry. + input.botId, + ); + return agents[input.botId] ?? null; + }; + + /* + * One client, used by the runtime and by anything reading a thread beside it, so a hop reads the + * history a person's run would read rather than a second view of it that could disagree. + */ + const intelligenceClient = new IntelligenceKnowingANewThread({ + apiUrl: intelligence.apiUrl, + wsUrl: intelligence.gatewayWsUrl, + apiKey: intelligence.apiKey, + }); + const runtime = new CopilotRuntime({ // `mode` is inferred from the presence of `intelligence`; passing it is a type error. // @@ -940,11 +1079,7 @@ export function mountCopilotRuntime( identifyUser, // The subclass, not the base: a thread nobody has run yet reads as empty rather than as a 500. // See IntelligenceKnowingANewThread. - intelligence: new IntelligenceKnowingANewThread({ - apiUrl: intelligence.apiUrl, - wsUrl: intelligence.gatewayWsUrl, - apiKey: intelligence.apiKey, - }), + intelligence: intelligenceClient, licenseToken: intelligence.licenseToken, // Carried on the events the runtime already sends, so OpenBot's traffic is separable from any // other deployment's. Adds no events of its own. @@ -971,8 +1106,109 @@ export function mountCopilotRuntime( loadVendors, selectionForActor, agentFetch, + handoffForActor, ) as never, }); - return createCopilotHonoHandler({ runtime, basePath }); + return { + handler: createCopilotHonoHandler({ runtime, basePath }), + /** + * How to reach the platform's runner, exactly as the runtime reaches it. + * + * TAKEN FROM THE CLIENT, NOT FROM CONFIGURATION, and this is the whole of a bug that only a real + * gateway could show. Built from `gatewayWsUrl` and the deployment's API key, every join was + * refused with `active_lock_mismatch`: a thread's active run is a lock the platform issues, and + * the token that holds it is not the API key. The runtime asks the client for both, so anything + * else driving a run has to ask the same client the same way. + */ + runnerConnection: () => ({ + url: intelligenceClient.ɵgetRunnerWsUrl(), + authToken: intelligenceClient.ɵgetRunnerAuthToken(), + }), + /** + * The conversation's run lock, as the platform issues it. + * + * ONE RUN AT A TIME PER CONVERSATION. Taken before anything is streamed, because the gateway + * checks every event against the run the lock names: a run that skips this is claiming to be one + * nobody was told about, and every event is refused. That refusal reads like a platform + * limitation and is a missing step. + * + * A conversation somebody else is already running in refuses rather than queues, which is right: + * the caller waits and tries again rather than two Bots writing over each other. + */ + threadLock: { + acquire: async (input: { + threadId: string; + runId: string; + userId: string; + agentId: string; + }) => { + try { + const held = await intelligenceClient.ɵacquireThreadLock(input); + /* + * The run id only. The lock also hands back a join token, which is what a browser presents + * to watch the conversation; the runner's socket has its own credential and passing this + * one in place of it means a socket that is refused and a run that never starts. See the + * note on `runner.run` in handoff-delivery.ts. + */ + return { runId: held.runId }; + } catch (error) { + /* + * ONLY A CONFLICT MEANS "NOT NOW". Everything else is raised. + * + * A conversation somebody is already running in answers 409, and that is ordinary: the hop + * waits and is tried again. Anything else is not — a platform that cannot be reached, a + * token that stopped working, or one of the underscored APIs below being renamed by a + * routine version bump. Returned as `null` those all read as contention: every hop retries + * to exhaustion, every person is told their question was never answered, and the only + * evidence is a warning line that looks like a busy conversation. + * + * Raised, the runner writes the real reason onto `agent.handoff_failed`, and the sentence + * the person eventually gets names it. + */ + const status = + error instanceof Error && "status" in error + ? (error as { status?: unknown }).status + : undefined; + if (status === 409) return null; + throw error; + } + }, + renew: async (input: { threadId: string; runId: string }) => { + await intelligenceClient.ɵrenewThreadLock({ + ...input, + ttlSeconds: THREAD_LOCK_TTL_SECONDS, + }); + }, + release: async (input: { threadId: string; runId: string }) => { + await intelligenceClient.ɵcleanupThreadLock(input); + }, + }, + agentFor, + /** + * A thread's messages, as the platform holds them. + * + * The same client the runtime uses, so a hop reads the history a person's run would read rather + * than a second view of it that could disagree. + */ + history: async (input: { threadId: string; actorId: string }) => { + /* + * The platform's own message type rather than AG-UI's, inferred rather than named: the two are + * compatible where it matters and naming the wrong one here would mean converting a history + * that does not need converting. + */ + type Read = Awaited< + ReturnType + >; + const read = await historyOrEmpty( + () => + intelligenceClient.getThreadMessages({ + threadId: input.threadId, + userId: input.actorId, + }), + { messages: [] } as Read, + ); + return read.messages; + }, + }; } diff --git a/server/src/index.ts b/server/src/index.ts index 15104de5..0c962d44 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -1,11 +1,17 @@ +import { randomUUID } from "node:crypto"; import { CopilotKitIntelligence, IntelligenceAgentRunner, } from "@copilotkit/runtime/v2"; import { serve } from "bun"; import { COMPUTER_GUIDANCE } from "../../shared/bot-prompt"; -import { mintRunAssertion } from "./agents/callback-token"; +import { mintRunAssertion, readRunAssertion } from "./agents/callback-token"; import { createAgentFetch } from "./agents/endpoint"; +import { askTheirOwnPerson, escalationTool } from "./agents/escalation"; +import { createHandoffDesk } from "./agents/handoff"; +import { createHandoffDelivery } from "./agents/handoff-delivery"; +import { createHandoffRunner } from "./agents/handoff-runner"; +import { handoffTool } from "./agents/handoff-tool"; import { createAgentProfileStore } from "./agents/profile-store"; import type { AgentActor } from "./agents/profile-types"; import { createRuntimeAgentLoader } from "./agents/runtime-agents"; @@ -68,6 +74,8 @@ import { loadTenantPackage, synchronizeTenantPackage, } from "./tenant-package"; +import { repeatAfterEach } from "./work/loop"; +import { createWorkQueue } from "./work/queue"; /** * Who is asking, for a CopilotKit request. @@ -315,6 +323,40 @@ const pluginStore = createPluginStore({ const routineStore = createRoutineStore(database); useRoutineTools(routineStore); +/** + * Where a Bot handing work to another gets decided. + * + * The queue is the one #216 shipped, shared with the idle-computer culler and with routines: durable + * work claimed by whichever replica gets to it, leased so a dead replica's work comes back. A hop is + * that, because the Bot being addressed will very likely run on a different pod from the Bot that + * addressed it, and a hop held in memory is lost the moment either is rescheduled. + */ +const handoffDesk = createHandoffDesk({ + queue: createWorkQueue(database), + profiles: agentProfileStore, + // Read per hop and never held, so revoking a grant applies to the next hop rather than after a + // restart. + mayAddress: async (fromBotId, toBotId) => + ( + await pluginStore + .botsReachableFrom(fromBotId) + // A grant that cannot be read is not a grant. Failing closed here costs a hop; failing open + // would let a Bot address one nobody gave it because the database blinked. + .catch(() => [] as string[]) + ).includes(toBotId), + /* + * Deferred rather than passed directly, because `actorFor` is defined further down with the rest + * of the run-building collaborators. It is only ever called during a hop, long after this module + * has finished loading. + */ + actorFor: (userId) => + // Null rather than a throw: see the seam's own note. A role that cannot be read is not a role, + // and the hop is refused with a sentence rather than ending the run in silence. + actorFor(userId).catch(() => null), + auditStore: bootAuditStore, + caps: config.handoff, +}); + void recordAuditEvent(bootAuditStore, { eventType: "computer.policy_loaded", targetType: "policy", @@ -466,8 +508,12 @@ const loadToolsForActor = (actorId: string) => (botId: string) => * from: its own token proves which agent is calling, this proves who it is calling for, and * neither is read out of the request body any more. */ -const signRunForActor = (actorId: string) => (botId: string, runId: string) => - mintRunAssertion({ botId, actorId, runId }, config.keyEncryptionKey); +const signRunForActor = + (actorId: string) => (botId: string, runId: string, threadId?: string) => + mintRunAssertion( + { botId, actorId, runId, threadId }, + config.keyEncryptionKey, + ); /* * Which vendors this deployment connects to, held by a Bot or not. @@ -603,6 +649,11 @@ const buildAgentFor = async ({ loadVendors, selectionForActor(actor.id), agentFetch, + undefined, + // Only the Bot this routine names. Same reason as the hop delivery: the roster is still read in + // full so a Bot this owner cannot see is still absent, but the other Bots are neither built nor + // asked what they hold. + agentId, ); const agent = agents[agentId]; if (!agent) { @@ -657,6 +708,281 @@ const routineRunner = createRoutineRunner({ }), }); +/** + * The runtime, and the two things beside it a hop needs. + * + * `agentFor` builds the addressed Bot exactly the way a person's run builds it, and `history` reads + * the conversation through the same client. Taken from here rather than assembled again, because a + * Bot built by parallel wiring drifts the first time one of these arguments changes, and the drift is + * invisible: it runs, and quietly holds different tools or a different role from the one the person + * is talking to. + */ +const copilotRuntime = mountCopilotRuntime( + config, + tenantPackage.model, + loadAgentsForActor, + resolveRuntimeModelApiKey, + identifyUser, + identifyActor, + stallGuard, + loadToolsForActor, + signRunForActor, + undefined, + loadVendors, + selectionForActor, + agentFetch, + /* + * What a Bot may reach past itself for: another Bot, and a person. Made per run and per person. + * + * Per person because which Bots may be reached is decided against the roster that person can + * see: a Bot must never be able to address one they cannot, or this becomes a way around agent + * visibility. Per run because the caps need to know how deep the chain already is and where an + * answer belongs, and both of those are the deployment's own statement about the run rather than + * anything the model can edit. + */ + (actorId) => async (botId, input) => { + const from = readRunAssertion( + (input.forwardedProps as { openbotRun?: unknown } | undefined) + ?.openbotRun, + config.keyEncryptionKey, + ); + const run = { + botId, + actorId, + runId: input.runId, + threadId: input.threadId, + depth: from?.depth ?? 0, + }; + /* + * The caps are checked BEFORE the grants query, not inside the tool that would discard it. + * + * `handoffTool` short-circuits on all three of these, but only after being handed a + * `hasSomebodyToAsk` that costs a query. So a deployment which switched the capability off + * still paid one grants read per run of every Bot, for a tool it was never going to be offered, + * and a run already at the cap paid it again. + */ + const couldHandOn = + config.handoff.maxDepth > 0 && + config.handoff.maxPerRun > 0 && + run.depth < config.handoff.maxDepth; + + const passing = couldHandOn + ? handoffTool({ + desk: handoffDesk, + /* + * How deep this run already is comes from the assertion the deployment signed when it handed + * this work on. A run a person started carries none, and none means zero. + * + * NOT `from.botId`. The assertion proves what this run is, and the Bot is whichever one the + * runtime is building right now: on a hop those agree, and taking the id from the signed + * value rather than from the build would let a stale assertion aim the next hop at the + * wrong Bot's grants. + */ + from: run, + // Read now rather than at boot, so a grant made a minute ago counts and one revoked a + // minute ago stops counting. + hasSomebodyToAsk: + ( + await pluginStore + .botsReachableFrom(botId) + .catch(() => [] as string[]) + ).length > 0, + maxDepth: config.handoff.maxDepth, + maxPerRun: config.handoff.maxPerRun, + }) + : null; + /* + * The way to stop and ask is offered whether or not there is a Bot to hand to. + * + * It is the cheaper of the two and the one a Bot should reach for first: asking the person who + * is already in the conversation spends nothing and cannot be aimed anywhere they cannot see. + * A deployment that offered only the expensive exit would push every unanswerable question + * sideways into another run. + */ + const asking = escalationTool({ + from: run, + route: askTheirOwnPerson, + auditStore: bootAuditStore, + }); + return passing ? [passing, asking] : [asking]; + }, +); + +/** + * Delivering hops, on every replica. + * + * A loop rather than a schedule, because a hop is somebody waiting for an answer rather than + * housekeeping: the culler's minute-granularity CronJob would be an unexplainable pause in a + * conversation. Every replica sweeps, and the queue decides which of them gets which hop, so adding a + * replica adds delivery capacity rather than contention. + * + * Only where the capability is switched on. A deployment with a depth cap of zero never has a hop to + * deliver, and a loop polling for work that cannot exist is a query a second for nothing. + */ +/* + * Both zeros switch the capability off, so both have to stop the loop. + * + * Gated on the depth alone, a deployment that set the fan-out cap to zero still swept every two + * seconds for hops that can never be offered: roughly forty thousand claim transactions per replica + * per day, for a feature it had turned off. + */ +if (config.handoff.maxDepth > 0 && config.handoff.maxPerRun > 0) { + /** + * The person a delivery acts as, with a failure a person can be told about. + * + * `actorFor` throws when a role cannot be established — a revoked role, or a database that + * blinked. Thrown from inside a delivery that message becomes the reason on a failed hop, and the + * reason is paraphrased to somebody by the Bot that asked: "A routine requires an authorized + * owner." is not a sentence to put in front of a person who asked about a refund policy. + */ + const theirActor = async (userId: string) => { + const actor = await actorFor(userId).catch(() => null); + if (!actor) { + throw new Error( + "who this is for could not be confirmed, so the answer had nowhere to go", + ); + } + return actor; + }; + + const runner = createHandoffRunner({ + queue: createWorkQueue(database), + owner: `handoff/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`, + auditStore: bootAuditStore, + /* + * The signed statement of the run the addressed Bot is about to start, carrying how deep the + * chain has gone. Minted here, where the key lives, and one deeper than the run that asked. + */ + sign: (work) => + mintRunAssertion( + { + botId: work.toBotId, + actorId: work.actorId, + runId: randomUUID(), + threadId: work.threadId, + depth: work.depth, + }, + config.keyEncryptionKey, + ), + delivery: createHandoffDelivery({ + /* + * Built as the person, WITH THEIR ROLE. The desk resolved it to decide the hop was allowed; a + * delivery that then rebuilt them as an ordinary user could not find the Bot the desk had just + * agreed to, and the person was told it never answered. + */ + agentFor: async ({ actorId, botId }) => { + const actor = await actorFor(actorId).catch(() => null); + if (!actor) { + throw new Error( + "who this is for could not be confirmed, so the Bot was not run", + ); + } + return copilotRuntime.agentFor({ actor, botId }); + }, + history: copilotRuntime.history, + lock: copilotRuntime.threadLock, + /* + * A conversation of the addressed Bot's own, with the same person. + * + * An Intelligence thread has exactly one agent, so a second Bot cannot answer inside the first + * Bot's conversation however it asks. Rather than pretend otherwise, the answer lands where + * that Bot can speak and the conversation that asked says where it went. + */ + answerIn: async (input) => { + // The conversation this person already has with that Bot, made only if they have not had + // one. See ChannelStore.direct: a hop is retried, and creating here left an empty channel + // behind for every attempt. + // The person's own role, for the same reason the desk resolves it: an administrator sees Bots + // a user does not, and a conversation with one of those is still theirs. + const channel = await channelStore.direct( + await theirActor(input.actorId), + input.botId, + ); + return { threadId: channel.threadId, channelId: channel.id }; + }, + // The roster is written by whoever finished a run, and for a hop that is this server rather + // than a browser. See ChannelStore.recordActivity. + announce: async (input) => + channelStore.recordActivity( + await theirActor(input.actorId), + input.channelId, + { text: input.text, agentId: input.agentId, at: new Date() }, + ), + newRunId: () => randomUUID(), + // The same address and the same token the runtime uses. Assembling either from configuration + // produced a runner every join was refused for, because the thread's active run is a lock the + // platform issues rather than something an API key can claim. + runner: new IntelligenceAgentRunner( + copilotRuntime.runnerConnection(), + ) as never, + }), + }); + + const sweep = async () => { + try { + const report = await runner.sweep(); + if (report.delivered.length > 0 || report.skipped.length > 0) { + console.info(JSON.stringify({ type: "bot-handoff", ...report })); + } + } catch (error) { + // A sweep that failed must not take the loop with it: the next one may find the database back. + console.warn( + "[handoff] a sweep could not run:", + error instanceof Error ? error.message : error, + ); + } + }; + + /* + * ONE SWEEP AT A TIME ON THIS REPLICA. See repeatAfterEach: an interval would start another sweep + * every two seconds while a five-minute delivery runs, each claiming a different batch, and this + * replica's concurrent agent runs would grow with the backlog rather than stopping at the limit + * it was asked for. + */ + repeatAfterEach(sweep, 2_000); +} + +/* + * And dropping the hops that are over, whether or not the capability is switched on. + * + * OUTSIDE THE GATE ABOVE, deliberately. A deployment that switches handing work off still has + * whatever it made while it was on, and rows that stop being reaped are rows that stay at the head + * of the queue: switched back on a month later, the first thing that happens is a month-old question + * being delivered to somebody who has long since stopped waiting. Reaping is housekeeping about the + * past rather than part of the feature. + * + * Every replica reaps; the statement is a delete by age, so two doing it is the same as one doing it. + * Its own loop rather than a phase of the sweep, so an hour of failing to reap never delays an answer. + */ +const reaper = createHandoffRunner({ + queue: createWorkQueue(database), + owner: `reaper/${process.env.HOSTNAME ?? randomUUID().slice(0, 8)}`, + sign: () => "", + auditStore: bootAuditStore, + // Never called: `reap` deletes rows by age and claims nothing. + delivery: { + deliver: async () => { + throw new Error("the reaper does not deliver hops"); + }, + }, +}); +repeatAfterEach( + async () => { + try { + const purged = await reaper.reap(); + if (purged > 0) { + console.info(JSON.stringify({ type: "bot-handoff-reaped", purged })); + } + } catch (error) { + console.warn( + "[handoff] hops that are over could not be dropped:", + error instanceof Error ? error.message : error, + ); + } + }, + 60 * 60 * 1_000, +); + const app = createApp( config, auth, @@ -670,21 +996,7 @@ const app = createApp( createPackageStatusReader(database), // The runtime call: the model, per-actor agent loading, and the two identity // functions are how a run is attributed to a person. - mountCopilotRuntime( - config, - tenantPackage.model, - loadAgentsForActor, - resolveRuntimeModelApiKey, - identifyUser, - identifyActor, - stallGuard, - loadToolsForActor, - signRunForActor, - undefined, - loadVendors, - selectionForActor, - agentFetch, - ), + copilotRuntime.handler, // The only path to an acting call. computerGateway, policyStore, diff --git a/server/src/plugins/builtin-routines.ts b/server/src/plugins/builtin-routines.ts index e0371420..b0b626ab 100644 --- a/server/src/plugins/builtin-routines.ts +++ b/server/src/plugins/builtin-routines.ts @@ -1,9 +1,9 @@ import { MAX_RUN_ERROR, - RoutineNotFoundError, - RoutineRefusedError, type Routine, + RoutineNotFoundError, type RoutinePatch, + RoutineRefusedError, type RoutineStore, type RoutineSummary, } from "../routines/store"; diff --git a/server/src/plugins/routes.ts b/server/src/plugins/routes.ts index 30d73294..d03ac556 100644 --- a/server/src/plugins/routes.ts +++ b/server/src/plugins/routes.ts @@ -7,17 +7,18 @@ import { CATALOGUE, catalogueEntry } from "./catalogue"; import { authorizationUrlFor, challengeFor, + connectedAccountsUrlFor, createVerifier, readConnectState, redeemAuthorizationCode, redirectUriFor, - connectedAccountsUrlFor, sealConnectState, } from "./oauth"; import { CatalogueEntryUnknownError, CustomServerRefusedError, type OAuthClient, + type PluginKind, PluginRefusedError, type PluginStore, } from "./store"; @@ -626,6 +627,19 @@ export function createPluginRoutes( * same thing, so a reader is never left wondering whether skills are governed differently. */ + /** + * The kinds of grant this API will act on. + * + * CHECKED AT RUNTIME, not only in the types. `kind` arrives in a JSON body, so a type annotation + * on it is a comment: before this, anything at all could be written into the grant table through + * the ordinary endpoint, and one kind that was never meant to be settable this way already could. + */ + const GRANT_KINDS = new Set(["mcp", "skill", "bot"]); + const asGrantKind = (value: unknown): PluginKind | null => + typeof value === "string" && GRANT_KINDS.has(value as PluginKind) + ? (value as PluginKind) + : null; + /** * May this person put this on that Bot? * @@ -636,16 +650,77 @@ export function createPluginRoutes( */ async function enablementRefusal( context: { var: AppVariables }, - kind: "mcp" | "skill", + kind: PluginKind, ref: string, agentId: string, + /** + * Which way this is going, because they are not symmetric. + * + * TAKING SOMETHING AWAY IS ALWAYS ALLOWED. The checks below decide whether a grant should exist, + * and applying them to a revoke turns every one of them into a trap: a `bot` grant made before + * the grantee moved to its own endpoint — or before this check existed — could never be removed, + * because the reason it is wrong is the same reason the revoke was refused. An administrator + * looking at a dead row in the UI would have had no way to delete it. + */ + intent: "grant" | "revoke", ): Promise { const actor = skillActor(context); - if (actor.isAdmin) return null; + if (kind === "mcp") { - return "An administrator decides which Bots may reach a tool."; + return actor.isAdmin + ? null + : "An administrator decides which Bots may reach a tool."; + } + + if (kind === "bot") { + /* + * THE ROLE IS CHECKED BEFORE ANYTHING IS LOOKED UP, and that ordering is the point. + * + * One Bot reaching another lets it spend that Bot's model calls, wake its computer and reach + * whatever it may reach, so it is an administrator's decision rather than something somebody + * attaches to a coworker they own. But this route only requires a signed-in user, so every + * refusal below is readable by anybody: checking whether the Bot exists, and whether it runs + * here, before this line handed out three distinguishable answers and turned a 403 into an + * oracle for other people's private Bots. `handoff.ts` in this same feature collapses exactly + * this, deliberately, and this had it backwards. + */ + if (!actor.isAdmin) { + return "An administrator decides which Bots may hand work to another Bot."; + } + // Taking something away is always allowed: see the note on `intent`. + if (intent === "revoke") return null; + + /* + * A grant that could never do anything is refused rather than stored, from both ends. + * + * The GRANTEE has to run here, because handing work on is a tool this deployment executes: a + * Bot at an endpoint runs its own loop and is handed descriptions of what it may call back + * for, and there is no callback path that would execute a hop. + * + * The TARGET only has to exist. Being handed work is not the same as being able to hand it on, + * so a target at its own endpoint is perfectly ordinary — but `ref` is bare text with no + * foreign key, so a typo stored happily and every hop then refused as not-granted. + */ + /* + * A Bot cannot be granted itself. The desk refuses a self-hop outright — "a Bot cannot hand + * work to itself" — so the row is dead the moment it is written, and reads as configured. + */ + if (ref === agentId) { + return "A Bot cannot be granted itself to hand work to."; + } + const runsHere = await store.agentRunsHere(agentId); + if (runsHere === undefined) return "There is no such Bot."; + if (!runsHere) { + return `${agentId} runs at its own endpoint, so this deployment cannot offer it a tool for handing work on. Only a Bot that runs here can be given one.`; + } + if (!(await store.agentIsRegistered(ref))) { + return `There is no Bot called ${ref} to hand work to.`; + } + return null; } + if (actor.isAdmin) return null; + const owner = await store.skillOwner(ref); if (owner === undefined) return `There is no skill called ${ref}.`; if (owner !== actor.id) { @@ -666,11 +741,12 @@ export function createPluginRoutes( routes.post("/grants", requireUser, async (context) => { const body = (await context.req.json().catch(() => null)) as { - kind?: "mcp" | "skill"; + kind?: unknown; ref?: string; agentId?: string; } | null; - if (!body?.kind || !body.ref || !body.agentId) { + const kind = asGrantKind(body?.kind); + if (!kind || !body?.ref || !body.agentId) { return context.json( { error: "A kind, a ref and a Bot are required." }, 400, @@ -678,27 +754,34 @@ export function createPluginRoutes( } const refusal = await enablementRefusal( context, - body.kind, + kind, body.ref, body.agentId, + "grant", ); if (refusal) return context.json({ error: refusal }, 403); - await store.grant(body.kind, body.ref, body.agentId, actorEmail(context)); + await store.grant(kind, body.ref, body.agentId, actorEmail(context)); return context.json({ ok: true }); }); routes.delete("/grants", requireUser, async (context) => { - const kind = context.req.query("kind"); + const kind = asGrantKind(context.req.query("kind")); const ref = context.req.query("ref"); const agentId = context.req.query("agentId"); - if ((kind !== "mcp" && kind !== "skill") || !ref || !agentId) { + if (!kind || !ref || !agentId) { return context.json( { error: "A kind, a ref and a Bot are required." }, 400, ); } - const refusal = await enablementRefusal(context, kind, ref, agentId); + const refusal = await enablementRefusal( + context, + kind, + ref, + agentId, + "revoke", + ); if (refusal) return context.json({ error: refusal }, 403); await store.revoke(kind, ref, agentId, actorEmail(context)); diff --git a/server/src/plugins/selection.ts b/server/src/plugins/selection.ts index 042bd4da..33ad0799 100644 --- a/server/src/plugins/selection.ts +++ b/server/src/plugins/selection.ts @@ -1,3 +1,4 @@ +import { textOf } from "../agents/message-text"; /** * Choosing which of a Bot's tools to put in front of the model, one run at a time. * @@ -252,31 +253,18 @@ export async function selectTools(input: { * turn being taken. Feeding the transcript in would make an early mention of Drive keep Drive tools * loaded for the rest of the conversation, which is the opposite of narrowing. */ + export function latestUserText( messages: readonly { role?: string; content?: unknown }[], ): string { for (let index = messages.length - 1; index >= 0; index -= 1) { const message = messages[index]; if (message?.role !== "user") continue; + // AG-UI allows structured content, and text parts are the only part a selector can read. See + // textOf: a hop asks the same question of the same shapes. if (typeof message.content === "string") return message.content; - // AG-UI allows structured content. Text parts are the only part a selector can read. - if (Array.isArray(message.content)) { - const text = message.content - .map((part) => - typeof part === "object" && - part !== null && - typeof (part as { text?: unknown }).text === "string" - ? ((part as { text: string }).text as string) - : "", - ) - // Dropped before joining, so an image between two sentences does not leave a double space - // in the middle of the one thing the selector reads. - .filter((part) => part !== "") - .join(" ") - .trim(); - if (text !== "") return text; - } - return ""; + const text = textOf(message.content); + return text === "" ? "" : text; } return ""; } diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 7b49a897..4164c39a 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -16,6 +16,7 @@ import { import type { Database } from "../db/client"; import { agentProfiles, + agents, // Aliased: `credentials` is already the injected vault interface in this module, and the table and // the interface are two different things to reach for. credentials as credentialRows, @@ -48,7 +49,30 @@ import { transportFor } from "./transport"; * mean an operator who granted a Bot a server had also, invisibly, waived every rule about it. */ -export type PluginKind = "mcp" | "skill"; +/** + * What a grant is a grant OF. + * + * `bot` is one Bot's permission to hand work to another, and it lives here rather than in a table of + * its own on purpose: an administrator already understands "this Bot may use that", a fork's policy + * layer already applies to grants, and reachability between Bots is the same kind of decision as + * reachability to a vendor's tools. A second table would be a second thing to reason about and a + * second thing for a fork to reimplement. + */ +export type PluginKind = "mcp" | "skill" | "bot"; + +/** + * What an audit row about a grant is a row ABOUT. + * + * A mapping rather than a ternary, because a ternary quietly labelled everything that was not an MCP + * tool a skill. Adding a third kind made that wrong rather than merely terse: a grant letting one Bot + * address another would have been filed in the trail as a skill, which is the sort of small lie an + * investigation trips over months later. + */ +function grantTargetType(kind: PluginKind): string { + if (kind === "mcp") return "mcp_tool"; + if (kind === "bot") return "agent"; + return "skill"; +} export type ToolRecord = { serverId: string; @@ -2141,6 +2165,44 @@ export function createPluginStore(options: PluginStoreOptions) { * Read here rather than through the coworker store because the only question this file asks is * "may this person put their skill on that Bot", and a whole profile is more than that needs. */ + /** + * Whether this Bot's run happens in this process, rather than at an endpoint somewhere. + * + * Undefined for a Bot nobody has heard of. Asked because a tool this deployment executes can + * only be offered to a run it builds: a Bot at an endpoint runs its own loop and is handed + * descriptions of what it may call back for, and handing work to another Bot is not one of them. + */ + async agentRunsHere(agentId: string): Promise { + const [row] = await database + .select({ type: agents.type }) + .from(agents) + .innerJoin(agentProfiles, eq(agentProfiles.agentId, agents.id)) + // A deleted Bot is not one anybody may be given, and answering about it at all would say it + // had existed. + .where(and(eq(agents.id, agentId), isNull(agentProfiles.deletedAt))) + .limit(1); + return row ? row.type === "built_in" : undefined; + }, + + /** + * Whether this Bot is one somebody could be handed work by, at all. + * + * The TARGET of a bot grant, unlike the grantee, may perfectly well run at its own endpoint — + * being handed work is not the same as being able to hand it on. What it may not be is absent: + * `ref` is bare text with no foreign key, so a typo stored happily, `message_bot` was offered, + * and every hop refused as not-granted. That is the same row-that-cannot-work this check exists + * to stop, arriving from the other side. + */ + async agentIsRegistered(agentId: string): Promise { + const [row] = await database + .select({ id: agents.id }) + .from(agents) + .innerJoin(agentProfiles, eq(agentProfiles.agentId, agents.id)) + .where(and(eq(agents.id, agentId), isNull(agentProfiles.deletedAt))) + .limit(1); + return row !== undefined; + }, + async agentOwner(agentId: string): Promise { const [row] = await database .select({ ownerUserId: agentProfiles.ownerUserId }) @@ -2255,6 +2317,41 @@ export function createPluginStore(options: PluginStoreOptions) { }); }, + /** + * The Bots one Bot has been granted, read fresh. + * + * NEVER CACHED. Whether one Bot may address another is a decision an administrator can change, + * and a grant revoked a minute ago has to apply to the next hop rather than after a restart. It + * is a single indexed read, which is the right price for that. + */ + /** + * The Bots this one may hand work to, and can actually reach. + * + * FILTERED AT READ TIME, not only when the grant is made. Refusing a new grant to a Bot that + * runs at its own endpoint stops one being created; it does nothing about the ones already + * there, or about a Bot that was built in when it was granted and was pointed at an endpoint + * afterwards. Those rows read as configured and are inert, which is the shape of thing an + * administrator debugs for an afternoon: the grant is right there in the table and no hop ever + * happens. + * + * The asking side is the one that matters here — a Bot at an endpoint runs its own loop and is + * never offered this tool — so it is the grantee, `agent_id`, that is checked. + */ + async botsReachableFrom(agentId: string): Promise { + const rows = await database + .select({ ref: pluginGrants.ref }) + .from(pluginGrants) + .innerJoin(agents, eq(agents.id, pluginGrants.agentId)) + .where( + and( + eq(pluginGrants.kind, "bot"), + eq(pluginGrants.agentId, agentId), + eq(agents.type, "built_in"), + ), + ); + return rows.map((row) => row.ref); + }, + async grant( kind: PluginKind, ref: string, @@ -2271,7 +2368,7 @@ export function createPluginStore(options: PluginStoreOptions) { await recordAuditEvent(auditStore, { eventType: "configuration.changed", - targetType: kind === "mcp" ? "mcp_tool" : "skill", + targetType: grantTargetType(kind), targetId: ref, payload: { actor: by, @@ -2301,7 +2398,7 @@ export function createPluginStore(options: PluginStoreOptions) { await recordAuditEvent(auditStore, { eventType: "configuration.changed", - targetType: kind === "mcp" ? "mcp_tool" : "skill", + targetType: grantTargetType(kind), targetId: ref, payload: { actor: by, diff --git a/server/src/plugins/transport.ts b/server/src/plugins/transport.ts index 0918826a..4fecdb32 100644 --- a/server/src/plugins/transport.ts +++ b/server/src/plugins/transport.ts @@ -1,8 +1,8 @@ -import type { CatalogueEntry } from "./catalogue"; import * as builtinRoutines from "./builtin-routines"; +import type { CatalogueEntry } from "./catalogue"; import * as driveRest from "./google-drive-rest"; -import * as mcp from "./mcp"; import type { McpCallResult, McpTool } from "./mcp"; +import * as mcp from "./mcp"; /** * How this deployment reaches one vendor: which protocol, chosen per catalogue entry. diff --git a/server/src/routines/store.ts b/server/src/routines/store.ts index 932cc78e..af3a22a4 100644 --- a/server/src/routines/store.ts +++ b/server/src/routines/store.ts @@ -40,7 +40,7 @@ import { routineRuns, routines, } from "../db/schema"; -import { ScheduleRefusedError, describeCron, nextOccurrence } from "./schedule"; +import { describeCron, nextOccurrence, ScheduleRefusedError } from "./schedule"; export class RoutineNotFoundError extends Error { constructor(message = "That routine does not exist.") { diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index e8ae0720..637713a3 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -1,9 +1,9 @@ -import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AgentProfileStore } from "../agents/profile-store"; import type { AuditStore } from "../audit"; import { recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; -import type { AgentProfileStore } from "../agents/profile-store"; import type { IntentRouter, RoutingCandidate, diff --git a/server/src/work/loop.ts b/server/src/work/loop.ts new file mode 100644 index 00000000..6a89f92d --- /dev/null +++ b/server/src/work/loop.ts @@ -0,0 +1,64 @@ +/** + * Running something over and over, one at a time. + * + * A `setInterval` fires on the clock whether or not the last run has finished, which is right for + * housekeeping that takes milliseconds and wrong for anything that takes a turn. The sweep this was + * written for claims work with `for update skip locked`, so overlapping runs do not contend over the + * same row: they each take a DIFFERENT batch, which is the worse failure. One delivery may run for + * its whole deadline, and a two-second interval starts a hundred and fifty more sweeps while it + * does, each claiming another batch and starting its own agent runs. The concurrency has no bound + * but the backlog. + * + * So the next run is scheduled when the last one ends, and the gap is measured from the end rather + * than from the start. An idle deployment also stops paying for a claim every two seconds. + */ +export type Repeating = { + /** Stop scheduling. A run already in flight is left to finish. */ + stop: () => void; +}; + +export function repeatAfterEach( + work: () => Promise, + everyMs: number, + /** + * The timer, so a test can drive this without waiting in real time. + * + * Defaulted rather than required: every caller in this deployment wants the real one, and a seam + * nobody uses in production is a seam that can be wrong without anybody noticing. + */ + schedule: ( + run: () => void, + ms: number, + ) => { unref?: () => void } = setTimeout, +): Repeating { + let stopped = false; + const next = () => { + if (stopped) return; + const timer = schedule(() => { + if (stopped) return; + /* + * Both outcomes schedule the next run, and the failure is swallowed HERE rather than left to + * `finally`. + * + * A loop that stopped the first time the database blinked would stay stopped until somebody + * restarted the pod, silently. But `finally` re-raises what it caught, so `void work().finally` + * keeps looping and leaves an unhandled rejection behind every failed run — which on Bun ends + * the process by default, turning a blink into a crash loop. + * + * Swallowed rather than reported because the caller is the one that knows what a failure + * means: `sweep` already logs its own. A caller that wants this to be loud should say so in + * `work` rather than throwing past it. + */ + void work().then(next, next); + }, everyMs); + // Unref'd where the timer supports it, so this never holds the process open on its own. A pod + // draining should drain. + timer.unref?.(); + }; + next(); + return { + stop: () => { + stopped = true; + }, + }; +} diff --git a/server/src/work/queue.ts b/server/src/work/queue.ts index 30aa7ba5..82249791 100644 --- a/server/src/work/queue.ts +++ b/server/src/work/queue.ts @@ -15,7 +15,7 @@ * Postgres considered expired on arrival, and the next replica to look took the item straight out * from under the first. Both then ran it. Every time this file names a moment it names it in SQL. */ -import { and, eq, gte, isNull, lt, or, sql } from "drizzle-orm"; +import { and, eq, gte, isNull, like, lt, or, sql } from "drizzle-orm"; import type { Database } from "../db/client"; import { workItems } from "../db/schema"; @@ -44,13 +44,34 @@ export type WorkItem = { export const DEFAULT_MAX_ATTEMPTS = 5; export type WorkQueue = { - /** Put work on the queue, or leave what is there. Idempotent on (kind, key). */ + /** + * Put work on the queue, or leave what is there. Idempotent on (kind, key). + * + * `"queued"` is new work. `"already"` is the same key again — the caller asked for this work to be + * queued and it is, but it is NOT a second piece of work, and a caller that reports it as one is + * announcing something that will not happen. `"refused"` is `atMost` saying no. + * + * Three answers rather than a boolean because two of them used to be true: a hop offered under a + * key that already existed was reported to the model as handed over, while the row it named had + * long since been delivered and finished. Nothing was queued and nobody was ever going to run it. + */ offer: (item: { kind: string; key: string; payload?: Record; runAt?: Date; - }) => Promise; + /** + * Refuse this if the prefix is already that full. + * + * COUNTED AND WRITTEN AS ONE STEP, which is the whole reason it lives here rather than in the + * caller. Counting first and offering second is a cap that holds only while nothing else is + * offering: a model that emits five tool calls in one turn runs all five at once, each reads a + * count taken before any of the others had written, and all five pass a cap of three. The + * failure needs no cluster and no unusual timing; it is what asking for several things at once + * looks like. + */ + atMost?: { keyPrefix: string; max: number }; + }) => Promise<"queued" | "already" | "refused">; /** Take up to `limit` due items, leased to `owner`. */ claim: (input: { kind: string; @@ -103,6 +124,17 @@ export type WorkQueue = { }) => Promise; }; +/** + * A literal prefix, safe to put in a `like`. + * + * `%` and `_` are wildcards there, and a key is allowed to contain both. Without this a run whose id + * held an underscore would count rows belonging to other runs, and a fan-out cap that counts the + * wrong rows is a cap that refuses the wrong hops. + */ +function escapeLike(value: string): string { + return value.replace(/[\\%_]/g, (match) => `\\${match}`); +} + /** A moment `ms` from now, named in SQL so it is the database's clock and not the caller's. */ function fromNow(ms: number) { return sql`now() + make_interval(secs => ${ms / 1000})`; @@ -125,20 +157,66 @@ export function createWorkQueue(database: Database): WorkQueue { ); return { - async offer({ kind, key, payload = {}, runAt }) { - await database - .insert(workItems) - .values({ kind, key, payload, ...(runAt ? { runAt } : {}) }) + async offer({ kind, key, payload = {}, runAt, atMost }) { + const write = async (transaction: Database) => + transaction + .insert(workItems) + .values({ kind, key, payload, ...(runAt ? { runAt } : {}) }) + /* + * Nothing on conflict, deliberately. + * + * The key is the identity of the work, so a second offer of the same thing is the same + * thing, not a new one. For a routine the key carries the minute it was due, which is what + * makes "three replicas woke at 07:00" produce one run instead of three. A finished row + * still counts as a conflict, which is what makes that true after the run as well as during + * it. + */ + .onConflictDoNothing() + // Returning the key, so a caller can tell work it just queued from work that was already + // there. Nothing is written on conflict, so this comes back empty for a duplicate. + .returning({ key: workItems.key }); + + if (!atMost) { + const [written] = await write(database); + return written ? "queued" : "already"; + } + + return database.transaction(async (transaction) => { /* - * Nothing on conflict, deliberately. + * Everything offered under this prefix, one at a time, across every replica. * - * The key is the identity of the work, so a second offer of the same thing is the same - * thing, not a new one. For a routine the key carries the minute it was due, which is what - * makes "three replicas woke at 07:00" produce one run instead of three. A finished row - * still counts as a conflict, which is what makes that true after the run as well as during - * it. + * An advisory lock rather than a stricter isolation level, because the thing being counted + * is rows another transaction has not committed yet: under `read committed` two concurrent + * offers each see a count taken before the other wrote, and both pass. The lock is held for + * the transaction and taken on the prefix, so it serialises one run's own hops and nothing + * else on the queue waits behind them. */ - .onConflictDoNothing(); + await transaction.execute( + sql`select pg_advisory_xact_lock(hashtext(${`${kind}:${atMost.keyPrefix}`}))`, + ); + const [row] = await transaction + .select({ total: sql`count(*)::int` }) + .from(workItems) + .where( + and( + eq(workItems.kind, kind), + like(workItems.key, `${escapeLike(atMost.keyPrefix)}%`), + ), + ); + /* + * The same key again is not a new one. Counted as already there rather than refused, or a + * retried offer of work that is on the queue would report the cap as the reason it is not. + */ + const already = await transaction + .select({ key: workItems.key }) + .from(workItems) + .where(and(eq(workItems.kind, kind), eq(workItems.key, key))) + .limit(1); + if (already.length > 0) return "already"; + if ((row?.total ?? 0) >= atMost.max) return "refused"; + const [written] = await write(transaction as unknown as Database); + return written ? "queued" : "already"; + }); }, async claim({ diff --git a/server/tests/agent-callback-token.test.ts b/server/tests/agent-callback-token.test.ts index 29b18327..dc5ad86e 100644 --- a/server/tests/agent-callback-token.test.ts +++ b/server/tests/agent-callback-token.test.ts @@ -42,7 +42,8 @@ describe("an agent's callback token", () => { describe("the run assertion", () => { test("survives a round trip", () => { const signed = mintRunAssertion(RUN, KEY); - expect(readRunAssertion(signed, KEY)).toEqual(RUN); + // A run that began with a person is depth zero, which is what an unstated depth means. + expect(readRunAssertion(signed, KEY)).toEqual({ ...RUN, depth: 0 }); }); test("is refused when signed with another key", () => { @@ -67,7 +68,10 @@ describe("the run assertion", () => { // Eleven minutes later: past the ten-minute life of an assertion. expect(readRunAssertion(signed, KEY, 11 * 60 * 1000)).toBeNull(); // Still good a minute in, so the bound is a real window rather than nothing. - expect(readRunAssertion(signed, KEY, 60 * 1000)).toEqual(RUN); + expect(readRunAssertion(signed, KEY, 60 * 1000)).toEqual({ + ...RUN, + depth: 0, + }); }); test("is refused when it is missing, empty or not a string", () => { @@ -255,3 +259,47 @@ describe("a callback that cannot prove which Bot it is", () => { expect(verdict).not.toHaveProperty("actorId"); }); }); + +/** + * How deep a chain of Bots already is travels here because it has to cross a process. + * + * A Bot handing work to another is A to B to C, three runs on up to three pods. A counter in a + * variable stops applying the moment the second hop lands somewhere else, which is also the moment a + * loop starts costing real money: the cap would go quiet exactly when it was needed. Signed with the + * rest, so it is the deployment's number rather than one a Bot can edit. + */ +describe("how deep a run is", () => { + test("survives a round trip", () => { + const signed = mintRunAssertion({ ...RUN, depth: 2 }, KEY); + expect(readRunAssertion(signed, KEY)?.depth).toBe(2); + }); + + test("a run that began with a person is zero", () => { + expect(readRunAssertion(mintRunAssertion(RUN, KEY), KEY)?.depth).toBe(0); + }); + + /* + * Read as zero rather than refused. The signature has already been checked, so this is a field + * that predates the feature being absent rather than a caller lying, and the cap refuses on the + * way out anyway. + */ + test("a depth that is not a depth reads as zero", () => { + for (const nonsense of [-1, 1.5, "2", null]) { + const signed = mintRunAssertion( + { ...RUN, depth: nonsense as never }, + KEY, + ); + expect(readRunAssertion(signed, KEY)?.depth).toBe(0); + } + }); + + test("the conversation survives a round trip, and is absent when there is none", () => { + expect( + readRunAssertion(mintRunAssertion({ ...RUN, threadId: "t1" }, KEY), KEY) + ?.threadId, + ).toBe("t1"); + expect(readRunAssertion(mintRunAssertion(RUN, KEY), KEY)?.threadId).toBe( + undefined, + ); + }); +}); diff --git a/server/tests/agent-escalation.test.ts b/server/tests/agent-escalation.test.ts new file mode 100644 index 00000000..4b2b76ff --- /dev/null +++ b/server/tests/agent-escalation.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, test } from "bun:test"; +import { + askTheirOwnPerson, + ESCALATE_TOOL, + escalationTool, + PUT_TO, +} from "../src/agents/escalation"; +import type { AuditEventInput } from "../src/audit"; + +/** + * Asking a person, as a first-class answer. + * + * The property that matters is that a Bot which cannot settle something has a named way to stop that + * is not "hand it to another Bot", and that taking it leaves a row saying so. + */ + +const FROM = { + botId: "assistant", + actorId: "user-1", + runId: "run-1", + threadId: "thread-1", + depth: 0, +}; + +function recorder() { + const written: AuditEventInput[] = []; + return { + written, + store: { + insert: async (event: AuditEventInput) => { + written.push(event); + }, + } as never, + }; +} + +describe("asking a person", () => { + test("is offered to every run, granted anybody or not", () => { + const tool = escalationTool({ from: FROM, route: askTheirOwnPerson }); + expect(tool.name).toBe(ESCALATE_TOOL); + }); + + test("names who was reached, so the Bot can say what it did", async () => { + const tool = escalationTool({ from: FROM, route: askTheirOwnPerson }); + + const said = await tool.execute({ question: "which account?" }); + + expect(said).toContain("the person in this conversation"); + }); + + test("the question is on the record", async () => { + const { written, store } = recorder(); + const tool = escalationTool({ + from: FROM, + route: askTheirOwnPerson, + auditStore: store, + }); + + await tool.execute({ question: "which account?", why: "two match" }); + + expect(written[0]).toMatchObject({ + eventType: "agent.escalated", + targetId: "assistant", + actorUserId: "user-1", + }); + expect(written[0]?.payload).toMatchObject({ + question: "which account?", + why: "two match", + }); + }); + + /* + * A route that reaches nobody is the row worth finding later: the Bot stopped, the person was + * never asked, and without it nothing anywhere says so. + */ + test("a question that reached nobody is recorded as one", async () => { + const { written, store } = recorder(); + const tool = escalationTool({ + from: FROM, + route: async () => ({ refusal: "The on-call rota is not configured." }), + auditStore: store, + }); + + const said = await tool.execute({ question: "which account?" }); + + expect(said).toBe("The on-call rota is not configured."); + expect(written[0]?.eventType).toBe("agent.escalation_failed"); + }); + + /* + * Mid-run with a person waiting: a throw ends the run with nothing said, which reads as the Bot + * ignoring them. + */ + test("a call with nothing in it is refused as a sentence", async () => { + const tool = escalationTool({ from: FROM, route: askTheirOwnPerson }); + + const said = await tool.execute({}); + + expect(said).toContain("say what you need"); + }); +}); + +/* + * Same property, other tool: the transcript reads the first words of this to decide whether the + * question reached anybody. + */ +describe("what a routed question answers with", () => { + test("starts with the marker the transcript matches on", async () => { + const tool = escalationTool({ from: FROM, route: askTheirOwnPerson }); + + const said = await tool.execute({ question: "which account?" }); + + expect(said as string).toStartWith(PUT_TO); + }); +}); diff --git a/server/tests/agent-handoff-delivery.test.ts b/server/tests/agent-handoff-delivery.test.ts new file mode 100644 index 00000000..7252c6d7 --- /dev/null +++ b/server/tests/agent-handoff-delivery.test.ts @@ -0,0 +1,510 @@ +import { describe, expect, test } from "bun:test"; +import type { AbstractAgent, BaseEvent, Message } from "@ag-ui/client"; +import { Observable } from "rxjs"; +import { createHandoffDelivery } from "../src/agents/handoff-delivery"; +import type { HandoffWork } from "../src/agents/handoff-runner"; + +/** + * Turning a hop into a turn. + * + * The property that matters is that the addressed Bot joins a conversation rather than answering a + * question in the dark, and that a run which ended in an error is not mistaken for one that answered. + */ + +const WORK: HandoffWork = { + fromBotId: "assistant", + toBotId: "researcher", + actorId: "user-1", + threadId: "thread-1", + runId: "run-1", + depth: 1, + task: "find the outage window", +}; + +const PRIOR: Message[] = [ + { id: "m1", role: "user", content: "we had an outage yesterday" }, + { id: "m2", role: "assistant", content: "I will find out when" }, +]; + +const FINISHED = [{ type: "RUN_FINISHED" }] as unknown as BaseEvent[]; + +/** Enough of an agent for the delivery to hand a conversation to. */ +function stubAgent(): AbstractAgent { + const agent = { + threadId: "", + messages: [] as unknown[], + setMessages(messages: unknown[]) { + agent.messages = messages; + }, + }; + return agent as unknown as AbstractAgent; +} + +function delivery( + events: BaseEvent[], + agent: AbstractAgent | null = stubAgent(), + lockHeld = true, + options: { history?: readonly unknown[]; deadlineMs?: number } = {}, +) { + const requests: Array<{ + threadId: string; + input: Record; + persistedInputMessages?: readonly unknown[]; + }> = []; + const lockCalls: string[] = []; + const released: string[] = []; + return { + requests, + lockCalls, + released, + delivery: createHandoffDelivery({ + ...(options.deadlineMs === undefined + ? {} + : { deadlineMs: options.deadlineMs }), + agentFor: async () => agent, + history: async () => options.history ?? PRIOR, + newRunId: () => "run-2", + answerIn: async () => ({ threadId: "answer-thread" }), + lock: { + acquire: async () => { + lockCalls.push("acquire"); + // The platform's own run id, not the one asked for. + return lockHeld ? { runId: "platform-run" } : null; + }, + renew: async () => { + lockCalls.push("renew"); + }, + release: async (input) => { + lockCalls.push("release"); + released.push(input.threadId); + }, + }, + runner: { + run: (request) => { + requests.push({ + threadId: request.threadId, + input: request.input as Record, + ...(request.persistedInputMessages + ? { persistedInputMessages: request.persistedInputMessages } + : {}), + }); + return new Observable((subscriber) => { + for (const event of events) subscriber.next(event); + subscriber.complete(); + }); + }, + }, + }), + }; +} + +describe("turning a hop into a turn", () => { + test("the addressed Bot reads the conversation before the ask", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "assistant has asked you to help", + shown: "Assistant asked Researcher for this on your behalf: find it", + assertion: "signed", + }); + + const messages = requests[0]?.input.messages as Message[]; + // The conversation, then the ask. A Bot handed only the task answers a question whose other half + // was settled three messages ago. + expect(messages.map((m) => m.id).slice(0, 2)).toEqual(["m1", "m2"]); + expect(messages.at(-1)).toMatchObject({ + role: "user", + content: "assistant has asked you to help", + }); + }); + + test("the run carries the deployment's signed statement of what it is", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "signed-assertion", + }); + + expect(requests[0]?.input.forwardedProps).toEqual({ + openbotRun: "signed-assertion", + }); + // The addressed Bot's own conversation, because a thread has exactly one agent. + expect(requests[0]?.threadId).toBe("answer-thread"); + }); + + /* + * A run that errored said nothing in the conversation. Treating it as delivered finishes the work + * and leaves the person waiting for an answer that will never come. + */ + test("a run that ended in an error is not a delivery", async () => { + const { delivery: deliver } = delivery([ + { type: "RUN_ERROR", message: "the model refused" }, + ] as unknown as BaseEvent[]); + + await expect( + deliver.deliver({ work: WORK, message: "m", shown: "s", assertion: "s" }), + ).rejects.toThrow("the model refused"); + }); + + test("a Bot that cannot be built is worth another go rather than a silent drop", async () => { + const { delivery: deliver } = delivery(FINISHED, null); + + await expect( + deliver.deliver({ work: WORK, message: "m", shown: "s", assertion: "s" }), + ).rejects.toThrow("researcher"); + }); +}); + +/** + * The conversation's run lock. + * + * ONE RUN AT A TIME, and the gateway checks every streamed event against the run the lock names. A + * delivery that skips this is claiming to be a run nobody was told about, so every event is refused + * and the refusal reads like a platform limitation rather than a missing step. It was one. + */ +describe("holding the conversation while a Bot answers", () => { + test("the lock is taken before anything is streamed, and given back after", async () => { + const { delivery: deliver, lockCalls } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "s", + }); + + expect(lockCalls[0]).toBe("acquire"); + expect(lockCalls.at(-1)).toBe("release"); + }); + + test("the run uses the platform's own run id", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "s", + }); + + // The platform's id, not the one asked for: it is what the gateway checks every event against. + expect(requests[0]?.input.runId).toBe("platform-run"); + }); + + /* + * A person mid-question, or the asking Bot still finishing its own sentence, is a wait rather than + * a failure. The hop goes back on the queue and is tried again. + */ + test("a conversation somebody else is running in is waited for, not failed", async () => { + const { delivery: deliver, requests } = delivery( + FINISHED, + stubAgent(), + false, + ); + + await expect( + deliver.deliver({ work: WORK, message: "m", shown: "s", assertion: "s" }), + ).rejects.toThrow("busy"); + expect(requests).toEqual([]); + }); + + /* + * Left held, the conversation is unusable by anybody until it expires: the person cannot ask a + * follow-up and the next hop is refused. One failed delivery would stop the conversation working. + */ + test("the lock is given back even when the run fails", async () => { + const { delivery: deliver, lockCalls } = delivery([ + { type: "RUN_ERROR", message: "the model refused" }, + ] as unknown as BaseEvent[]); + + await expect( + deliver.deliver({ work: WORK, message: "m", shown: "s", assertion: "s" }), + ).rejects.toThrow(); + expect(lockCalls).toContain("release"); + }); +}); + +/** + * Where an answer can land, which the platform decides rather than this code. + * + * An Intelligence thread is owned by exactly one agent. A second Bot answering inside the first + * Bot's conversation is refused however it asks, so the answer goes where that Bot can speak. + */ +describe("which conversation the answer lands in", () => { + test("the addressed Bot's own, not the one that asked", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "s", + }); + + expect(requests[0]?.threadId).toBe("answer-thread"); + expect(requests[0]?.input.threadId).toBe("answer-thread"); + }); + + test("but it reads the conversation that asked", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "s", + }); + + // Its own conversation is new and empty; reading that would tell it nothing. + const messages = requests[0]?.input.messages as Array<{ id: string }>; + expect(messages.map((m) => m.id).slice(0, 2)).toEqual(["m1", "m2"]); + }); +}); + +/** + * What crosses a hop. + * + * A thread's stored history is what a person is shown, not a prompt: the assistant message that made + * a tool call is not kept, so the result of that call is stored on its own with a `toolCallId` + * matching nothing. The asking Bot's last act is always the call that handed the work on, so every + * hop carried one of these and every delivery hung on it. + */ +describe("the conversation that crosses a hop", () => { + test("the asking Bot's tool traffic is left behind", async () => { + const { delivery: deliver, requests } = delivery( + FINISHED, + undefined, + true, + { + history: [ + { id: "m1", role: "user", content: "ask the researcher" }, + // The orphan: a result whose call was never kept. + { + id: "m2", + role: "tool", + toolCallId: "call_1", + content: '"Handed to Researcher."', + }, + // A tool call and nothing else, which is the other half of the same pair. + { id: "m3", role: "assistant", content: "" }, + { id: "m4", role: "assistant", content: "I have asked them." }, + ], + }, + ); + + await deliver.deliver({ + work: WORK, + message: "the ask", + shown: "one line", + assertion: "s", + }); + + const messages = requests[0]?.input.messages as Message[]; + expect(messages.map((message) => message.id)).toEqual([ + "m1", + "m4", + `handoff-platform-run`, + ]); + }); +}); + +/** + * A hop nobody is watching. + * + * On a person's own run there is somebody who can reload the page. A hop that never finishes holds + * the conversation's lock and its place on the queue for as long as the process lives, and the + * person waits on an answer that is not coming. + */ +describe("a delivery that never finishes", () => { + test("is given up on, and says so", async () => { + const { delivery: deliver, lockCalls } = delivery([], stubAgent(), true, { + deadlineMs: 20, + }); + // A run that emits nothing and never completes, which is what a stalled Bot looks like. + const stalled = createHandoffDelivery({ + deadlineMs: 20, + agentFor: async () => stubAgent(), + history: async () => PRIOR, + newRunId: () => "run-2", + answerIn: async () => ({ threadId: "answer-thread" }), + lock: { + acquire: async () => ({ runId: "platform-run" }), + renew: async () => {}, + release: async () => { + lockCalls.push("release"); + }, + }, + runner: { run: () => new Observable(() => {}) }, + }); + + await expect( + stalled.deliver({ work: WORK, message: "m", shown: "s", assertion: "s" }), + ).rejects.toThrow("did not finish within"); + // Given back, or the conversation stays unusable until the lock expires. + expect(lockCalls).toContain("release"); + void deliver; + }); + + test("the lock is given back on the conversation it was taken on", async () => { + const { delivery: deliver, released } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "m", + shown: "s", + assertion: "s", + }); + + // Not `thread-1`, which is the conversation that ASKED and whose lock this run never held. + expect(released).toEqual(["answer-thread"]); + }); +}); + +/** + * What the conversation keeps. + * + * The person did not send the ask and it is not addressed to them: their conversation with one Bot + * has a message in it because another Bot asked for something. Persisting the whole prompt puts the + * asking conversation's history into a second conversation, and a paragraph of instructions to a + * model into a bubble that looks like something they typed. + */ +describe("what a hop leaves in the transcript", () => { + test("is the one line, not the prompt", async () => { + const { delivery: deliver, requests } = delivery(FINISHED); + + await deliver.deliver({ + work: WORK, + message: "assistant has asked you to help\n\nTask: ...\nConstraints: ...", + shown: "Assistant asked Researcher for this on your behalf: find it", + assertion: "s", + }); + + expect(requests[0]?.persistedInputMessages).toEqual([ + { + id: "handoff-platform-run", + role: "user", + content: "Assistant asked Researcher for this on your behalf: find it", + }, + ]); + // The model still gets the whole envelope, and the conversation that asked. + const messages = requests[0]?.input.messages as Message[]; + expect(messages.at(-1)).toMatchObject({ + content: expect.stringContaining("Task:"), + }); + }); +}); + +/** + * Where the conversation has to be put. + * + * `runAgent` takes `runId`, `tools`, `context` and `forwardedProps`. AG-UI keeps the messages and + * the thread on the agent, so a `messages` array passed as a run parameter is ignored in silence: + * the Bot runs, reads nothing, and answers "how can I help?" to a question printed directly above + * its reply. Nothing fails, which is why this is a test rather than a comment. + */ +describe("what the addressed Bot is actually given", () => { + test("the conversation is set on the agent, not only in the run", async () => { + const agent = stubAgent(); + const { delivery: deliver } = delivery(FINISHED, agent); + + await deliver.deliver({ + work: WORK, + message: "the ask", + shown: "one line", + assertion: "s", + }); + + const given = (agent as unknown as { messages: Message[] }).messages; + expect(given.map((message) => message.id)).toEqual([ + "m1", + "m2", + "handoff-platform-run", + ]); + expect(given.at(-1)).toMatchObject({ role: "user", content: "the ask" }); + // And it runs in its own conversation, which the agent also carries. + expect((agent as unknown as { threadId: string }).threadId).toBe( + "answer-thread", + ); + }); +}); + +/** + * A message is not always a string. + * + * AG-UI's user message takes `string | InputContent[]` and the platform types thread content as + * unknown. Nothing here writes an array yet, which is why a `typeof content === "string"` test + * looked complete — and why the day attachments ship, every message carrying one would vanish from + * the conversation handed across a hop with nothing recording it. + */ +describe("a conversation that is not all plain strings", () => { + test("a message made of parts is carried across, not dropped", async () => { + const { delivery: deliver, requests } = delivery( + FINISHED, + undefined, + true, + { + history: [ + { + id: "m1", + role: "user", + content: [ + { type: "text", text: "here is the invoice" }, + { type: "image", url: "https://example.test/a.png" }, + ], + }, + { id: "m2", role: "assistant", content: "I will read it" }, + ], + }, + ); + + await deliver.deliver({ + work: WORK, + message: "the ask", + shown: "one line", + assertion: "s", + }); + + const messages = requests[0]?.input.messages as Message[]; + expect(messages.map((message) => message.id)).toEqual([ + "m1", + "m2", + "handoff-platform-run", + ]); + }); + + /* + * Still dropped: a message whose only content is parts this does not understand says nothing, and + * an assistant message with nothing in it is a tool call whose other half was never kept. + */ + test("a message with no text in it at all is still left behind", async () => { + const { delivery: deliver, requests } = delivery( + FINISHED, + undefined, + true, + { + history: [ + { id: "m1", role: "user", content: [{ type: "image", url: "x" }] }, + { id: "m2", role: "assistant", content: [] }, + { id: "m3", role: "user", content: "what does it say?" }, + ], + }, + ); + + await deliver.deliver({ + work: WORK, + message: "the ask", + shown: "one line", + assertion: "s", + }); + + const messages = requests[0]?.input.messages as Message[]; + expect(messages.map((message) => message.id)).toEqual([ + "m3", + "handoff-platform-run", + ]); + }); +}); diff --git a/server/tests/agent-handoff-endtoend.integration.test.ts b/server/tests/agent-handoff-endtoend.integration.test.ts new file mode 100644 index 00000000..f4cd49f5 --- /dev/null +++ b/server/tests/agent-handoff-endtoend.integration.test.ts @@ -0,0 +1,252 @@ +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { and, eq, like } from "drizzle-orm"; +import { createHandoffDesk } from "../src/agents/handoff"; +import { + createHandoffRunner, + type HandoffWork, +} from "../src/agents/handoff-runner"; +import { handoffTool } from "../src/agents/handoff-tool"; +import { createAgentProfileStore } from "../src/agents/profile-store"; +import { createAuditStore } from "../src/audit"; +import { createDatabase } from "../src/db/client"; +import { + agentProfiles, + agents, + auditEvents, + pluginGrants, + workItems, +} from "../src/db/schema"; +import { createWorkQueue } from "../src/work/queue"; +import { TEST_POOL } from "./support/database"; + +/** + * A hop from end to end: a Bot calls the tool, and another replica delivers it. + * + * THE TWO HALVES NEVER SPEAK. Deciding happens in one run and delivering in another process, and the + * only thing between them is a row. That is the property worth an integration test: unit tests on + * either side pass while the row they agree on is written by one and unreadable by the other. + * + * The delivery is faked and nothing else is. Running a real model against a real thread is not what + * this is asking about, and it would make the test slow, expensive and non-deterministic for a + * question the two files either side already answer. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const suite = randomUUID().slice(0, 8); +const ASKER = `e2e-asker-${suite}`; +const TARGET = `e2e-target-${suite}`; +const ACTOR = `e2e-actor-${suite}`; +const RUN = `e2e-run-${suite}`; + +const queue = createWorkQueue(database); +const auditStore = createAuditStore(database); +const profiles = createAgentProfileStore(database); + +const desk = createHandoffDesk({ + queue, + profiles, + // The person's own role, as the request path resolves it: an administrator sees Bots a user does + // not, and a hop to one of those is theirs to make. + actorFor: async (id: string) => ({ id, role: "user" as const }), + mayAddress: async (fromBotId, toBotId) => + ( + await database + .select({ ref: pluginGrants.ref }) + .from(pluginGrants) + .where( + and( + eq(pluginGrants.kind, "bot"), + eq(pluginGrants.agentId, fromBotId), + ), + ) + ).some((row) => row.ref === toBotId), + auditStore, + caps: { maxDepth: 2, maxPerRun: 3 }, +}); + +async function clean() { + await database.delete(workItems).where(like(workItems.key, `${RUN}%`)); + for (const id of [ASKER, TARGET]) { + await database.delete(pluginGrants).where(eq(pluginGrants.agentId, id)); + await database.delete(agentProfiles).where(eq(agentProfiles.agentId, id)); + await database.delete(agents).where(eq(agents.id, id)); + } +} + +beforeEach(async () => { + await clean(); + for (const [id, name] of [ + [ASKER, "Asker"], + [TARGET, "Target"], + ]) { + await database + .insert(agents) + .values({ id, name, type: "built_in", configuration: {} }) + .onConflictDoNothing(); + await database + .insert(agentProfiles) + .values({ + agentId: id, + name, + title: "", + roleDescription: "", + avatarSeed: id, + visibility: "public", + }) + .onConflictDoNothing(); + } + await database + .insert(pluginGrants) + .values({ kind: "bot", ref: TARGET, agentId: ASKER, grantedBy: "test" }) + .onConflictDoNothing(); +}); + +afterAll(async () => { + await clean(); + await database.$client.end({ timeout: 5 }); +}); + +describe("a hop, from the tool call to the delivery", () => { + test("what one Bot asked for is what the other is shown", async () => { + const tool = handoffTool({ + desk, + from: { + botId: ASKER, + actorId: ACTOR, + runId: RUN, + threadId: `thread-${suite}`, + depth: 0, + }, + hasSomebodyToAsk: true, + maxDepth: 2, + }); + + // The asking Bot's own words, through the tool it is offered. + const said = await tool?.execute({ + bot: "Target", + task: "find the outage window", + constraints: "yesterday only", + expecting: "a date range", + }); + expect(said).toContain("Target"); + + // A different process entirely, sharing nothing but the row. + const delivered: Array<{ work: HandoffWork; message: string }> = []; + const runner = createHandoffRunner({ + queue: createWorkQueue(database), + owner: `replica-${suite}`, + sign: () => "signed", + auditStore, + delivery: { + deliver: async ({ work, message }) => { + delivered.push({ work, message }); + }, + }, + }); + + const report = await runner.sweep(); + + expect(report.delivered).toContain(TARGET); + const seen = delivered.find((entry) => entry.work.toBotId === TARGET); + expect(seen?.work).toMatchObject({ + fromBotId: ASKER, + actorId: ACTOR, + threadId: `thread-${suite}`, + depth: 1, + }); + // Every part the asking model was made to name survives to the other side. + expect(seen?.message).toContain("find the outage window"); + expect(seen?.message).toContain("yesterday only"); + expect(seen?.message).toContain("a date range"); + // Attributed by the deployment, from the row rather than from anything a model wrote. + expect(seen?.message).toContain(ASKER); + }); + + test("a delivered hop is finished, so a second sweep does not run the Bot again", async () => { + const tool = handoffTool({ + desk, + from: { + botId: ASKER, + actorId: ACTOR, + runId: RUN, + threadId: `thread-${suite}`, + depth: 0, + }, + hasSomebodyToAsk: true, + maxDepth: 2, + }); + await tool?.execute({ bot: "Target", task: "have a look" }); + + const sweepWith = (owner: string) => { + const seen: string[] = []; + return { + seen, + runner: createHandoffRunner({ + queue: createWorkQueue(database), + owner, + sign: () => "signed", + auditStore, + delivery: { + deliver: async ({ work }) => { + seen.push(work.toBotId); + }, + }, + }), + }; + }; + + const first = sweepWith(`replica-a-${suite}`); + await first.runner.sweep(); + const second = sweepWith(`replica-b-${suite}`); + await second.runner.sweep(); + + expect(first.seen).toEqual([TARGET]); + // The row is finished rather than deleted, so re-offering the same hop collides too. + expect(second.seen).toEqual([]); + }); + + test("the whole path leaves a trail somebody can follow", async () => { + const tool = handoffTool({ + desk, + from: { + botId: ASKER, + actorId: ACTOR, + runId: RUN, + threadId: `thread-${suite}`, + depth: 0, + }, + hasSomebodyToAsk: true, + maxDepth: 2, + }); + await tool?.execute({ bot: "Target", task: "have a look" }); + + const runner = createHandoffRunner({ + queue: createWorkQueue(database), + owner: `replica-${suite}`, + sign: () => "signed", + auditStore, + delivery: { deliver: async () => {} }, + }); + await runner.sweep(); + + const rows = await database + .select({ + eventType: auditEvents.eventType, + payload: auditEvents.payload, + }) + .from(auditEvents) + .where(eq(auditEvents.targetId, TARGET)); + const kinds = rows.map((row) => row.eventType); + expect(kinds).toContain("agent.handoff_offered"); + expect(kinds).toContain("agent.handoff_delivered"); + expect( + rows.every((row) => (row.payload as { run?: string }).run === RUN), + ).toBe(true); + }); +}); diff --git a/server/tests/agent-handoff-runner.integration.test.ts b/server/tests/agent-handoff-runner.integration.test.ts new file mode 100644 index 00000000..91a76045 --- /dev/null +++ b/server/tests/agent-handoff-runner.integration.test.ts @@ -0,0 +1,166 @@ +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq } from "drizzle-orm"; +import { + createHandoffRunner, + type HandoffWork, +} from "../src/agents/handoff-runner"; +import type { AuditStore } from "../src/audit"; +import { createDatabase } from "../src/db/client"; +import { workItems } from "../src/db/schema"; +import { createWorkQueue } from "../src/work/queue"; +import { TEST_POOL } from "./support/database"; + +/** + * Two replicas and one batch of hops, against a real PostgreSQL. + * + * A lease is a promise the database keeps about time passing, and every stub of this queue answers + * whatever it was told to. The whole suite was green while the tail of every batch was delivered + * twice, because a fake cannot let a lease quietly run out. + */ +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); +const queue = createWorkQueue(database); +const kind = "bot.message"; + +const silent: AuditStore = { insert: async () => {} }; + +afterAll(async () => { + await database.delete(workItems).where(eq(workItems.kind, kind)); + await database.$client.close(); +}); + +beforeEach(async () => { + await database.delete(workItems).where(eq(workItems.kind, kind)); +}); + +function hop(run: string, n: number): HandoffWork { + return { + fromBotId: "assistant", + toBotId: `bot-${n}`, + actorId: "user-1", + threadId: "thread-1", + runId: run, + depth: 1, + task: `task ${n}`, + }; +} + +describe("a batch of hops and a lease that can run out", () => { + /* + * A claim leases the whole batch from one moment, and the batch is delivered one at a time. A + * heartbeat that only covers the hop in flight leaves the rest on a lease that expires while the + * first one runs: another replica claims them, and this one delivers them anyway. Two model calls, + * two answers in somebody's conversation, and both replicas reporting success. + */ + test("the tail of a batch is not delivered twice while its head is running", async () => { + const run = randomUUID(); + for (const n of [1, 2, 3]) { + await queue.offer({ + kind, + key: `${run}:${n}`, + payload: hop(run, n) as unknown as Record, + }); + } + + const ran: string[] = []; + const held = Promise.withResolvers(); + const shared = { + queue, + sign: () => "signed", + auditStore: silent, + // Small enough to drive in milliseconds; the property is one duration outrunning another. + leaseMs: 400, + renewEveryMs: 100, + limit: 3, + }; + + const slow = createHandoffRunner({ + ...shared, + owner: "replica-a", + delivery: { + deliver: async ({ work }) => { + ran.push(`a:${work.toBotId}`); + if (work.toBotId === "bot-1") await held.promise; + }, + }, + }); + const quick = createHandoffRunner({ + ...shared, + owner: "replica-b", + delivery: { + deliver: async ({ work }) => { + ran.push(`b:${work.toBotId}`); + }, + }, + }); + + const sweepA = slow.sweep(); + // Long enough that an unrenewed lease taken at the same moment would have lapsed twice over. + await new Promise((resolve) => setTimeout(resolve, 900)); + const reportB = await quick.sweep(); + held.resolve(); + const reportA = await sweepA; + + expect(reportA.delivered).toEqual(["bot-1", "bot-2", "bot-3"]); + // Nothing was left for the other replica to take, so nothing ran twice. + expect(reportB.delivered).toEqual([]); + expect(ran).toEqual(["a:bot-1", "a:bot-2", "a:bot-3"]); + }); + + /* + * And when a lease really has gone, the model call is the thing not to spend. Finding out after + * delivering is finding out too late. + */ + test("a hop whose lease went elsewhere is not run again by its old owner", async () => { + const run = randomUUID(); + for (const n of [1, 2]) { + await queue.offer({ + kind, + key: `${run}:${n}`, + payload: hop(run, n) as unknown as Record, + }); + } + + const ran: string[] = []; + const held = Promise.withResolvers(); + const slow = createHandoffRunner({ + queue, + owner: "replica-a", + sign: () => "signed", + auditStore: silent, + leaseMs: 400, + // Never refreshed, which is what a paused process looks like from the database's side. + renewEveryMs: 60_000, + limit: 2, + delivery: { + deliver: async ({ work }) => { + ran.push(work.toBotId); + if (work.toBotId === "bot-1") await held.promise; + }, + }, + }); + + const sweep = slow.sweep(); + await new Promise((resolve) => setTimeout(resolve, 700)); + // Somebody else takes the lapsed hop while the first is still running. + const taken = await queue.claim({ + kind, + owner: "replica-b", + leaseMs: 10_000, + limit: 5, + }); + held.resolve(); + const report = await sweep; + + expect(taken.map((item) => item.key)).toContain(`${run}:2`); + // Delivered once, by whoever holds it now, and not a second time by its old owner. + expect(ran).toEqual(["bot-1"]); + expect(report.skipped.map((entry) => entry.reason)).toContain( + "the lease went elsewhere", + ); + }); +}); diff --git a/server/tests/agent-handoff-runner.test.ts b/server/tests/agent-handoff-runner.test.ts new file mode 100644 index 00000000..b8252c05 --- /dev/null +++ b/server/tests/agent-handoff-runner.test.ts @@ -0,0 +1,366 @@ +import { describe, expect, test } from "bun:test"; +import { + createHandoffRunner, + type HandoffWork, +} from "../src/agents/handoff-runner"; +import type { AuditStore } from "../src/audit"; +import type { WorkItem, WorkQueue } from "../src/work/queue"; + +/** + * Delivering a hop, and the three ways it must not go wrong. + * + * Running the other Bot twice for one hop. Finishing work that is no longer this replica's. And + * letting a lease lapse in the middle of a run, which is the same as the first with extra steps. + */ + +const WORK: HandoffWork = { + fromBotId: "assistant", + toBotId: "researcher", + actorId: "user-1", + threadId: "thread-1", + runId: "run-1", + depth: 1, + task: "find the outage window", + expecting: "a date range", +}; + +function runner(options?: { + claimed?: WorkItem[]; + deliver?: (input: { + work: HandoffWork; + message: string; + shown?: string; + }) => Promise; +}) { + const calls: Array<{ verb: string; key: string; owner?: string }> = []; + const events: string[] = []; + const delivered: Array<{ message: string; assertion: string }> = []; + const offered: HandoffWork[] = []; + + const queue = { + claim: async () => + options?.claimed ?? [ + { kind: "bot.message", key: "run-1:abc", payload: WORK, attempts: 1 }, + ], + renew: async () => true, + finish: async ({ key, owner }: { key: string; owner: string }) => { + calls.push({ verb: "finish", key, owner }); + return true; + }, + release: async ({ key, owner }: { key: string; owner: string }) => { + calls.push({ verb: "release", key, owner }); + return true; + }, + offer: async ({ + key, + payload, + }: { + key: string; + payload?: Record; + }) => { + calls.push({ verb: "offer", key }); + offered.push(payload as unknown as HandoffWork); + }, + } as unknown as WorkQueue; + + const auditStore: AuditStore = { + insert: async (event) => { + events.push(event.eventType); + }, + }; + + return { + calls, + events, + delivered, + offered, + runner: createHandoffRunner({ + queue, + owner: "replica-a", + sign: (work) => `signed:${work.toBotId}:${work.depth}`, + auditStore, + delivery: { + deliver: async ({ work, message, shown, assertion }) => { + delivered.push({ message, assertion }); + await options?.deliver?.({ work, message, shown }); + }, + }, + }), + }; +} + +describe("delivering a hop", () => { + test("runs the addressed Bot and finishes the work as its owner", async () => { + const { runner: sweep, calls, delivered } = runner(); + + const report = await sweep.sweep(); + + expect(report.delivered).toEqual(["researcher"]); + expect(calls).toEqual([ + { verb: "finish", key: "run-1:abc", owner: "replica-a" }, + ]); + expect(delivered).toHaveLength(1); + }); + + /* + * Who is asking is stamped by the deployment, from the row it wrote. A Bot able to write its own + * attribution is a Bot able to claim to be another one. + */ + test("the addressed Bot is told who asked, and what for, in parts", async () => { + const { runner: sweep, delivered } = runner(); + + await sweep.sweep(); + + const message = delivered[0]?.message ?? ""; + expect(message).toContain("assistant"); + expect(message).toContain("Task: find the outage window"); + // The parts stay parts: the asking model was made to name them so this one need not infer them. + expect(message).toContain("What a good answer looks like: a date range"); + }); + + test("the run it starts carries the depth this hop reached", async () => { + const { runner: sweep, delivered } = runner(); + + await sweep.sweep(); + + expect(delivered[0]?.assertion).toBe("signed:researcher:1"); + }); + + /* + * A Bot that answered has answered, whatever it said. Retrying a delivery because the answer was + * unhelpful would ask it the same question again and bill for the same non-answer. + */ + test("a delivery that fails is released rather than finished", async () => { + const { + runner: sweep, + calls, + events, + } = runner({ + deliver: async () => { + throw new Error("the gateway was unreachable"); + }, + }); + + const report = await sweep.sweep(); + + expect(report.delivered).toEqual([]); + expect(calls).toEqual([ + { verb: "release", key: "run-1:abc", owner: "replica-a" }, + ]); + expect(events).toContain("agent.handoff_failed"); + }); + + /* + * A second attempt may already have run that Bot, spent a model call and posted an answer before + * its owner died. Somebody reading two similar answers should be able to tell which happened. + */ + test("a second attempt says so, before it runs anything", async () => { + const { runner: sweep, events } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:abc", payload: WORK, attempts: 2 }, + ], + }); + + await sweep.sweep(); + + expect(events[0]).toBe("agent.handoff_retried"); + expect(events).toContain("agent.handoff_delivered"); + }); + + /* Releasing an unusable row would put it back on the queue for ever. */ + test("a row that is not a hop is finished rather than released", async () => { + const { runner: sweep, calls } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:junk", payload: {}, attempts: 1 }, + ], + }); + + const report = await sweep.sweep(); + + expect(report.skipped).toEqual([ + { key: "run-1:junk", reason: "not a hop" }, + ]); + expect(calls).toEqual([ + { verb: "finish", key: "run-1:junk", owner: "replica-a" }, + ]); + }); +}); + +/** + * A hop that will not be tried again. + * + * The person was told their question had been handed on. If nothing ever comes back and nothing ever + * says so, they cannot tell a slow Bot from a broken one, and the conversation simply stops. + */ +describe("a hop that failed for good", () => { + test("the Bot that asked is sent back to tell the person", async () => { + const { runner: sweeper, offered } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:abc", payload: WORK, attempts: 5 }, + ] as unknown as WorkItem[], + deliver: async () => { + throw new Error("researcher did not finish within 300s"); + }, + }); + + await sweeper.sweep(); + + expect(offered).toHaveLength(1); + // Back to the Bot that asked, in the conversation the person is watching. + expect(offered[0]).toMatchObject({ + fromBotId: "researcher", + toBotId: "assistant", + answerIn: "thread-1", + threadId: "thread-1", + }); + expect(offered[0]?.task).toContain("did not finish within 300s"); + }); + + /* + * The fan-out cap counts every row whose key starts with the run's own prefix. A notice is not one + * of the Bots this run asked for, and a run long enough to see a hop fail for good is exactly the + * run that still has asking to do. + */ + test("its key is outside the run's own prefix, so it costs no fan-out budget", async () => { + const { + runner: sweeper, + calls, + offered, + } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:abc", payload: WORK, attempts: 5 }, + ] as unknown as WorkItem[], + deliver: async () => { + throw new Error("nope"); + }, + }); + + await sweeper.sweep(); + + const key = calls.find((call) => call.verb === "offer")?.key ?? ""; + expect(key.startsWith("run-1:")).toBe(false); + expect(key).toContain("run-1:abc"); + expect(offered).toHaveLength(1); + }); + + /* + * One run may legally ask the same Bot two different things. Keyed on the Bot alone both notices + * are the same work to `offer`, the second is dropped on conflict, and nothing purges this kind — + * so the person hears about one of their two lost questions, for good. + */ + test("two lost questions to one Bot leave two notices", async () => { + const { runner: sweeper, calls } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:aaa", payload: WORK, attempts: 5 }, + { kind: "bot.message", key: "run-1:bbb", payload: WORK, attempts: 5 }, + ] as unknown as WorkItem[], + deliver: async () => { + throw new Error("nope"); + }, + }); + + await sweeper.sweep(); + + const keys = calls + .filter((call) => call.verb === "offer") + .map((call) => call.key); + expect(keys).toHaveLength(2); + expect(new Set(keys).size).toBe(2); + }); + + /* + * Otherwise a Bot nobody can reach produces a notice that cannot be delivered either, which + * produces a notice, for ever. + */ + test("a notice that fails is not itself noticed", async () => { + const { runner: sweeper, offered } = runner({ + claimed: [ + { + kind: "bot.message", + key: "run-1:notice:researcher", + payload: { ...WORK, answerIn: "thread-1" }, + attempts: 5, + }, + ] as unknown as WorkItem[], + deliver: async () => { + throw new Error("nope"); + }, + }); + + await sweeper.sweep(); + + expect(offered).toEqual([]); + }); + + test("a hop with tries left is simply released", async () => { + const { + runner: sweeper, + offered, + calls, + } = runner({ + claimed: [ + { kind: "bot.message", key: "run-1:abc", payload: WORK, attempts: 2 }, + ] as unknown as WorkItem[], + deliver: async () => { + throw new Error("busy"); + }, + }); + + await sweeper.sweep(); + + expect(offered).toEqual([]); + expect(calls.map((call) => call.verb)).toContain("release"); + }); +}); + +/** + * What a notice leaves in the transcript. + * + * Nothing. The asking Bot's own sentence is the whole message; the text that prompted it is an + * instruction to a model, and kept it appears as something the person typed and had read back. + */ +describe("what a notice shows", () => { + test("the instruction that produced it is not shown to anybody", async () => { + const shownTexts: Array = []; + const { runner: sweeper } = runner({ + claimed: [ + { + kind: "bot.message", + key: "run-1:notice:researcher", + payload: { ...WORK, answerIn: "thread-1" }, + attempts: 1, + }, + ] as unknown as WorkItem[], + deliver: async (input) => { + shownTexts.push(input.shown); + }, + }); + + await sweeper.sweep(); + + expect(shownTexts).toEqual([undefined]); + }); + + test("an ordinary hop shows who asked and what for", async () => { + const shownTexts: Array = []; + const { runner: sweeper } = runner({ + claimed: [ + { + kind: "bot.message", + key: "run-1:abc", + payload: { ...WORK, fromName: "Assistant", toName: "Researcher" }, + attempts: 1, + }, + ] as unknown as WorkItem[], + deliver: async (input) => { + shownTexts.push(input.shown); + }, + }); + + await sweeper.sweep(); + + expect(shownTexts[0]).toBe( + "Assistant asked Researcher for this on your behalf: find the outage window", + ); + }); +}); diff --git a/server/tests/agent-handoff-tool.test.ts b/server/tests/agent-handoff-tool.test.ts new file mode 100644 index 00000000..5900dd50 --- /dev/null +++ b/server/tests/agent-handoff-tool.test.ts @@ -0,0 +1,182 @@ +import { describe, expect, test } from "bun:test"; +import type { HandoffDesk, HandoffOutcome } from "../src/agents/handoff"; +import { + HANDED_OVER, + HANDOFF_TOOL, + handoffTool, +} from "../src/agents/handoff-tool"; + +/** + * What the model is offered, and what it is told when it is refused. + * + * A tool a Bot may never successfully use is worse than no tool: the model spends attention on it, + * calls it, and tells the person it tried and could not, which reads as the deployment being broken + * rather than as it working correctly. + */ + +const FROM = { + botId: "assistant", + actorId: "user-1", + runId: "run-1", + threadId: "thread-1", + depth: 0, +}; + +function deskReturning(outcome: HandoffOutcome): HandoffDesk { + return { send: async () => outcome }; +} + +const ALLOWED: HandoffOutcome = { + ok: true, + to: "researcher", + toName: "Researcher", +}; + +describe("the handoff tool", () => { + test("is offered to a Bot that has somebody to ask", () => { + const tool = handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + maxPerRun: 3, + }); + + expect(tool?.name).toBe(HANDOFF_TOOL); + }); + + test("is not offered to a Bot nobody granted", () => { + expect( + handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: false, + maxDepth: 1, + maxPerRun: 3, + }), + ).toBe(null); + }); + + test("is not offered where the deployment has switched handoff off", () => { + expect( + handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 0, + maxPerRun: 3, + }), + ).toBe(null); + }); + + /* + * The desk would refuse it anyway. This is about what the model is shown: one at the cap reaches + * for the tool, is told no, and often reports that failure to the person. + */ + test("is not offered to a run already at the cap", () => { + expect( + handoffTool({ + desk: deskReturning(ALLOWED), + from: { ...FROM, depth: 1 }, + hasSomebodyToAsk: true, + maxDepth: 1, + }), + ).toBe(null); + }); + + test("tells the model not to answer on the other Bot's behalf", async () => { + const tool = handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + maxPerRun: 3, + }); + + const said = await tool?.execute({ + bot: "Researcher", + task: "find the outage window", + }); + + expect(said).toContain("Researcher"); + expect(said).toContain("do not answer on its behalf"); + }); + + /* A throw would end the run with nothing said, which reads as the Bot ignoring the person. */ + test("hands a refusal back as something the Bot can say", async () => { + const tool = handoffTool({ + desk: deskReturning({ + ok: false, + refusal: "You have not been given that Bot.", + }), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + }); + + await expect(tool?.execute({ bot: "payroll", task: "t" })).resolves.toBe( + "You have not been given that Bot.", + ); + }); + + test("a call missing the task is answered rather than thrown", async () => { + const tool = handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + maxPerRun: 3, + }); + + await expect(tool?.execute({ bot: "researcher" })).resolves.toContain( + "say what you are asking it to do", + ); + }); +}); + +/** + * The other zero. + * + * A run allowed to go no Bots deep and a run allowed to address no Bots are the same deployment + * decision from two directions, and only one of them was closing the door. With a fan-out cap of + * zero the tool was offered, every call was refused, and the model told the person it had tried and + * failed — which reads as the deployment being broken rather than as it being switched off. + */ +describe("a deployment that allows no hops at all", () => { + test("offers nothing when the fan-out cap is zero", () => { + expect( + handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + maxPerRun: 0, + }), + ).toBeNull(); + }); +}); + +/** + * The sentence and the marker, held together. + * + * The transcript decides whether to draw a hop or a boundary by reading the first words of this + * result. With one declaration the two sides cannot disagree about the PHRASE; what they can still + * disagree about is whether the sentence actually starts with it, which is what shipped once and + * drew every accepted hop as Blocked. + */ +describe("what an accepted hop answers with", () => { + test("starts with the marker the transcript matches on", async () => { + const tool = handoffTool({ + desk: deskReturning(ALLOWED), + from: FROM, + hasSomebodyToAsk: true, + maxDepth: 1, + maxPerRun: 3, + }); + + const said = await tool?.execute({ bot: "researcher", task: "find it" }); + + expect(typeof said).toBe("string"); + expect(said as string).toStartWith(HANDED_OVER); + }); +}); diff --git a/server/tests/agent-handoff.integration.test.ts b/server/tests/agent-handoff.integration.test.ts new file mode 100644 index 00000000..fdbc2e76 --- /dev/null +++ b/server/tests/agent-handoff.integration.test.ts @@ -0,0 +1,253 @@ +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { and, eq, like } from "drizzle-orm"; +import { createHandoffDesk, HANDOFF_KIND } from "../src/agents/handoff"; +import { createAgentProfileStore } from "../src/agents/profile-store"; +import { createAuditStore } from "../src/audit"; +import { createDatabase } from "../src/db/client"; +import { + agentProfiles, + agents, + auditEvents, + pluginGrants, + workItems, +} from "../src/db/schema"; +import { createWorkQueue } from "../src/work/queue"; +import { TEST_POOL } from "./support/database"; + +/** + * A hop, driven against the real database rather than through fakes. + * + * Three of the four properties here belong to Postgres rather than to the code: whether a second + * offer of the same hop collides, whether the fan-out count sees rows another replica wrote, and + * whether a grant read now reflects one made a moment ago. A fake answers all three the way its + * author expected, which is the wrong witness for exactly the questions worth asking. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const suite = randomUUID().slice(0, 8); +const ASKER = `handoff-asker-${suite}`; +const TARGET = `handoff-target-${suite}`; +const ACTOR = `handoff-actor-${suite}`; + +const profiles = createAgentProfileStore(database); +const queue = createWorkQueue(database); +const desk = createHandoffDesk({ + queue, + profiles, + // The person's own role, as the request path resolves it: an administrator sees Bots a user does + // not, and a hop to one of those is theirs to make. + actorFor: async (id: string) => ({ id, role: "user" as const }), + mayAddress: async (fromBotId, toBotId) => { + const rows = await database + .select({ ref: pluginGrants.ref }) + .from(pluginGrants) + .where( + and(eq(pluginGrants.kind, "bot"), eq(pluginGrants.agentId, fromBotId)), + ); + return rows.some((row) => row.ref === toBotId); + }, + auditStore: createAuditStore(database), + caps: { maxDepth: 2, maxPerRun: 2 }, +}); + +async function clean() { + await database.delete(workItems).where(like(workItems.key, `run-${suite}%`)); + for (const id of [ASKER, TARGET]) { + await database.delete(pluginGrants).where(eq(pluginGrants.agentId, id)); + await database.delete(agentProfiles).where(eq(agentProfiles.agentId, id)); + await database.delete(agents).where(eq(agents.id, id)); + } +} + +beforeEach(async () => { + await clean(); + for (const [id, name] of [ + [ASKER, "Asker"], + [TARGET, "Target"], + ]) { + await database + .insert(agents) + .values({ id, name, type: "built_in", configuration: {} }) + .onConflictDoNothing(); + await database + .insert(agentProfiles) + .values({ + agentId: id, + name, + title: "", + roleDescription: "", + avatarSeed: id, + visibility: "public", + }) + .onConflictDoNothing(); + } +}); + +afterAll(async () => { + await clean(); + await database.$client.end({ timeout: 5 }); +}); + +const from = (over: Partial<{ runId: string; depth: number }> = {}) => ({ + botId: ASKER, + actorId: ACTOR, + runId: `run-${suite}-1`, + threadId: `thread-${suite}`, + depth: 0, + ...over, +}); + +async function grantTarget() { + await database + .insert(pluginGrants) + .values({ kind: "bot", ref: TARGET, agentId: ASKER, grantedBy: "test" }) + .onConflictDoNothing(); +} + +describe("a hop, against the database", () => { + test("an ungranted Bot is refused, and granting it now is enough", async () => { + const before = await desk.send({ + from: from(), + target: "Target", + envelope: { task: "have a look" }, + }); + expect(before.ok).toBe(false); + + // Read per hop and never held, so this applies to the very next one rather than after a restart. + await grantTarget(); + + const after = await desk.send({ + from: from(), + target: "Target", + envelope: { task: "have a look" }, + }); + expect(after).toMatchObject({ ok: true, to: TARGET }); + }); + + /* + * The key is the only thing between a retried delivery and a second run of the receiving Bot, and + * it is the database that decides whether two offers collide. + */ + test("the same hop offered twice leaves one row", async () => { + await grantTarget(); + const send = () => + desk.send({ + from: from(), + target: "Target", + envelope: { task: "have a look" }, + }); + + await send(); + await send(); + + /* + * Found by payload rather than by key prefix. The run is HASHED into the key — `runId` arrives + * on the request, and written in raw a run calling itself `notice` aliased the prefix every + * failure notice is keyed under — so a test that greps for the raw id is asserting the bug. + */ + const rows = await database + .select({ key: workItems.key, payload: workItems.payload }) + .from(workItems) + .where(eq(workItems.kind, HANDOFF_KIND)); + const mine = rows.filter( + (row) => (row.payload as { runId?: string }).runId === `run-${suite}-1`, + ); + expect(mine).toHaveLength(1); + // And the id the caller chose is nowhere in the key it produced. + expect(mine[0]?.key).not.toContain(`run-${suite}-1`); + }); + + /* + * Counted from rows rather than a variable, because the hops of one run land on several pods and a + * count held in a process counts one of them. + */ + test("the fan-out cap counts rows another replica could have written", async () => { + await grantTarget(); + const runId = `run-${suite}-2`; + + expect( + ( + await desk.send({ + from: from({ runId }), + target: "Target", + envelope: { task: "first" }, + }) + ).ok, + ).toBe(true); + expect( + ( + await desk.send({ + from: from({ runId }), + target: "Target", + envelope: { task: "second" }, + }) + ).ok, + ).toBe(true); + + // Two is the cap for this suite, so the third is refused whichever replica asks. + const third = await desk.send({ + from: from({ runId }), + target: "Target", + envelope: { task: "third" }, + }); + expect(third.ok).toBe(false); + }); + + test("a chain at the cap is refused and leaves a row saying why", async () => { + await grantTarget(); + const runId = `run-${suite}-3`; + + const outcome = await desk.send({ + from: from({ runId, depth: 2 }), + target: "Target", + envelope: { task: "keep going" }, + }); + + expect(outcome.ok).toBe(false); + const rows = await database + .select({ payload: auditEvents.payload }) + .from(auditEvents) + .where(eq(auditEvents.eventType, "agent.handoff_refused")); + expect( + rows.some( + (row) => + (row.payload as { run?: string; reason?: string }).run === runId && + (row.payload as { reason?: string }).reason === "depth_cap", + ), + ).toBe(true); + }); + + test("an accepted hop carries the actor, the thread and the next depth", async () => { + await grantTarget(); + const runId = `run-${suite}-4`; + + await desk.send({ + from: from({ runId, depth: 1 }), + target: "Target", + envelope: { task: "have a look", expecting: "a date range" }, + }); + + const rows = await database + .select({ payload: workItems.payload }) + .from(workItems) + .where(eq(workItems.kind, HANDOFF_KIND)); + const row = rows.find( + (candidate) => (candidate.payload as { runId?: string }).runId === runId, + ); + expect(row?.payload).toMatchObject({ + fromBotId: ASKER, + toBotId: TARGET, + actorId: ACTOR, + threadId: `thread-${suite}`, + depth: 2, + task: "have a look", + expecting: "a date range", + }); + }); +}); diff --git a/server/tests/agent-handoff.test.ts b/server/tests/agent-handoff.test.ts new file mode 100644 index 00000000..0aebe548 --- /dev/null +++ b/server/tests/agent-handoff.test.ts @@ -0,0 +1,533 @@ +import { describe, expect, test } from "bun:test"; +import { + createHandoffDesk, + HANDOFF_KIND, + type HandoffCaps, +} from "../src/agents/handoff"; +import type { + AgentProfile, + AgentProfileStore, +} from "../src/agents/profile-store"; +import type { AuditStore } from "../src/audit"; +import type { WorkQueue } from "../src/work/queue"; + +/** + * One Bot handing work to another, and the four things that must never happen. + * + * A loop that bills for every hop. A fan-out that wakes four sleeping computers because one Bot was + * chatty. A Bot reaching a Bot its person cannot see. And a Bot reaching one nobody granted it. + * + * Every refusal is an answer rather than an exception, because the asking Bot is mid-run with a + * person waiting: a throw ends the run with nothing said, which reads as the Bot ignoring them. + */ + +const CAPS: HandoffCaps = { maxDepth: 2, maxPerRun: 3 }; + +function profile(over: Partial & { id: string }): AgentProfile { + return { + name: over.id, + title: "", + roleDescription: "", + avatarSeed: over.id, + visibility: "public", + endpoint: null, + hasAuth: false, + hasCallbackToken: false, + hidden: false, + systemOwned: false, + canManage: false, + mine: false, + ownerUserId: null, + deletedAt: null, + ...over, + } as AgentProfile; +} + +function desk(options?: { + roster?: AgentProfile[]; + granted?: boolean; + offered?: number; + caps?: HandoffCaps; + role?: "admin" | "user"; +}) { + const rows: Array<{ kind: string; key: string; payload: unknown }> = []; + const events: Array<{ eventType: string; payload: Record }> = + []; + + const queue = { + offer: async (item: { + kind: string; + key: string; + payload?: unknown; + atMost?: { keyPrefix: string; max: number }; + }) => { + // Idempotent on the key, exactly as the real one is — and it says so, because "already + // there" and "just queued" are different answers to the caller. + if (rows.some((row) => row.key === item.key)) return "already"; + // And the cap, counted and written as one step, exactly as the real one is. + if (item.atMost && (options?.offered ?? rows.length) >= item.atMost.max) { + return "refused"; + } + rows.push({ kind: item.kind, key: item.key, payload: item.payload }); + return "queued"; + }, + } as unknown as WorkQueue; + + const profiles = { + list: async () => + options?.roster ?? [profile({ id: "researcher", name: "Researcher" })], + } as unknown as AgentProfileStore; + + const recorded = events; + const auditStore: AuditStore = { + insert: async (event) => { + recorded.push({ + eventType: event.eventType, + payload: event.payload ?? {}, + }); + }, + }; + + return { + rows, + events: recorded, + desk: createHandoffDesk({ + queue, + profiles, + mayAddress: async () => options?.granted ?? true, + actorFor: async (id: string) => ({ + id, + role: options?.role ?? ("user" as const), + }), + auditStore, + caps: options?.caps ?? CAPS, + }), + }; +} + +const FROM = { + botId: "assistant", + actorId: "user-1", + runId: "run-1", + threadId: "thread-1", + depth: 0, +}; + +describe("handing work to another Bot", () => { + test("an allowed hop becomes one durable row", async () => { + const { desk: handoff, rows } = desk(); + + const outcome = await handoff.send({ + from: FROM, + target: "Researcher", + envelope: { task: "find the outage window", expecting: "a date range" }, + }); + + expect(outcome).toMatchObject({ ok: true, to: "researcher" }); + expect(rows).toHaveLength(1); + expect(rows[0]?.kind).toBe(HANDOFF_KIND); + expect(rows[0]?.payload).toMatchObject({ + fromBotId: "assistant", + toBotId: "researcher", + actorId: "user-1", + // One deeper than the run that asked, so the cap keeps counting across pods. + depth: 1, + }); + }); + + /* + * The key is what stops a retried delivery running the other Bot twice, so the same envelope sent + * twice in one run has to land on the same key. A fresh id per attempt is at-least-once with no + * ceiling. + */ + test("the same request twice in one run is one hop", async () => { + const { desk: handoff, rows } = desk(); + const send = () => + handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "find the outage window" }, + }); + + await send(); + await send(); + + expect(rows).toHaveLength(1); + }); + + test("a different request in the same run is a different hop", async () => { + const { desk: handoff, rows } = desk(); + + await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "find the outage window" }, + }); + await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "find who was on call" }, + }); + + expect(rows).toHaveLength(2); + }); + + /* A asks B asks C asks A, which is the obvious failure and spends real money going round. */ + test("a chain already at the depth cap is refused", async () => { + const { desk: handoff, rows } = desk(); + + const outcome = await handoff.send({ + from: { ...FROM, depth: 2 }, + target: "researcher", + envelope: { task: "keep going" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + test("a deployment with a depth cap of zero allows no hop at all", async () => { + const { desk: handoff, rows } = desk({ + caps: { maxDepth: 0, maxPerRun: 3 }, + }); + + const outcome = await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "anything" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + /* Counted from the rows rather than a variable, because the hops land on several pods. */ + test("a run that has already asked its limit is refused", async () => { + const { desk: handoff, rows } = desk({ offered: 3 }); + + const outcome = await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "one more" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + /* + * Resolved against the roster the asking PERSON may see. Otherwise a Bot names anything and the + * deployment goes and finds it, which is a way around agent visibility. + */ + test("a Bot the person cannot see cannot be reached", async () => { + const { desk: handoff, rows } = desk({ roster: [] }); + + const outcome = await handoff.send({ + from: FROM, + target: "payroll", + envelope: { task: "what is everyone paid" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + /* + * And it reads the same as one that does not exist. Two different sentences would let a Bot + * enumerate the roster by asking for names and reading which refusal came back. + */ + test("an unreachable Bot and a missing one are refused in the same words", async () => { + const hidden = await desk({ + roster: [profile({ id: "payroll", name: "Payroll", hidden: true })], + }).desk.send({ + from: FROM, + target: "Payroll", + envelope: { task: "t" }, + }); + const missing = await desk({ roster: [] }).desk.send({ + from: FROM, + target: "Payroll", + envelope: { task: "t" }, + }); + + expect(hidden.ok).toBe(false); + expect(missing.ok).toBe(false); + expect((hidden as { refusal: string }).refusal).toBe( + (missing as { refusal: string }).refusal, + ); + }); + + test("a Bot nobody granted cannot be reached", async () => { + const { desk: handoff, rows } = desk({ granted: false }); + + const outcome = await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "have a look" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + test("a Bot cannot hand work to itself", async () => { + const { desk: handoff, rows } = desk({ + roster: [profile({ id: "assistant", name: "Assistant" })], + }); + + const outcome = await handoff.send({ + from: FROM, + target: "assistant", + envelope: { task: "do it again" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + test("a hop with nothing asked is refused", async () => { + const { desk: handoff, rows } = desk(); + + const outcome = await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: " " }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); + + /* + * The refused row matters more than the accepted one. A hop that happened shows in the transcript; + * a hop that was refused is invisible everywhere else, and "why did it not ask the specialist" is + * the question somebody asks about a thin answer. + */ + test("both outcomes leave a row naming the run and the reason", async () => { + const allowed = desk(); + await allowed.desk.send({ + from: FROM, + target: "researcher", + envelope: { task: "t" }, + }); + expect(allowed.events.map((event) => event.eventType)).toEqual([ + "agent.handoff_offered", + ]); + expect(allowed.events[0]?.payload).toMatchObject({ + from: "assistant", + to: "researcher", + run: "run-1", + }); + + const refused = desk({ granted: false }); + await refused.desk.send({ + from: FROM, + target: "researcher", + envelope: { task: "t" }, + }); + expect(refused.events.map((event) => event.eventType)).toEqual([ + "agent.handoff_refused", + ]); + expect(refused.events[0]?.payload).toMatchObject({ + reason: "not_granted", + run: "run-1", + }); + }); +}); + +/* + * Where the answer goes comes from the signed assertion, never from the model. A Bot naming its own + * thread would be a Bot able to drop a turn into a conversation it was never part of. + */ +describe("where a hop's answer lands", () => { + test("comes from the assertion", async () => { + const { desk: handoff, rows } = desk(); + + await handoff.send({ + from: FROM, + target: "researcher", + envelope: { task: "t" }, + }); + + expect(rows[0]?.payload).toMatchObject({ threadId: "thread-1" }); + }); + + test("a run with no conversation cannot hand work on", async () => { + const { desk: handoff, rows } = desk(); + + const outcome = await handoff.send({ + from: { ...FROM, threadId: undefined }, + target: "researcher", + envelope: { task: "t" }, + }); + + expect(outcome.ok).toBe(false); + expect(rows).toEqual([]); + }); +}); + +/** + * Two Bots with one name. + * + * `agents.name` has no unique constraint and duplicating a Bot deliberately makes a second with the + * same name, so a person can be looking at two called Knowledge. Taking whichever sorted first sends + * the work to a Bot nobody meant, or refuses a legitimate hop as "not granted" because the other + * twin is the granted one. Neither says a word about there having been two. + */ +describe("a name that means more than one Bot", () => { + test("is refused, naming the ids to choose between", async () => { + const twins = desk({ + roster: [ + profile({ id: "knowledge-a", name: "Knowledge" }), + profile({ id: "knowledge-b", name: "Knowledge" }), + ], + }); + + const outcome = await twins.desk.send({ + from: FROM, + target: "Knowledge", + envelope: { task: "find the policy" }, + }); + + expect(outcome.ok).toBe(false); + if (!outcome.ok) { + expect(outcome.refusal).toContain("knowledge-a"); + expect(outcome.refusal).toContain("knowledge-b"); + } + expect( + twins.events.map((event) => ({ + eventType: event.eventType, + reason: (event.payload as { reason?: string }).reason, + })), + ).toEqual([ + { eventType: "agent.handoff_refused", reason: "ambiguous_bot" }, + ]); + }); + + test("but the id still reaches exactly the one it names", async () => { + const twins = desk({ + roster: [ + profile({ id: "knowledge-a", name: "Knowledge" }), + profile({ id: "knowledge-b", name: "Knowledge" }), + ], + }); + + const outcome = await twins.desk.send({ + from: FROM, + target: "knowledge-b", + envelope: { task: "find the policy" }, + }); + + expect(outcome.ok).toBe(true); + if (outcome.ok) expect(outcome.to).toBe("knowledge-b"); + }); +}); + +/** + * Whose roster the target is resolved against. + * + * An administrator sees Bots a user does not. Assumed to be a user, an administrator'"'"'s hop to a Bot + * they can see and chat with in the UI was refused as "no such Bot" — the same failure `index.ts` + * warns about for a routine'"'"'s owner. + */ +describe("the role a hop is resolved as", () => { + test("is asked for rather than assumed", async () => { + const asked: Array<{ id: string; role: string }> = []; + const profiles = { + list: async (actor: { id: string; role: string }) => { + asked.push(actor); + return [profile({ id: "researcher", name: "Researcher" })]; + }, + } as unknown as AgentProfileStore; + + const built = createHandoffDesk({ + queue: { + offer: async () => "queued", + } as unknown as WorkQueue, + profiles, + mayAddress: async () => true, + actorFor: async (id) => ({ id, role: "admin" }), + auditStore: { insert: async () => {} }, + caps: CAPS, + }); + + await built.send({ + from: FROM, + target: "researcher", + envelope: { task: "find it" }, + }); + + expect(asked).toEqual([{ id: "user-1", role: "admin" }]); + }); +}); + +/** + * Asking for the same thing twice. + * + * `offer` is idempotent on the key, so a model repeating itself inside one run leaves one hop, which + * is the intent. What must not happen is being told "handed over" a second time: the row it names + * may already have been delivered and finished, so nothing is queued, nobody is going to run it, and + * the Bot has just promised the person an answer twice. + */ +describe("the same ask a second time", () => { + test("is refused plainly rather than reported as handed over", async () => { + const twice = desk(); + + const first = await twice.desk.send({ + from: FROM, + target: "researcher", + envelope: { task: "find the outage window" }, + }); + const second = await twice.desk.send({ + from: FROM, + target: "researcher", + envelope: { task: "find the outage window" }, + }); + + expect(first.ok).toBe(true); + expect(second.ok).toBe(false); + if (!second.ok) { + expect(second.refusal).toContain("already asked"); + // Not a claim about when: the run id arrives on the request, so "this turn" can be false. + expect(second.refusal).not.toContain("this turn"); + } + // One row, and one refusal on the trail beside the offer. + expect(twice.rows).toHaveLength(1); + expect( + twice.events.map((event) => ({ + eventType: event.eventType, + reason: (event.payload as { reason?: string }).reason, + })), + ).toEqual([ + { eventType: "agent.handoff_offered", reason: undefined }, + { eventType: "agent.handoff_refused", reason: "duplicate" }, + ]); + }); + + /* + * A role that cannot be read is not a role. Everything in this module answers with a sentence, so + * a seam that throws would end the run with nothing said at all. + */ + test("a person whose role cannot be established is refused, not thrown at", async () => { + const unknown = createHandoffDesk({ + queue: { offer: async () => "queued" } as unknown as WorkQueue, + profiles: { + list: async () => [profile({ id: "researcher", name: "Researcher" })], + } as unknown as AgentProfileStore, + mayAddress: async () => true, + actorFor: async () => null, + auditStore: { insert: async () => {} }, + caps: CAPS, + }); + + const outcome = await unknown.send({ + from: FROM, + target: "researcher", + envelope: { task: "find it" }, + }); + + expect(outcome.ok).toBe(false); + if (!outcome.ok) + expect(outcome.refusal).toContain("could not be confirmed"); + }); +}); diff --git a/server/tests/agent-key-rotation.integration.test.ts b/server/tests/agent-key-rotation.integration.test.ts index 24f81e6c..1475edf9 100644 --- a/server/tests/agent-key-rotation.integration.test.ts +++ b/server/tests/agent-key-rotation.integration.test.ts @@ -1,11 +1,11 @@ import { afterAll, beforeAll, describe, expect, test } from "bun:test"; import { randomUUID } from "node:crypto"; import { and, eq, inArray, isNull } from "drizzle-orm"; +import { createAgentProfileStore } from "../src/agents/profile-store"; +import type { AgentActor } from "../src/agents/profile-types"; import { createCredentialStore } from "../src/credentials"; import { createDatabase } from "../src/db/client"; import { agentProfiles, agents, credentials, users } from "../src/db/schema"; -import { createAgentProfileStore } from "../src/agents/profile-store"; -import type { AgentActor } from "../src/agents/profile-types"; /** * Editing a Bot's key, against a real database. diff --git a/server/tests/agent-profile-store.integration.test.ts b/server/tests/agent-profile-store.integration.test.ts index 32fc248f..01dc052e 100644 --- a/server/tests/agent-profile-store.integration.test.ts +++ b/server/tests/agent-profile-store.integration.test.ts @@ -16,7 +16,6 @@ import type { } from "../src/agents/profile-types"; import { DEPLOYMENT_ROUTES } from "../src/computer/deployment-routes"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentPreferences, agentProfiles, @@ -27,6 +26,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; const databaseUrl = process.env.DATABASE_URL ?? diff --git a/server/tests/builtin-routines.test.ts b/server/tests/builtin-routines.test.ts index e7e9757f..a9b3d8c6 100644 --- a/server/tests/builtin-routines.test.ts +++ b/server/tests/builtin-routines.test.ts @@ -2,14 +2,14 @@ import { afterEach, describe, expect, test } from "bun:test"; import { callTool, listTools, - useRoutineTools, type RoutineTools, + useRoutineTools, } from "../src/plugins/builtin-routines"; import { - RoutineNotFoundError, - RoutineRefusedError, type Routine, + RoutineNotFoundError, type RoutinePatch, + RoutineRefusedError, type RoutineSummary, } from "../src/routines/store"; diff --git a/server/tests/channel-activity.integration.test.ts b/server/tests/channel-activity.integration.test.ts index a592eb88..f8c857be 100644 --- a/server/tests/channel-activity.integration.test.ts +++ b/server/tests/channel-activity.integration.test.ts @@ -443,3 +443,54 @@ describe("a pinned channel in a paged roster", () => { ]); }); }); + +/** + * The one conversation a person has with one Bot. + * + * A hop delivers into it, and a Bot asked for several things in one turn produces several hops at + * once. Looking and then making is not find-or-create: each of two concurrent deliveries found + * nothing and made a conversation, so that person had two Knowledge channels holding two threads, + * with the answers split between them. + */ +describe("finding or making a person's channel with one Bot", () => { + test("two at once get the same conversation, not one each", async () => { + const owner = await createUser(); + const agentId = await createAgent(owner, "Knowledge"); + + const [first, second] = await Promise.all([ + store.direct(owner, agentId), + store.direct(owner, agentId), + ]); + createdChannelIds.push(first.id, second.id); + + expect(second.id).toBe(first.id); + expect(second.threadId).toBe(first.threadId); + }); + + test("an existing conversation is reused rather than added to", async () => { + const owner = await createUser(); + const agentId = await createAgent(owner, "Knowledge"); + const made = await createChannel(owner, [agentId]); + + const found = await store.direct(owner, agentId); + + expect(found.id).toBe(made.id); + }); + + /* + * A channel holding this Bot and another one matches an agent test on its own. Delivering into it + * would put a hop's answer in front of a Bot nobody had asked. + */ + test("a channel with a second Bot in it is not that person's direct one", async () => { + const owner = await createUser(); + const agentId = await createAgent(owner, "Knowledge"); + const other = await createAgent(owner, "Research"); + const shared = await createChannel(owner, [agentId, other]); + + const found = await store.direct(owner, agentId); + createdChannelIds.push(found.id); + + expect(found.id).not.toBe(shared.id); + expect(found.agentIds).toEqual([agentId]); + }); +}); diff --git a/server/tests/channel-events.integration.test.ts b/server/tests/channel-events.integration.test.ts index 697217a9..75c195f6 100644 --- a/server/tests/channel-events.integration.test.ts +++ b/server/tests/channel-events.integration.test.ts @@ -16,7 +16,6 @@ import { } from "../src/channels/routes"; import { createThreadIdentity } from "../src/channels/thread-identity"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, @@ -26,6 +25,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; function event(overrides: Partial = {}) { return { diff --git a/server/tests/channel-routes.test.ts b/server/tests/channel-routes.test.ts index 7019b1f9..031b063c 100644 --- a/server/tests/channel-routes.test.ts +++ b/server/tests/channel-routes.test.ts @@ -31,7 +31,6 @@ import { import { createThreadIdentity } from "../src/channels/thread-identity"; import { loadConfig } from "../src/config"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, @@ -42,6 +41,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; import { testEnvironment } from "./support/environment"; const actor = { diff --git a/server/tests/component-store.integration.test.ts b/server/tests/component-store.integration.test.ts index a71e25ae..f79220f2 100644 --- a/server/tests/component-store.integration.test.ts +++ b/server/tests/component-store.integration.test.ts @@ -8,13 +8,13 @@ import { createComponentStore, } from "../src/components/store"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agents, componentExclusions, componentFunctions, components, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; /** * The grant surface, against a real database. diff --git a/server/tests/config.test.ts b/server/tests/config.test.ts index 687b894a..0c887811 100644 --- a/server/tests/config.test.ts +++ b/server/tests/config.test.ts @@ -1,5 +1,5 @@ -import { readFileSync } from "node:fs"; import { describe, expect, spyOn, test } from "bun:test"; +import { readFileSync } from "node:fs"; import { configuredAuthProviders, loadConfig } from "../src/config"; // Intelligence is part of the MINIMUM contract, so it belongs in the base environment every other @@ -640,3 +640,37 @@ describe("AGENT_ENDPOINT_ALLOWED_HOSTS", () => { ).toThrow(/Patterns are not accepted/); }); }); + +/** + * A cap is a safety number, so a value that is not one has to stop the deployment rather than be + * quietly replaced by the default. Somebody who typed `two` would otherwise believe they had set a + * cap, and find out at the first loop. + */ +describe("how far a Bot may hand work on", () => { + test("defaults to one level and three per run", () => { + const config = loadConfig({ ...baseEnvironment }); + expect(config.handoff).toEqual({ maxDepth: 1, maxPerRun: 3 }); + }); + + test("a deployment can widen or switch it off", () => { + expect( + loadConfig({ + ...baseEnvironment, + BOT_HANDOFF_MAX_DEPTH: "0", + BOT_HANDOFF_MAX_PER_RUN: "10", + }).handoff, + ).toEqual({ maxDepth: 0, maxPerRun: 10 }); + }); + + test("refuses a cap that is not a whole number", () => { + expect(() => + loadConfig({ ...baseEnvironment, BOT_HANDOFF_MAX_DEPTH: "two" }), + ).toThrow("BOT_HANDOFF_MAX_DEPTH"); + expect(() => + loadConfig({ ...baseEnvironment, BOT_HANDOFF_MAX_PER_RUN: "-1" }), + ).toThrow("BOT_HANDOFF_MAX_PER_RUN"); + expect(() => + loadConfig({ ...baseEnvironment, BOT_HANDOFF_MAX_PER_RUN: "1.5" }), + ).toThrow("BOT_HANDOFF_MAX_PER_RUN"); + }); +}); diff --git a/server/tests/credentials.test.ts b/server/tests/credentials.test.ts index 32970be7..da18b1b8 100644 --- a/server/tests/credentials.test.ts +++ b/server/tests/credentials.test.ts @@ -14,8 +14,8 @@ import { rotateCredential, } from "../src/credentials"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { credentials } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; import { testEnvironment } from "./support/environment"; const key = "AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="; diff --git a/server/tests/dev-actor.integration.test.ts b/server/tests/dev-actor.integration.test.ts index 9b4872ba..5066227c 100644 --- a/server/tests/dev-actor.integration.test.ts +++ b/server/tests/dev-actor.integration.test.ts @@ -3,8 +3,8 @@ import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import { DEV_ACTOR, initializeDevActorUser } from "../src/auth/dev-actor"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { users } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; const databaseUrl = process.env.DATABASE_URL ?? diff --git a/server/tests/google-drive-rest.test.ts b/server/tests/google-drive-rest.test.ts index ec69e08e..0cf1cbdf 100644 --- a/server/tests/google-drive-rest.test.ts +++ b/server/tests/google-drive-rest.test.ts @@ -1,7 +1,7 @@ import { afterEach, describe, expect, test } from "bun:test"; +import { catalogueEntry } from "../src/plugins/catalogue"; import { callTool, listTools } from "../src/plugins/google-drive-rest"; import { transportFor } from "../src/plugins/transport"; -import { catalogueEntry } from "../src/plugins/catalogue"; /** * The Drive REST adapter, asserted without Google. diff --git a/server/tests/handoff-caps-defaults.test.ts b/server/tests/handoff-caps-defaults.test.ts new file mode 100644 index 00000000..a2f6efac --- /dev/null +++ b/server/tests/handoff-caps-defaults.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, test } from "bun:test"; +import { parse } from "yaml"; +import { loadConfig } from "../src/config"; +import { testEnvironment } from "./support/environment"; + +/** + * One number, written down in four places. + * + * The caps have a fallback in `config.ts`, a default in the chart's `values.yaml`, a second default + * in `_helpers.tpl` (which has to be there, because `--reuse-values` leaves the values key absent on + * an existing release), and a figure quoted in `docs/configuration.md`. The helper always renders + * the variable, so on Kubernetes the code fallback never runs and the docs describe a source the + * deployment is not using. + * + * Nothing here can merge them: they are read by three different things at three different times. So + * they are held together, and the next person to change one finds out here rather than from an + * operator debugging a refusal against a number their deployment never had. + */ + +const chart = parse(await Bun.file("charts/openbot/values.yaml").text()) as { + config?: { handoff?: { maxDepth?: number; maxPerRun?: number } }; +}; + +const docs = await Bun.file("docs/configuration.md").text(); + +/** What `handoffCaps` falls back to with nothing in the environment. */ +const code = loadConfig(testEnvironment()).handoff; + +describe("the handoff caps say the same thing everywhere", () => { + test("the chart's values match the code's fallbacks", () => { + expect(chart.config?.handoff?.maxDepth).toBe(code.maxDepth); + expect(chart.config?.handoff?.maxPerRun).toBe(code.maxPerRun); + }); + + /* + * The TEMPLATE's own fallback, and that it does not eat a deliberate zero, are asserted where Helm + * exists — `scripts/check-new-values-keys.ts`, run by the chart job. This suite runs in a job with + * no Helm binary, and a test that shells out to one that is not there does not fail, it returns + * undefined and compares it to nothing. That is how the first version of this passed locally and + * failed in CI. + * + * The two halves chain: the script holds the rendered fallback to values.yaml, and this holds + * values.yaml to the code and the docs. + */ + + test("and the documented defaults are those numbers", () => { + const row = (name: string) => + docs.split("\n").find((line) => line.includes(name)) ?? ""; + expect(row("BOT_HANDOFF_MAX_DEPTH")).toContain(`\`${code.maxDepth}\``); + expect(row("BOT_HANDOFF_MAX_PER_RUN")).toContain(`\`${code.maxPerRun}\``); + }); +}); diff --git a/server/tests/jsonb-encoding.integration.test.ts b/server/tests/jsonb-encoding.integration.test.ts index 1ec02261..50956e41 100644 --- a/server/tests/jsonb-encoding.integration.test.ts +++ b/server/tests/jsonb-encoding.integration.test.ts @@ -3,8 +3,8 @@ import { randomUUID } from "node:crypto"; import { eq, sql } from "drizzle-orm"; import { createAuditStore, recordAuditEvent } from "../src/audit"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agents } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; /** * A jsonb column must hold JSON, not a string that looks like it. diff --git a/server/tests/plugin-oauth.test.ts b/server/tests/plugin-oauth.test.ts index 853adc53..b8487c9d 100644 --- a/server/tests/plugin-oauth.test.ts +++ b/server/tests/plugin-oauth.test.ts @@ -4,12 +4,12 @@ import type { CatalogueAuth } from "../src/plugins/catalogue"; import { authorizationUrlFor, challengeFor, + connectedAccountsUrlFor, createVerifier, readConnectState, redeemAuthorizationCode, redirectUriFor, registerDynamicClient, - connectedAccountsUrlFor, sealConnectState, } from "../src/plugins/oauth"; diff --git a/server/tests/plugin-routes.test.ts b/server/tests/plugin-routes.test.ts index cace693d..14c07276 100644 --- a/server/tests/plugin-routes.test.ts +++ b/server/tests/plugin-routes.test.ts @@ -103,3 +103,307 @@ describe("adding a curated server", () => { expect((await request({ key: "google-drive" })).status).toBe(403); }); }); + +/** + * Granting one Bot to another, through the API an administrator actually has. + * + * The grant table gained a `bot` kind and the store learned it, but these two endpoints did not. + * Revoke rejected it outright, so enabling the capability meant writing a row by hand and revoking + * it was not possible at all — while the design says a revoked grant applies to the very next hop. + * + * `kind` also arrives in a JSON body, so a type annotation on it is a comment. It is checked here. + */ +function grantsApp( + role: "admin" | "user" = "admin", + runsHere: (agentId: string) => boolean | undefined = (agentId) => { + // Undefined is "no such Bot", which is what the store answers for one nobody registered. + if (agentId === "never-registered") return undefined; + return agentId !== "at-an-endpoint"; + }, +) { + const calls: Array<{ verb: string; kind: string; ref: string }> = []; + const store = { + listServers: async () => [], + listSkills: async () => [], + listGrants: async () => [], + grant: async (kind: string, ref: string) => { + calls.push({ verb: "grant", kind, ref }); + }, + revoke: async (kind: string, ref: string) => { + calls.push({ verb: "revoke", kind, ref }); + }, + skillOwner: async () => null, + agentOwner: async () => null, + agentRunsHere: async (agentId: string) => runsHere(agentId), + agentIsRegistered: async (agentId: string) => + agentId !== "never-registered", + }; + + const app = createApp( + loadConfig(testEnvironment()), + { + handler: () => new Response(null, { status: 204 }), + api: { getSession: async () => ({ user: ADMIN }) }, + } as never, + { rolesForUser: async () => [role] }, + ...(Array.from({ length: 11 }) as never[]), + store as never, + ); + + return { calls, app }; +} + +describe("granting one Bot to another", () => { + test("an administrator can grant it", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "knowledge", + agentId: "assistant", + }), + }, + ); + + expect(response.status).toBe(200); + expect(calls).toEqual([{ verb: "grant", kind: "bot", ref: "knowledge" }]); + }); + + /* + * The half that was missing entirely. "Nothing about who may address whom is cached in a process" + * is only true if there is a way to stop it. + */ + test("and revoke it again", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants?kind=bot&ref=knowledge&agentId=assistant", + { method: "DELETE" }, + ); + + expect(response.status).toBe(200); + expect(calls).toEqual([{ verb: "revoke", kind: "bot", ref: "knowledge" }]); + }); + + /* + * It lets one Bot spend another's model calls, wake its computer and reach whatever that Bot may + * reach. That is not an instruction somebody attaches to a coworker they own. + */ + test("somebody who is not an administrator cannot", async () => { + const { calls, app } = grantsApp("user"); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "knowledge", + agentId: "assistant", + }), + }, + ); + + expect(response.status).toBe(403); + expect(await response.json()).toEqual({ + error: + "An administrator decides which Bots may hand work to another Bot.", + }); + expect(calls).toEqual([]); + }); + + test("a kind nobody defined is refused rather than written", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "anything", + ref: "x", + agentId: "assistant", + }), + }, + ); + + expect(response.status).toBe(400); + expect(calls).toEqual([]); + }); +}); + +/** + * A grant that could never do anything. + * + * Handing work to another Bot is a tool this deployment executes, so it can only be offered to a run + * this deployment builds. A Bot at its own endpoint runs its own loop and is handed descriptions of + * what it may call back for; there is no callback path that would execute a hop. Stored anyway, the + * grant reads as configured and nothing ever happens. + */ +describe("granting a hop to a Bot that runs somewhere else", () => { + test("is refused, and says why", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "knowledge", + agentId: "at-an-endpoint", + }), + }, + ); + + expect(response.status).toBe(403); + expect((await response.json()).error).toContain("its own endpoint"); + expect(calls).toEqual([]); + }); + + test("a Bot nobody has heard of is refused too", async () => { + // Undefined is "no such Bot", which must not read as "runs somewhere else" or as permission. + const { calls, app } = grantsApp("admin", () => undefined); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "knowledge", + agentId: "never-registered", + }), + }, + ); + + expect(response.status).toBe(403); + expect((await response.json()).error).toBe("There is no such Bot."); + expect(calls).toEqual([]); + }); + + test("a Bot that does run here is granted as before", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "knowledge", + agentId: "general-assistant", + }), + }, + ); + + expect(response.status).toBe(200); + expect(calls).toEqual([{ verb: "grant", kind: "bot", ref: "knowledge" }]); + }); +}); + +/** + * What a refusal tells somebody who is not an administrator. + * + * This route only requires a signed-in user. Checking whether a Bot exists, and whether it runs + * here, before checking the role handed out three distinguishable 403s and turned the refusal into + * an oracle for other people's private Bots — the exact property `handoff.ts` collapses on purpose. + */ +describe("what a bot grant refusal reveals", () => { + const refusalFor = async ( + agentId: string, + role: "admin" | "user", + ref = "knowledge", + ) => { + const { calls, app } = grantsApp(role); + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ kind: "bot", ref, agentId }), + }, + ); + return { status: response.status, body: await response.json(), calls }; + }; + + test("a non-administrator gets one answer, whatever the Bot is", async () => { + const said = new Set(); + for (const agentId of [ + "general-assistant", + "at-an-endpoint", + "never-registered", + ]) { + const { status, body, calls } = await refusalFor(agentId, "user"); + expect(status).toBe(403); + expect(calls).toEqual([]); + said.add(body.error); + } + // One sentence for all three, so nothing distinguishes "exists" from "does not". + expect(said.size).toBe(1); + expect([...said][0]).toBe( + "An administrator decides which Bots may hand work to another Bot.", + ); + }); + + test("an administrator still gets the reason", async () => { + expect((await refusalFor("at-an-endpoint", "admin")).body.error).toContain( + "its own endpoint", + ); + expect((await refusalFor("never-registered", "admin")).body.error).toBe( + "There is no such Bot.", + ); + }); + + /* + * The target is bare text with no foreign key. A typo stored happily, `message_bot` was offered, + * and every hop then refused as not-granted. + */ + test("a target nobody has heard of is refused", async () => { + const { status, body, calls } = await refusalFor( + "general-assistant", + "admin", + "never-registered", + ); + expect(status).toBe(403); + expect(body.error).toContain("no Bot called never-registered"); + expect(calls).toEqual([]); + }); +}); + +/* + * The desk refuses a self-hop outright — "a Bot cannot hand work to itself" — so a grant of a Bot to + * itself is dead the moment it is written, and reads as configured. + */ +describe("granting a Bot itself", () => { + test("is refused rather than stored", async () => { + const { calls, app } = grantsApp(); + + const response = await app.request( + "http://openbot.test/api/plugins/grants", + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + kind: "bot", + ref: "general-assistant", + agentId: "general-assistant", + }), + }, + ); + + expect(response.status).toBe(403); + expect((await response.json()).error).toContain("cannot be granted itself"); + expect(calls).toEqual([]); + }); +}); diff --git a/server/tests/plugin-store.integration.test.ts b/server/tests/plugin-store.integration.test.ts index 646c108c..c82eb3d7 100644 --- a/server/tests/plugin-store.integration.test.ts +++ b/server/tests/plugin-store.integration.test.ts @@ -12,18 +12,17 @@ import { and, eq, inArray, like, sql } from "drizzle-orm"; import { createAuditStore } from "../src/audit"; import type { ActionPolicy } from "../src/computer/policy"; import { - createCredentialStore, type CredentialStoreValue, + createCredentialStore, decryptSecret, encryptSecret, } from "../src/credentials"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agents, auditEvents, - credentials, credentials as credentialRows, + credentials, mcpServers, mcpTools, mcpUserCredentials, @@ -34,8 +33,8 @@ import { catalogueEntry } from "../src/plugins/catalogue"; import { redirectUriFor } from "../src/plugins/oauth"; import { type AccessToken, - createPluginStore, CustomServerRefusedError, + createPluginStore, exchangeRefreshTokenOverHttp, INVALID_CLIENT, type OAuthClient, @@ -43,6 +42,7 @@ import { TokenRefusedError, unlistedAdvertisedTools, } from "../src/plugins/store"; +import { TEST_POOL } from "./support/database"; /** * The two questions a tool call has to pass, and the row each answer leaves behind. diff --git a/server/tests/policy-dry-run.test.ts b/server/tests/policy-dry-run.test.ts index cb875363..f6e3a07e 100644 --- a/server/tests/policy-dry-run.test.ts +++ b/server/tests/policy-dry-run.test.ts @@ -1,10 +1,10 @@ import { describe, expect, test } from "bun:test"; import type { AuditEvent } from "../src/audit"; +import type { ActionPolicy } from "../src/computer/policy"; import { contextFromAuditPayload, dryRunAgainstHistory, } from "../src/computer/policy-dry-run"; -import type { ActionPolicy } from "../src/computer/policy"; /** * The replay must judge a recorded action exactly as the gateway judged it live. Every case here is diff --git a/server/tests/policy-durability.integration.test.ts b/server/tests/policy-durability.integration.test.ts index d767297c..6e9e831a 100644 --- a/server/tests/policy-durability.integration.test.ts +++ b/server/tests/policy-durability.integration.test.ts @@ -5,8 +5,8 @@ import { DEFAULT_ACTION_POLICY, } from "../src/computer/policy-store"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { actionPolicy } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; /** * The boundary has to survive a restart. diff --git a/server/tests/routine-run-turn.test.ts b/server/tests/routine-run-turn.test.ts index de658158..05ce5ca9 100644 --- a/server/tests/routine-run-turn.test.ts +++ b/server/tests/routine-run-turn.test.ts @@ -1,6 +1,6 @@ -import { AbstractAgent, EventType } from "@ag-ui/client"; -import type { Message } from "@ag-ui/client"; import { describe, expect, test } from "bun:test"; +import type { Message } from "@ag-ui/client"; +import { AbstractAgent, EventType } from "@ag-ui/client"; import { EMPTY } from "rxjs"; import { createTurnRunner, diff --git a/server/tests/routine-sweep.integration.test.ts b/server/tests/routine-sweep.integration.test.ts index ebae2212..ff91094d 100644 --- a/server/tests/routine-sweep.integration.test.ts +++ b/server/tests/routine-sweep.integration.test.ts @@ -28,11 +28,11 @@ import { MINIMUM_INTERVAL_MS } from "../src/routines/schedule"; import { createRoutineStore } from "../src/routines/store"; import { DEFAULT_GRACE_MS, - ROUTINE_FIRE_KIND, dispatchClaimedRoutines, offerDueRoutines, + ROUTINE_FIRE_KIND, } from "../src/routines/sweep"; -import { DEFAULT_MAX_ATTEMPTS, createWorkQueue } from "../src/work/queue"; +import { createWorkQueue, DEFAULT_MAX_ATTEMPTS } from "../src/work/queue"; import { TEST_POOL } from "./support/database"; /** diff --git a/server/tests/routines-store.integration.test.ts b/server/tests/routines-store.integration.test.ts index 4702aa8f..b816de16 100644 --- a/server/tests/routines-store.integration.test.ts +++ b/server/tests/routines-store.integration.test.ts @@ -16,12 +16,12 @@ import { users, } from "../src/db/schema"; import { + createRoutineStore, MAX_ENABLED_ROUTINES, MAX_INSTRUCTION_CODE_POINTS, MAX_RUN_ERROR, RoutineNotFoundError, RoutineRefusedError, - createRoutineStore, } from "../src/routines/store"; import { TEST_POOL } from "./support/database"; diff --git a/server/tests/runtime-agents.integration.test.ts b/server/tests/runtime-agents.integration.test.ts index 82113b86..06881e23 100644 --- a/server/tests/runtime-agents.integration.test.ts +++ b/server/tests/runtime-agents.integration.test.ts @@ -8,7 +8,6 @@ import { createChannelStore } from "../src/channels/routes"; import { createThreadIdentity } from "../src/channels/thread-identity"; import { standingRoleMessage } from "../src/copilot"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, @@ -16,6 +15,7 @@ import { intelligenceChannelMappings, users, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; const databaseUrl = process.env.DATABASE_URL ?? diff --git a/server/tests/sandboxed-components.integration.test.ts b/server/tests/sandboxed-components.integration.test.ts index ed9b1445..767b681d 100644 --- a/server/tests/sandboxed-components.integration.test.ts +++ b/server/tests/sandboxed-components.integration.test.ts @@ -7,7 +7,6 @@ import { SandboxedNotFoundError, } from "../src/components/sandboxed"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agents, componentExclusions, @@ -15,6 +14,7 @@ import { components, sandboxedComponents, } from "../src/db/schema"; +import { TEST_POOL } from "./support/database"; /** * A component authored in a browser can be edited freely and still reach nobody until it is diff --git a/server/tests/schema.test.ts b/server/tests/schema.test.ts index 23843082..1a3104e9 100644 --- a/server/tests/schema.test.ts +++ b/server/tests/schema.test.ts @@ -13,8 +13,8 @@ import { channelAgents, channelMemberships, channels, - credentials, credentialKind, + credentials, intelligenceChannelMappings, mcpUserCredentials, sessions, diff --git a/server/tests/skill-ownership.integration.test.ts b/server/tests/skill-ownership.integration.test.ts index 770f7d80..2b125f9e 100644 --- a/server/tests/skill-ownership.integration.test.ts +++ b/server/tests/skill-ownership.integration.test.ts @@ -4,10 +4,10 @@ import { inArray } from "drizzle-orm"; import { createAuditStore } from "../src/audit"; import type { ActionPolicy } from "../src/computer/policy"; import { createDatabase } from "../src/db/client"; -import { TEST_POOL } from "./support/database"; import { agentProfiles, agents, skills, users } from "../src/db/schema"; import { createPluginRoutes } from "../src/plugins/routes"; import { createPluginStore } from "../src/plugins/store"; +import { TEST_POOL } from "./support/database"; /** * Whose skill is whose, and which Bots a person may put one on. diff --git a/server/tests/thread-routes.test.ts b/server/tests/thread-routes.test.ts index b44f3e8b..91d76cfa 100644 --- a/server/tests/thread-routes.test.ts +++ b/server/tests/thread-routes.test.ts @@ -1,9 +1,9 @@ import { describe, expect, test } from "bun:test"; -import { Hono } from "hono"; import type { MiddlewareHandler } from "hono"; -import type { ThreadReader } from "../src/channels/thread-routes"; +import { Hono } from "hono"; import type { AppVariables } from "../src/auth/guards"; import { createThreadIdentity } from "../src/channels/thread-identity"; +import type { ThreadReader } from "../src/channels/thread-routes"; import { createThreadRoutes } from "../src/channels/thread-routes"; /** diff --git a/server/tests/work-loop.test.ts b/server/tests/work-loop.test.ts new file mode 100644 index 00000000..a046261f --- /dev/null +++ b/server/tests/work-loop.test.ts @@ -0,0 +1,122 @@ +import { describe, expect, test } from "bun:test"; +import { repeatAfterEach } from "../src/work/loop"; + +/** + * Running something over and over without ever running two at once. + * + * The sweep this exists for claims work with `for update skip locked`, so two overlapping runs do + * not contend over one row — they take different rows, and each starts its own agent deliveries. An + * interval firing on the clock during a five-minute delivery starts a hundred and fifty more of + * them. The bound is the backlog, not the limit anybody configured. + */ + +/** A timer a test can advance by hand, so this is about ordering rather than about waiting. */ +function fakeClock() { + const pending: Array<{ at: number; run: () => void }> = []; + let now = 0; + return { + schedule: (run: () => void, ms: number) => { + pending.push({ at: now + ms, run }); + return {}; + }, + /** Fire everything due at or before `to`, in order, one at a time. */ + async advance(to: number) { + now = to; + for (;;) { + const index = pending.findIndex((entry) => entry.at <= now); + if (index === -1) return; + const [entry] = pending.splice(index, 1); + entry?.run(); + // Let whatever the callback started make progress before the next timer fires. + await Promise.resolve(); + await Promise.resolve(); + } + }, + get waiting() { + return pending.length; + }, + }; +} + +describe("repeating something one at a time", () => { + test("never starts a run while the last one is still going", async () => { + const clock = fakeClock(); + let inFlight = 0; + let most = 0; + let started = 0; + const release: Array<() => void> = []; + + repeatAfterEach( + () => { + started += 1; + inFlight += 1; + most = Math.max(most, inFlight); + return new Promise((resolve) => { + release.push(() => { + inFlight -= 1; + resolve(); + }); + }); + }, + 100, + clock.schedule, + ); + + // A run starts, and then a great deal of time passes while it is still going. + await clock.advance(100); + expect(started).toBe(1); + await clock.advance(10_000); + expect(started).toBe(1); + expect(most).toBe(1); + // Nothing is even scheduled while one is in flight, so nothing can pile up. + expect(clock.waiting).toBe(0); + + // It finishes; the next one is scheduled from there. + release[0]?.(); + await Promise.resolve(); + await clock.advance(10_100); + expect(started).toBe(2); + expect(most).toBe(1); + }); + + /* + * Otherwise the loop stops the first time the database blinks, silently, and stays stopped until + * somebody restarts the pod. + */ + test("a run that threw does not end the loop", async () => { + const clock = fakeClock(); + let started = 0; + + repeatAfterEach( + async () => { + started += 1; + throw new Error("the database blinked"); + }, + 100, + clock.schedule, + ); + + await clock.advance(100); + expect(started).toBe(1); + await clock.advance(200); + expect(started).toBe(2); + }); + + test("stopping means no further runs", async () => { + const clock = fakeClock(); + let started = 0; + const loop = repeatAfterEach( + async () => { + started += 1; + }, + 100, + clock.schedule, + ); + + await clock.advance(100); + expect(started).toBe(1); + loop.stop(); + await clock.advance(1_000); + expect(started).toBe(1); + }); +}); diff --git a/server/tests/work-queue.integration.test.ts b/server/tests/work-queue.integration.test.ts index 14b7d847..321a5bbe 100644 --- a/server/tests/work-queue.integration.test.ts +++ b/server/tests/work-queue.integration.test.ts @@ -373,3 +373,65 @@ describe("claiming durable work", () => { expect(await queue.purge({ kind, olderThanMs: 0, maxAttempts: 3 })).toBe(1); }); }); + +/** + * The fan-out cap, under the only conditions that matter. + * + * A model asked to do several things emits several tool calls in one turn and they run at once. A + * cap checked before the write holds only while nothing else is writing, so all of them pass it: + * each reads a count taken before any of the others had committed. This needs no cluster and no + * unusual timing, which is why it must be driven against a real database rather than a stub that + * awaits one call at a time. + */ +describe("offering at most so many under one prefix", () => { + test("five at once cannot get past a cap of three", async () => { + const run = `${randomUUID()}:`; + + const results = await Promise.all( + ["one", "two", "three", "four", "five"].map((word) => + queue.offer({ + kind, + key: `${run}${word}`, + atMost: { keyPrefix: run, max: 3 }, + }), + ), + ); + + expect(results.filter((result) => result === "queued")).toHaveLength(3); + expect(results.filter((result) => result === "refused")).toHaveLength(2); + const written = await database + .select({ key: workItems.key }) + .from(workItems) + .where(eq(workItems.kind, kind)); + expect(written).toHaveLength(3); + }); + + /* + * A retried offer of work that is already queued is not a new hop, and must not be reported as + * refused by the cap: the caller asked for it to be on the queue and it is. + */ + test("the same key again is not counted against the cap", async () => { + const run = `${randomUUID()}:`; + const cap = { keyPrefix: run, max: 1 }; + + expect(await queue.offer({ kind, key: `${run}a`, atMost: cap })).toBe( + "queued", + ); + // The same key again is work already queued, not a second piece of work — and not something the + // cap should refuse either. A caller that reports it as new promises an answer nobody will give. + expect(await queue.offer({ kind, key: `${run}a`, atMost: cap })).toBe( + "already", + ); + expect(await queue.offer({ kind, key: `${run}b`, atMost: cap })).toBe( + "refused", + ); + }); + + test("without a cap nothing is refused", async () => { + const run = `${randomUUID()}:`; + const results = await Promise.all( + [1, 2, 3, 4, 5].map((n) => queue.offer({ kind, key: `${run}${n}` })), + ); + expect(results.every((result) => result === "queued")).toBe(true); + }); +}); diff --git a/shared/handoff-markers.ts b/shared/handoff-markers.ts new file mode 100644 index 00000000..ef43cb00 --- /dev/null +++ b/shared/handoff-markers.ts @@ -0,0 +1,18 @@ +/** + * The first words of the sentences a server-side handoff tool answers with. + * + * ONE DECLARATION, READ FROM BOTH SIDES. A tool that runs on the server reaches the transcript as + * text meant for a model, so the only thing the renderer can tell an accepted hop from a refused one + * by is the wording. That is not a contract to be proud of, and the least it can be is a contract + * with one author: a rewording here changes the server and the transcript together. + * + * It lived in three places once — the server, the renderer, and a test — under a comment claiming it + * was shared. It was not, and the bug that produced is the one both renderers' comments recount: + * every accepted hop drawn as Blocked, with the whole suite green. + */ + +/** How `message_bot` starts its answer when a hop was accepted. */ +export const HANDED_OVER = "Handed to "; + +/** How `ask_person` starts its answer when the question was routed. */ +export const PUT_TO = "Put to ";