From b118c8ec32d7de11b59d7f526a18f63eee546740 Mon Sep 17 00:00:00 2001 From: hotragn Date: Fri, 21 Aug 2026 17:26:11 -0400 Subject: [PATCH] Answer from the documents the asker may read `connectors/sync-persistence.ts` has been writing `documents`, `chunks` and `document_acls` since the beginning and nothing ever read them back. A deployment that connected a source got rows in PostgreSQL and still no citation, while the Knowledge coworker answered as though something were behind it. This is the read half. `createKnowledgeSearch` matches the question against `chunks.content` and returns one citation per document: title, the canonical URL the connector stored, and the passage that matched. The ACL is evaluated in SQL, not in this process. A read path that fetches rows and filters them here has already fetched them: the wrong document is in memory and one refactor from being returned, and the cost scales with the corpus rather than the answer. A document is readable when some row allows one of the asker's principals and no row denies one, so deny beats allow whatever order the planner reaches them in, and a document with no ACL rows at all is readable by nobody. Absence is the refusal, as it is for every other grant here, so ACLs that failed to sync leave a document invisible rather than public. `group:` principals are matched even though `users.groups` is written by nothing today, so a group ACL starts working the day groups arrive from an identity provider rather than needing to be found and changed. Until then such a row matches nobody, which denies rather than permits. Reached as a server-executed tool beside the MCP ones, so a run needs no browser. It is offered without a per-Bot grant because the query is the access control: the search runs on the asker's own principals, so no Bot can return a document the person asking could not have opened themselves. It is offered only when there is something to search, because a tool in the list is a sentence in the prompt and a step the model may spend. Every call writes `knowledge.searched`, naming the query and the documents returned and never quoting their text. Ranking is PostgreSQL's full-text search, not vectors. `chunks.embedding` is `vector(1536)` and nothing in this repository has ever written one: `connectors/contract.ts` has the adapter supply embeddings, no adapter exists yet, and there is no embedding model in the tenant package or the environment. Full-text search needs no configuration a deployment does not already have. When an adapter starts writing embeddings the ranking changes behind this signature and the ACL predicate does not move. --- CHANGELOG.md | 14 + server/src/index.ts | 38 ++- server/src/knowledge/search.ts | 232 +++++++++++++++ server/src/knowledge/tool.ts | 106 +++++++ .../knowledge-search.integration.test.ts | 265 ++++++++++++++++++ server/tests/knowledge-tool.test.ts | 174 ++++++++++++ 6 files changed, 827 insertions(+), 2 deletions(-) create mode 100644 server/src/knowledge/search.ts create mode 100644 server/src/knowledge/tool.ts create mode 100644 server/tests/knowledge-search.integration.test.ts create mode 100644 server/tests/knowledge-tool.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 33b92b13..a76ead19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,20 @@ Sessions survive and nobody signs in again. ### Added +- **A Bot can answer from a connected source, as the person asking.** The connectors have been + writing `documents`, `chunks` and `document_acls` and nothing ever read them back, so a deployment + that connected a source got rows in PostgreSQL and still no citation. A Bot now has a + `search_company_knowledge` tool, and it returns only the documents the person asking is allowed to + read — filtered in the database against that person's own principals rather than fetched and + filtered in the server, so a document they may not read is never handed over. A deny beats an + allow, and a document with no ACL rows is readable by nobody rather than by everybody. Each result + carries the document's title, the link that opens it, and the passage that matched. A search that + finds nothing says so, rather than returning an empty string a model would fill in from memory. + Every search is on the audit trail as `knowledge.searched`, naming the query and the documents + returned and never quoting their text. The tool is only offered when there is something to search. + Matching is PostgreSQL's own full-text search over the stored passages: nothing in the deployment + produces embeddings yet, so the vector column is left alone and ranking by meaning follows the + first connector that writes one. - **Releases are cut by a workflow, not by hand.** `Create release PR` bumps the version and promotes `## Unreleased` to a numbered section; merging the pull request it opens is what publishes. Merging builds and pushes one image to `ghcr.io/copilotkit/openbot`, signs a build provenance attestation diff --git a/server/src/index.ts b/server/src/index.ts index 4cdac0bf..462334c9 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -44,6 +44,8 @@ import { resolveModelApiKey, } from "./credentials"; import { createDatabase } from "./db/client"; +import { askerFor, createKnowledgeSearch } from "./knowledge/search"; +import { knowledgeSearchTool } from "./knowledge/tool"; import { createPeopleStore } from "./people/store"; import { createPluginStore } from "./plugins/store"; import { grantedTools } from "./plugins/tools"; @@ -264,6 +266,15 @@ const pluginStore = createPluginStore({ policy: () => policyStore.get(), }); +/** + * Reading back what the connectors wrote. + * + * `connectors/sync-persistence.ts` has been filling `documents`, `chunks` and `document_acls`, and + * nothing has ever read them. This is the read half, and it filters on the asker's own principals in + * SQL rather than here. See server/src/knowledge/search.ts. + */ +const knowledgeSearch = createKnowledgeSearch(database); + void recordAuditEvent(bootAuditStore, { eventType: "computer.policy_loaded", targetType: "policy", @@ -388,8 +399,31 @@ const app = createApp( stallGuard, // Tools run here, not in the browser. Each one still executes through the plugin store, so the // grant, the policy and the audit row are exactly where they were. - (actorId) => (botId) => - grantedTools({ store: pluginStore, botId, actorId }), + // + // The knowledge search is beside them rather than inside the plugin store, because it has no + // vendor to reach: it is a query against this deployment's own tables, and it writes its own + // `knowledge.searched` row. It is offered without a per-Bot grant because the ACL filter in the + // query is the access control — the search runs on the asker's principals, so no Bot can return a + // document the person asking could not open themselves. Offered only when there is something to + // search, so a deployment that has connected nothing does not describe a tool that can only + // answer "nothing found". + (actorId) => async (botId) => { + const granted = await grantedTools({ + store: pluginStore, + botId, + actorId, + }); + if (!(await knowledgeSearch.anyDocuments())) return granted; + return [ + ...granted, + knowledgeSearchTool({ + search: knowledgeSearch, + auditStore: bootAuditStore, + asker: await askerFor(database, actorId), + botId, + }), + ]; + }, /* * What the deployment tells a remote Bot about the run it is starting. * diff --git a/server/src/knowledge/search.ts b/server/src/knowledge/search.ts new file mode 100644 index 00000000..c0496228 --- /dev/null +++ b/server/src/knowledge/search.ts @@ -0,0 +1,232 @@ +import { eq, isNull, sql } from "drizzle-orm"; +import type { Database } from "../db/client"; +import { documents, users } from "../db/schema"; + +/** + * Answering from the company's documents, with only the ones the asker may read. + * + * WHY THIS EXISTS. `connectors/sync-persistence.ts` writes `documents`, `chunks` and `document_acls` + * and nothing has ever read them back. The Knowledge coworker answers as though something were + * behind it, so a deployment that connects a source gets rows in Postgres and no citation. + * + * THE ACL IS THE POINT, and it is evaluated in SQL rather than in this process. A read path that + * fetches rows and filters them here has already fetched them: the wrong document is in memory, it is + * one refactor from being returned, and the query cost scales with the corpus rather than with the + * answer. The predicate below is the boundary, so the database never hands over a row the asker may + * not read. + * + * DENY WINS, AND SILENCE DENIES. A document is readable when some ACL row allows one of the asker's + * principals and no ACL row denies one. A document with no ACL rows at all is readable by nobody, + * which is the same shape the rest of this repo uses for a grant: absence is the refusal, so a + * document whose ACLs failed to sync is invisible rather than public. + * + * WHAT THIS DOES NOT DO is rank by meaning. `chunks.embedding` is `vector(1536)` and nothing in this + * repository has ever written one: `connectors/contract.ts` has the adapter supply embeddings and no + * adapter exists yet, and there is no embedding model in the tenant package or the environment. So + * matching here is PostgreSQL's own full-text search over `chunks.content`, which needs no + * configuration that a deployment does not already have. When an adapter starts writing embeddings, + * the ranking changes behind this signature and the ACL predicate does not move. + */ + +/** + * Who is asking, as principals rather than as a user row. + * + * `groups` is accepted and matched, and is empty in every deployment today: `users.groups` is written + * by nothing (see #82 and #92, which established that both halves of the group control are missing). + * Matching it anyway means a `group:` ACL starts working the day groups arrive from an identity + * provider instead of needing to be found and changed. Until then such a row matches nobody, which + * denies rather than permits. + */ +export type KnowledgeAsker = { + userId: string; + groups: readonly string[]; +}; + +export type KnowledgeCitation = { + documentId: string; + title: string; + /** Where a person opens the document. Stored by the connector, never built here. */ + url: string; + /** The matching passage, marked up by PostgreSQL around the terms that matched. */ + snippet: string; +}; + +export type KnowledgeSearch = { + search: (input: { + asker: KnowledgeAsker; + query: string; + limit?: number; + }) => Promise; + /** + * Whether there is anything to search at all. + * + * Asked so a deployment that has connected nothing does not offer its Bots a tool that can only + * answer "nothing found". A tool in the list is a sentence in the prompt and a call the model may + * spend a step on. + * + * Asked per run rather than at boot, for the reason plugins/tools.ts gives about grants: a source + * connected this morning should work this afternoon, not after a restart. + */ + anyDocuments: () => Promise; +}; + +/** Enough to answer from, few enough to fit a reply. */ +const DEFAULT_LIMIT = 5; +const MAX_LIMIT = 20; + +/** + * The text search configuration. + * + * Named rather than left to `default_text_search_config`, which is a server setting a deployment may + * have changed: the same query would then stem differently on two databases and neither would say so. + * + * Cast to `regconfig` at every use below. Passed as a bound parameter it arrives typed as text, and + * there is no `to_tsvector(text, text)` for PostgreSQL to resolve to — only the one-argument form and + * `to_tsvector(regconfig, text)` — so without the cast the query fails to plan rather than falling + * back to anything. + */ +const TEXT_CONFIG = "english"; + +/** + * How the asker's identity becomes the strings an ACL row is written against. + * + * One shape, in one place, because a mismatch between what is written and what is matched is a + * silent read failure rather than an error. + */ +export function principalsFor(asker: KnowledgeAsker): string[] { + return [ + `user:${asker.userId}`, + ...asker.groups.map((group) => `group:${group}`), + ]; +} + +function boundedLimit(requested: number | undefined): number { + if (requested === undefined || !Number.isFinite(requested)) { + return DEFAULT_LIMIT; + } + return Math.min(MAX_LIMIT, Math.max(1, Math.trunc(requested))); +} + +/** + * The asker, with the groups the deployment holds for them. + * + * A separate read rather than something carried on the request, because the actor a run is for is + * identified by id and nothing downstream of that has ever needed more. `users.groups` is `[]` for + * everybody today; see the note on {@link KnowledgeAsker}. + */ +export async function askerFor( + database: Database, + userId: string, +): Promise { + const [row] = await database + .select({ groups: users.groups }) + .from(users) + .where(eq(users.id, userId)) + .limit(1); + return { userId, groups: row?.groups ?? [] }; +} + +export function createKnowledgeSearch(database: Database): KnowledgeSearch { + return { + async anyDocuments() { + const [row] = await database + .select({ id: documents.id }) + .from(documents) + .where(isNull(documents.deletedAt)) + .limit(1); + return row !== undefined; + }, + + async search({ asker, query, limit }) { + const terms = query.trim(); + /* + * Answered here rather than by the database. `websearch_to_tsquery` turns an empty string into + * an empty query, which matches nothing, so the result would be the same — but a Bot calling + * this with no arguments should not become a query against every chunk in the deployment. + */ + if (terms === "") return []; + + /* + * One bound parameter per principal. + * + * Handing the driver a single JSON array and casting it to `jsonb` read better and was wrong: + * the value arrives already encoded, so `$1::jsonb` was a JSON *string* rather than an array and + * `jsonb_array_elements_text` refused it with "cannot extract elements from a scalar". A list of + * parameters has no encoding to get wrong, and the list is never empty because + * {@link principalsFor} always yields the asker's own `user:` principal. + */ + const principals = sql.join( + principalsFor(asker).map((principal) => sql`${principal}`), + sql`, `, + ); + + /* + * One row per document, not one per chunk. A long document matching in six places is one + * citation, and returning it six times would spend the answer's whole budget on it. + * `row_number` picks the best-ranked chunk per document, tie-broken by position so the same + * corpus and the same question give the same passage twice running. + */ + const rows = await database.execute<{ + document_id: string; + title: string; + canonical_url: string; + snippet: string; + }>( + sql` + with asked as ( + select websearch_to_tsquery(${TEXT_CONFIG}::regconfig, ${terms}) as tsq + ), + matched as ( + select d.id as document_id, + d.title as title, + d.canonical_url as canonical_url, + ts_rank(to_tsvector(${TEXT_CONFIG}::regconfig, c.content), asked.tsq) as rank, + ts_headline( + ${TEXT_CONFIG}::regconfig, + c.content, + asked.tsq, + 'MaxFragments=1, MaxWords=40, MinWords=12' + ) as snippet, + row_number() over ( + partition by d.id + order by ts_rank( + to_tsvector(${TEXT_CONFIG}::regconfig, c.content), + asked.tsq + ) desc, + c.position asc + ) as best + from chunks c + join documents d on d.id = c.document_id + cross join asked + where d.deleted_at is null + and to_tsvector(${TEXT_CONFIG}::regconfig, c.content) @@ asked.tsq + and exists ( + select 1 from document_acls a + where a.document_id = d.id + and a.effect = 'allow' + and a.principal in (${principals}) + ) + and not exists ( + select 1 from document_acls a + where a.document_id = d.id + and a.effect = 'deny' + and a.principal in (${principals}) + ) + ) + select document_id, title, canonical_url, snippet + from matched + where best = 1 + order by rank desc, title asc + limit ${boundedLimit(limit)} + `, + ); + + return [...rows].map((row) => ({ + documentId: row.document_id, + title: row.title, + url: row.canonical_url, + snippet: row.snippet, + })); + }, + }; +} diff --git a/server/src/knowledge/tool.ts b/server/src/knowledge/tool.ts new file mode 100644 index 00000000..474d8a1c --- /dev/null +++ b/server/src/knowledge/tool.ts @@ -0,0 +1,106 @@ +import { z } from "zod"; +import { type AuditStore, recordAuditEvent } from "../audit"; +import type { GrantedTool } from "../plugins/tools"; +import type { KnowledgeAsker, KnowledgeSearch } from "./search"; + +/** + * The company's own documents, as something a Bot can call. + * + * WHY IT IS A TOOL rather than retrieval stapled to the prompt. A Bot that is handed passages before + * it speaks is answering from whatever the retriever guessed the question was. A Bot that calls this + * asks its own question, and the call is a thing the trail can record, the model can be refused, and + * a person can read back. It reaches the model the same way an MCP tool does — through + * `builtInAgentConfiguration`'s `tools`, executed on the server, never registered by a browser. + * + * WHAT MAKES IT SAFE IS THE QUERY, not this file. Every row is filtered against the asker's own + * principals in SQL before it leaves PostgreSQL (see search.ts). So this tool cannot return a + * document the person asking could not have opened themselves, whichever Bot is holding it. That is + * the reason it is offered without a per-Bot grant of its own: the grant would refine who may ask, + * and the ACL already decides what any answer may contain. + * + * SAYING NOTHING IS AN ANSWER. An empty result returns a sentence saying so rather than an empty + * string, because a model handed nothing fills the gap from training and cites a document that does + * not exist. The Knowledge coworker's own prompt already tells it to say when nothing is connected; + * this is the same instruction arriving as data. + */ + +/** Named for the model reading it. A Bot picks a tool by what the name and description promise. */ +export const KNOWLEDGE_TOOL_NAME = "search_company_knowledge"; + +const NOTHING_FOUND = + "No authorized document matched that. Say so plainly rather than answering from memory, and do not cite anything."; + +const parameters = z.object({ + query: z + .string() + .describe("What to look for, in the words a person would use."), +}); + +export function knowledgeSearchTool(options: { + search: KnowledgeSearch; + auditStore: AuditStore; + asker: KnowledgeAsker; + botId: string; +}): GrantedTool { + const { search, auditStore, asker, botId } = options; + + return { + name: KNOWLEDGE_TOOL_NAME, + description: + "Search the company's connected documents and return the matching ones with a link to each. " + + "Use this for any question about company policy, process, or history. Only documents the " + + "person asking is allowed to read are returned.", + parameters, + execute: async (args: unknown) => { + const parsed = parameters.safeParse(args); + /* + * A malformed call is answered, not thrown. The model is mid-run and an exception here ends the + * turn with nothing said; the same reasoning as the refusal path in plugins/tools.ts. + */ + if (!parsed.success) { + return "That search needs a query: a short phrase describing what to look for."; + } + + const citations = await search.search({ + asker, + query: parsed.data.query, + }); + + /* + * The row is written after the search rather than before it, and this is the one place in the + * deployment where that is the right way round. Nothing is being acted on: no document changes, + * nothing leaves the deployment, and the interesting fact is what came back rather than what was + * attempted. `knowledge.searched` was already declared in audit.ts for exactly this. + * + * The query text is recorded, the way a shell command's text is. What is not recorded is any + * passage: `content` is on the redaction list in audit.ts, and a snippet is the document's text + * under another name, so the row names the documents and never quotes them. + */ + await recordAuditEvent(auditStore, { + eventType: "knowledge.searched", + targetType: "knowledge", + targetId: botId, + actorUserId: asker.userId, + payload: { + bot: botId, + query: parsed.data.query, + matched: citations.length, + documents: citations.map((citation) => citation.documentId), + }, + }); + + if (citations.length === 0) return NOTHING_FOUND; + + /* + * Formatted for a model that has to cite it, so each document is one block with its link beside + * its passage. A JSON blob would be read as data to summarise; this reads as sources to quote. + */ + return citations + .map( + (citation) => + `${citation.title}\n${citation.url}\n${citation.snippet}`, + ) + .join("\n\n"); + }, + }; +} diff --git a/server/tests/knowledge-search.integration.test.ts b/server/tests/knowledge-search.integration.test.ts new file mode 100644 index 00000000..063eb630 --- /dev/null +++ b/server/tests/knowledge-search.integration.test.ts @@ -0,0 +1,265 @@ +import { afterAll, beforeAll, describe, expect, test } from "bun:test"; +import { randomUUID } from "node:crypto"; +import { eq, inArray } from "drizzle-orm"; +import { createDatabase } from "../src/db/client"; +import { + chunks, + connectorInstances, + documentAcls, + documents, +} from "../src/db/schema"; +import { createKnowledgeSearch } from "../src/knowledge/search"; +import { TEST_POOL } from "./support/database"; + +/** + * The read path, against a real database, because every property worth asserting here is a property + * of the query. + * + * The ACL is evaluated in SQL, so a test that stubbed the database would assert the shape of some + * TypeScript rather than whether the database hands over a document the asker may not read. That is + * the whole claim, so it is made against PostgreSQL: full-text search, `row_number`, and the two + * correlated subqueries are all things only PostgreSQL can be wrong about. + */ + +const database = createDatabase( + process.env.DATABASE_URL ?? + "postgres://openbot:openbot@localhost:5432/openbot", + TEST_POOL, +); + +const search = createKnowledgeSearch(database); + +const suite = randomUUID().slice(0, 8).replace(/-/g, ""); +const asker = { userId: `user_${suite}`, groups: [] as string[] }; + +/** Ids kept so teardown removes exactly what this file made and nothing else. */ +let connectorId = ""; +const documentIds: string[] = []; + +/** + * One document with one chunk and a set of ACL rows. + * + * `embedding` is required by the schema and is not read by this path, so it is a fixed vector rather + * than a pretend one: nothing here should read as though it carried meaning. + */ +async function addDocument(input: { + title: string; + body: string; + acls: { principal: string; effect: "allow" | "deny" }[]; + deleted?: boolean; +}): Promise { + const [document] = await database + .insert(documents) + .values({ + connectorInstanceId: connectorId, + sourceId: `${suite}-${input.title}`, + title: input.title, + canonicalUrl: `https://example.invalid/${encodeURIComponent(input.title)}`, + metadata: {}, + contentHash: randomUUID(), + ...(input.deleted ? { deletedAt: new Date() } : {}), + }) + .returning({ id: documents.id }); + + const id = document?.id as string; + documentIds.push(id); + + await database.insert(chunks).values({ + documentId: id, + position: 0, + content: input.body, + embedding: Array.from({ length: 1536 }, () => 0), + }); + + if (input.acls.length > 0) { + await database.insert(documentAcls).values( + input.acls.map((acl) => ({ + documentId: id, + principal: acl.principal, + effect: acl.effect, + })), + ); + } + + return id; +} + +beforeAll(async () => { + const [instance] = await database + .insert(connectorInstances) + .values({ + type: "google_drive", + status: "succeeded", + sourceMetadata: { suite }, + }) + .returning({ id: connectorInstances.id }); + connectorId = instance?.id as string; +}); + +afterAll(async () => { + // Chunks and ACLs cascade from the document, and documents cascade from the connector instance. + if (documentIds.length > 0) { + await database.delete(documents).where(inArray(documents.id, documentIds)); + } + if (connectorId) { + await database + .delete(connectorInstances) + .where(eq(connectorInstances.id, connectorId)); + } +}); + +describe("answering from what the asker may read", () => { + test("returns a document the asker is allowed, with a link and a passage", async () => { + await addDocument({ + title: "Expense policy", + body: "Meals under seventy five dollars need no receipt. Anything above five hundred needs your manager before you spend it.", + acls: [{ principal: `user:${asker.userId}`, effect: "allow" }], + }); + + const hits = await search.search({ asker, query: "receipt for meals" }); + + expect(hits).toHaveLength(1); + expect(hits[0]?.title).toBe("Expense policy"); + // The URL is the connector's, so a citation opens the document rather than a path built here. + expect(hits[0]?.url).toContain("Expense"); + // A passage rather than the whole chunk, with the matched term marked. This is what makes a + // citation a citation instead of a title. + expect(hits[0]?.snippet).toContain("receipt"); + }); + + test("does not return a document nothing allows", async () => { + // Absence is the refusal. A document whose ACLs failed to sync is invisible, not public, which is + // the same shape every other grant in this repo uses. + await addDocument({ + title: "Unowned runbook", + body: "The checkout certificate expired and nobody owned it.", + acls: [], + }); + + const hits = await search.search({ asker, query: "checkout certificate" }); + expect(hits.map((hit) => hit.title)).not.toContain("Unowned runbook"); + }); + + test("does not return a document allowed to somebody else", async () => { + await addDocument({ + title: "Board minutes", + body: "The board discussed the acquisition timeline in detail.", + acls: [{ principal: "user:someone_else", effect: "allow" }], + }); + + const hits = await search.search({ asker, query: "acquisition timeline" }); + expect(hits.map((hit) => hit.title)).not.toContain("Board minutes"); + }); + + test("a deny beats an allow on the same document", async () => { + /* + * Both rows match this asker. Deny has to win regardless of which row the planner reaches first, + * which is why it is a `not exists` over the whole ACL set rather than an ordering. + */ + await addDocument({ + title: "Salary bands", + body: "Engineering salary bands for the coming year, by level.", + acls: [ + { principal: `user:${asker.userId}`, effect: "allow" }, + { principal: `user:${asker.userId}`, effect: "deny" }, + ], + }); + + const hits = await search.search({ asker, query: "salary bands level" }); + expect(hits.map((hit) => hit.title)).not.toContain("Salary bands"); + }); + + test("a deny on a group beats an allow on the person", async () => { + // The asker is allowed by name and denied through a group they are in. Still denied. + const grouped = { userId: asker.userId, groups: ["contractors"] }; + await addDocument({ + title: "Employee handbook", + body: "Parental leave and sabbatical entitlements for permanent staff.", + acls: [ + { principal: `user:${asker.userId}`, effect: "allow" }, + { principal: "group:contractors", effect: "deny" }, + ], + }); + + const hits = await search.search({ + asker: grouped, + query: "parental leave sabbatical", + }); + expect(hits.map((hit) => hit.title)).not.toContain("Employee handbook"); + }); + + test("a group allow reaches somebody in that group and nobody else", async () => { + /* + * `users.groups` is written by nothing today, so in this deployment the second half of this is + * what actually happens for everybody. It is asserted both ways so that the day groups arrive + * from an identity provider, this test says whether they work rather than needing to be written + * then. + */ + await addDocument({ + title: "Finance calendar", + body: "Quarterly close dates and the reforecast window for each quarter.", + acls: [{ principal: "group:finance", effect: "allow" }], + }); + + const inFinance = await search.search({ + asker: { userId: asker.userId, groups: ["finance"] }, + query: "quarterly close reforecast", + }); + expect(inFinance.map((hit) => hit.title)).toContain("Finance calendar"); + + const notInFinance = await search.search({ + asker, + query: "quarterly close reforecast", + }); + expect(notInFinance.map((hit) => hit.title)).not.toContain( + "Finance calendar", + ); + }); + + test("does not return a document the connector has deleted", async () => { + // A soft delete is how a connector reports something removed at the source. Returning it would + // cite a document that is no longer there. + await addDocument({ + title: "Retired pricing sheet", + body: "Legacy pricing tiers withdrawn from sale last year.", + acls: [{ principal: `user:${asker.userId}`, effect: "allow" }], + deleted: true, + }); + + const hits = await search.search({ asker, query: "legacy pricing tiers" }); + expect(hits.map((hit) => hit.title)).not.toContain("Retired pricing sheet"); + }); + + test("one citation per document, however many passages match", async () => { + // A long document matching in several places is one citation. Returning it once per chunk would + // spend the whole answer on it. + const id = await addDocument({ + title: "Security review", + body: "Rotation of the signing key is scheduled quarterly.", + acls: [{ principal: `user:${asker.userId}`, effect: "allow" }], + }); + await database.insert(chunks).values({ + documentId: id, + position: 1, + content: "The signing key rotation runbook lives with the platform team.", + embedding: Array.from({ length: 1536 }, () => 0), + }); + + const hits = await search.search({ asker, query: "signing key rotation" }); + const mine = hits.filter((hit) => hit.title === "Security review"); + expect(mine).toHaveLength(1); + }); + + test("an empty question asks the database nothing", async () => { + expect(await search.search({ asker, query: " " })).toEqual([]); + }); + + test("the number of citations is bounded whatever is asked for", async () => { + const hits = await search.search({ + asker, + query: "receipt for meals", + limit: 9_999, + }); + expect(hits.length).toBeLessThanOrEqual(20); + }); +}); diff --git a/server/tests/knowledge-tool.test.ts b/server/tests/knowledge-tool.test.ts new file mode 100644 index 00000000..64c74ac7 --- /dev/null +++ b/server/tests/knowledge-tool.test.ts @@ -0,0 +1,174 @@ +import { describe, expect, test } from "bun:test"; +import type { AuditEventInput, AuditStore } from "../src/audit"; +import type { + KnowledgeCitation, + KnowledgeSearch, +} from "../src/knowledge/search"; +import { knowledgeSearchTool } from "../src/knowledge/tool"; + +/** + * What the model is handed, and what the trail is left. + * + * The query itself is a property of PostgreSQL and is asserted against a real database in + * knowledge-search.integration.test.ts. Everything here is a property of this file: that an empty + * result becomes a sentence rather than an empty string, that a row is written either way, and that + * no passage reaches the row. + */ + +const asker = { userId: "u_1", groups: ["finance"] }; + +function recorder() { + const written: AuditEventInput[] = []; + const auditStore: AuditStore = { + insert: async (event) => { + written.push(event); + }, + }; + return { written, auditStore }; +} + +function searchReturning(citations: KnowledgeCitation[]): { + search: KnowledgeSearch; + asked: { query: string; userId: string }[]; +} { + const asked: { query: string; userId: string }[] = []; + return { + asked, + search: { + anyDocuments: async () => true, + search: async ({ query, asker: who }) => { + asked.push({ query, userId: who.userId }); + return citations; + }, + }, + }; +} + +const oneCitation: KnowledgeCitation[] = [ + { + documentId: "d_1", + title: "Expense policy", + url: "https://notion.example/expense-policy", + snippet: "Meals under $75 need no receipt.", + }, +]; + +describe("the knowledge tool", () => { + test("hands back each document with its link and its passage", async () => { + const { search } = searchReturning(oneCitation); + const tool = knowledgeSearchTool({ + search, + auditStore: recorder().auditStore, + asker, + botId: "knowledge", + }); + + const answer = await tool.execute({ query: "receipts" }); + + expect(answer).toContain("Expense policy"); + // The link, because a citation the person cannot open is not a citation. + expect(answer).toContain("https://notion.example/expense-policy"); + expect(answer).toContain("receipt"); + }); + + test("says nothing was found rather than returning an empty string", async () => { + // A model handed "" fills the gap from training and cites a document that does not exist. This is + // the same instruction the Knowledge coworker's prompt carries, arriving as data. + const { search } = searchReturning([]); + const tool = knowledgeSearchTool({ + search, + auditStore: recorder().auditStore, + asker, + botId: "knowledge", + }); + + const answer = await tool.execute({ query: "something nobody wrote down" }); + + expect(answer).not.toBe(""); + expect(answer.toLowerCase()).toContain("no authorized document"); + expect(answer.toLowerCase()).toContain("do not cite"); + }); + + test("asks on behalf of the person, not the Bot", async () => { + // The whole boundary. If this ever passed a service identity the ACL filter would be filtering + // against the wrong principals and would still look like it worked. + const { search, asked } = searchReturning(oneCitation); + const tool = knowledgeSearchTool({ + search, + auditStore: recorder().auditStore, + asker, + botId: "knowledge", + }); + + await tool.execute({ query: "receipts" }); + + expect(asked).toHaveLength(1); + expect(asked[0]?.userId).toBe("u_1"); + }); + + test("writes a row naming the documents and never quoting them", async () => { + const { search } = searchReturning(oneCitation); + const { written, auditStore } = recorder(); + const tool = knowledgeSearchTool({ + search, + auditStore, + asker, + botId: "knowledge", + }); + + await tool.execute({ query: "receipts" }); + + expect(written).toHaveLength(1); + const row = written[0]; + expect(row?.eventType).toBe("knowledge.searched"); + expect(row?.actorUserId).toBe("u_1"); + expect(row?.payload.query).toBe("receipts"); + expect(row?.payload.documents).toEqual(["d_1"]); + expect(row?.payload.matched).toBe(1); + + // The passage is the document's text under another name. A trail that quotes it has copied the + // document into a table with a different retention policy and a different audience. + const serialised = JSON.stringify(row?.payload); + expect(serialised).not.toContain("Meals under"); + expect(serialised).not.toContain("receipt."); + }); + + test("records a search that found nothing", async () => { + // "This Bot was asked six times last week and found nothing" is a fact about the corpus, and it + // only exists if the empty case writes a row too. + const { search } = searchReturning([]); + const { written, auditStore } = recorder(); + const tool = knowledgeSearchTool({ + search, + auditStore, + asker, + botId: "knowledge", + }); + + await tool.execute({ query: "nothing" }); + + expect(written).toHaveLength(1); + expect(written[0]?.payload.matched).toBe(0); + expect(written[0]?.payload.documents).toEqual([]); + }); + + test("answers a malformed call instead of ending the run", async () => { + // An exception here ends the turn with nothing said. Same reasoning as the refusal path in + // plugins/tools.ts. + const { search, asked } = searchReturning(oneCitation); + const { written, auditStore } = recorder(); + const tool = knowledgeSearchTool({ + search, + auditStore, + asker, + botId: "knowledge", + }); + + const answer = await tool.execute({ notAQuery: 12 }); + + expect(answer).toContain("needs a query"); + // And nothing was searched or recorded, because nothing was asked. + expect(asked).toHaveLength(0); + expect(written).toHaveLength(0); + }); +});