diff --git a/server/src/app.ts b/server/src/app.ts index 327430d3..23595c94 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -659,6 +659,25 @@ export function createApp( intentRouter, requireUser, auditStore, + /* + * Which vendors each coworker holds tools for, so the router weighs what a coworker can + * reach and not only what somebody wrote it was for. Only when there is a plugin store to + * ask: a deployment with no connectors routes exactly as it did. + */ + pluginStore + ? async (agentId) => { + const granted = await pluginStore.listForAgent(agentId); + return [ + ...new Set( + granted.tools.map( + (tool) => + tool.toolName.replace(/^mcp__/, "").split("__")[0] ?? + tool.toolName, + ), + ), + ]; + } + : undefined, ), ); } diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 766ce4f2..8582a85d 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -8,6 +8,7 @@ import { import { createCopilotHonoHandler } from "@copilotkit/runtime/v2/hono"; import { z } from "zod"; import { COMPUTER_GUIDANCE } from "../../shared/bot-prompt"; +import { grantedToolGuidance } from "./plugins/tools"; import type { AgentActor } from "./agents/profile-types"; import type { StallGuard } from "./channels/stall-guard"; import type { DeploymentConfig } from "./config"; @@ -200,9 +201,18 @@ export function builtInAgentConfiguration( return { model: `${model.provider}/${model.defaultModel}`, - prompt: computerGuidance - ? `${agent.systemPrompt}\n\n${computerGuidance}` - : agent.systemPrompt, + /* + * The package's role, then what this Bot actually holds, then the computer. + * + * The grants go BEFORE the computer prose on purpose. That prose is long and emphatic about the + * browser and mentions connectors nowhere, so a Bot that read it last reached for the browser + * even when it held a tool for the exact system being asked about. + */ + prompt: [ + agent.systemPrompt, + ...(grantedToolGuidance(tools) ? [grantedToolGuidance(tools)] : []), + ...(computerGuidance ? [computerGuidance] : []), + ].join("\n\n"), apiKey, /* * A run stops after one step unless told otherwise, which for a Bot with tools means it calls @@ -327,13 +337,37 @@ function remoteAgentWithStandingRole( ? { fetch: stallGuard.watch({ id: agent.id, name: agent.name }) } : {}), }); + /* + * What this Bot holds, as a second standing message. + * + * Beside the role rather than inside it, because the role comes from the package and this comes + * from the grants: they change for different reasons and at different times. Sent on every run for + * the same reason the tools are, so switching a connector on reaches the next run. + * + * The remote path needs this more than the built-in one, not less. A framework Bot is handed the + * tools as an offer and decides for itself what to call, with `COMPUTER_GUIDANCE` as its whole + * prompt — a page about the browser that mentions connectors nowhere. That is the Bot that browsed + * to drive.google.com holding four Drive tools. + */ + const holdings = grantedToolGuidance(tools); + const holdingsMessage = holdings + ? { + id: `granted-tools:${agent.id}`, + role: "system" as const, + content: holdings, + } + : null; + remote.use((input, next) => next.run({ ...input, messages: [ agent.standingMessage, + ...(holdingsMessage ? [holdingsMessage] : []), ...input.messages.filter( - (message) => message.id !== agent.standingMessage.id, + (message) => + message.id !== agent.standingMessage.id && + message.id !== holdingsMessage?.id, ), ], /* diff --git a/server/src/plugins/tools.ts b/server/src/plugins/tools.ts index 0bc35518..35aa4753 100644 --- a/server/src/plugins/tools.ts +++ b/server/src/plugins/tools.ts @@ -48,6 +48,48 @@ export function parametersFor(inputSchema: Record): z.ZodType { return z.object({}).catchall(z.unknown()); } +/** + * What this Bot holds, said in its instructions rather than left to be inferred from a tool list. + * + * A tool array tells a model a tool exists. It does not tell it that the tool is the right way to + * reach that system, and it competes with a page of prose about the browser that every Bot is given + * whether or not it has any connectors at all. The browser prose wins: it is emphatic, it is about + * capability, and it says "never claim you cannot browse". + * + * So a Bot holding four Google Drive tools browsed to drive.google.com, met a sign-in page its + * container could never satisfy, and asked its person to sign in to a vendor that person had already + * connected. The tools were there the whole time. + * + * Generated from the grants rather than written down, because the point is that it tracks them. An + * administrator switching a connector on, or granting one more of its tools, changes what the Bot is + * told on its next run with nothing else to remember and nothing to keep in step. + * + * Empty when the Bot holds nothing, so a deployment with no connectors says nothing about them. + */ +export function grantedToolGuidance(tools: GrantedTool[]): string { + if (tools.length === 0) return ""; + + const bySystem = new Map(); + for (const tool of tools) { + // `mcp__server__tool`, which is the shape the model is offered. + const parts = tool.name.replace(/^mcp__/, "").split("__"); + const system = parts.length > 1 ? (parts[0] as string) : "this deployment"; + const rest = parts.length > 1 ? parts.slice(1).join("__") : tool.name; + bySystem.set(system, [...(bySystem.get(system) ?? []), rest]); + } + + return [ + "You can reach these systems directly, as the person asking, with their own access:", + ...[...bySystem.entries()].map( + ([system, names]) => `- ${system}: ${names.join(", ")}`, + ), + "Use them for anything about those systems. Do NOT browse to one of their websites instead: your", + "browser is signed in as nobody, so it sees less than these tools do and will meet a sign-in wall", + "that connecting an account has already solved. If one of these systems is involved but no tool", + "above covers the part you need, say which part is missing rather than going around it.", + ].join("\n"); +} + /** * Every MCP tool granted to one Bot, ready to hand to the runtime. * diff --git a/server/src/routing/classify.ts b/server/src/routing/classify.ts index 84377337..65905c21 100644 --- a/server/src/routing/classify.ts +++ b/server/src/routing/classify.ts @@ -18,6 +18,20 @@ export type RoutingCandidate = { name: string; /** What this coworker is for. The one line an operator wrote to say when to reach them. */ roleDescription: string; + /** + * The systems this coworker can actually reach, by name. + * + * Routing on the role description alone routes on what somebody wrote a coworker was for, which is + * not the same as what it can do. A question about a document in Google Drive went to the coworker + * whose description says "company knowledge" and which held no Drive grants at all, so it browsed + * to the vendor, met a sign-in wall and asked the person to sign in to an account they had already + * connected. The coworker that could have answered was one line further down the roster. + * + * Empty for a coworker holding nothing, which is most of them in most deployments. It is a hint + * rather than a filter: a specialist with no connectors is still the right answer to a question + * about its specialism. + */ + reaches?: readonly string[]; }; export type RoutingDecision = { @@ -37,7 +51,16 @@ export function routingPrompt( candidates: readonly RoutingCandidate[], ): string { const roster = candidates - .map((c) => `- id: ${c.id}\n name: ${c.name}\n for: ${c.roleDescription}`) + .map((c) => + [ + `- id: ${c.id}`, + ` name: ${c.name}`, + ` for: ${c.roleDescription}`, + ...(c.reaches && c.reaches.length > 0 + ? [` can reach: ${c.reaches.join(", ")}`] + : []), + ].join("\n"), + ) .join("\n"); return [ "You route a person's message to the one coworker best suited to it.", @@ -46,6 +69,18 @@ export function routingPrompt( "", 'Reply with only JSON: {"agentId": "", "reason": "", "confidence": <0..1>}.', "Pick the specialist whose purpose matches the message. If none clearly fits, use the most general coworker and give it a low confidence.", + /* + * Only when somebody on the roster can actually reach something. + * + * A deployment with no connectors would otherwise carry a rule about systems none of its + * coworkers have, in every routing prompt it ever sends. Same principle as the guidance a Bot + * gets about its own grants: say nothing about what is not there. + */ + ...(candidates.some((c) => c.reaches && c.reaches.length > 0) + ? [ + "When the message names a system a coworker can reach, prefer that coworker: one that cannot reach it has no way to answer and will fall back to a browser that is signed in as nobody. Purpose still comes first — a specialist with no systems listed is right for a question about its specialism.", + ] + : []), "", `Message: ${text}`, ].join("\n"); diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index c884d4d1..f951ac45 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -21,6 +21,14 @@ export function createRoutingRoutes( router: IntentRouter, requireUser: MiddlewareHandler<{ Variables: AppVariables }>, auditStore?: AuditStore, + /** + * Which systems a coworker can reach, for the router to weigh alongside what it is for. + * + * Optional, and absent leaves routing exactly as it was: a deployment with no connectors has + * nothing to add here, and one that cannot answer the question should not have routing fail over + * it. Asked per request rather than held, because a grant added a minute ago has to count. + */ + reachableSystems?: (agentId: string) => Promise, ) { const routes = new Hono<{ Variables: AppVariables }>(); @@ -39,11 +47,25 @@ export function createRoutingRoutes( if (!preferred) { return context.json({ error: "No coworker is available." }, 409); } - const candidates: RoutingCandidate[] = roster.map((a) => ({ - id: a.id, - name: a.name, - roleDescription: a.roleDescription, - })); + const candidates: RoutingCandidate[] = await Promise.all( + roster.map(async (a) => ({ + id: a.id, + name: a.name, + roleDescription: a.roleDescription, + /* + * Never allowed to break routing. A connector store that is slow or unhappy must not turn + * "who is this for" into an error, so a failure here is the same as holding nothing: the + * router falls back to matching on purpose alone, which is what it did before. + */ + ...(reachableSystems + ? { + reaches: await reachableSystems(a.id).catch( + () => [] as readonly string[], + ), + } + : {}), + })), + ); const decision = await router.route(text, candidates, preferred.id); diff --git a/server/tests/copilot.test.ts b/server/tests/copilot.test.ts index 5bd14d47..ee623640 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -9,6 +9,7 @@ import { resolveRuntimeAgents, standingRoleMessage, } from "../src/copilot"; +import { grantedToolGuidance } from "../src/plugins/tools"; // Every agent row now joins its profile, so the row a coworker is built from always names it. const assistantRow = { @@ -502,3 +503,63 @@ function fakeAgUiEndpoint() { [Symbol.asyncDispose]: () => server.stop(true), }; } + +/** + * A Bot is told what it holds, not only handed it. + * + * A tool array tells a model a tool exists. It does not say the tool is the right way to reach that + * system, and it competes with `COMPUTER_GUIDANCE`: a page of emphatic prose about the browser that + * every Bot gets whether or not it has a single connector, and that mentions connectors nowhere. + * + * The browser prose won. A Bot holding four Google Drive tools browsed to drive.google.com, met a + * sign-in page its container could never satisfy, and asked its person to sign in to a vendor that + * person had already connected. Asked a question with no tool for it, another went reading a + * government website and looped on its 404 page. + * + * Both kinds are asserted because they are built by different functions, and the remote one is the + * one that failed in the product. + */ +describe("what a Bot is told it holds", () => { + const drive = [ + { name: "mcp__google-drive__search_files" }, + { name: "mcp__google-drive__read_file_content" }, + ] as never[]; + + test("names the system and its tools", () => { + const guidance = grantedToolGuidance(drive); + expect(guidance).toContain("google-drive"); + expect(guidance).toContain("search_files"); + expect(guidance).toContain("read_file_content"); + }); + + test("says not to browse to a vendor it has a tool for", () => { + // The whole point. Without this line the tool list is inert beside the browser prose. + expect(grantedToolGuidance(drive).toLowerCase()).toContain("do not browse"); + }); + + test("says nothing at all when the Bot holds nothing", () => { + // A deployment with no connectors must not be told about connectors it does not have. + expect(grantedToolGuidance([])).toBe(""); + }); + + test("a built-in Bot is told before it is told about the browser", () => { + const prompt = builtInAgentConfiguration( + { + id: "risk-analyst", + name: "Risk Analyst", + type: "built_in", + systemPrompt: "Investigate policies.", + }, + { provider: "openai", defaultModel: "gpt-4.1" }, + "openai-secret", + drive, + "BROWSER GUIDANCE HERE", + ).prompt as string; + + // Order is the fix, not merely presence: the grants have to land before the browser prose. + expect(prompt.indexOf("google-drive")).toBeGreaterThan(-1); + expect(prompt.indexOf("google-drive")).toBeLessThan( + prompt.indexOf("BROWSER GUIDANCE HERE"), + ); + }); +}); diff --git a/server/tests/routing-classify.test.ts b/server/tests/routing-classify.test.ts index ef93da24..75a1271f 100644 --- a/server/tests/routing-classify.test.ts +++ b/server/tests/routing-classify.test.ts @@ -1,6 +1,7 @@ import { describe, expect, test } from "bun:test"; import { createIntentRouter, + routingPrompt, type RoutingCandidate, } from "../src/routing/classify"; @@ -100,3 +101,62 @@ describe("routing a message with no @mention", () => { expect(r.fallback).toBe(true); }); }); + +/** + * The router is told what each coworker can reach, not only what it is for. + * + * Routing on the role description alone routes on what somebody wrote a coworker was for, which is + * not the same as what it can do. A question about a document in Google Drive went to the coworker + * whose description says "company knowledge" and which held no Drive grants at all. It browsed to + * the vendor, met a sign-in wall, and asked the person to sign in to an account they had already + * connected. The coworker that could have answered was one line further down the roster. + */ +describe("routing on what a coworker can reach", () => { + const withReach: RoutingCandidate[] = [ + { + id: "knowledge", + name: "Knowledge", + roleDescription: "company knowledge questions", + }, + { + id: "risk-analyst", + name: "Risk Analyst", + roleDescription: "risk and compliance", + reaches: ["google-drive"], + }, + ]; + + test("names the systems in the roster the model is given", () => { + const prompt = routingPrompt("what is in my Drive doc?", withReach); + expect(prompt).toContain("can reach: google-drive"); + }); + + test("says nothing about reach for a coworker that holds nothing", () => { + // Most coworkers in most deployments. An empty line here would be noise in every prompt. + const prompt = routingPrompt("anything", withReach); + const knowledgeBlock = prompt.slice( + prompt.indexOf("id: knowledge"), + prompt.indexOf("id: risk-analyst"), + ); + expect(knowledgeBlock).not.toContain("can reach"); + }); + + test("tells the model to prefer reach without letting it override purpose", () => { + /* + * Both halves matter. Preferring a coworker that can reach the system is the fix; letting that + * outrank purpose would send every question to whichever coworker happens to hold a connector, + * which is a different bug with the same shape. + */ + const prompt = routingPrompt("anything", withReach); + expect(prompt).toContain("prefer that coworker"); + expect(prompt).toContain("Purpose still comes first"); + }); + + test("a roster with no reach at all reads exactly as it did before", () => { + // A deployment with no connectors must not have its routing prompt changed by this. + const plain = routingPrompt("anything", [ + { id: "a", name: "A", roleDescription: "alpha" }, + ]); + expect(plain).not.toContain("can reach"); + }); +}); diff --git a/server/tests/routing.test.ts b/server/tests/routing.test.ts new file mode 100644 index 00000000..e69de29b