From 9af70e44e54f68387e0971301a8168d6bc44e613 Mon Sep 17 00:00:00 2001 From: David McKay Date: Fri, 21 Aug 2026 15:56:27 -0700 Subject: [PATCH] Do not search documents out of our own database Reverts #113, which was merged by accident, and says why rather than only undoing it. It added a knowledge search over `documents`, `chunks` and `document_acls`: our own copy of a customer's corpus, ranked here, with an ACL predicate of our own writing deciding who may see what. The code is careful and the ACL filter is in the right place. The design is the thing being taken back. A Bot answers from a live system by calling that system's own search, as the person asking. The vendor decides what they may see, because the vendor is the only thing that actually knows: an index here is a permission model we have to keep in step with theirs, and every gap between the two is an answer assembled from documents somebody cannot open. It is also a second copy of their data to secure, to keep current, and to remember to delete when somebody leaves. Retrieval has a place later, over the tool catalogue rather than over documents: choosing which of a hundred tools to call is a search problem, and one about our own metadata rather than a customer's files. That is a different thing wearing the same word. The write half of the index goes with #97, which removes the connector that filled it. The three tables and the `knowledge/` modules are read by nothing after this and should be dropped in a change that says so, rather than as a side effect of this one. --- CHANGELOG.md | 23 +- 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, 11 insertions(+), 827 deletions(-) delete mode 100644 server/src/knowledge/search.ts delete mode 100644 server/src/knowledge/tool.ts delete mode 100644 server/tests/knowledge-search.integration.test.ts delete mode 100644 server/tests/knowledge-tool.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d6bc8424..c70dbe4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,22 +36,17 @@ already refused the same way. Sessions survive and nobody signs in again. +### Changed + +- **This deployment does not search documents itself.** A Bot answers from a live system by calling + that system's own search as the person asking, so the vendor decides what they may see and there is + no second copy of anybody's documents here to keep in step, to secure, or to leave behind when + somebody is removed. The local index that was being filled — `documents`, `chunks` and + `document_acls` — is read by nothing, and the connector that filled it is going away. Retrieval over + a copy of a customer's corpus is not a thing OpenBot does. + ### 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 462334c9..4cdac0bf 100644 --- a/server/src/index.ts +++ b/server/src/index.ts @@ -44,8 +44,6 @@ 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"; @@ -266,15 +264,6 @@ 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", @@ -399,31 +388,8 @@ 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. - // - // 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, - }), - ]; - }, + (actorId) => (botId) => + grantedTools({ store: pluginStore, botId, actorId }), /* * 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 deleted file mode 100644 index c0496228..00000000 --- a/server/src/knowledge/search.ts +++ /dev/null @@ -1,232 +0,0 @@ -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 deleted file mode 100644 index 474d8a1c..00000000 --- a/server/src/knowledge/tool.ts +++ /dev/null @@ -1,106 +0,0 @@ -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 deleted file mode 100644 index 063eb630..00000000 --- a/server/tests/knowledge-search.integration.test.ts +++ /dev/null @@ -1,265 +0,0 @@ -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 deleted file mode 100644 index 64c74ac7..00000000 --- a/server/tests/knowledge-tool.test.ts +++ /dev/null @@ -1,174 +0,0 @@ -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); - }); -});