From 6ac1dfdf01940c9777763a46d9a686e2d0764677 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 21:16:11 -0700 Subject: [PATCH] Say why every conversation went where it did `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. The trail therefore answered "why did this go to Risk Analyst" for conversations the router placed and said nothing whatever for ones the person chose by hand, which reads exactly like a row that failed to write. So a mention is recorded too. The endpoint takes the named coworker, checks it against the same roster the router picks from, and records it with `viaMention` true and the person as the reason. No model is called: they already decided, and asking would be spend, latency and a chance to disagree. A name that is not on the roster is refused rather than quietly turned into somebody else, because silently redirecting a message somebody addressed by hand is the worst answer available. The client throws the response away and starts the conversation with the coworker named regardless, so failing to write the row can never cost somebody their message. Both branches now write the row through one function, because two call sites writing the same event is two payloads that drift. The audit page had to change with it or none of this would be visible. A routing row rendered with no subject, no Bot and a decision of "Allowed", which is the same nothing the missing row was. It now names the coworker it went to and separates the three cases that matter: the person chose them, the router matched them, or the router gave up and used the default. The reason sits underneath, at a fixed width so a sentence a model wrote wraps inside the table instead of running off the edge. Closes #133. --- app/src/lib/channels/route.ts | 24 ++-- app/src/routes/_authed/_app/index.tsx | 9 +- app/src/routes/_authed/admin/audit.tsx | 50 ++++++- server/src/routing/routes.ts | 109 ++++++++++++--- server/tests/routing-routes.test.ts | 185 +++++++++++++++++++++++++ 5 files changed, 346 insertions(+), 31 deletions(-) create mode 100644 server/tests/routing-routes.test.ts 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"]); + }); +});