diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index 42b3ebf..baeb23d 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -22,6 +22,8 @@ import { useMessageScroller, } from "@/components/ui/message-scroller"; import { toVisibleChatItems } from "./chat-messages"; +import { asText, forDisplay, REFUSAL_MARKER } from "@/lib/plugins/tool-result"; +import { readToolName } from "@/lib/plugins/tool-name"; import type { QueuedMessage } from "./composer"; import { ToolLine } from "./tool-line"; import { ToolRenderBoundary } from "./tool-boundary"; @@ -467,21 +469,60 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ {/* - * A TOOL WITH NO REGISTERED RENDERER STILL HAPPENED. `renderToolCall` draws whatever was - * registered for the name and nothing at all for anything else, which left a Bot that called - * something the app does not know about looking like a Bot that did nothing — the same - * failure `ToolRenderBoundary` exists to prevent, arriving by a different route. + * A TOOL WITH NO REGISTERED RENDERER STILL HAPPENED, and since tools moved to the server + * that is now the ordinary case rather than the exception: MCP tools execute in the runtime + * and register no renderer here at all. `renderToolCall` still draws the components the app + * registers, and everything else lands below. * - * The fallback is a plain tool line: what was called, shimmering until its result lands. It - * is the same line the computer and MCP tools draw, so an unrecognised call reads as an - * ordinary event rather than as damage. + * What was called, shimmering until its result arrives, and then the server's own words + * drawn the way a Bot's prose is drawn. */} - {drawn ?? } + {drawn ?? } ); }); +/** + * A tool the runtime executed, drawn for the person watching. + * + * Named from the reader's side: what was done, against which server, with the server's own words + * behind a disclosure. The identifier the model was offered never reaches the screen. + */ +function ServerToolLine({ name, result }: { name: string; result?: string }) { + const { label, detail } = readToolName(name); + /* + * A refusal is not a result, and must not read like one. + * + * The server says which it is rather than the browser inferring it from the wording, because the + * wording is a policy message an administrator can rewrite and the first rephrasing would break + * any guess made here. See REFUSAL_MARKER in server/src/plugins/tools.ts. + */ + const answer = result === undefined ? undefined : asText(result); + const refused = answer?.startsWith(REFUSAL_MARKER) ?? false; + /* + * The marker is for this component, not for the reader. Left in, a refusal reads "Blocked" in the + * label and then "Refused." again in the first two words of the body, which is the same fact three + * times over by the end of the sentence. Stripped here rather than on the server, because the + * server's copy is what the model is told and "Refused." in front of a reason is right for it. + */ + const body = refused ? answer?.slice(REFUSAL_MARKER.length).trim() : answer; + return ( + + {body ? ( + + {forDisplay(body)} + + ) : null} + + ); +} + export function ChatTranscript({ busy = false, commandNames = "", diff --git a/app/src/lib/copilot/plugin-tools.tsx b/app/src/lib/copilot/plugin-tools.tsx deleted file mode 100644 index 3e2ae66..0000000 --- a/app/src/lib/copilot/plugin-tools.tsx +++ /dev/null @@ -1,209 +0,0 @@ -import { useFrontendTool } from "@copilotkit/react-core/v2"; -import { useQuery } from "@tanstack/react-query"; -import { useRef, useState } from "react"; -import { Streamdown } from "streamdown"; -import { ToolLine } from "@/components/channels/tool-line"; -import { markdownComponents } from "@/lib/markdown"; -import * as z from "zod"; -import { useActiveBotId } from "@/lib/copilot/active-bot"; -import { - agentPluginsQueryOptions, - callPluginTool, - type GrantedPlugins, -} from "@/lib/plugins/queries"; - -/** - * Runtime-discovered MCP tools granted to the active Bot. Registration controls what is offered; - * the server still rechecks each call. - */ -export function PluginTools() { - const botId = useActiveBotId(); - const { data } = useQuery(agentPluginsQueryOptions(botId)); - const granted: GrantedPlugins = data ?? { tools: [], skills: [] }; - - /** Keep previously offered tools mounted so mid-run revocations can return explicit refusals. */ - const seen = useRef(new Map()); - for (const tool of granted.tools) seen.current.set(tool.ref, tool); - const offered = [...seen.current.values()]; - - return ( - <> - {offered.map((tool) => ( - - ))} - - ); -} - -/** Convert vendor JSON Schema to SDK parameters; unreadable schemas fall back to catchall. */ -function parametersFor(inputSchema: Record) { - try { - const converted = z.fromJSONSchema(inputSchema as never); - // Only object schemas describe tool arguments; other vendor schemas fall back to catchall. - if (converted instanceof z.ZodObject) return converted; - } catch {} - return z.object({}).catchall(z.unknown()); -} - -/** - * A tool result, as something worth looking at. - * - * MCP says a text part, and vendors fill it with anything from plain markdown to a JSON envelope - * with the markdown inside one field. Markdown is drawn as markdown, a JSON wrapper is unwrapped to - * the markdown it was hiding, and anything else is fenced as JSON. Nothing is discarded. - */ -function forDisplay(text: string): string { - const trimmed = text.trim(); - if (!trimmed.startsWith("{") && !trimmed.startsWith("[")) return text; - - let parsed: unknown; - try { - parsed = JSON.parse(trimmed); - } catch { - // Not JSON after all. Draw what the server sent. - return text; - } - - if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { - const entries = Object.entries(parsed as Record); - // The field carrying the answer, told apart from the ones carrying bookkeeping: Slack sends - // `{ results, pagination_info }`. The rest is kept below rather than dropped. - const markdown = entries - .filter( - ([, value]) => - typeof value === "string" && - (value.includes("\n#") || value.startsWith("#")), - ) - .sort((a, b) => String(b[1]).length - String(a[1]).length)[0]; - - if (markdown) { - const rest = entries.filter(([key]) => key !== markdown[0]); - const body = String(markdown[1]); - if (rest.length === 0) return body; - return `${body}\n\n\`\`\`json\n${JSON.stringify( - Object.fromEntries(rest), - null, - 2, - )}\n\`\`\``; - } - } - - return `\`\`\`json\n${JSON.stringify(parsed, null, 2)}\n\`\`\``; -} - -function PluginTool({ - name, - toolRef, - description, - inputSchema, - botId, -}: { - name: string; - toolRef: string; - description: string; - inputSchema: Record; - botId: string; -}) { - /** - * Per-call render state. The SDK captures `render` at registration, so the renderer must read - * current results from a ref keyed by tool call id. - */ - const calls = useRef( - new Map< - string, - { - result?: { text: string; isError: boolean }; - outcome?: { refused: boolean; reason: string }; - } - >(), - ); - /** Bumped only to make React redraw; the data itself lives in the ref above. */ - const [, redraw] = useState(0); - const touch = () => redraw((tick) => tick + 1); - - const [serverId, ...rest] = toolRef.split("/"); - const bareName = rest.join("/"); - - useFrontendTool({ - name, - // The vendor's own description, with the server named. A model choosing between two servers that - // both offer "search" needs to know which is which, and vendors do not write their descriptions - // expecting to sit beside another vendor's. - description: description - ? `${description} (${serverId})` - : `${bareName} on ${serverId}.`, - parameters: parametersFor(inputSchema), - handler: async ( - args: Record, - // DEFAULTED, because the context argument is optional and a handler that destructures it - // unconditionally throws on any call that omits it. - context: { signal?: AbortSignal; toolCall?: { id?: string } } = {}, - ) => { - const id = context.toolCall?.id ?? ""; - const result = await callPluginTool( - toolRef, - args ?? {}, - botId, - context.signal, - ); - - if (result.ok) { - calls.current.set(id, { - result: { text: result.text, isError: result.isError }, - }); - touch(); - // Return MCP text to the model; vendor errors stay as tool results instead of thrown errors. - return result.isError - ? `The tool reported an error: ${result.text}` - : result.text; - } - - calls.current.set(id, { - outcome: { refused: result.refused, reason: result.reason }, - }); - touch(); - return result.reason; - }, - render: ({ status, toolCallId }) => { - const entry = calls.current.get(toolCallId ?? "") ?? {}; - const { result, outcome } = entry; - - // Render policy refusals separately from vendor/server failures. - if (outcome) { - return ( - - ); - } - - return ( - - {result ? ( - /* The server's own words, drawn the way a Bot's prose is drawn. */ - - {forDisplay(result.text)} - - ) : null} - - ); - }, - }); - - return null; -} diff --git a/app/src/lib/copilot/provider.tsx b/app/src/lib/copilot/provider.tsx index 1096397..1d58b74 100644 --- a/app/src/lib/copilot/provider.tsx +++ b/app/src/lib/copilot/provider.tsx @@ -3,7 +3,6 @@ import type { ReactNode } from "react"; import { ActiveBotProvider } from "./active-bot"; import { ComputerTools } from "./computer-tools"; import { GalleryTools } from "./gallery-tools"; -import { PluginTools } from "./plugin-tools"; import { SandboxedTools } from "./sandboxed-tools"; /** @@ -28,8 +27,6 @@ export function CopilotProvider({ children }: { children: ReactNode }) { {/* Gallery tools are registered once; their handlers re-read the active Bot to avoid shadowing renderers. */} - {/* MCP tools share the same active-Bot context and server-side grant checks. */} - {/* Browser-authored components use the same component grants as the compiled gallery. */} {children} diff --git a/app/src/lib/plugins/tool-name.ts b/app/src/lib/plugins/tool-name.ts new file mode 100644 index 0000000..7b82e40 --- /dev/null +++ b/app/src/lib/plugins/tool-name.ts @@ -0,0 +1,51 @@ +/** + * A tool call, named the way the person watching would name it. + * + * The model is offered `mcp__notes__search_notes`, because a tool name has to be unique across every + * server a Bot holds and has to survive two vendors both calling something `search`. None of that is + * the reader's problem, and putting it on screen tells them how the thing is built rather than what + * their Bot just did. + * + * Anything that is not a prefixed MCP name is left exactly as it is: a component the app registered + * already has a name somebody chose. + */ +export type ToolName = { + /** What was done, for the line itself. */ + label: string; + /** Which server it was done against, muted beside the label. Absent for anything not MCP. */ + detail?: string; +}; + +export function readToolName(name: string): ToolName { + const parts = name.split("__"); + if (parts.length < 3 || parts[0] !== "mcp") return { label: name }; + + const [, server, ...rest] = parts; + const tool = rest.join("__"); + const label = humanise(tool); + + /* + * The server is dropped when the action already says it. Vendors name a tool after the thing it + * searches, so `mcp__notes__search_notes` would otherwise read "Search notes notes", which looks + * like a bug rather than a label. + */ + const named = label.toLowerCase().includes((server ?? "").toLowerCase()); + return named ? { label } : { label, detail: server }; +} + +/** + * `search_notes` as "Search notes". + * + * Vendors write tool names in snake_case, camelCase or a mixture, and the only thing they agree on + * is that the first word is a verb. Splitting on both and sentence-casing the result gets a phrase + * that reads as an action without anybody maintaining a table of names. + */ +function humanise(tool: string): string { + const words = tool + .replace(/[_-]+/g, " ") + .replace(/([a-z\d])([A-Z])/g, "$1 $2") + .trim() + .toLowerCase(); + if (words.length === 0) return tool; + return words.charAt(0).toUpperCase() + words.slice(1); +} diff --git a/app/src/lib/plugins/tool-result.ts b/app/src/lib/plugins/tool-result.ts new file mode 100644 index 0000000..1249566 --- /dev/null +++ b/app/src/lib/plugins/tool-result.ts @@ -0,0 +1,83 @@ +/** + * What the server puts in front of a refused call's reason. + * + * Written out here rather than imported, because this crosses a network: the server's copy travels + * in a tool result and this one reads it, the same way a status code is declared at both ends. It + * must match `REFUSAL_MARKER` in `server/src/plugins/tools.ts`. + * + * The alternative is guessing a refusal from its wording, which breaks the first time an + * administrator rephrases a policy message, and fails silently by drawing a refusal as a result. + */ +export const REFUSAL_MARKER = "Refused."; + +/** + * The tool's own answer, out of whatever the transcript is carrying it in. + * + * A tool returns a string and the runtime puts it in a message as JSON, so what arrives here is that + * string encoded: quotes around the whole thing, and every quote inside it escaped. Drawn as-is, a + * refusal reads `"This deployment's policy does not allow that: ... the rule \"mcp.server ...`, with + * the escapes on screen. + * + * It also breaks more than the look. `REFUSAL_MARKER` is matched against the start of this text, and + * an encoded string starts with a quote, so a refusal is drawn as an ordinary result: the one thing + * the marker exists to prevent. + * + * Only a JSON string is unwrapped. An object or an array is a vendor's envelope, which is + * {@link forDisplay}'s job, and anything that is not JSON at all is already what it says. + */ +export function asText(text: string): string { + const trimmed = text.trim(); + if (!trimmed.startsWith('"')) return text; + try { + const parsed: unknown = JSON.parse(trimmed); + return typeof parsed === "string" ? parsed : text; + } catch { + return text; + } +} + +/** + * A tool result, as something worth looking at. + * + * MCP says a text part, and vendors fill it with anything from plain markdown to a JSON envelope + * with the markdown inside one field. Markdown is drawn as markdown, a JSON wrapper is unwrapped to + * the markdown it was hiding, and anything else is fenced as JSON. Nothing is discarded. + */ +export function forDisplay(text: string): string { + const trimmed = asText(text).trim(); + if (!trimmed.startsWith("{") && !trimmed.startsWith("[")) return trimmed; + + let parsed: unknown; + try { + parsed = JSON.parse(trimmed); + } catch { + // Not JSON after all. Draw what the server sent. + return text; + } + + if (parsed && typeof parsed === "object" && !Array.isArray(parsed)) { + const entries = Object.entries(parsed as Record); + // The field carrying the answer, told apart from the ones carrying bookkeeping: Slack sends + // `{ results, pagination_info }`. The rest is kept below rather than dropped. + const markdown = entries + .filter( + ([, value]) => + typeof value === "string" && + (value.includes("\n#") || value.startsWith("#")), + ) + .sort((a, b) => String(b[1]).length - String(a[1]).length)[0]; + + if (markdown) { + const rest = entries.filter(([key]) => key !== markdown[0]); + const body = String(markdown[1]); + if (rest.length === 0) return body; + return `${body}\n\n\`\`\`json\n${JSON.stringify( + Object.fromEntries(rest), + null, + 2, + )}\n\`\`\``; + } + } + + return `\`\`\`json\n${JSON.stringify(parsed, null, 2)}\n\`\`\``; +} diff --git a/app/tests/tool-name.test.ts b/app/tests/tool-name.test.ts new file mode 100644 index 0000000..21693d0 --- /dev/null +++ b/app/tests/tool-name.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "bun:test"; +import { readToolName } from "../src/lib/plugins/tool-name"; + +/** + * What the person watching is told a Bot just did. + * + * The names under test are the ones the model is offered, which carry the server and a separator + * that exists to keep two vendors' `search` apart. A reader should never see either. + */ +describe("naming a tool call", () => { + test("an MCP tool reads as an action against a named server", () => { + expect(readToolName("mcp__slack__post_message")).toEqual({ + label: "Post message", + detail: "slack", + }); + }); + + test("the server is dropped when the action already names it", () => { + // "Search notes notes" reads as a bug rather than a label. + expect(readToolName("mcp__notes__search_notes")).toEqual({ + label: "Search notes", + }); + }); + + test("camelCase from a vendor reads the same way", () => { + // The server is dropped here too: the vendor put it in the tool name themselves. + expect(readToolName("mcp__jira__searchJiraIssues")).toEqual({ + label: "Search jira issues", + }); + }); + + test("a tool name containing the separator keeps all of it", () => { + // The server is the first segment and everything after it is the tool, however many + // separators the vendor used. + expect(readToolName("mcp__box__list__files")).toEqual({ + label: "List files", + detail: "box", + }); + }); + + test("a component the app registered is left alone", () => { + // These names were chosen by somebody and are already what the reader should see. + expect(readToolName("showBarChart")).toEqual({ label: "showBarChart" }); + }); +}); diff --git a/app/tests/tool-result.test.ts b/app/tests/tool-result.test.ts new file mode 100644 index 0000000..92bb14c --- /dev/null +++ b/app/tests/tool-result.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, test } from "bun:test"; +import { asText, forDisplay } from "../src/lib/plugins/tool-result"; + +/** + * What a tool actually said, recovered from how the transcript carries it. + * + * A tool returns a string and the runtime puts it into a message as JSON, so everything arriving at + * the screen is encoded. Getting this wrong is not only ugly: the refusal marker is matched against + * the start of this text, and an encoded string starts with a quote. + */ +describe("reading a tool's answer", () => { + test("a JSON-encoded string is unwrapped, escapes and all", () => { + const refusal = + 'This deployment\'s policy does not allow that: search_notes on notes is blocked by the rule `mcp.server == "notes"`.'; + expect(asText(JSON.stringify(refusal))).toBe(refusal); + }); + + test("a refusal is still recognisable as one after decoding", () => { + const encoded = JSON.stringify("Refused. The rule says no."); + // The check the transcript makes. Against the raw text it is false, which is the bug. + expect(encoded.startsWith("Refused.")).toBe(false); + expect(asText(encoded).startsWith("Refused.")).toBe(true); + }); + + test("plain text is left alone", () => { + expect(asText("Meals under $75 need no receipt.")).toBe( + "Meals under $75 need no receipt.", + ); + }); + + /* + * An envelope is not a string, and unwrapping one here would take the vendor's structure apart + * before forDisplay has had the chance to find the answer inside it. + */ + test("a JSON object or array is left for forDisplay", () => { + expect(asText('{"results":"# Found"}')).toBe('{"results":"# Found"}'); + expect(asText("[1,2]")).toBe("[1,2]"); + }); + + test("something that only looks like JSON is drawn as it came", () => { + expect(asText('"unterminated')).toBe('"unterminated'); + }); + + test("an encoded envelope is decoded and then unwrapped", () => { + expect(forDisplay(JSON.stringify('{"results":"# Found\\n\\nBody"}'))).toBe( + "# Found\n\nBody", + ); + }); +}); diff --git a/bun.lock b/bun.lock index f62f688..fa1e4b1 100644 --- a/bun.lock +++ b/bun.lock @@ -6,6 +6,7 @@ "name": "openbot", "devDependencies": { "@biomejs/biome": "^2.3.8", + "@copilotkit/aimock": "^1.38.0", "@types/bun": "^1.3.3", "roughjs": "^4.6.6", "typescript": "^5.9.3", @@ -68,6 +69,7 @@ "hono": "^4.10.0", "postgres": "^3.4.9", "yaml": "^2.9.0", + "zod": "^4.4.3", }, "devDependencies": { "@copilotkit/aimock": "^1.38.0", diff --git a/package.json b/package.json index ceb5596..72e9376 100644 --- a/package.json +++ b/package.json @@ -20,10 +20,12 @@ "test:ci": "bun scripts/test-ci.ts", "pretest": "bun run generate:app-config", "test:smoke": "OPENBOT_SMOKE=1 bun test tests/smoke", - "diagram": "bun scripts/architecture-diagram.ts" + "diagram": "bun scripts/architecture-diagram.ts", + "mock:knowledge": "bun scripts/mock-knowledge-mcp.ts" }, "devDependencies": { "@biomejs/biome": "^2.3.8", + "@copilotkit/aimock": "^1.38.0", "@types/bun": "^1.3.3", "roughjs": "^4.6.6", "typescript": "^5.9.3", diff --git a/scripts/mock-knowledge-mcp.ts b/scripts/mock-knowledge-mcp.ts new file mode 100644 index 0000000..c6fe45b --- /dev/null +++ b/scripts/mock-knowledge-mcp.ts @@ -0,0 +1,63 @@ +/** + * A knowledge MCP server with nothing real behind it. + * + * Slice 1 puts a Bot in front of a stranger with no account, which means it must answer without + * anybody having connected anything. This stands in for the customer's Notion: real MCP over HTTP, + * fixed content, safe to point at the open internet. + * + * `@copilotkit/aimock` rather than a hand-rolled server, so what a Bot talks to here is the same + * protocol implementation the tests talk to. + */ +import { MCPMock } from "@copilotkit/aimock/mcp"; + +const PORT = Number.parseInt(process.env.MOCK_KNOWLEDGE_PORT ?? "4300", 10); + +const NOTES = [ + { + title: "Expense policy", + url: "https://notion.example/expense-policy", + body: "Meals under $75 need no receipt. Anything above needs one, and anything above $500 needs your manager before you spend it.", + }, + { + title: "Onboarding checklist", + url: "https://notion.example/onboarding", + body: "Day one: laptop, SSO, and the team channel. Week one: shadow two customer calls. Month one: ship something small to production.", + }, + { + title: "Incident review: checkout outage", + url: "https://notion.example/incident-checkout", + body: "Checkout was down for 41 minutes. Cause was an expired certificate nobody owned. Action: certificates get an owner and an alert at 30 days.", + }, +]; + +const mock = new MCPMock({ port: PORT } as never); + +mock.addTool({ + name: "search_notes", + description: + "Search the company's notes and return the matching ones with a link to each. Use this for any question about company policy, process or history.", + inputSchema: { + type: "object", + properties: { + query: { type: "string", description: "What to look for." }, + }, + required: ["query"], + }, +}); + +mock.onToolCall("search_notes", (args) => { + const query = String((args as { query?: string })?.query ?? "").toLowerCase(); + const hits = NOTES.filter((note) => + `${note.title} ${note.body}`.toLowerCase().includes(query), + ); + const found = hits.length > 0 ? hits : NOTES; + return found + .map( + (note) => + `## ${note.title}\n\n${note.body}\n\n[Open the note](${note.url})`, + ) + .join("\n\n---\n\n"); +}); + +const url = await mock.start(); +console.info(JSON.stringify({ type: "mock-knowledge-mcp", url })); diff --git a/server/package.json b/server/package.json index 304377f..d5f6fe6 100644 --- a/server/package.json +++ b/server/package.json @@ -21,7 +21,8 @@ "drizzle-orm": "^0.45.2", "hono": "^4.10.0", "postgres": "^3.4.9", - "yaml": "^2.9.0" + "yaml": "^2.9.0", + "zod": "^4.4.3" }, "devDependencies": { "@copilotkit/aimock": "^1.38.0", diff --git a/server/src/computer/policy.ts b/server/src/computer/policy.ts index 7b18cee..a5b96d0 100644 --- a/server/src/computer/policy.ts +++ b/server/src/computer/policy.ts @@ -266,9 +266,21 @@ export function evaluateActionPolicy( /** A refusal a person can act on: what was refused, and on what. */ function describeRefusal(context: PolicyContext, expression: string): string { + // A tool call is named by its server and its tool, and nothing else here fits it. The browser + // fields are all present on an MCP context and all empty, deliberately, so that a rule written + // about a page evaluates to false rather than being unevaluable. That makes every one of the + // tests below true of a tool call and all of them wrong about it: without this branch a refused + // Jira call reads "the file is blocked", naming a workspace it never touched and a path that is + // not there. Checked first because it is the only one of these that is ever certain. + if (context.mcp) { + return ( + `This deployment's policy does not allow that: ${context.mcp.tool} on ` + + `${context.mcp.server} is blocked by the rule \`${expression}\`.` + ); + } // A file refusal must not be phrased as happening "on ": the workspace has nothing to do with // whatever page the browser happens to be showing, and saying so sends somebody to the wrong place. - if (context.file) { + if (context.file?.path) { return ( `This deployment's policy does not allow that: the file ${context.file.path} ` + `is blocked by the rule \`${expression}\`.` diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 2425823..ac4d934 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -9,6 +9,7 @@ import { createCopilotHonoHandler } from "@copilotkit/runtime/v2/hono"; import type { AgentActor } from "./agents/profile-types"; import type { StallGuard } from "./channels/stall-guard"; import type { DeploymentConfig } from "./config"; +import type { GrantedTool } from "./plugins/tools"; /** * The CopilotKit runtime, always in Intelligence mode. @@ -164,6 +165,15 @@ export function builtInAgentConfiguration( agent: RegisteredBuiltInAgent, model: RuntimeModel, apiKey: string | null, + /** + * What this Bot may call, resolved for the person asking. + * + * Handed to the agent rather than registered by the surface, so a run needs no browser. These are + * not raw MCP servers on purpose: each one executes through the plugin store, which checks the + * grant, evaluates the policy and writes the audit row. Passing `mcpServers` here instead would + * let the agent reach a vendor directly and walk around all three. + */ + tools: GrantedTool[] = [], ): BuiltInAgentConfiguration { if (!apiKey) { return { @@ -181,38 +191,68 @@ export function builtInAgentConfiguration( model: `${model.provider}/${model.defaultModel}`, prompt: agent.systemPrompt, apiKey, + /* + * A run stops after one step unless told otherwise, which for a Bot with tools means it calls + * one and never speaks: the tool executes, the result arrives, and the run ends before the model + * can say what it found. The person sees their own question and nothing else. + * + * Only set when there are tools, because a Bot with none has nothing to continue for. The cap + * bounds a model that would otherwise call tools in a circle. Interrupt tools, if any are ever + * added here, require the default of one and must not be mixed in. + */ + ...(tools.length > 0 ? { tools, maxSteps: TOOL_STEPS } : {}), }; } +/** + * How many turns of the tool loop one run may take. + * + * Enough for a Bot to search, read what came back, search again on a better term, and answer. + * Beyond that a model is not making progress, and every extra step is somebody's money. + */ +const TOOL_STEPS = 8; + /** * Build the built-in and remote AG-UI agent map the runtime serves. * * Keyed by the registry id, which is what the browser sends as the agent name, so the two cannot * drift apart without the lookup failing loudly rather than silently running the wrong Bot. */ -export function buildAgents( +export async function buildAgents( agents: RegisteredAgent[], model: RuntimeModel, apiKey: string | null, /** Absent leaves every stream unwatched, which is what an unconfigured timeout means. */ stallGuard?: StallGuard, -): Record { + /** Absent leaves every Bot with no tools, which is the correct answer when nothing is granted. */ + loadTools: LoadToolsForBot = async () => [], +): Promise> { return Object.fromEntries( - agents.map((agent) => [ - agent.id, - buildAgent(agent, model, apiKey, stallGuard), - ]), + await Promise.all( + agents.map(async (agent) => [ + agent.id, + await buildAgent(agent, model, apiKey, stallGuard, loadTools), + ]), + ), ); } -function buildAgent( +async function buildAgent( agent: RegisteredAgent, model: RuntimeModel, apiKey: string | null, - stallGuard?: StallGuard, -): AbstractAgent { + stallGuard: StallGuard | undefined, + loadTools: LoadToolsForBot, +): Promise { if (agent.type === "built_in") { - return new BuiltInAgent(builtInAgentConfiguration(agent, model, apiKey)); + return new BuiltInAgent( + builtInAgentConfiguration( + agent, + model, + apiKey, + await loadTools(agent.id), + ), + ); } if (agent.type === "unavailable") { return new UnavailableAgent(agent); @@ -280,6 +320,7 @@ export async function resolveRuntimeAgents( model: RuntimeModel, resolveModelApiKey: () => Promise, stallGuard?: StallGuard, + loadTools?: LoadToolsForBot, ): Promise> { const registered = await loadAgents(); if (registered.length === 0) { @@ -291,9 +332,12 @@ export async function resolveRuntimeAgents( const apiKey = registered.some((agent) => agent.type === "built_in") ? await resolveModelApiKey() : null; - return buildAgents(registered, model, apiKey, stallGuard); + return buildAgents(registered, model, apiKey, stallGuard, loadTools); } +/** What one Bot may call, for the person whose request this is. */ +export type LoadToolsForBot = (botId: string) => Promise; + /** Who is asking. Agent visibility is decided per person, so a run has to know this first. */ export type IdentifyActor = (request: Request) => Promise; @@ -320,6 +364,8 @@ export function createRequestAgents( * that opened it has been answered. */ stallGuard?: StallGuard, + /** What each Bot may call, resolved for whoever is asking. Absent means no tools. */ + loadToolsForActor?: (actorId: string) => LoadToolsForBot, ) { return async ({ request }: { request: Request }) => { const actor = await identifyActor(request); @@ -328,6 +374,7 @@ export function createRequestAgents( model, resolveModelApiKey, stallGuard, + loadToolsForActor?.(actor.id), ); }; } @@ -352,6 +399,7 @@ export function mountCopilotRuntime( * there is no reason for a caller to have to say `undefined` here to reach `basePath`. */ stallGuard: StallGuard, + loadToolsForActor?: (actorId: string) => LoadToolsForBot, basePath = "/api/copilotkit", ) { const { intelligence } = config.runtime; @@ -377,6 +425,7 @@ export function mountCopilotRuntime( model, resolveModelApiKey, stallGuard, + loadToolsForActor, ) as never, }); diff --git a/server/src/index.ts b/server/src/index.ts index 3fc067a..76addb5 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -38,6 +38,7 @@ import { } from "./credentials"; import { createDatabase } from "./db/client"; import { createPluginStore } from "./plugins/store"; +import { grantedTools } from "./plugins/tools"; import { createPackageStatusReader, loadTenantPackage, @@ -333,6 +334,10 @@ const app = createApp( identifyUser, identifyActor, stallGuard, + // Tools run here, not in the browser. Each one still executes through the plugin store, so the + // grant, the policy and the audit row are exactly where they were. + (actorId) => (botId) => + grantedTools({ store: pluginStore, botId, actorId }), ), computerClient, // The only path to an acting call. diff --git a/server/src/plugins/tools.ts b/server/src/plugins/tools.ts new file mode 100644 index 0000000..2c284fb --- /dev/null +++ b/server/src/plugins/tools.ts @@ -0,0 +1,96 @@ +import { z } from "zod"; +import { PluginRefusedError, type PluginStore } from "./store"; + +/** + * The tools a Bot may call, as the runtime's own tool definitions, executed on the server. + * + * The loop used to run in the browser: every MCP tool was registered with `useFrontendTool` and its + * handler posted back to `/api/plugins/call`. That made a browser a hard requirement for a Bot to do + * anything, which rules out an embedded widget, a run nobody is watching, and any surface that is + * not our own app. + * + * Nothing about governance moves with it. `callTool` is still the only path to a vendor: it checks + * the grant, evaluates the policy, writes the audit row, and only then calls out. This module hands + * the model a description of what it may call; the store remains what decides whether a call happens. + * + * Read at run time rather than captured, so a grant an administrator adds or revokes applies to the + * next run rather than after a restart. + */ +/** + * What a refused call answers with. + * + * The transcript draws a refusal differently from a result, and it has only the tool's answer to go + * on. Guessing from the wording would break the first time an administrator rephrased a policy + * message, so the answer says which it is. The model reads this too, and "Refused." in front of a + * reason is what it should be told anyway. + */ +export const REFUSAL_MARKER = "Refused."; + +export type GrantedTool = { + name: string; + description: string; + parameters: z.ZodType; + execute: (args: unknown) => Promise; +}; + +/** + * A vendor's JSON Schema as something the model can be handed. + * + * Anything that is not an object schema describes something other than a tool's arguments, and a + * schema we cannot read must not stop the tool being offered: an open object lets the model call it + * and lets the vendor be the one to reject a bad argument, which is where that error belongs. + */ +export function parametersFor(inputSchema: Record): z.ZodType { + try { + const converted = z.fromJSONSchema(inputSchema as never); + if (converted instanceof z.ZodObject) return converted; + } catch {} + return z.object({}).catchall(z.unknown()); +} + +/** + * Every MCP tool granted to one Bot, ready to hand to the runtime. + * + * A refusal is returned as the tool's result rather than thrown. The model is mid-run and the person + * is owed a sentence about what was blocked; an exception here ends the run with nothing said, and + * the refusal is already in the audit trail either way. + */ +export async function grantedTools(options: { + store: PluginStore; + botId: string; + actorId: string; +}): Promise { + const { store, botId, actorId } = options; + const granted = await store.listForAgent(botId); + + return granted.tools.map((tool) => ({ + name: tool.toolName, + description: tool.description, + parameters: parametersFor(tool.inputSchema), + execute: async (args: unknown) => { + try { + const result = await store.callTool({ + ref: tool.ref, + // The runtime hands through whatever the model produced. Anything that is not an object + // is not a set of arguments, and the vendor should be the one to say so. + args: + args && typeof args === "object" && !Array.isArray(args) + ? (args as Record) + : {}, + botId, + actorId, + }); + return result.text; + } catch (error) { + if (error instanceof PluginRefusedError) { + return `${REFUSAL_MARKER} ${error.message}`; + } + // A vendor that failed is not a refusal, and the difference matters to the person reading + // the answer: one means "not allowed", the other means "it broke". + return error instanceof Error + ? `That tool could not be called: ${error.message}` + : "That tool could not be called."; + } + }, + })); +} diff --git a/server/tests/computer-policy.test.ts b/server/tests/computer-policy.test.ts index 5191793..94c2b7d 100644 --- a/server/tests/computer-policy.test.ts +++ b/server/tests/computer-policy.test.ts @@ -350,3 +350,60 @@ describe("a rule that names an identifier only some actions carry", () => { expect(decision.allowed).toBe(false); }); }); + +/** + * What a refusal says it refused. + * + * A tool call carries every browser field the engine knows about, all of them empty, so that a rule + * written about a page evaluates to false against it rather than being unevaluable. That is correct + * for the decision and wrong for the sentence: each of those empty fields is present, so a refusal + * described from them names a page nobody visited or a file nobody touched. + */ +describe("describing a refusal", () => { + const mcpContext: PolicyContext = { + tool: { name: "mcp__notes__search_notes" }, + bot: { id: "knowledge" }, + actor: { id: "dev-local-user" }, + page: { url: "", host: "" }, + element: { ref: "", role: "", name: "", type: "" }, + key: "", + file: { path: "", name: "", extension: "" }, + mcp: { server: "notes", tool: "search_notes", effect: "read" }, + }; + + test("a refused tool call names the tool and the server it was aimed at", () => { + const decision = evaluateActionPolicy( + { mode: "enforce", deny: ['mcp.server == "notes"'], allow: ["true"] }, + mcpContext, + ); + + expect(decision.allowed).toBe(false); + expect(decision.reason).toBe( + 'This deployment\'s policy does not allow that: search_notes on notes is blocked by the rule `mcp.server == "notes"`.', + ); + // The neutral file field is present and empty on every tool call. Described from it, the + // sentence read "the file is blocked", naming a workspace the call never went near. + expect(decision.reason).not.toContain("the file"); + }); + + test("a refused file action still names the file", () => { + const decision = evaluateActionPolicy( + { + mode: "enforce", + deny: ['contains(file.path, "secrets")'], + allow: ["true"], + }, + context({ + tool: { name: "computer_read_file" }, + file: { + path: "/workspace/secrets.env", + name: "secrets.env", + extension: "env", + }, + }), + ); + + expect(decision.allowed).toBe(false); + expect(decision.reason).toContain("the file /workspace/secrets.env"); + }); +}); diff --git a/server/tests/copilot.test.ts b/server/tests/copilot.test.ts index a2951ba..5bd14d4 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -104,7 +104,7 @@ describe("registered Copilot agents", () => { }); test("fails an unavailable built-in agent through the AG-UI lifecycle", async () => { - const agents = buildAgents( + const agents = await buildAgents( [ { id: "general-assistant", @@ -139,8 +139,8 @@ describe("registered Copilot agents", () => { ); }); - test("constructs built-in and remote agents together", () => { - const agents = buildAgents( + test("constructs built-in and remote agents together", async () => { + const agents = await buildAgents( [ { id: "general-assistant", @@ -171,7 +171,7 @@ describe("registered Copilot agents", () => { * Bot's own name reaches it matters because that name is what the person is shown when its stream * goes quiet, and a guard handed the wrong one would say so convincingly. */ - test("hands a remote Bot's fetch to the stall guard, and a built-in Bot none", () => { + test("hands a remote Bot's fetch to the stall guard, and a built-in Bot none", async () => { const watched: { id: string; name: string }[] = []; const stallGuard = { watch: (bot: { id: string; name: string }) => { @@ -181,7 +181,7 @@ describe("registered Copilot agents", () => { stop: () => undefined, }; - const agents = buildAgents( + const agents = await buildAgents( [ { id: "general-assistant", @@ -213,7 +213,7 @@ describe("registered Copilot agents", () => { * The same registration is built twice, with a guard whose watch returns a fetch nothing else * could have produced and then without one, and the two are compared. */ - test("leaves a remote Bot's fetch alone when no timeout is configured", () => { + test("leaves a remote Bot's fetch alone when no timeout is configured", async () => { const sentinel = async () => new Response(null); const registered = [ { @@ -225,11 +225,13 @@ describe("registered Copilot agents", () => { ]; const model = { provider: "openai" as const, defaultModel: "gpt-4.1" }; - const guarded = buildAgents(registered, model, null, { - watch: () => sentinel, - stop: () => undefined, - }).risk; - const unguarded = buildAgents(registered, model, null).risk; + const guarded = ( + await buildAgents(registered, model, null, { + watch: () => sentinel, + stop: () => undefined, + }) + ).risk; + const unguarded = (await buildAgents(registered, model, null)).risk; if (!(guarded instanceof HttpAgent) || !(unguarded instanceof HttpAgent)) { throw new Error("Expected the remote agent"); } @@ -327,7 +329,7 @@ describe("standing agent roles", () => { test("sends one standing role message ahead of the conversation", async () => { await using endpoint = fakeAgUiEndpoint(); - const agents = buildAgents( + const agents = await buildAgents( [remoteAgent(endpoint.url)], { provider: "openai", defaultModel: "gpt-4.1" }, null, @@ -351,7 +353,7 @@ describe("standing agent roles", () => { test("keeps the standing role out of forwarded props and agent state", async () => { await using endpoint = fakeAgUiEndpoint(); - const agents = buildAgents( + const agents = await buildAgents( [remoteAgent(endpoint.url)], { provider: "openai", defaultModel: "gpt-4.1" }, null, @@ -370,7 +372,7 @@ describe("standing agent roles", () => { test("resolves a deleted coworker as a tombstone that never reaches its endpoint", async () => { await using endpoint = fakeAgUiEndpoint(); - const agents = buildAgents( + const agents = await buildAgents( [ { id: "agent_expense", diff --git a/server/tests/server-side-tools.integration.test.ts b/server/tests/server-side-tools.integration.test.ts new file mode 100644 index 0000000..d0fbffd --- /dev/null +++ b/server/tests/server-side-tools.integration.test.ts @@ -0,0 +1,208 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { MCPMock } from "@copilotkit/aimock/mcp"; +import { and, eq } from "drizzle-orm"; +import { createAuditStore } from "../src/audit"; +import { createDatabase } from "../src/db/client"; +import { + agents, + auditEvents, + mcpServers, + mcpTools, + pluginGrants, +} from "../src/db/schema"; +import { createPluginStore } from "../src/plugins/store"; +import { grantedTools, REFUSAL_MARKER } from "../src/plugins/tools"; +import type { ActionPolicy } from "../src/policy/engine"; +import { TEST_POOL } from "./support/database"; + +/** + * A tool call that happens with no browser involved. + * + * The loop used to run in the surface, so every one of these paths needed somebody watching. What is + * asserted here is that the same call runs from the server and arrives at a real MCP server, and + * that moving it did not take the grant, the policy or the audit row with it. + * + * The server on the other end is `@copilotkit/aimock`, ours, speaking real MCP over HTTP rather than + * a stubbed fetch. A stub would pass whether or not we had understood the protocol; this fails if + * the wire format moves underneath us, which is the whole promise of pointing a Bot at somebody + * else's server. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openkai:openkai@localhost:5432/openkai", + TEST_POOL, +); + +const suite = randomUUID().slice(0, 8); +const holderId = `agent_tools_holder_${suite}`; +const strangerId = `agent_tools_stranger_${suite}`; +const serverId = `notes_${suite}`; +const toolName = "search_notes"; +const ref = `${serverId}/${toolName}`; + +let policy: ActionPolicy = { mode: "enforce", deny: [], allow: ["true"] }; + +const store = createPluginStore({ + database, + auditStore: createAuditStore(database), + credentials: { readSecret: async () => null }, + encryptionKey: "x".repeat(44), + policy: () => policy, +}); + +const mock = new MCPMock(); +/** What the server on the other end was actually asked, recorded where the assertion can see it. */ +const received: unknown[] = []; + +beforeAll(async () => { + // Both Bots must exist: a grant is a row against a real agent, which is what keeps a grant from + // outliving the Bot it was given to. + for (const id of [holderId, strangerId]) { + await database + .insert(agents) + .values({ id, name: id, type: "remote_ag_ui", configuration: {} }) + .onConflictDoNothing(); + } + + mock.addTool({ + name: toolName, + description: "Search the notes.", + inputSchema: { + type: "object", + properties: { query: { type: "string" } }, + required: ["query"], + }, + }); + mock.onToolCall(toolName, (args) => { + received.push(args); + const query = (args as { query?: string })?.query ?? ""; + return `Found one note about ${query}.`; + }); + const url = await mock.start(); + + await database + .insert(mcpServers) + .values({ + id: serverId, + title: "Notes", + vendor: "notes", + url, + provenance: "custom", + }) + .onConflictDoNothing(); + await database + .insert(mcpTools) + .values({ + serverId, + name: toolName, + description: "Search the notes.", + inputSchema: { + type: "object", + properties: { query: { type: "string" } }, + required: ["query"], + }, + }) + .onConflictDoNothing(); +}); + +afterAll(async () => { + await database + .delete(pluginGrants) + .where(and(eq(pluginGrants.kind, "skill"), eq(pluginGrants.ref, ref))); + await database.delete(pluginGrants).where(eq(pluginGrants.ref, ref)); + await database.delete(mcpTools).where(eq(mcpTools.serverId, serverId)); + await database.delete(mcpServers).where(eq(mcpServers.id, serverId)); + await database.delete(agents).where(eq(agents.id, holderId)); + await database.delete(agents).where(eq(agents.id, strangerId)); + await mock.stop(); +}); + +describe("the tools a Bot is handed on the server", () => { + test("a Bot with no grant is offered nothing", async () => { + const tools = await grantedTools({ + store, + botId: strangerId, + actorId: "someone@openkai.local", + }); + expect(tools).toHaveLength(0); + }); + + test("a granted tool is offered with the vendor's own schema", async () => { + await store.grant("mcp", ref, holderId, "admin@openkai.local"); + + const tools = await grantedTools({ + store, + botId: holderId, + actorId: "someone@openkai.local", + }); + + expect(tools).toHaveLength(1); + expect(tools[0]?.description).toBe("Search the notes."); + // The vendor's schema reached the model rather than being flattened to "any object": a tool + // offered without its arguments is a tool the model calls wrongly every time. + const parsed = tools[0]?.parameters.safeParse({ query: "invoices" }); + expect(parsed?.success).toBe(true); + expect(tools[0]?.parameters.safeParse({}).success).toBe(false); + }); + + test("executing it reaches the real MCP server, with no browser anywhere", async () => { + const tools = await grantedTools({ + store, + botId: holderId, + actorId: "someone@openkai.local", + }); + + const text = await tools[0]?.execute({ query: "invoices" }); + + expect(text).toContain("Found one note about invoices"); + // The server on the other end was really called, with the arguments the model chose, rather + // than us asserting against our own hopes. + expect(received).toContainEqual({ query: "invoices" }); + }); + + test("the call is still audited, and still names the Bot", async () => { + const rows = await database + .select() + .from(auditEvents) + .where(eq(auditEvents.targetId, ref)); + + const succeeded = rows.filter( + (row) => row.eventType === "mcp.call_succeeded", + ); + expect(succeeded.length).toBeGreaterThan(0); + expect((succeeded[0]?.payload as { bot?: string } | null)?.bot).toBe( + holderId, + ); + }); + + test("a policy that forbids it refuses, and says so as the tool's answer", async () => { + policy = { + mode: "enforce", + deny: [`mcp.server == "${serverId}"`], + allow: ["true"], + }; + try { + const tools = await grantedTools({ + store, + botId: holderId, + actorId: "someone@openkai.local", + }); + const text = await tools[0]?.execute({ query: "invoices" }); + + // Returned, not thrown: the run continues and the person is told what was blocked. + expect(text).toContain("policy"); + // And marked, so the transcript can draw it as a refusal rather than as a result. The wording + // is an administrator's to change; the marker is not, which is the whole reason it exists. + expect(text?.startsWith(REFUSAL_MARKER)).toBe(true); + const rejected = await database + .select() + .from(auditEvents) + .where(eq(auditEvents.eventType, "mcp.call_rejected")); + expect(rejected.length).toBeGreaterThan(0); + } finally { + policy = { mode: "enforce", deny: [], allow: ["true"] }; + } + }); +});