From a5107875aa9499d983f31fc0bfc7be7d62fe5c00 Mon Sep 17 00:00:00 2001 From: David McKay Date: Tue, 18 Aug 2026 17:49:31 -0700 Subject: [PATCH 1/4] Run MCP tools on the server The loop ran in the browser: every granted 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, an unattended run, and any surface that is not our own app. `BuiltInAgent` takes `tools` directly, so the agent is handed what the Bot may call and executes it itself. Governance does not move with it. The tools are NOT raw `mcpServers`, which would let the agent reach a vendor directly and walk around everything: each definition executes through `pluginStore.callTool`, which checks the grant, evaluates the policy and writes the audit row before anything leaves the process. A refusal comes back as the tool's result rather than as a thrown error, so the run continues and the person is told what was blocked. Tested against a real MCP server (`@copilotkit/aimock`) rather than a stubbed fetch: a stub passes whether or not we understood the protocol. --- bun.lock | 1 + server/package.json | 3 +- server/src/copilot.ts | 54 ++++- server/src/index.ts | 5 + server/src/plugins/tools.ts | 84 +++++++ server/tests/copilot.test.ts | 30 +-- .../server-side-tools.integration.test.ts | 205 ++++++++++++++++++ 7 files changed, 356 insertions(+), 26 deletions(-) create mode 100644 server/src/plugins/tools.ts create mode 100644 server/tests/server-side-tools.integration.test.ts diff --git a/bun.lock b/bun.lock index f62f688..43d855f 100644 --- a/bun.lock +++ b/bun.lock @@ -68,6 +68,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/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/copilot.ts b/server/src/copilot.ts index 2425823..f01794d 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,6 +191,7 @@ export function builtInAgentConfiguration( model: `${model.provider}/${model.defaultModel}`, prompt: agent.systemPrompt, apiKey, + ...(tools.length > 0 ? { tools } : {}), }; } @@ -190,29 +201,41 @@ export function builtInAgentConfiguration( * 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 +303,7 @@ export async function resolveRuntimeAgents( model: RuntimeModel, resolveModelApiKey: () => Promise, stallGuard?: StallGuard, + loadTools?: LoadToolsForBot, ): Promise> { const registered = await loadAgents(); if (registered.length === 0) { @@ -291,9 +315,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 +347,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 +357,7 @@ export function createRequestAgents( model, resolveModelApiKey, stallGuard, + loadToolsForActor?.(actor.id), ); }; } @@ -352,6 +382,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 +408,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..ed74e5b --- /dev/null +++ b/server/src/plugins/tools.ts @@ -0,0 +1,84 @@ +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. + */ +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 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/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..6c2760b --- /dev/null +++ b/server/tests/server-side-tools.integration.test.ts @@ -0,0 +1,205 @@ +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 } 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"); + 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"] }; + } + }); +}); From 0780b355432dca767dd422217e31d53d59c04f8e Mon Sep 17 00:00:00 2001 From: David McKay Date: Tue, 18 Aug 2026 17:51:17 -0700 Subject: [PATCH 2/4] Stop registering MCP tools in the browser They execute in the runtime now, so a second registration in the surface would offer the model every tool twice. The result rendering survives: `forDisplay` moves to a shared module and the transcript's fallback draws it, which is no longer the exception but the ordinary path for every MCP call. --- .../components/channels/chat-transcript.tsx | 24 +- app/src/lib/copilot/plugin-tools.tsx | 209 ------------------ app/src/lib/copilot/provider.tsx | 3 - app/src/lib/plugins/tool-result.ts | 45 ++++ 4 files changed, 61 insertions(+), 220 deletions(-) delete mode 100644 app/src/lib/copilot/plugin-tools.tsx create mode 100644 app/src/lib/plugins/tool-result.ts diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index 42b3ebf..5052fbb 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -22,6 +22,7 @@ import { useMessageScroller, } from "@/components/ui/message-scroller"; import { toVisibleChatItems } from "./chat-messages"; +import { forDisplay } from "@/lib/plugins/tool-result"; import type { QueuedMessage } from "./composer"; import { ToolLine } from "./tool-line"; import { ToolRenderBoundary } from "./tool-boundary"; @@ -467,16 +468,23 @@ 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 ?? ( + + {result ? ( + + {forDisplay(result)} + + ) : null} + + )} ); 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-result.ts b/app/src/lib/plugins/tool-result.ts new file mode 100644 index 0000000..ecdbd42 --- /dev/null +++ b/app/src/lib/plugins/tool-result.ts @@ -0,0 +1,45 @@ +/** + * 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 = 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\`\`\``; +} From 81fa2f18f67fcda7ae76f97fbe8028165729b555 Mon Sep 17 00:00:00 2001 From: David McKay Date: Tue, 18 Aug 2026 18:01:43 -0700 Subject: [PATCH 3/4] Let a run answer after it calls a tool, and name the call for a reader Two things found by driving it in a browser, neither of which any gate would have caught. A run stopped after one step. The AI SDK sets no `stopWhen` unless `maxSteps` is given, so a Bot called its tool, the result arrived, and the run ended before the model could say what it found: the person saw their own question and nothing else, while the audit trail recorded a successful call. Only set when there are tools, and capped at eight. The tool line read `mcp__notes__search_notes`. That prefix exists so a tool name is unique across every server a Bot holds, which is not the reader's problem. It now reads "Search notes", with the server beside it, and the server dropped when the action already names it. The server's own words stay behind the disclosure. Also here: a mock knowledge MCP server standing in for a customer's Notion, so slice 1 can answer with nothing real connected. It is `@copilotkit/aimock`, so a Bot talks to the same protocol implementation the tests do. --- .../components/channels/chat-transcript.tsx | 34 +++++++--- app/src/lib/plugins/tool-name.ts | 51 +++++++++++++++ app/tests/tool-name.test.ts | 45 +++++++++++++ bun.lock | 1 + package.json | 4 +- scripts/mock-knowledge-mcp.ts | 63 +++++++++++++++++++ server/src/copilot.ts | 19 +++++- 7 files changed, 206 insertions(+), 11 deletions(-) create mode 100644 app/src/lib/plugins/tool-name.ts create mode 100644 app/tests/tool-name.test.ts create mode 100644 scripts/mock-knowledge-mcp.ts diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index 5052fbb..0ed26c5 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -23,6 +23,7 @@ import { } from "@/components/ui/message-scroller"; import { toVisibleChatItems } from "./chat-messages"; import { forDisplay } 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"; @@ -476,20 +477,35 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ * What was called, shimmering until its result arrives, and then the server's own words * drawn the way a Bot's prose is drawn. */} - {drawn ?? ( - - {result ? ( - - {forDisplay(result)} - - ) : null} - - )} + {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); + return ( + + {result ? ( + + {forDisplay(result)} + + ) : null} + + ); +} + export function ChatTranscript({ busy = false, commandNames = "", 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/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/bun.lock b/bun.lock index 43d855f..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", 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/src/copilot.ts b/server/src/copilot.ts index f01794d..ac4d934 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -191,10 +191,27 @@ export function builtInAgentConfiguration( model: `${model.provider}/${model.defaultModel}`, prompt: agent.systemPrompt, apiKey, - ...(tools.length > 0 ? { tools } : {}), + /* + * 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. * From 6fd8196f47c9518ae749a5d7b376549b4f0bf089 Mon Sep 17 00:00:00 2001 From: David McKay Date: Wed, 19 Aug 2026 20:37:14 -0700 Subject: [PATCH 4/4] Say which tool answers were refused, and in words about the tool Three defects on the path the server-side tool loop now takes, found by driving it. A refused call was described from the browser fields. Those are present on an MCP context and deliberately empty, so that a rule written about a page evaluates to false against a tool call rather than being unevaluable, and `describeRefusal` branched on the file field being there rather than on it having a path. Every refused tool call therefore read "the file is blocked by the rule", naming a workspace it never went near and a path that was not there. It now names the tool and the server, and the file branch asks for a path. A tool returns a string and the runtime carries it in a message as JSON, so what reached the screen was that string encoded: the whole thing in quotes, every quote inside it escaped. A refusal read `"... blocked by the rule \"mcp.server == \\"notes\\"\"."`. That also broke more than the look, because the refusal marker is matched against the start of the text and an encoded string starts with a quote. And there was no marker to match. Refusals came back as the policy message alone, so the transcript had only the wording to go on, and drew a refusal as an ordinary result: the Bot said it had been blocked while the line above it looked like a successful call. `REFUSAL_MARKER` now leads a refused answer. The model is told it too, which is right for the model. The reader is not, because the line already says "Blocked" and three sayings of the same thing in one sentence is two too many. The marker is written out at both ends rather than shared, like a status code, because it crosses a network. The integration test asserts it rather than only that the reason mentions policy, which is what let it be missing. --- .../components/channels/chat-transcript.tsx | 23 +++++++- app/src/lib/plugins/tool-result.ts | 42 +++++++++++++- app/tests/tool-result.test.ts | 49 ++++++++++++++++ server/src/computer/policy.ts | 14 ++++- server/src/plugins/tools.ts | 14 ++++- server/tests/computer-policy.test.ts | 57 +++++++++++++++++++ .../server-side-tools.integration.test.ts | 5 +- 7 files changed, 196 insertions(+), 8 deletions(-) create mode 100644 app/tests/tool-result.test.ts diff --git a/app/src/components/channels/chat-transcript.tsx b/app/src/components/channels/chat-transcript.tsx index 0ed26c5..baeb23d 100644 --- a/app/src/components/channels/chat-transcript.tsx +++ b/app/src/components/channels/chat-transcript.tsx @@ -22,7 +22,7 @@ import { useMessageScroller, } from "@/components/ui/message-scroller"; import { toVisibleChatItems } from "./chat-messages"; -import { forDisplay } from "@/lib/plugins/tool-result"; +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"; @@ -491,15 +491,32 @@ const TranscriptToolCall = memo(function TranscriptToolCall({ */ 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 ( - {result ? ( + {body ? ( - {forDisplay(result)} + {forDisplay(body)} ) : null} diff --git a/app/src/lib/plugins/tool-result.ts b/app/src/lib/plugins/tool-result.ts index ecdbd42..1249566 100644 --- a/app/src/lib/plugins/tool-result.ts +++ b/app/src/lib/plugins/tool-result.ts @@ -1,3 +1,41 @@ +/** + * 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. * @@ -6,8 +44,8 @@ * the markdown it was hiding, and anything else is fenced as JSON. Nothing is discarded. */ export function forDisplay(text: string): string { - const trimmed = text.trim(); - if (!trimmed.startsWith("{") && !trimmed.startsWith("[")) return text; + const trimmed = asText(text).trim(); + if (!trimmed.startsWith("{") && !trimmed.startsWith("[")) return trimmed; let parsed: unknown; try { 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/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/plugins/tools.ts b/server/src/plugins/tools.ts index ed74e5b..2c284fb 100644 --- a/server/src/plugins/tools.ts +++ b/server/src/plugins/tools.ts @@ -16,6 +16,16 @@ import { PluginRefusedError, type PluginStore } from "./store"; * 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; @@ -72,7 +82,9 @@ export async function grantedTools(options: { }); return result.text; } catch (error) { - if (error instanceof PluginRefusedError) return error.message; + 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 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/server-side-tools.integration.test.ts b/server/tests/server-side-tools.integration.test.ts index 6c2760b..d0fbffd 100644 --- a/server/tests/server-side-tools.integration.test.ts +++ b/server/tests/server-side-tools.integration.test.ts @@ -12,7 +12,7 @@ import { pluginGrants, } from "../src/db/schema"; import { createPluginStore } from "../src/plugins/store"; -import { grantedTools } from "../src/plugins/tools"; +import { grantedTools, REFUSAL_MARKER } from "../src/plugins/tools"; import type { ActionPolicy } from "../src/policy/engine"; import { TEST_POOL } from "./support/database"; @@ -193,6 +193,9 @@ describe("the tools a Bot is handed on the server", () => { // 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)