From 004fad6951fb94d6f46e4fd790fe6ffa69e34502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20=F0=9F=94=B6=20Tarbert?= <66887028+NathanTarbert@users.noreply.github.com> Date: Fri, 21 Aug 2026 13:13:07 -0400 Subject: [PATCH] fix: remove the knowledge path nothing reaches (#59) createKnowledgeAgent, createAgentInvoker and InMemoryKnowledgeRepository have no caller outside their own tests: a second, unreachable design for retrieval sitting beside the one the product uses. The repository also holds documents, chunks and ACLs in a Map in the server process, which is what #21 took back, and canRead filters ACL rows already loaded into the process rather than in SQL, so neither is a starting point for the Postgres read path that is missing. The write half of that pipeline is already Postgres and is untouched here. Docs now say a knowledge.yaml declaration is not a connection, and name the MCP grant as the retrieval path that works today. The agents.yaml example's role_description keeps the hedge #58 added to the system_prompt. --- docs/architecture.md | 7 +++ docs/configuration.md | 10 +++- server/src/agents/invocation.ts | 20 -------- server/src/agents/knowledge-agent.ts | 22 --------- server/src/knowledge/acl.ts | 22 --------- server/src/knowledge/repository.ts | 57 ----------------------- server/src/knowledge/types.ts | 9 ---- server/tests/agent-invocation.test.ts | 30 ------------ server/tests/knowledge-acl.test.ts | 30 ------------ server/tests/knowledge-agent.test.ts | 37 --------------- server/tests/knowledge-repository.test.ts | 51 -------------------- 11 files changed, 15 insertions(+), 280 deletions(-) delete mode 100644 server/src/agents/invocation.ts delete mode 100644 server/src/agents/knowledge-agent.ts delete mode 100644 server/src/knowledge/acl.ts delete mode 100644 server/src/knowledge/repository.ts delete mode 100644 server/src/knowledge/types.ts delete mode 100644 server/tests/agent-invocation.test.ts delete mode 100644 server/tests/knowledge-acl.test.ts delete mode 100644 server/tests/knowledge-agent.test.ts delete mode 100644 server/tests/knowledge-repository.test.ts diff --git a/docs/architecture.md b/docs/architecture.md index 9c108b30..b35bb147 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -148,6 +148,13 @@ Required package files: The server validates the package at startup. Channel agent IDs must match declared agents. Knowledge sources currently support Google Drive and Microsoft OneDrive declarations. +A declaration is not a connection. Google Drive can be configured from `/admin/connectors`, which +stores the credential and writes the connector instance; OneDrive has no setup screen yet. No +connector adapter or sync schedule exists yet either, so the `documents`, `chunks` and +`document_acls` tables stay empty and a built-in coworker has nothing of its own to search. The +retrieval path that works today is an MCP tool granted to a Bot: it is handed to the model as a +server-executed tool and every call goes through the grant, the policy engine and the audit trail. + Connector credentials are stored through the credential vault and referenced by id, not stored inline in YAML. ## Security boundaries diff --git a/docs/configuration.md b/docs/configuration.md index 4836f0d4..10c45219 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -267,7 +267,7 @@ agents: - id: knowledge name: Knowledge title: Company Knowledge - role_description: Answer company knowledge questions and cite sources. + role_description: Help answer company knowledge questions and cite sources when available. avatar_seed: knowledge type: built-in system_prompt: Answer from authorized company knowledge and cite your sources. When none is connected, say so plainly rather than inventing a citation. @@ -327,7 +327,13 @@ sources: roots: [Risk, Operations] ``` -Supported source types are `google-drive` and `microsoft-onedrive`. +Supported source types are `google-drive` and `microsoft-onedrive`. This file declares what a +deployment is allowed to connect, not what it has connected. Google Drive can be configured from +`/admin/connectors`, which stores the service-account credential and creates the connector +instance; Microsoft OneDrive has no setup screen yet and that page says so rather than showing a +dead control. Neither type has a sync running behind it yet, so a declared source contributes no +documents. A coworker that has to search and cite today does it through an MCP tool granted from +`/admin/plugins`, which executes here through the grant, the policy and the audit row. ## Change workflow diff --git a/server/src/agents/invocation.ts b/server/src/agents/invocation.ts deleted file mode 100644 index 592405b6..00000000 --- a/server/src/agents/invocation.ts +++ /dev/null @@ -1,20 +0,0 @@ -type Agent = { - id: string; - type: "built_in" | "remote_ag_ui"; - available: boolean; - reason?: string; -}; -type Response = { text: string; citations: unknown[] }; - -export function createAgentInvoker(ports: { - knowledge: (question: string) => Promise; - remote: (agentId: string, question: string) => Promise; -}) { - return async (agent: Agent, question: string) => { - if (!agent.available) - throw new Error(agent.reason ?? "Agent is unavailable."); - return agent.type === "built_in" - ? ports.knowledge(question) - : ports.remote(agent.id, question); - }; -} diff --git a/server/src/agents/knowledge-agent.ts b/server/src/agents/knowledge-agent.ts deleted file mode 100644 index 007b006c..00000000 --- a/server/src/agents/knowledge-agent.ts +++ /dev/null @@ -1,22 +0,0 @@ -type Citation = { title: string; canonicalUrl: string; content: string }; - -export function createKnowledgeAgent(input: { - available: boolean; - search: (question: string) => Promise; - complete: (input: { - question: string; - context: Citation[]; - }) => Promise; -}) { - return { - async respond(question: string) { - if (!input.available) - throw new Error("Model credential is not configured."); - const citations = await input.search(question); - return { - text: await input.complete({ question, context: citations }), - citations, - }; - }, - }; -} diff --git a/server/src/knowledge/acl.ts b/server/src/knowledge/acl.ts deleted file mode 100644 index d9dc3e97..00000000 --- a/server/src/knowledge/acl.ts +++ /dev/null @@ -1,22 +0,0 @@ -import type { KnowledgeAclEntry, KnowledgeActor } from "./types"; - -export function canRead( - actor: KnowledgeActor, - entries: KnowledgeAclEntry[], -): boolean { - let allowed = false; - - for (const entry of entries) { - if (!matchesPrincipal(actor, entry.principal)) continue; - if (entry.effect === "deny") return false; - allowed = true; - } - - return allowed; -} - -function matchesPrincipal(actor: KnowledgeActor, principal: string): boolean { - if (principal === `user:${actor.userId}`) return true; - if (!principal.startsWith("group:")) return false; - return actor.groups.includes(principal.slice("group:".length)); -} diff --git a/server/src/knowledge/repository.ts b/server/src/knowledge/repository.ts deleted file mode 100644 index 883cf9e3..00000000 --- a/server/src/knowledge/repository.ts +++ /dev/null @@ -1,57 +0,0 @@ -import { canRead } from "./acl"; -import type { KnowledgeAclEntry, KnowledgeActor } from "./types"; - -type SourceChange = { - connectorInstanceId: string; - sourceId: string; - title: string; - canonicalUrl: string; - contentHash: string; - chunks: { position: number; content: string }[]; - acls: KnowledgeAclEntry[]; -}; - -type Citation = { - documentId: string; - title: string; - canonicalUrl: string; - chunkId: string; - content: string; -}; - -export class InMemoryKnowledgeRepository { - #sources = new Map(); - #deleted = new Set(); - - apply(change: SourceChange) { - const key = sourceKey(change.connectorInstanceId, change.sourceId); - this.#sources.set(key, structuredClone(change)); - this.#deleted.delete(key); - } - - delete(connectorInstanceId: string, sourceId: string) { - this.#deleted.add(sourceKey(connectorInstanceId, sourceId)); - } - - documents(): SourceChange[] { - return [...this.#sources.values()].map((source) => structuredClone(source)); - } - - search(actor: KnowledgeActor): Citation[] { - return [...this.#sources.entries()].flatMap(([key, source]) => { - if (this.#deleted.has(key) || !canRead(actor, source.acls)) return []; - const documentId = key; - return source.chunks.map((chunk) => ({ - documentId, - title: source.title, - canonicalUrl: source.canonicalUrl, - chunkId: `${documentId}:${chunk.position}`, - content: chunk.content, - })); - }); - } -} - -function sourceKey(connectorInstanceId: string, sourceId: string) { - return `${connectorInstanceId}:${sourceId}`; -} diff --git a/server/src/knowledge/types.ts b/server/src/knowledge/types.ts deleted file mode 100644 index 5ef9b202..00000000 --- a/server/src/knowledge/types.ts +++ /dev/null @@ -1,9 +0,0 @@ -export type KnowledgeActor = { - userId: string; - groups: string[]; -}; - -export type KnowledgeAclEntry = { - principal: string; - effect: "allow" | "deny"; -}; diff --git a/server/tests/agent-invocation.test.ts b/server/tests/agent-invocation.test.ts deleted file mode 100644 index b2f6a8db..00000000 --- a/server/tests/agent-invocation.test.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { expect, test } from "bun:test"; -import { createAgentInvoker } from "../src/agents/invocation"; - -test("invokes the selected available built-in agent", async () => { - const invoke = createAgentInvoker({ - knowledge: async () => ({ text: "Answer", citations: [] }), - remote: async () => ({ text: "unused", citations: [] }), - }); - await expect( - invoke({ id: "knowledge", type: "built_in", available: true }, "question"), - ).resolves.toEqual({ text: "Answer", citations: [] }); -}); - -test("rejects unavailable agents before invocation", async () => { - const invoke = createAgentInvoker({ - knowledge: async () => ({ text: "unused", citations: [] }), - remote: async () => ({ text: "unused", citations: [] }), - }); - await expect( - invoke( - { - id: "knowledge", - type: "built_in", - available: false, - reason: "Model credential is not configured.", - }, - "question", - ), - ).rejects.toThrow("Model credential is not configured."); -}); diff --git a/server/tests/knowledge-acl.test.ts b/server/tests/knowledge-acl.test.ts deleted file mode 100644 index e7608922..00000000 --- a/server/tests/knowledge-acl.test.ts +++ /dev/null @@ -1,30 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { canRead } from "../src/knowledge/acl"; - -describe("knowledge ACL evaluation", () => { - test("allows a matching user principal", () => { - expect( - canRead({ userId: "u1", groups: [] }, [ - { principal: "user:u1", effect: "allow" }, - ]), - ).toBe(true); - }); - - test("fails closed for an unmatched or empty ACL", () => { - expect( - canRead({ userId: "u1", groups: ["finance"] }, [ - { principal: "group:engineering", effect: "allow" }, - ]), - ).toBe(false); - expect(canRead({ userId: "u1", groups: [] }, [])).toBe(false); - }); - - test("makes a matching deny override a matching allow", () => { - expect( - canRead({ userId: "u1", groups: ["finance"] }, [ - { principal: "group:finance", effect: "allow" }, - { principal: "user:u1", effect: "deny" }, - ]), - ).toBe(false); - }); -}); diff --git a/server/tests/knowledge-agent.test.ts b/server/tests/knowledge-agent.test.ts deleted file mode 100644 index 53d188a4..00000000 --- a/server/tests/knowledge-agent.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { expect, test } from "bun:test"; -import { createKnowledgeAgent } from "../src/agents/knowledge-agent"; - -test("returns authorized knowledge citations to the model port", async () => { - const agent = createKnowledgeAgent({ - available: true, - search: async () => [ - { - title: "Policy", - canonicalUrl: "https://example.test/policy", - content: "Use MFA.", - }, - ], - complete: async ({ context }) => `Answer: ${context[0]?.content}`, - }); - await expect(agent.respond("What is required?")).resolves.toEqual({ - text: "Answer: Use MFA.", - citations: [ - { - title: "Policy", - canonicalUrl: "https://example.test/policy", - content: "Use MFA.", - }, - ], - }); -}); - -test("refuses to run when its model credential is unavailable", async () => { - const agent = createKnowledgeAgent({ - available: false, - search: async () => [], - complete: async () => "unused", - }); - await expect(agent.respond("question")).rejects.toThrow( - "Model credential is not configured.", - ); -}); diff --git a/server/tests/knowledge-repository.test.ts b/server/tests/knowledge-repository.test.ts deleted file mode 100644 index d3863a1c..00000000 --- a/server/tests/knowledge-repository.test.ts +++ /dev/null @@ -1,51 +0,0 @@ -import { describe, expect, test } from "bun:test"; -import { InMemoryKnowledgeRepository } from "../src/knowledge/repository"; - -const change = { - connectorInstanceId: "connector-1", - sourceId: "source-1", - title: "Finance policy", - canonicalUrl: "https://example.test/policy", - contentHash: "hash-1", - chunks: [{ position: 0, content: "finance content" }], - acls: [{ principal: "group:finance", effect: "allow" as const }], -}; - -describe("knowledge repository", () => { - test("replaces source content idempotently", () => { - const repository = new InMemoryKnowledgeRepository(); - repository.apply(change); - repository.apply({ - ...change, - contentHash: "hash-2", - chunks: [{ position: 0, content: "updated" }], - }); - - expect(repository.documents()).toEqual([ - { - ...change, - contentHash: "hash-2", - chunks: [{ position: 0, content: "updated" }], - }, - ]); - }); - - test("returns citations only to authorized actors and hides deleted sources", () => { - const repository = new InMemoryKnowledgeRepository(); - repository.apply(change); - expect(repository.search({ userId: "u1", groups: ["finance"] })).toEqual([ - { - documentId: "connector-1:source-1", - title: "Finance policy", - canonicalUrl: "https://example.test/policy", - chunkId: "connector-1:source-1:0", - content: "finance content", - }, - ]); - expect(repository.search({ userId: "u2", groups: [] })).toEqual([]); - repository.delete("connector-1", "source-1"); - expect(repository.search({ userId: "u1", groups: ["finance"] })).toEqual( - [], - ); - }); -});