diff --git a/CHANGELOG.md b/CHANGELOG.md index c851fcd3..032f829a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,22 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A coworker named in the message is routed to without asking a model + +Naming a coworker in the text — "ask Risk Analyst to review this" — went to the intent router like +any other message, so the deployment paid a model call to be told what the person had already said, +and sometimes was told something else. A name that matches exactly one coworker on that person's +roster now routes straight to them, recorded as `named by the person asking` on the same +`channel.routed` row. A name that matches more than one is refused with both names rather than +guessed at, and a name nobody on the roster answers to falls through to the router as before. + +### Routing refuses rather than routes on a connector read it could not make + +Which systems a coworker can reach is weighed by the router alongside what the coworker is for. A +failed read of that used to be treated as "reaches nothing", which is a statement about the +deployment rather than an absence of one: a database that blinked quietly re-routed messages away +from the coworker that could actually do the work. It now fails the request instead. + ### A Bot's shell can no longer reach the embedded database without a password In the all-in-one image the cluster was `trust`-auth on loopback, and the Bot's shell runs in the diff --git a/server/src/agents/agent-resolver.ts b/server/src/agents/agent-resolver.ts new file mode 100644 index 00000000..db7d1e96 --- /dev/null +++ b/server/src/agents/agent-resolver.ts @@ -0,0 +1,101 @@ +import type { AbstractAgent } from "@ag-ui/client"; +import type { AgentFetch, StallGuard } from "../channels/stall-guard"; +import { + type HandoffForRun, + type LoadAgentsForActor, + type LoadToolsForBot, + type RuntimeModel, + resolveRuntimeAgents, + type SignRun, + type ToolSelection, +} from "../copilot"; +import type { AgentActor } from "./profile-types"; + +export type ActorAgentResolver = { + resolveAgentsForActor( + actor: AgentActor, + ): Promise>; + resolveAgentForActor( + actor: AgentActor, + agentId: string, + ): Promise; +}; + +export type ActorAgentResolverDependencies = { + loadAgents: LoadAgentsForActor; + model: RuntimeModel; + resolveModelApiKey: () => Promise; + stallGuard?: StallGuard; + loadToolsForActor?: (actorId: string) => LoadToolsForBot; + signRunForActor?: (actorId: string) => SignRun; + computerGuidance?: string; + loadVendors?: () => Promise; + selectionForActor?: (actorId: string) => ToolSelection; + agentFetch?: AgentFetch; + /** + * What a Bot may reach past itself for, resolved for whoever is asking. + * + * Per actor for the same reason the tools are: which Bots may be reached is decided against the + * roster that person can see, so a Bot must never be able to address one they cannot. + */ + handoffForActor?: (actorId: string) => HandoffForRun; +}; + +/** + * Resolves the coworkers available to one OpenBot actor. + * + * Every surface enters through this boundary so it shares the same visibility, grants, assertions, + * skill selection, and endpoint dial policy for a person. + */ +export function createActorAgentResolver( + deps: ActorAgentResolverDependencies, +): ActorAgentResolver { + const resolveRegisteredAgents = ( + actor: AgentActor, + registered: Awaited>, + /** + * Build only this Bot, when the caller already knows which one it wants. + * + * The roster is still read in full, so a Bot this person cannot see is still absent. The others + * are simply neither built nor asked what they hold, which is a query per Bot a headless turn + * or a Slack thread has no use for. + */ + onlyAgentId?: string, + ) => + resolveRuntimeAgents( + () => Promise.resolve(registered), + deps.model, + deps.resolveModelApiKey, + deps.stallGuard, + deps.loadToolsForActor?.(actor.id), + deps.signRunForActor?.(actor.id), + deps.computerGuidance, + deps.loadVendors, + deps.selectionForActor?.(actor.id), + deps.agentFetch, + deps.handoffForActor?.(actor.id), + onlyAgentId, + ); + + const resolveAgentsForActor = async (actor: AgentActor) => + resolveRegisteredAgents(actor, await deps.loadAgents(actor)); + + return { + resolveAgentsForActor, + async resolveAgentForActor(actor, agentId) { + const registered = await deps.loadAgents(actor); + if (!registered.some((agent) => agent.id === agentId)) { + throw new Error(`Coworker ${agentId} is unavailable to this user.`); + } + + const agents = await resolveRegisteredAgents(actor, registered, agentId); + const agent = Object.hasOwn(agents, agentId) + ? agents[agentId] + : undefined; + if (!agent) { + throw new Error(`Coworker ${agentId} is unavailable to this user.`); + } + return agent; + }, + }; +} diff --git a/server/src/app.ts b/server/src/app.ts index 20561446..e7a5155d 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -44,6 +44,7 @@ import { createRoutineRoutes, type RoutineStore } from "./routines/routes"; import type { RoutineRunner } from "./routines/runner"; import type { IntentRouter } from "./routing/classify"; import { createRoutingRoutes } from "./routing/routes"; +import { createCoworkerRoutingService } from "./routing/service"; import type { PackageStatusReader } from "./tenant-package"; /** @@ -788,29 +789,31 @@ export function createApp( app.route( "/api/route", createRoutingRoutes( - agentProfileStore, - 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, + createCoworkerRoutingService({ + store: agentProfileStore, + router: intentRouter, + 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. + */ + reachableSystems: 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, + ]; + } + : undefined, + }), + requireUser, ), ); } diff --git a/server/src/copilot.ts b/server/src/copilot.ts index 891c53ee..d8d44f2f 100644 --- a/server/src/copilot.ts +++ b/server/src/copilot.ts @@ -10,10 +10,8 @@ import { createCopilotHonoHandler } from "@copilotkit/runtime/v2/hono"; import type { Observable } from "rxjs"; import { defer, from, switchMap } from "rxjs"; import { z } from "zod"; -import { - COMPUTER_GUIDANCE, - PROVENANCE_GUIDANCE, -} from "../../shared/bot-prompt"; +import { PROVENANCE_GUIDANCE } from "../../shared/bot-prompt"; +import type { ActorAgentResolver } from "./agents/agent-resolver"; import type { AgentActor } from "./agents/profile-types"; import type { AgentFetch, StallGuard } from "./channels/stall-guard"; import type { DeploymentConfig } from "./config"; @@ -856,55 +854,10 @@ export type LoadAgentsForActor = ( */ export function createRequestAgents( identifyActor: IdentifyActor, - loadAgents: LoadAgentsForActor, - model: RuntimeModel, - resolveModelApiKey: () => Promise, - /** - * Shared across every request rather than built per run, because it is the thing that has to - * outlive one: the sweep that notices a silent stream has to still be running after the request - * 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, - /** Resolved per request, because what it signs is who this request turned out to be. */ - signRunForActor?: (actorId: string) => SignRun, - /** What every built-in Bot is told about the computer. Absent means this deployment has none. */ - computerGuidance?: string, - /** Which vendors this deployment connects to, held by a Bot or not. Absent means none. */ - loadVendors?: () => Promise, - /** - * How a run's tools are narrowed, resolved for whoever is asking. - * - * Per actor like the tools themselves, because the skills a Bot holds are read through the same - * grants, and because the discovery row has to name the person the run belongs to. - */ - selectionForActor?: (actorId: string) => ToolSelection, - /** The fetch remote agents are dialled with. See {@link buildAgents}. */ - agentFetch?: AgentFetch, - /** - * How a run gets its tool for handing work to another Bot, resolved for whoever is asking. - * - * Per actor for the same reason the tools are: which Bots may be reached is decided against the - * roster that person can see, so a Bot must never be able to address one they cannot. - */ - handoffForActor?: (actorId: string) => HandoffForRun, + resolver: ActorAgentResolver, ) { return async ({ request }: { request: Request }) => { - const actor = await identifyActor(request); - return resolveRuntimeAgents( - () => loadAgents(actor), - model, - resolveModelApiKey, - stallGuard, - loadToolsForActor?.(actor.id), - signRunForActor?.(actor.id), - computerGuidance, - loadVendors, - selectionForActor?.(actor.id), - agentFetch, - handoffForActor?.(actor.id), - ); + return resolver.resolveAgentsForActor(await identifyActor(request)); }; } @@ -994,26 +947,10 @@ const THREAD_LOCK_TTL_SECONDS = 120; export function mountCopilotRuntime( config: DeploymentConfig, - model: RuntimeModel, - loadAgents: LoadAgentsForActor, - resolveModelApiKey: () => Promise, + resolver: ActorAgentResolver, identifyUser: IdentifyUser, identifyActor: IdentifyActor, - /** - * The watch on Bot streams. Not optional, unlike the parameter it forwards to: a guard built from - * a timeout of zero already watches nothing, so an unconfigured deployment has one to hand and - * there is no reason for a caller to have to say `undefined` here to reach `basePath`. - */ - stallGuard: StallGuard, - loadToolsForActor?: (actorId: string) => LoadToolsForBot, - signRunForActor?: (actorId: string) => SignRun, basePath = "/api/copilotkit", - loadVendors?: () => Promise, - selectionForActor?: (actorId: string) => ToolSelection, - /** The fetch remote agents are dialled with. See {@link buildAgents}. */ - agentFetch?: AgentFetch, - /** How a run gets its tool for handing work on. Absent means no Bot is offered one. */ - handoffForActor?: (actorId: string) => HandoffForRun, ) { const { intelligence } = config.runtime; @@ -1039,25 +976,12 @@ export function mountCopilotRuntime( actor: AgentActor; botId: string; }): Promise => { - const { actor } = input; - const agents = await resolveRuntimeAgents( - () => loadAgents(actor), - model, - resolveModelApiKey, - stallGuard, - loadToolsForActor?.(actor.id), - signRunForActor?.(actor.id), - config.computer ? COMPUTER_GUIDANCE : undefined, - loadVendors, - selectionForActor?.(actor.id), - agentFetch, - handoffForActor?.(actor.id), - // Only the Bot this hop is for. The roster is still read in full, so a Bot this person cannot - // see is still absent; what this skips is constructing the other Bots and asking the database - // what each of them was granted, on every delivery and again on every retry. - input.botId, - ); - return agents[input.botId] ?? null; + // Only the Bot this hop is for. The roster is still read in full, so a Bot this person cannot + // see is still absent; what this skips is constructing the other Bots and asking the database + // what each of them was granted, on every delivery and again on every retry. + return await resolver + .resolveAgentForActor(input.actor, input.botId) + .catch(() => null); }; /* @@ -1088,26 +1012,7 @@ export function mountCopilotRuntime( : {}), // `identifyUser` is the Intelligence projection of the same person `identifyActor` returns: // one resolver decides both whose threads these are and whose coworkers exist. - agents: createRequestAgents( - identifyActor, - loadAgents, - model, - resolveModelApiKey, - stallGuard, - loadToolsForActor, - signRunForActor, - /* - * Only when a computer exists. The tools themselves are registered by the surface, so a Bot is - * offered them without this and the guidance is what tells it how they go together: snapshot - * before acting, and ask a person to take the wheel at a sign-in rather than reporting the task - * as impossible. Absent computer, absent guidance: a Bot is not told about hands it has not got. - */ - config.computer ? COMPUTER_GUIDANCE : undefined, - loadVendors, - selectionForActor, - agentFetch, - handoffForActor, - ) as never, + agents: createRequestAgents(identifyActor, resolver) as never, }); return { diff --git a/server/src/index.ts b/server/src/index.ts index a3b18ef6..f41b3feb 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -5,6 +5,7 @@ import { } from "@copilotkit/runtime/v2"; import { serve } from "bun"; import { COMPUTER_GUIDANCE } from "../../shared/bot-prompt"; +import { createActorAgentResolver } from "./agents/agent-resolver"; import { mintRunAssertion, readRunAssertion } from "./agents/callback-token"; import { createAgentFetch } from "./agents/endpoint"; import { askTheirOwnPerson, escalationTool } from "./agents/escalation"; @@ -47,10 +48,10 @@ import { import { createSnapshotStore } from "./computer/snapshot-store"; import { loadConfig } from "./config"; import { + type HandoffForRun, type IdentifyActor, type IdentifyUser, mountCopilotRuntime, - resolveRuntimeAgents, type ToolSelection, } from "./copilot"; import { @@ -595,6 +596,114 @@ const agentFetch = createAgentFetch({ }, }); +/* + * What a Bot may reach past itself for: another Bot, and a person. Made per run and per person. + * + * Per person because which Bots may be reached is decided against the roster that person can + * see: a Bot must never be able to address one they cannot, or this becomes a way around agent + * visibility. Per run because the caps need to know how deep the chain already is and where an + * answer belongs, and both of those are the deployment's own statement about the run rather than + * anything the model can edit. + */ +const handoffForActor = + (actorId: string): HandoffForRun => + async (botId, input) => { + const from = readRunAssertion( + (input.forwardedProps as { openbotRun?: unknown } | undefined) + ?.openbotRun, + config.keyEncryptionKey, + ); + const run = { + botId, + actorId, + runId: input.runId, + threadId: input.threadId, + depth: from?.depth ?? 0, + }; + /* + * The caps are checked BEFORE the grants query, not inside the tool that would discard it. + * + * `handoffTool` short-circuits on all three of these, but only after being handed a + * `hasSomebodyToAsk` that costs a query. So a deployment which switched the capability off + * still paid one grants read per run of every Bot, for a tool it was never going to be offered, + * and a run already at the cap paid it again. + */ + const couldHandOn = + config.handoff.maxDepth > 0 && + config.handoff.maxPerRun > 0 && + run.depth < config.handoff.maxDepth; + + const passing = couldHandOn + ? handoffTool({ + desk: handoffDesk, + /* + * How deep this run already is comes from the assertion the deployment signed when it handed + * this work on. A run a person started carries none, and none means zero. + * + * NOT `from.botId`. The assertion proves what this run is, and the Bot is whichever one the + * runtime is building right now: on a hop those agree, and taking the id from the signed + * value rather than from the build would let a stale assertion aim the next hop at the + * wrong Bot's grants. + */ + from: run, + // Read now rather than at boot, so a grant made a minute ago counts and one revoked a + // minute ago stops counting. + hasSomebodyToAsk: + ( + await pluginStore + .botsReachableFrom(botId) + .catch(() => [] as string[]) + ).length > 0, + maxDepth: config.handoff.maxDepth, + maxPerRun: config.handoff.maxPerRun, + }) + : null; + /* + * The way to stop and ask is offered whether or not there is a Bot to hand to. + * + * It is the cheaper of the two and the one a Bot should reach for first: asking the person who + * is already in the conversation spends nothing and cannot be aimed anywhere they cannot see. + * A deployment that offered only the expensive exit would push every unanswerable question + * sideways into another run. + */ + const asking = escalationTool({ + from: run, + route: askTheirOwnPerson, + auditStore: bootAuditStore, + }); + return passing ? [passing, asking] : [asking]; + }; + +/** + * One place a coworker is built for one person, for every surface that runs one. + * + * The named constants above exist because two callers had to build the SAME Bot. There are now + * four — a person's chat request, a routine's headless turn, a hop delivered to another Bot, and a + * Slack thread — and passing eleven collaborators to each of them in the right order is a drift + * waiting to happen: a surface that got one argument wrong would run, and quietly hold different + * tools or a different role from the Bot the person is talking to. So the collaborators are bound + * once here, and every surface asks this for a coworker instead. + */ +const actorAgentResolver = createActorAgentResolver({ + loadAgents: loadAgentsForActor, + model: tenantPackage.model, + resolveModelApiKey: resolveRuntimeModelApiKey, + stallGuard, + loadToolsForActor, + signRunForActor, + /* + * Only when a computer exists. The tools themselves are registered by the surface, so a Bot is + * offered them without this and the guidance is what tells it how they go together: snapshot + * before acting, and ask a person to take the wheel at a sign-in rather than reporting the task + * as impossible. Absent computer, absent guidance: a Bot is not told about hands it has not got. + */ + computerGuidance: config.computer ? COMPUTER_GUIDANCE : undefined, + loadVendors, + selectionForActor, + agentFetch, + handoffForActor, +}); + /** * Who a routine acts as, resolved the way {@link resolveRequestActor} resolves it. * @@ -636,24 +745,12 @@ const buildAgentFor = async ({ agentId: string; }) => { const actor = await actorFor(ownerUserId); - const agents = await resolveRuntimeAgents( - () => loadAgentsForActor(actor), - tenantPackage.model, - resolveRuntimeModelApiKey, - stallGuard, - loadToolsForActor(actor.id), - signRunForActor(actor.id), - config.computer ? COMPUTER_GUIDANCE : undefined, - loadVendors, - selectionForActor(actor.id), - agentFetch, - undefined, - // Only the Bot this routine names. Same reason as the hop delivery: the roster is still read in - // full so a Bot this owner cannot see is still absent, but the other Bots are neither built nor - // asked what they hold. - agentId, - ); - const agent = agents[agentId]; + // Only the Bot this routine names. Same reason as the hop delivery: the roster is still read in + // full so a Bot this owner cannot see is still absent, but the other Bots are neither built nor + // asked what they hold. + const agent = await actorAgentResolver + .resolveAgentForActor(actor, agentId) + .catch(() => null); if (!agent) { /* * Named, and raised rather than swallowed. The routine's Bot was deleted, or made private by @@ -717,93 +814,9 @@ const routineRunner = createRoutineRunner({ */ const copilotRuntime = mountCopilotRuntime( config, - tenantPackage.model, - loadAgentsForActor, - resolveRuntimeModelApiKey, + actorAgentResolver, identifyUser, identifyActor, - stallGuard, - loadToolsForActor, - signRunForActor, - undefined, - loadVendors, - selectionForActor, - agentFetch, - /* - * What a Bot may reach past itself for: another Bot, and a person. Made per run and per person. - * - * Per person because which Bots may be reached is decided against the roster that person can - * see: a Bot must never be able to address one they cannot, or this becomes a way around agent - * visibility. Per run because the caps need to know how deep the chain already is and where an - * answer belongs, and both of those are the deployment's own statement about the run rather than - * anything the model can edit. - */ - (actorId) => async (botId, input) => { - const from = readRunAssertion( - (input.forwardedProps as { openbotRun?: unknown } | undefined) - ?.openbotRun, - config.keyEncryptionKey, - ); - const run = { - botId, - actorId, - runId: input.runId, - threadId: input.threadId, - depth: from?.depth ?? 0, - }; - /* - * The caps are checked BEFORE the grants query, not inside the tool that would discard it. - * - * `handoffTool` short-circuits on all three of these, but only after being handed a - * `hasSomebodyToAsk` that costs a query. So a deployment which switched the capability off - * still paid one grants read per run of every Bot, for a tool it was never going to be offered, - * and a run already at the cap paid it again. - */ - const couldHandOn = - config.handoff.maxDepth > 0 && - config.handoff.maxPerRun > 0 && - run.depth < config.handoff.maxDepth; - - const passing = couldHandOn - ? handoffTool({ - desk: handoffDesk, - /* - * How deep this run already is comes from the assertion the deployment signed when it handed - * this work on. A run a person started carries none, and none means zero. - * - * NOT `from.botId`. The assertion proves what this run is, and the Bot is whichever one the - * runtime is building right now: on a hop those agree, and taking the id from the signed - * value rather than from the build would let a stale assertion aim the next hop at the - * wrong Bot's grants. - */ - from: run, - // Read now rather than at boot, so a grant made a minute ago counts and one revoked a - // minute ago stops counting. - hasSomebodyToAsk: - ( - await pluginStore - .botsReachableFrom(botId) - .catch(() => [] as string[]) - ).length > 0, - maxDepth: config.handoff.maxDepth, - maxPerRun: config.handoff.maxPerRun, - }) - : null; - /* - * The way to stop and ask is offered whether or not there is a Bot to hand to. - * - * It is the cheaper of the two and the one a Bot should reach for first: asking the person who - * is already in the conversation spends nothing and cannot be aimed anywhere they cannot see. - * A deployment that offered only the expensive exit would push every unanswerable question - * sideways into another run. - */ - const asking = escalationTool({ - from: run, - route: askTheirOwnPerson, - auditStore: bootAuditStore, - }); - return passing ? [passing, asking] : [asking]; - }, ); /** diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index 637713a3..c37b9754 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -1,96 +1,21 @@ import type { MiddlewareHandler } from "hono"; import { Hono } from "hono"; -import type { AgentProfileStore } from "../agents/profile-store"; -import type { AuditStore } from "../audit"; -import { recordAuditEvent } from "../audit"; import type { AppVariables } from "../auth/guards"; -import type { - IntentRouter, - RoutingCandidate, - RoutingUndecided, -} from "./classify"; - -const DEV_ACTOR_EMAIL = "dev@openbot.local"; +import type { HttpCoworkerRoutingService } from "./service"; /** - * Who to record the routing against, or nobody. + * Translate the shared coworker-routing result into the established HTTP contract. * - * The single-user development actor is not a real person and has no row to point at, so it is left - * off rather than written as a user id that resolves to nothing. - */ -function actorId( - actor: - | { - id?: string; - email?: string; - } - | null - | undefined, -): string | undefined { - return actor?.id && actor.email !== DEV_ACTOR_EMAIL ? actor.id : undefined; -} - -/** - * Decide which coworker a message is for, before a channel is pinned to one. - * - * The roster is read for the person asking, so this can only ever land on a coworker they are - * already allowed to reach. The decision is recorded like every other one in the product: a - * `channel.routed` row names where it went and why, and carries the candidate ids but never the - * message itself, which the audit payload redaction would drop anyway. - * - * A person who named a coworker with `@` has already decided, so nothing is inferred and no model - * is called. It is still recorded, with `viaMention` true and the person as the reason. Without - * that the trail answered "why did this go to Risk Analyst" for routed conversations and said - * nothing at all for chosen ones, which reads exactly like a row that failed to write. + * Choosing a coworker, applying visibility, invoking the intent model, and recording the canonical + * audit row are deliberately owned by CoworkerRoutingService. This layer only validates HTTP input + * and turns its outcome into status codes and JSON. */ export function createRoutingRoutes( - store: AgentProfileStore, - router: IntentRouter, + routing: HttpCoworkerRoutingService, 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 }>(); - /* - * The one place a `channel.routed` row is written, for both ways a message finds a coworker. - * - * Two call sites writing the same event is two payloads that drift, and a trail whose rows mean - * slightly different things depending on which branch produced them cannot be read at all. - */ - async function record( - actorUserId: string | undefined, - chosen: string, - reason: string, - fallback: boolean, - viaMention: boolean, - candidates: readonly string[], - /* - * Why the router did not decide, when it did not. - * - * On the row rather than only in the sentence, because this is the field a deployment counts. A - * router that has been unreachable for a week produced rows that read like ordinary - * "no confident match" ones, which is how #178 went unnoticed for as long as it did. - */ - undecided: RoutingUndecided | null, - ): Promise { - if (!auditStore) return; - await recordAuditEvent(auditStore, { - eventType: "channel.routed", - targetType: "agent", - targetId: chosen, - ...(actorUserId ? { actorUserId } : {}), - payload: { chosen, reason, fallback, viaMention, candidates, undecided }, - }); - } - routes.post("/", requireUser, async (context) => { const body = (await context.req.json().catch(() => null)) as { text?: unknown; @@ -98,104 +23,48 @@ export function createRoutingRoutes( } | null; const text = typeof body?.text === "string" ? body.text.trim() : ""; if (!text) return context.json({ error: "A message is required." }, 400); - const named = + const agentId = typeof body?.agentId === "string" && body.agentId.trim() ? body.agentId.trim() : null; - const actor = context.var.actor; - const roster = await store.list(actor, false); - // The same default the composer shows: the first public coworker, else the first at all. - const preferred = - roster.find((a) => a.visibility === "public") ?? roster[0]; - if (!preferred) { - return context.json({ error: "No coworker is available." }, 409); + const detail = await routing.routeDetailed({ + actor: context.var.actor, + text, + agentId, + }); + const { result } = detail; + if (result.kind === "none") { + return context.json( + { + error: agentId + ? "That coworker is not on your roster." + : "No coworker is available.", + }, + agentId ? 404 : 409, + ); } - - /* - * A named coworker is an instruction, not a question, so it is honoured as given. - * - * Checked against the same roster the router picks from, so `@` cannot reach further than - * routing can: a name that is not on it is refused rather than quietly turned into somebody - * else, because silently redirecting a message the person addressed by hand is the worst - * available answer. - */ - if (named) { - const chosen = roster.find((a) => a.id === named); - if (!chosen) { - return context.json( - { error: "That coworker is not on your roster." }, - 404, - ); - } - /* - * Third person, because the audit page is not read by the person who chose. - * - * This said "you chose them yourself", which is true in the conversation and false on an - * administrator's screen, where every row is somebody else's. The person is already on the - * row as `actorUserId`; the reason only has to say what kind of decision it was. - * - * @zopeVaibhav had this right in #134. - */ - const reason = "named by the person asking"; - // The person chose. Nothing was left to the router, so nothing about it was undecided. - await record( - actorId(actor), - chosen.id, - reason, - false, - true, - [chosen.id], - null, + if (result.kind === "ambiguous") { + return context.json( + { + error: "More than one coworker matches that name.", + names: result.names, + }, + 409, ); - return context.json({ - agentId: chosen.id, - name: chosen.name, - reason, - fallback: false, - viaMention: true, - }); } - 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); - - await record( - actorId(actor), - decision.agentId, - decision.reason, - decision.fallback, - false, - candidates.map((c) => c.id), - decision.undecided, + const response = { + agentId: result.agentId, + name: result.name, + reason: result.reason, + fallback: result.fallback, + viaMention: result.viaMention, + }; + // The composer chose this coworker directly, and the legacy response did not expose a model + // fallback cause for that path. Keep the model-routed response shape unchanged below. + return context.json( + agentId ? response : { ...response, undecided: detail.undecided }, ); - - return context.json({ - agentId: decision.agentId, - name: decision.name, - reason: decision.reason, - fallback: decision.fallback, - undecided: decision.undecided, - viaMention: false, - }); }); return routes; diff --git a/server/src/routing/service.ts b/server/src/routing/service.ts new file mode 100644 index 00000000..c9dadf78 --- /dev/null +++ b/server/src/routing/service.ts @@ -0,0 +1,382 @@ +import { canAccessAgent } from "../agents/profile-policy"; +import type { AgentProfileStore } from "../agents/profile-store"; +import type { AgentActor, AgentProfile } from "../agents/profile-types"; +import type { AuditStore } from "../audit"; +import { recordAuditEvent } from "../audit"; +import type { + IntentRouter, + RoutingCandidate, + RoutingUndecided, +} from "./classify"; + +const DEV_ACTOR_EMAIL = "dev@openbot.local"; +const WORD_CHARACTER = /[\p{L}\p{N}\p{M}_]/u; + +export type CoworkerRouteResult = + | { + kind: "selected"; + agentId: string; + name: string; + reason: string; + fallback: boolean; + viaMention: boolean; + } + | { kind: "ambiguous"; names: string[] } + | { kind: "none" }; + +type RoutingActor = AgentActor & { email?: string }; + +export type CoworkerRoutingInput = { + actor: RoutingActor; + text: string; + /** An explicit picker selection from a surface such as the web composer. */ + agentId?: string | null; +}; + +export type CoworkerRouteDetail = { + result: CoworkerRouteResult; + /** Kept for surfaces that have historically returned the model's fallback cause. */ + undecided: RoutingUndecided | null; +}; + +export type CoworkerRoutingService = { + route(input: CoworkerRoutingInput): Promise; +}; + +export type HttpCoworkerRoutingService = CoworkerRoutingService & { + routeDetailed(input: CoworkerRoutingInput): Promise; +}; + +export type CreateCoworkerRoutingServiceOptions = { + store: AgentProfileStore; + router: IntentRouter; + auditStore?: AuditStore; + reachableSystems?: (agentId: string) => Promise; +}; + +/** A safe categorical failure for a roster whose connector reachability could not be checked. */ +export class CoworkerReachabilityUnavailableError extends Error { + readonly code = "coworker_reachability_unavailable"; + + constructor() { + super("Coworker reachability is temporarily unavailable"); + this.name = "CoworkerReachabilityUnavailableError"; + } +} + +/** Normalize people-facing names before matching, without making matching fuzzy. */ +export function normalizeCoworkerName(value: string): string { + return value.normalize("NFKC").toLowerCase().trim().replace(/\s+/gu, " "); +} + +function hasTokenBoundaries(text: string, start: number, end: number): boolean { + const before = [...text.slice(0, start)].at(-1); + const after = [...text.slice(end)][0]; + return !before || !WORD_CHARACTER.test(before) + ? !after || !WORD_CHARACTER.test(after) + : false; +} + +type AliasOccurrence = { + start: number; + end: number; + profiles: ReadonlyMap; +}; + +function occurrencesOf( + text: string, + alias: string, + profiles: ReadonlyMap, +): AliasOccurrence[] { + const occurrences: AliasOccurrence[] = []; + let start = text.indexOf(alias); + while (start >= 0) { + const end = start + alias.length; + if (hasTokenBoundaries(text, start, end)) { + occurrences.push({ start, end, profiles }); + } + start = text.indexOf(alias, start + alias.length); + } + return occurrences; +} + +function actorId(actor: RoutingActor): string | undefined { + return actor.id && actor.email !== DEV_ACTOR_EMAIL ? actor.id : undefined; +} + +function suffixes(name: string): string[] { + const tokens = name.split(" "); + return tokens.slice(1).map((_, index) => tokens.slice(index + 1).join(" ")); +} + +function displayName(name: string): string { + return name.normalize("NFKC").trim().replace(/\s+/gu, " "); +} + +function utf8Hex(value: string): string { + return [...new TextEncoder().encode(value)] + .map((byte) => byte.toString(16).padStart(2, "0")) + .join(""); +} + +function codePointCompare(left: string, right: string): number { + const leftPoints = [...left]; + const rightPoints = [...right]; + for (let index = 0; index < leftPoints.length; index += 1) { + const leftPoint = leftPoints[index]?.codePointAt(0); + const rightPoint = rightPoints[index]?.codePointAt(0); + if (leftPoint === undefined) return -1; + if (rightPoint === undefined) return 1; + if (leftPoint !== rightPoint) return leftPoint - rightPoint; + } + return leftPoints.length - rightPoints.length; +} + +type AliasIndex = { + aliases: Map>; + labels: Map; +}; + +function addAlias( + aliases: AliasIndex["aliases"], + alias: string, + profile: AgentProfile, +): void { + if (!alias) return; + const profiles = aliases.get(alias) ?? new Map(); + profiles.set(profile.id, profile); + aliases.set(alias, profiles); +} + +function buildAliasIndex(roster: readonly AgentProfile[]): AliasIndex { + const byNormalizedName = new Map(); + for (const profile of roster) { + const normalized = normalizeCoworkerName(profile.name); + const profiles = byNormalizedName.get(normalized); + if (profiles) profiles.push(profile); + else byNormalizedName.set(normalized, [profile]); + } + + const aliases = new Map>(); + const labels = new Map(); + for (const profile of roster) { + const normalized = normalizeCoworkerName(profile.name); + const duplicates = byNormalizedName.get(normalized) ?? []; + const label = + duplicates.length > 1 + ? `${displayName(profile.name)} (id ${utf8Hex(profile.id)})` + : profile.name; + labels.set(profile.id, label); + addAlias(aliases, normalized, profile); + for (const suffix of suffixes(normalized)) + addAlias(aliases, suffix, profile); + if (duplicates.length > 1) { + addAlias(aliases, normalizeCoworkerName(label), profile); + } + } + return { aliases, labels }; +} + +type ExplicitOccurrence = { + start: number; + end: number; + profiles: Map; +}; + +/** + * Discard only aliases that a strictly longer explicit occurrence fully contains. + * + * Intervals are ordered by start, then widest first. A running maximum end therefore proves that a + * prior interval starts no later and reaches at least as far as the current one, which is exactly + * containment. Partial overlaps extend the maximum only for later contained intervals; they never + * suppress each other. + */ +function withoutContainedOccurrences( + occurrences: readonly AliasOccurrence[], +): ExplicitOccurrence[] { + const bySpan = new Map(); + for (const occurrence of occurrences) { + const key = `${occurrence.start}:${occurrence.end}`; + const merged = + bySpan.get(key) ?? + ({ + start: occurrence.start, + end: occurrence.end, + profiles: new Map(), + } satisfies ExplicitOccurrence); + for (const profile of occurrence.profiles.values()) { + merged.profiles.set(profile.id, profile); + } + bySpan.set(key, merged); + } + + const sorted = [...bySpan.values()].sort( + (left, right) => left.start - right.start || right.end - left.end, + ); + let maximumEnd = -1; + return sorted.filter((occurrence) => { + const contained = maximumEnd >= occurrence.end; + maximumEnd = Math.max(maximumEnd, occurrence.end); + return !contained; + }); +} + +function labelsFor( + profiles: Iterable, + labels: ReadonlyMap, +): string[] { + return [...profiles] + .map((profile) => labels.get(profile.id) ?? profile.name) + .sort(codePointCompare); +} + +function explicitNameRoute( + text: string, + roster: readonly AgentProfile[], +): CoworkerRouteResult | null { + const normalizedText = normalizeCoworkerName(text); + const { aliases, labels } = buildAliasIndex(roster); + const occurrences = [...aliases.entries()].flatMap(([alias, profiles]) => + occurrencesOf(normalizedText, alias, profiles), + ); + const explicitOccurrences = withoutContainedOccurrences(occurrences); + const profiles = new Map(); + for (const occurrence of explicitOccurrences) { + for (const profile of occurrence.profiles.values()) { + profiles.set(profile.id, profile); + } + } + if (profiles.size === 1) { + const chosen = profiles.values().next().value as AgentProfile; + return { + kind: "selected", + agentId: chosen.id, + name: chosen.name, + reason: "named by the person asking", + fallback: false, + viaMention: true, + }; + } + if (profiles.size > 1) { + return { kind: "ambiguous", names: labelsFor(profiles.values(), labels) }; + } + return null; +} + +function auditReason( + selected: Extract, + undecided: RoutingUndecided | null, +): string { + if (selected.viaMention) return "named by the person asking"; + if (selected.fallback) + return undecided ? `fallback: ${undecided}` : "fallback"; + return "intent match"; +} + +export function createCoworkerRoutingService( + options: CreateCoworkerRoutingServiceOptions, +): HttpCoworkerRoutingService { + async function record( + actor: RoutingActor, + selected: Extract, + candidates: readonly string[], + undecided: RoutingUndecided | null, + ): Promise { + if (!options.auditStore) return; + await recordAuditEvent(options.auditStore, { + eventType: "channel.routed", + targetType: "agent", + targetId: selected.agentId, + ...(actorId(actor) ? { actorUserId: actorId(actor) } : {}), + payload: { + chosen: selected.agentId, + reason: auditReason(selected, undecided), + fallback: selected.fallback, + viaMention: selected.viaMention, + candidates, + undecided, + }, + }); + } + + async function routeDetailed( + input: CoworkerRoutingInput, + ): Promise { + // The store applies this same policy in SQL; keep this canonical policy check at the service + // boundary so a broader store implementation cannot leak a coworker into routing. + const roster = (await options.store.list(input.actor, false)).filter( + (profile) => canAccessAgent(input.actor, profile), + ); + const namedId = input.agentId?.trim() || null; + if (namedId) { + const chosen = roster.find(({ id }) => id === namedId); + if (!chosen) return { result: { kind: "none" }, undecided: null }; + const result: Extract = { + kind: "selected", + agentId: chosen.id, + name: chosen.name, + reason: "named by the person asking", + fallback: false, + viaMention: true, + }; + await record(input.actor, result, [chosen.id], null); + return { result, undecided: null }; + } + + if (roster.length === 0) + return { result: { kind: "none" }, undecided: null }; + + const explicit = explicitNameRoute(input.text, roster); + if (explicit) { + if (explicit.kind === "selected") { + await record(input.actor, explicit, [explicit.agentId], null); + } + return { result: explicit, undecided: null }; + } + + const preferred = + roster.find(({ visibility }) => visibility === "public") ?? roster[0]; + if (!preferred) return { result: { kind: "none" }, undecided: null }; + const candidates: RoutingCandidate[] = await Promise.all( + roster.map(async (profile) => ({ + id: profile.id, + name: profile.name, + roleDescription: profile.roleDescription, + ...(options.reachableSystems + ? { + reaches: await options.reachableSystems(profile.id).catch(() => { + throw new CoworkerReachabilityUnavailableError(); + }), + } + : {}), + })), + ); + const decision = await options.router.route( + input.text, + candidates, + preferred.id, + ); + const result: Extract = { + kind: "selected", + agentId: decision.agentId, + name: decision.name, + reason: decision.reason, + fallback: decision.fallback, + viaMention: false, + }; + await record( + input.actor, + result, + candidates.map(({ id }) => id), + decision.undecided, + ); + return { result, undecided: decision.undecided }; + } + + return { + async route(input) { + return (await routeDetailed(input)).result; + }, + routeDetailed, + }; +} diff --git a/server/tests/agent-resolver.test.ts b/server/tests/agent-resolver.test.ts new file mode 100644 index 00000000..53bfd746 --- /dev/null +++ b/server/tests/agent-resolver.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, test } from "bun:test"; +import { BuiltInAgent } from "@copilotkit/runtime/v2"; +import { createActorAgentResolver } from "../src/agents/agent-resolver"; + +describe("actor-scoped agent resolver", () => { + test("uses the same actor for web maps and individual agent resolution", async () => { + const seenActorIds: string[] = []; + const resolver = createActorAgentResolver({ + loadAgents: async (actor) => { + seenActorIds.push(actor.id); + return [ + { + id: "risk", + name: "Risk Analyst", + type: "built_in" as const, + systemPrompt: "Assess operational risk.", + }, + ]; + }, + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => "openai-secret", + }); + const actor = { id: "u1", role: "user" as const }; + + const visibleAgents = await resolver.resolveAgentsForActor(actor); + const risk = await resolver.resolveAgentForActor(actor, "risk"); + + expect(seenActorIds).toEqual(["u1", "u1"]); + expect(visibleAgents.risk).toBeInstanceOf(BuiltInAgent); + expect(risk).toBeInstanceOf(BuiltInAgent); + }); + + test("rejects an agent absent from the actor's visible map", async () => { + const resolver = createActorAgentResolver({ + loadAgents: async () => [ + { + id: "risk", + name: "Risk Analyst", + type: "built_in" as const, + systemPrompt: "Assess operational risk.", + }, + ], + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => "openai-secret", + }); + + let rejection: unknown; + try { + await resolver.resolveAgentForActor( + { id: "u1", role: "user" }, + "private-risk", + ); + } catch (error) { + rejection = error; + } + + expect(rejection).toBeInstanceOf(Error); + expect((rejection as Error).message).toBe( + "Coworker private-risk is unavailable to this user.", + ); + }); + + test("rejects an agent when the actor has no visible coworkers", async () => { + const resolver = createActorAgentResolver({ + loadAgents: async () => [], + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => "openai-secret", + }); + + expect( + await rejectionMessage(() => + resolver.resolveAgentForActor( + { id: "u1", role: "user" }, + "private-risk", + ), + ), + ).toBe("Coworker private-risk is unavailable to this user."); + }); + + test("rejects inherited object keys as unavailable coworkers", async () => { + const resolver = createActorAgentResolver({ + loadAgents: async () => [ + { + id: "risk", + name: "Risk Analyst", + type: "built_in" as const, + systemPrompt: "Assess operational risk.", + }, + ], + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => "openai-secret", + }); + + for (const agentId of ["constructor", "toString", "__proto__"]) { + expect( + await rejectionMessage(() => + resolver.resolveAgentForActor({ id: "u1", role: "user" }, agentId), + ), + ).toBe(`Coworker ${agentId} is unavailable to this user.`); + } + }); +}); + +async function rejectionMessage(run: () => Promise) { + try { + await run(); + } catch (error) { + if (error instanceof Error) return error.message; + throw error; + } + throw new Error("Expected the run to reject."); +} diff --git a/server/tests/copilot.test.ts b/server/tests/copilot.test.ts index 6d439464..e362c4a8 100644 --- a/server/tests/copilot.test.ts +++ b/server/tests/copilot.test.ts @@ -2,6 +2,7 @@ import { describe, expect, spyOn, test } from "bun:test"; import { HttpAgent } from "@ag-ui/client"; import { BuiltInAgent } from "@copilotkit/runtime/v2"; import { PROVENANCE_GUIDANCE } from "../../shared/bot-prompt"; +import { createActorAgentResolver } from "../src/agents/agent-resolver"; import { buildAgents, builtInAgentConfiguration, @@ -518,12 +519,14 @@ describe("standing agent roles", () => { seen.request = request; return { id: "user-7", role: "user" as const }; }, - async (actor) => { - seen.actors.push(actor); - return [remoteAgent("http://coworker.internal/ag-ui")]; - }, - { provider: "openai", defaultModel: "gpt-5.6-terra" }, - async () => null, + createActorAgentResolver({ + loadAgents: async (actor) => { + seen.actors.push(actor); + return [remoteAgent("http://coworker.internal/ag-ui")]; + }, + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => null, + }), ); const request = new Request("http://openbot.test/api/copilotkit"); @@ -538,11 +541,13 @@ describe("standing agent roles", () => { let roleDescription = "Review receipts."; const factory = createRequestAgents( async () => ({ id: "user-7", role: "user" as const }), - async () => [ - remoteAgent("http://coworker.internal/ag-ui", { roleDescription }), - ], - { provider: "openai", defaultModel: "gpt-5.6-terra" }, - async () => null, + createActorAgentResolver({ + loadAgents: async () => [ + remoteAgent("http://coworker.internal/ag-ui", { roleDescription }), + ], + model: { provider: "openai", defaultModel: "gpt-5.6-terra" }, + resolveModelApiKey: async () => null, + }), ); const request = new Request("http://openbot.test/api/copilotkit"); diff --git a/server/tests/routing-routes.test.ts b/server/tests/routing-routes.test.ts index b974d136..5ac6df66 100644 --- a/server/tests/routing-routes.test.ts +++ b/server/tests/routing-routes.test.ts @@ -6,6 +6,7 @@ import type { AuditStore } from "../src/audit"; import type { AppVariables } from "../src/auth/guards"; import type { IntentRouter, RoutingUndecided } from "../src/routing/classify"; import { createRoutingRoutes } from "../src/routing/routes"; +import { createCoworkerRoutingService } from "../src/routing/service"; /** * Why a conversation went where it went, for every conversation. @@ -32,12 +33,16 @@ const ROSTER = [ name: "Risk Analyst", roleDescription: "regulatory and compliance questions", visibility: "public", + ownerUserId: null, + deletedAt: null, }, { id: "knowledge", name: "Knowledge", roleDescription: "company knowledge", visibility: "public", + ownerUserId: null, + deletedAt: null, }, ]; @@ -87,7 +92,10 @@ function app(options: { routed?: string; undecided?: RoutingUndecided } = {}) { const server = new Hono<{ Variables: AppVariables }>(); server.route( "/api/route", - createRoutingRoutes(store, router, asActor, auditStore), + createRoutingRoutes( + createCoworkerRoutingService({ store, router, auditStore }), + asActor, + ), ); return { server, written, asked }; } @@ -113,9 +121,10 @@ describe("recording which coworker a message went to", () => { }); expect(response.status).toBe(200); - expect(await response.json()).toMatchObject({ + expect(await response.json()).toEqual({ agentId: "risk-analyst", name: "Risk Analyst", + reason: "named by the person asking", viaMention: true, fallback: false, }); diff --git a/server/tests/routing-service.test.ts b/server/tests/routing-service.test.ts new file mode 100644 index 00000000..4b039b50 --- /dev/null +++ b/server/tests/routing-service.test.ts @@ -0,0 +1,576 @@ +import { describe, expect, test } from "bun:test"; +import type { AgentProfileStore } from "../src/agents/profile-store"; +import type { AgentProfile } from "../src/agents/profile-types"; +import type { AuditStore } from "../src/audit"; +import type { + IntentRouter, + RoutingCandidate, + RoutingUndecided, +} from "../src/routing/classify"; +import { + createCoworkerRoutingService, + normalizeCoworkerName, +} from "../src/routing/service"; + +const ACTOR = { id: "u1", role: "user" } as const; + +function profile( + id: string, + name: string, + visibility: "public" | "private" = "public", + ownerUserId: string | null = null, +): AgentProfile { + return { + id, + name, + title: name, + roleDescription: `${name} work`, + avatarSeed: id, + visibility, + ownerUserId, + systemOwned: false, + hidden: false, + deletedAt: null, + endpoint: null, + hasAuth: false, + hasCallbackToken: false, + }; +} + +function makeService( + options: { + roster?: AgentProfile[]; + decision?: { + agentId: string; + reason: string; + fallback: boolean; + undecided: RoutingUndecided | null; + }; + reachableSystems?: (agentId: string) => Promise; + } = {}, +) { + const roster = options.roster ?? [ + profile("risk", "Risk Analyst"), + profile("knowledge", "Knowledge"), + ]; + const modelCalls: Array<{ + text: string; + candidates: readonly RoutingCandidate[]; + defaultId: string; + }> = []; + const audits: Array<{ + payload: Record; + targetId: string | null; + }> = []; + const store = { list: async () => roster } as unknown as AgentProfileStore; + const router = { + route: async ( + text: string, + candidates: readonly RoutingCandidate[], + defaultId: string, + ) => { + modelCalls.push({ text, candidates, defaultId }); + const selected = options.decision ?? { + agentId: "knowledge", + reason: "matches what it is for", + fallback: false, + undecided: null, + }; + const candidate = candidates.find(({ id }) => id === selected.agentId); + return { ...selected, name: candidate?.name ?? selected.agentId }; + }, + } as unknown as IntentRouter; + const auditStore = { + insert: async (event: { + payload: Record; + targetId: string | null; + }) => { + audits.push(event); + }, + } as unknown as AuditStore; + + return { + service: createCoworkerRoutingService({ + store, + router, + auditStore, + reachableSystems: options.reachableSystems, + }), + modelCalls, + audits, + }; +} + +describe("CoworkerRoutingService", () => { + test("routes a unique explicit coworker name without invoking the model", async () => { + const { service, modelCalls } = makeService(); + + const result = await service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }); + + expect(result).toMatchObject({ + kind: "selected", + agentId: "risk", + viaMention: true, + }); + expect(modelCalls).toEqual([]); + }); + + test("normalizes explicit names with NFKC, case, and whitespace", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("risk", "Risk Analyst")], + }); + + expect( + await service.route({ actor: ACTOR, text: "Ask risk\tanalyst please" }), + ).toMatchObject({ + kind: "selected", + agentId: "risk", + viaMention: true, + }); + expect(modelCalls).toEqual([]); + }); + + test("does not match a coworker name inside a larger word", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("risk", "Risk")], + }); + + await service.route({ actor: ACTOR, text: "de-risking the portfolio" }); + + expect(modelCalls).toHaveLength(1); + }); + + test("uses Unicode token boundaries instead of ASCII word boundaries", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("risk", "Risk")], + }); + + await service.route({ actor: ACTOR, text: "Risk\u{10400} review" }); + + expect(modelCalls).toHaveLength(1); + }); + + test("returns visible choices for an ambiguous explicit name", async () => { + const { service, modelCalls } = makeService({ + roster: [ + profile("risk", "Risk Analyst"), + profile("data", "Data Analyst"), + ], + }); + + expect( + await service.route({ actor: ACTOR, text: "ask analyst to review this" }), + ).toEqual({ + kind: "ambiguous", + names: ["Data Analyst", "Risk Analyst"], + }); + expect(modelCalls).toEqual([]); + }); + + test("treats a nested full name as ambiguous when its alias belongs to another coworker", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("analyst", "Analyst"), profile("risk", "Risk Analyst")], + }); + + expect( + await service.route({ actor: ACTOR, text: "ask analyst to review this" }), + ).toEqual({ + kind: "ambiguous", + names: ["Analyst", "Risk Analyst"], + }); + expect(modelCalls).toEqual([]); + }); + + test("prefers a unique longer explicit alias over a shared suffix", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("analyst", "Analyst"), profile("risk", "Risk Analyst")], + }); + + expect( + await service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }), + ).toMatchObject({ + kind: "selected", + agentId: "risk", + viaMention: true, + }); + expect(modelCalls).toEqual([]); + }); + + test("returns choices when two independent explicit names appear in long-to-short order", async () => { + const { service, modelCalls } = makeService(); + + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst and Knowledge to review this", + }), + ).toEqual({ + kind: "ambiguous", + names: ["Knowledge", "Risk Analyst"], + }); + expect(modelCalls).toEqual([]); + }); + + test("returns choices when two independent explicit names appear in short-to-long order", async () => { + const { service, modelCalls } = makeService(); + + expect( + await service.route({ + actor: ACTOR, + text: "ask Knowledge and Risk Analyst to review this", + }), + ).toEqual({ + kind: "ambiguous", + names: ["Knowledge", "Risk Analyst"], + }); + expect(modelCalls).toEqual([]); + }); + + test("selects a profile when all explicit mentions refer to that same profile", async () => { + const { service, modelCalls } = makeService(); + + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst and Risk Analyst to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "risk", viaMention: true }); + expect(modelCalls).toEqual([]); + }); + + test("keeps partially overlapping full names as independent explicit choices", async () => { + const { service, modelCalls } = makeService({ + roster: [profile("ann", "Ann Marie"), profile("curie", "Marie Curie")], + }); + + expect( + await service.route({ actor: ACTOR, text: "ask Ann Marie Curie" }), + ).toEqual({ + kind: "ambiguous", + names: ["Ann Marie", "Marie Curie"], + }); + expect(modelCalls).toEqual([]); + }); + + test("suppresses contained prefix aliases but retains a partially overlapping suffix name", async () => { + const { service, modelCalls } = makeService({ + roster: [ + profile("ann", "Ann"), + profile("ann-marie", "Ann Marie"), + profile("curie", "Marie Curie"), + ], + }); + + expect( + await service.route({ actor: ACTOR, text: "ask Ann Marie Curie" }), + ).toEqual({ + kind: "ambiguous", + names: ["Ann Marie", "Marie Curie"], + }); + expect(modelCalls).toEqual([]); + }); + + test("handles many repeated explicit mentions without changing their selection", async () => { + const { service, modelCalls } = makeService(); + const text = Array.from({ length: 1_500 }, () => "Risk Analyst").join( + " and ", + ); + + expect(await service.route({ actor: ACTOR, text })).toMatchObject({ + kind: "selected", + agentId: "risk", + viaMention: true, + }); + expect(modelCalls).toEqual([]); + }); + + test("labels duplicate normalized names distinctly and resolves a chosen label", async () => { + const { service, modelCalls } = makeService({ + roster: [ + profile("risk-id", "Risk Analyst"), + profile("risk-copy", "Risk Analyst"), + ], + }); + + expect( + await service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }), + ).toEqual({ + kind: "ambiguous", + names: [ + "Risk Analyst (id 7269736b2d636f7079)", + "Risk Analyst (id 7269736b2d6964)", + ], + }); + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id 7269736b2d636f7079) to review this", + }), + ).toMatchObject({ + kind: "selected", + agentId: "risk-copy", + viaMention: true, + }); + expect(modelCalls).toEqual([]); + }); + + test("uses stable id labels when duplicate ids differ only by case", async () => { + const roster = [ + profile("risk", "Risk Analyst"), + profile("Risk", "Risk Analyst"), + ]; + const { service, modelCalls } = makeService({ roster }); + + const result = await service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }); + expect(result).toEqual({ + kind: "ambiguous", + names: ["Risk Analyst (id 5269736b)", "Risk Analyst (id 7269736b)"], + }); + expect(new Set(result.names.map(normalizeCoworkerName)).size).toBe( + result.names.length, + ); + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id 5269736b) to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "Risk" }); + expect(modelCalls).toEqual([]); + }); + + test("uses NFKC-distinct id labels with an order-independent mapping", async () => { + const optionA = profile("A", "Risk Analyst"); + const optionFullWidthA = profile("A", "Risk Analyst"); + const forward = makeService({ roster: [optionFullWidthA, optionA] }); + const reverse = makeService({ roster: [optionA, optionFullWidthA] }); + + const forwardResult = await forward.service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }); + const reverseResult = await reverse.service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }); + + expect(forwardResult).toEqual({ + kind: "ambiguous", + names: ["Risk Analyst (id 41)", "Risk Analyst (id efbca1)"], + }); + expect(reverseResult).toEqual(forwardResult); + expect( + await forward.service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id 41) to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "A" }); + expect( + await forward.service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id efbca1) to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "A" }); + }); + + test("keeps a duplicate label bound to its id when new duplicates are added or reordered", async () => { + const idA = profile("a", "Risk Analyst"); + const idB = profile("b", "Risk Analyst"); + const addedEarlier = profile("A", "Risk Analyst"); + const original = makeService({ roster: [idB, idA] }); + const expanded = makeService({ roster: [idB, addedEarlier, idA] }); + const stableLabel = "Risk Analyst (id 62)"; + + expect( + await original.service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }), + ).toEqual({ + kind: "ambiguous", + names: ["Risk Analyst (id 61)", stableLabel], + }); + expect( + await expanded.service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }), + ).toEqual({ + kind: "ambiguous", + names: ["Risk Analyst (id 41)", "Risk Analyst (id 61)", stableLabel], + }); + expect( + await expanded.service.route({ + actor: ACTOR, + text: `ask ${stableLabel}`, + }), + ).toMatchObject({ kind: "selected", agentId: "b" }); + }); + + test("uses normalization-safe labels for base64url case collisions", async () => { + const first = profile("\u0800", "Risk Analyst"); + const second = profile("\u081A", "Risk Analyst"); + const { service } = makeService({ roster: [second, first] }); + + const result = await service.route({ + actor: ACTOR, + text: "ask risk analyst to review this", + }); + + expect(result).toEqual({ + kind: "ambiguous", + names: ["Risk Analyst (id e0a080)", "Risk Analyst (id e0a09a)"], + }); + expect(new Set(result.names.map(normalizeCoworkerName)).size).toBe( + result.names.length, + ); + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id e0a080) to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "\u0800" }); + expect( + await service.route({ + actor: ACTOR, + text: "ask Risk Analyst (id e0a09a) to review this", + }), + ).toMatchObject({ kind: "selected", agentId: "\u081A" }); + }); + + test("returns none for an absent or empty visible roster", async () => { + const { service } = makeService({ roster: [] }); + + expect(await service.route({ actor: ACTOR, text: "anything" })).toEqual({ + kind: "none", + }); + }); + + test("returns none when an explicit composer id is inaccessible", async () => { + const { service, modelCalls } = makeService({ + roster: [ + profile("risk", "Risk Analyst"), + profile("private", "Private Analyst", "private", "u2"), + ], + }); + + expect( + await service.route({ + actor: ACTOR, + text: "anything", + agentId: "private", + }), + ).toEqual({ kind: "none" }); + expect(modelCalls).toEqual([]); + }); + + test("falls back to intent routing when no explicit name appears", async () => { + const { service, modelCalls } = makeService(); + + expect( + await service.route({ actor: ACTOR, text: "what is our PTO policy" }), + ).toMatchObject({ + kind: "selected", + agentId: "knowledge", + viaMention: false, + }); + expect(modelCalls).toHaveLength(1); + }); + + test("fails safely when coworker reachability cannot be loaded", async () => { + const { service, modelCalls } = makeService({ + reachableSystems: async () => { + throw new Error("private reachability detail"); + }, + }); + + await expect( + service.route({ actor: ACTOR, text: "what is in Drive?" }), + ).rejects.toThrow("Coworker reachability is temporarily unavailable"); + expect(modelCalls).toEqual([]); + }); + + test("passes only the actor-visible roster to intent routing", async () => { + const visible = profile("mine", "My Private", "private", ACTOR.id); + const inaccessible = profile("other", "Other Private", "private", "u2"); + const deleted = { + ...profile("deleted", "Deleted", "public"), + deletedAt: new Date(), + }; + const { service, modelCalls } = makeService({ + roster: [profile("public", "Public"), visible, inaccessible, deleted], + }); + + await service.route({ actor: ACTOR, text: "anything" }); + + expect(modelCalls[0]?.candidates.map(({ id }) => id)).toEqual([ + "public", + "mine", + ]); + }); + + test("writes selected audit fields exactly once without message text", async () => { + const { service, audits } = makeService(); + + await service.route({ actor: ACTOR, text: "private payroll details" }); + + expect(audits).toHaveLength(1); + expect(audits[0]).toMatchObject({ targetId: "knowledge" }); + expect(audits[0]?.payload).toEqual({ + chosen: "knowledge", + reason: "intent match", + fallback: false, + viaMention: false, + candidates: ["risk", "knowledge"], + undecided: null, + }); + expect(JSON.stringify(audits[0])).not.toContain("private payroll details"); + }); + + test("preserves a model fallback audit cause", async () => { + const { service, audits } = makeService({ + decision: { + agentId: "knowledge", + reason: "sent to your default while the router was unreachable", + fallback: true, + undecided: "unreachable", + }, + }); + + await service.route({ actor: ACTOR, text: "anything" }); + + expect(audits[0]?.payload).toMatchObject({ + fallback: true, + undecided: "unreachable", + }); + }); + + test("does not persist a model reason that echoes the message", async () => { + const text = "private payroll details for Sam"; + const { service, audits } = makeService({ + decision: { + agentId: "knowledge", + reason: text, + fallback: false, + undecided: null, + }, + }); + + const result = await service.route({ actor: ACTOR, text }); + + expect(result).toMatchObject({ kind: "selected", reason: text }); + expect(audits[0]?.payload.reason).toBe("intent match"); + expect(JSON.stringify(audits[0])).not.toContain(text); + }); +});