diff --git a/app/src/lib/channels/route.ts b/app/src/lib/channels/route.ts index 5c3fc183..df60c970 100644 --- a/app/src/lib/channels/route.ts +++ b/app/src/lib/channels/route.ts @@ -1,13 +1,14 @@ import { client } from "@/lib/client"; /** - * Which coworker an untagged message should go to. + * Which coworker a first 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. + * With no `@`, the server reads the roster for the person asking and picks by what each coworker is + * for. With an `@`, pass its `agentId`: the server honours that choice as-is and records it, so the + * audit trail is not silent about mentioned conversations. Either way this can only ever return a + * coworker the person is already allowed to reach. `fallback` is true when the result 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 mentioned coworker, or to the default — exactly what the server does. */ export type RoutingDecision = { agentId: string; @@ -16,10 +17,13 @@ export type RoutingDecision = { fallback: 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..252a5154 100644 --- a/app/src/routes/_authed/_app/index.tsx +++ b/app/src/routes/_authed/_app/index.tsx @@ -38,18 +38,18 @@ function RouteComponent() { disabled={!fallback} onSubmit={async (draft) => { // A channel is pinned to one coworker for the life of its thread, so the coworker is - // chosen now, before it is created. An `@` is an explicit choice and is honoured as-is. - // With no `@`, the message is routed to the coworker it is for; if that routing cannot - // run, it falls back to the same default the composer used to always use. + // chosen now, before it is created. Both cases go through the router so the choice is + // recorded: an `@` is honoured as-is and logged as a mention, and with no `@` the message + // is routed to the coworker it is for. If routing cannot run, it falls back to the + // mentioned coworker when there was one, else the same default the composer always used. setError(null); try { - let agentId: string | undefined = draft.agentId ?? undefined; - if (!agentId) { - try { - agentId = (await routeMessage(draft.text)).agentId; - } catch { - agentId = fallback?.id; - } + const mentioned = draft.agentId ?? undefined; + let agentId: string | undefined; + try { + agentId = (await routeMessage(draft.text, mentioned)).agentId; + } catch { + agentId = mentioned ?? fallback?.id; } if (!agentId) return; await start(agentId, draft.text); diff --git a/server/src/routing/routes.ts b/server/src/routing/routes.ts index c884d4d1..3f93faaf 100644 --- a/server/src/routing/routes.ts +++ b/server/src/routing/routes.ts @@ -9,12 +9,17 @@ 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. + * Decide which coworker a message is for, before a channel is pinned to one. * - * The roster is read for the person asking, so the router can only ever pick 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. + * Two ways a message gets a coworker, and the trail records both so it can tell them apart. When the + * person named one with `@`, the body carries that `agentId`: no model runs, the choice is honoured + * as-is, and the row is written with `viaMention: true` naming the person as the reason. When they + * named no one, the router reads the message against what each coworker is for and picks, and the + * row is written with `viaMention: false` and the model's reason. + * + * The roster is read for the person asking, so neither path can land on a coworker they are not + * already allowed to reach. The 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. */ export function createRoutingRoutes( store: AgentProfileStore, @@ -27,9 +32,14 @@ export function createRoutingRoutes( 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 mentionedId = + typeof body?.agentId === "string" && body.agentId.trim() + ? body.agentId.trim() + : undefined; const actor = context.var.actor; const roster = await store.list(actor, false); @@ -45,26 +55,51 @@ export function createRoutingRoutes( roleDescription: a.roleDescription, })); - const decision = await router.route(text, candidates, preferred.id); - - if (auditStore) { - await recordAuditEvent(auditStore, { + const record = (payload: Record) => { + if (!auditStore) return; + return recordAuditEvent(auditStore, { eventType: "channel.routed", targetType: "agent", - targetId: decision.agentId, + targetId: String(payload.chosen), ...(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), - }, + payload: { ...payload, candidates: candidates.map((c) => c.id) }, + }); + }; + + // An `@` names a coworker: nothing to decide, but the choice still gets a row so the trail is + // not silent about mentioned conversations. Only honoured when the named coworker is one the + // person can reach — the same guarantee the router path carries. + const mentioned = mentionedId + ? candidates.find((c) => c.id === mentionedId) + : undefined; + if (mentioned) { + const who = actor?.name?.trim() || actor?.email || "a person"; + const reason = `named with @ by ${who}`; + await record({ + chosen: mentioned.id, + reason, + fallback: false, + viaMention: true, + }); + return context.json({ + agentId: mentioned.id, + name: mentioned.name, + reason, + fallback: false, }); } + const decision = await router.route(text, candidates, preferred.id); + + await record({ + chosen: decision.agentId, + reason: decision.reason, + fallback: decision.fallback, + viaMention: false, + }); + return context.json({ agentId: decision.agentId, name: decision.name, diff --git a/server/tests/routing-routes.test.ts b/server/tests/routing-routes.test.ts new file mode 100644 index 00000000..d4acb3c1 --- /dev/null +++ b/server/tests/routing-routes.test.ts @@ -0,0 +1,162 @@ +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 { AuditEventInput, AuditStore } from "../src/audit"; +import type { AppVariables } from "../src/auth/guards"; +import type { IntentRouter } from "../src/routing/classify"; +import { createRoutingRoutes } from "../src/routing/routes"; + +/** + * The `/api/route` decision, and the row it leaves behind. + * + * `channel.routed` carried a `viaMention` field hardcoded `false`, because choosing a coworker with + * `@` short-circuited the router and wrote no row at all. So the trail answered "why did this go + * here" for model-chosen conversations and was silent for mentioned ones — indistinguishable from a + * row that failed to write. The mention now goes through here too, and these tests hold the line on + * both shapes: a model choice, and a mention that is honoured as-is and recorded as `viaMention`. + */ + +const ACTOR = { + id: "u1", + email: "member@openbot.test", + name: "Dana Reader", + role: "user", +} as const; + +const ROSTER = [ + { + id: "knowledge", + name: "Knowledge", + roleDescription: "Docs", + visibility: "public", + }, + { + id: "risk-analyst", + name: "Risk Analyst", + roleDescription: "Risk", + visibility: "public", + }, +]; + +function harness(overrides: { route?: IntentRouter["route"] } = {}) { + const rows: AuditEventInput[] = []; + const auditStore: AuditStore = { + insert: async (event) => void rows.push(event), + }; + + const store = { + list: async () => ROSTER, + } as unknown as AgentProfileStore; + + const router = { + route: + overrides.route ?? + (async () => ({ + agentId: "knowledge", + name: "Knowledge", + reason: "matches Knowledge", + fallback: true, + })), + } as unknown as IntentRouter; + + const requireUser: MiddlewareHandler<{ Variables: AppVariables }> = async ( + context, + next, + ) => { + context.set("actor", ACTOR as never); + await next(); + }; + + const routes = createRoutingRoutes(store, router, requireUser, auditStore); + return { rows, hono: new Hono().route("/api/route", routes) }; +} + +const post = (body: unknown) => + new Request("http://test/api/route", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }); + +describe("POST /api/route", () => { + test("a message with no @ is routed by the model and recorded viaMention=false", async () => { + const { rows, hono } = harness(); + + const response = await hono.request(post({ text: "help with the docs" })); + expect(response.status).toBe(200); + expect(await response.json()).toEqual({ + agentId: "knowledge", + name: "Knowledge", + reason: "matches Knowledge", + fallback: true, + }); + + expect(rows).toHaveLength(1); + const row = rows[0]; + expect(row.eventType).toBe("channel.routed"); + expect(row.targetId).toBe("knowledge"); + expect(row.actorUserId).toBe("u1"); + expect(row.payload).toMatchObject({ + chosen: "knowledge", + viaMention: false, + fallback: true, + candidates: ["knowledge", "risk-analyst"], + }); + }); + + test("an @-named coworker is honoured as-is and recorded viaMention=true, no model call", async () => { + let modelCalled = false; + const { rows, hono } = harness({ + route: async () => { + modelCalled = true; + return { + agentId: "knowledge", + name: "Knowledge", + reason: "should not run", + fallback: true, + }; + }, + }); + + const response = await hono.request( + post({ text: "look at this exposure", agentId: "risk-analyst" }), + ); + expect(response.status).toBe(200); + const decision = await response.json(); + expect(decision.agentId).toBe("risk-analyst"); + expect(decision.name).toBe("Risk Analyst"); + expect(decision.fallback).toBe(false); + expect(modelCalled).toBe(false); + + expect(rows).toHaveLength(1); + expect(rows[0].targetId).toBe("risk-analyst"); + expect(rows[0].payload).toMatchObject({ + chosen: "risk-analyst", + viaMention: true, + fallback: false, + candidates: ["knowledge", "risk-analyst"], + }); + // The person is the reason, so the trail can say who named the coworker. + expect(rows[0].payload.reason).toContain("Dana Reader"); + }); + + test("an @ naming a coworker off the roster falls through to model routing, never honoured blind", async () => { + const { rows, hono } = harness(); + + const response = await hono.request( + post({ text: "help", agentId: "not-on-roster" }), + ); + expect(response.status).toBe(200); + // The unreachable id is ignored; the model picks and the row says viaMention=false. + expect((await response.json()).agentId).toBe("knowledge"); + expect(rows[0].payload).toMatchObject({ viaMention: false }); + }); + + test("an empty message is rejected before any routing", async () => { + const { rows, hono } = harness(); + const response = await hono.request(post({ text: " " })); + expect(response.status).toBe(400); + expect(rows).toHaveLength(0); + }); +});