diff --git a/app/src/lib/channels/route.ts b/app/src/lib/channels/route.ts index 5c3fc183..e224a118 100644 --- a/app/src/lib/channels/route.ts +++ b/app/src/lib/channels/route.ts @@ -1,25 +1,33 @@ import { client } from "@/lib/client"; /** - * Which coworker an untagged message should go to. + * Which coworker a message should go to. * - * Called only when the composer draft names no one with `@`. The server reads the roster for the - * person asking and picks by what each coworker is for, so this can only ever return a coworker they - * are already allowed to reach. `fallback` is true when it is the default rather than an inferred - * match, which the caller can say out loud. A thrown error here is not fatal: the caller falls back - * to the default coworker, which is exactly what the server does too. + * The server reads the roster for the person asking and picks by what each coworker is for, so this + * can only ever return a coworker they are already allowed to reach. `fallback` is true when it is + * the default rather than an inferred match, which the caller can say out loud. A thrown error here + * is not fatal: the caller falls back to the default coworker, which is exactly what the server + * does too. + * + * Pass `agentId` when the draft named somebody with `@`. Nothing is inferred in that case and no + * model is called; the call exists so the choice reaches the audit trail, which otherwise had a row + * for every routed conversation and none at all for chosen ones. */ export type RoutingDecision = { agentId: string; name: string; reason: string; fallback: boolean; + viaMention: boolean; }; -export async function routeMessage(text: string): Promise { +export async function routeMessage( + text: string, + agentId?: string, +): Promise { const response = await client("/api/route", { method: "POST", - body: { text }, + body: agentId ? { text, agentId } : { text }, fallback: "Could not choose a coworker.", }); return (await response.json()) as RoutingDecision; diff --git a/app/src/routes/_authed/_app/index.tsx b/app/src/routes/_authed/_app/index.tsx index 2bec34ca..ec18a431 100644 --- a/app/src/routes/_authed/_app/index.tsx +++ b/app/src/routes/_authed/_app/index.tsx @@ -44,7 +44,14 @@ function RouteComponent() { setError(null); try { let agentId: string | undefined = draft.agentId ?? undefined; - if (!agentId) { + if (agentId) { + /* + * Told to the server so the choice is recorded, and its answer thrown away: the + * person already decided and nothing here may change that. Failing to write the + * audit row must not stop the conversation, so a rejection is swallowed whole. + */ + await routeMessage(draft.text, agentId).catch(() => undefined); + } else { try { agentId = (await routeMessage(draft.text)).agentId; } catch { diff --git a/app/src/routes/_authed/admin/audit.tsx b/app/src/routes/_authed/admin/audit.tsx index 4bf3dd1c..ab0f0027 100644 --- a/app/src/routes/_authed/admin/audit.tsx +++ b/app/src/routes/_authed/admin/audit.tsx @@ -155,6 +155,22 @@ function Row({ */ event.eventType === "mcp.callback_refused"; const stalled = event.eventType === "agent.stream_stalled"; + /* + * Three different things, and the difference is what somebody comes to this row to find out. + * + * A person naming a coworker, the router matching one, and the router giving up and using the + * default are not the same event, and one label covering all three would make the row worth less + * than the reason line under it. Nothing here is a refusal, so none of them take the refusal + * colour. + */ + const routed = + event.eventType === "channel.routed" + ? payload.viaMention === true + ? "The person chose this coworker" + : payload.fallback === true + ? "Sent to the default coworker" + : "Sent to the coworker it is for" + : null; // Allowed by policy but not carried out. A stalled turn belongs in the same family: the Bot was // asked and the answer never arrived. Colour is how this table is read, and a row left in the // muted foreground reads as "Allowed", which a turn nobody ever got an answer to was not. @@ -173,8 +189,17 @@ function Row({ : event.eventType} - {/* Named targets and file paths are the audit subject before page elements. */} - {NAMED_TARGETS.has(event.targetType) && event.targetId ? ( + {/* + * A routing row's subject is the coworker it went to, and it is the only thing on the row + * worth reading. Its target type is `agent`, which is not a named target because everywhere + * else an agent id appears it belongs in the Bot column; here nothing acted, so there is no + * Bot and the target is all there is. Rendered through `nameFor` so it reads as the name on + * the roster rather than the immutable id. + */} + {event.eventType === "channel.routed" && event.targetId ? ( + {nameFor(event.targetId)} + ) : /* Named targets and file paths are the audit subject before page elements. */ + NAMED_TARGETS.has(event.targetType) && event.targetId ? ( {event.targetId} {typeof payload.function === "string" ? ( @@ -229,7 +254,8 @@ function Row({ : "text-muted-foreground" } > - {DECISIONS[event.eventType] ?? + {routed ?? + DECISIONS[event.eventType] ?? (refused ? "Blocked" : failed ? "Did not happen" : "Allowed")} {/* Refusal reasons mirror the conversation-facing reason. */} @@ -247,6 +273,24 @@ function Row({ {payload.refusal} ) : null} + {/* + * Why the conversation went where it went, which is the whole reason the row is written. + * Without it a routing row says "Allowed" and names nobody, which is indistinguishable from + * a row that failed to write. + */} + {event.eventType === "channel.routed" && + typeof payload.reason === "string" ? ( + /* + * A width rather than a max-width, because the table lays itself out from its content and + * a max-width on a block inside a cell does not constrain that. A router's reason is a + * sentence a model wrote, not a rule name, and left unbounded in the last column it + * pushes the table wider than the page and the end of the sentence goes off the edge, + * where nobody scrolls to find it. + */ +
+ {payload.reason} +
+ ) : null} {event.eventType === "bot.declined" && typeof payload.reason === "string" ? (
diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index f951ac45..665ee4fd 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -9,12 +9,35 @@ import type { IntentRouter, RoutingCandidate } from "./classify"; const DEV_ACTOR_EMAIL = "dev@openbot.local"; /** - * Decide which coworker an untagged message is for, before a channel is pinned to one. + * Who to record the routing against, or nobody. * - * The roster is read for the person asking, so the router can only ever pick a coworker they are + * 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. */ export function createRoutingRoutes( store: AgentProfileStore, @@ -32,12 +55,41 @@ export function createRoutingRoutes( ) { 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[], + ): Promise { + if (!auditStore) return; + await recordAuditEvent(auditStore, { + eventType: "channel.routed", + targetType: "agent", + targetId: chosen, + ...(actorUserId ? { actorUserId } : {}), + payload: { chosen, reason, fallback, viaMention, candidates }, + }); + } + routes.post("/", requireUser, async (context) => { const body = (await context.req.json().catch(() => null)) as { text?: unknown; + agentId?: unknown; } | null; const text = typeof body?.text === "string" ? body.text.trim() : ""; if (!text) return context.json({ error: "A message is required." }, 400); + const named = + typeof body?.agentId === "string" && body.agentId.trim() + ? body.agentId.trim() + : null; const actor = context.var.actor; const roster = await store.list(actor, false); @@ -47,6 +99,33 @@ export function createRoutingRoutes( if (!preferred) { return context.json({ error: "No coworker is available." }, 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, + ); + } + const reason = "you chose them yourself"; + await record(actorId(actor), chosen.id, reason, false, true, [chosen.id]); + 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, @@ -69,29 +148,21 @@ export function createRoutingRoutes( const decision = await router.route(text, candidates, preferred.id); - if (auditStore) { - await recordAuditEvent(auditStore, { - eventType: "channel.routed", - targetType: "agent", - targetId: decision.agentId, - ...(actor?.id && actor.email !== DEV_ACTOR_EMAIL - ? { actorUserId: actor.id } - : {}), - payload: { - chosen: decision.agentId, - reason: decision.reason, - fallback: decision.fallback, - viaMention: false, - candidates: candidates.map((c) => c.id), - }, - }); - } + await record( + actorId(actor), + decision.agentId, + decision.reason, + decision.fallback, + false, + candidates.map((c) => c.id), + ); return context.json({ agentId: decision.agentId, name: decision.name, reason: decision.reason, fallback: decision.fallback, + viaMention: false, }); }); diff --git a/server/tests/routing-routes.test.ts b/server/tests/routing-routes.test.ts new file mode 100644 index 00000000..d59ee589 --- /dev/null +++ b/server/tests/routing-routes.test.ts @@ -0,0 +1,185 @@ +import { describe, expect, test } from "bun:test"; +import type { MiddlewareHandler } from "hono"; +import { Hono } from "hono"; +import type { AgentProfileStore } from "../src/agents/profile-store"; +import type { AuditStore } from "../src/audit"; +import type { AppVariables } from "../src/auth/guards"; +import type { IntentRouter } from "../src/routing/classify"; +import { createRoutingRoutes } from "../src/routing/routes"; + +/** + * Why a conversation went where it went, for every conversation. + * + * `channel.routed` carries a `viaMention` field that was hardcoded false at the only place the row + * was written, so it could never be true. The reason was structural rather than a typo: naming a + * coworker with `@` short-circuited the router in the composer, so that code never ran and no row + * was written at all. + * + * Four routings in a validation pass, every one `viaMention=false`, and a conversation sent to Risk + * Analyst through the picker that produced tool calls recorded against `bot=risk-analyst` with no + * `channel.routed` row anywhere near it. The mention worked; the trail could not say so, and a + * missing row is indistinguishable from one that failed to write. + * + * So the choice is recorded too, and these hold both halves: the field earns its place, and the + * model is never asked a question the person already answered. + */ + +const ACTOR = { id: "u1", email: "person@openbot.test", role: "user" } as const; + +const ROSTER = [ + { + id: "risk-analyst", + name: "Risk Analyst", + roleDescription: "regulatory and compliance questions", + visibility: "public", + }, + { + id: "knowledge", + name: "Knowledge", + roleDescription: "company knowledge", + visibility: "public", + }, +]; + +type Recorded = { + eventType: string; + targetId: string | null; + payload: Record; +}; + +function app(options: { routed?: string } = {}) { + const written: Recorded[] = []; + /** Every call the router was asked to make, so "never asked" is an assertion and not a hope. */ + const asked: string[] = []; + + const asActor: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", { ...ACTOR }); + await next(); + }; + + const store = { + list: async () => ROSTER, + } as unknown as AgentProfileStore; + + const router = { + route: async (text: string) => { + asked.push(text); + const chosen = options.routed ?? "knowledge"; + return { + agentId: chosen, + name: ROSTER.find((a) => a.id === chosen)?.name ?? chosen, + reason: "matches what it is for", + fallback: false, + }; + }, + } as unknown as IntentRouter; + + const auditStore = { + insert: async (event: Recorded) => { + written.push(event); + }, + } as unknown as AuditStore; + + const server = new Hono<{ Variables: AppVariables }>(); + server.route( + "/api/route", + createRoutingRoutes(store, router, asActor, auditStore), + ); + return { server, written, asked }; +} + +async function post( + server: Hono<{ Variables: AppVariables }>, + body: unknown, +): Promise { + return server.request("/api/route", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); +} + +describe("recording which coworker a message went to", () => { + test("a named coworker is recorded as the person's own choice", async () => { + const { server, written } = app(); + + const response = await post(server, { + text: "what is the SAR filing deadline", + agentId: "risk-analyst", + }); + + expect(response.status).toBe(200); + expect(await response.json()).toMatchObject({ + agentId: "risk-analyst", + name: "Risk Analyst", + viaMention: true, + fallback: false, + }); + + expect(written).toHaveLength(1); + expect(written[0]?.eventType).toBe("channel.routed"); + expect(written[0]?.targetId).toBe("risk-analyst"); + expect(written[0]?.payload).toMatchObject({ + chosen: "risk-analyst", + viaMention: true, + fallback: false, + }); + }); + + test("naming a coworker never asks the model", async () => { + // The person already decided. A model call here would be spend and latency for a question with + // an answer, and a chance to disagree with it. + const { server, asked } = app(); + + await post(server, { text: "anything", agentId: "risk-analyst" }); + + expect(asked).toEqual([]); + }); + + test("an inferred choice is still recorded as inferred", async () => { + const { server, written, asked } = app({ routed: "knowledge" }); + + const response = await post(server, { text: "what is our PTO policy" }); + + expect(await response.json()).toMatchObject({ + agentId: "knowledge", + viaMention: false, + }); + expect(asked).toEqual(["what is our PTO policy"]); + expect(written[0]?.payload).toMatchObject({ + chosen: "knowledge", + viaMention: false, + }); + }); + + test("a coworker who is not on the roster is refused, not redirected", async () => { + /* + * The dangerous answer is the quiet one. Turning a name the person typed by hand into somebody + * else, because the first was not reachable, sends their message to a coworker they did not + * choose and says nothing. Refusing is worse for that one request and better for everything. + */ + const { server, written } = app(); + + const response = await post(server, { + text: "hello", + agentId: "not-on-the-roster", + }); + + expect(response.status).toBe(404); + expect(written).toEqual([]); + }); + + test("a blank agentId is a message with no mention, not a broken one", async () => { + // The composer sends the field only when the draft named somebody, but a caller that sends it + // empty means the same thing and must not be answered with a 404. + const { server, asked } = app(); + + const response = await post(server, { text: "hello", agentId: " " }); + + expect(response.status).toBe(200); + expect(asked).toEqual(["hello"]); + }); +});