Remove the knowledge path until it's supported. - #89
Closed
NathanTarbert wants to merge 1 commit into
Closed
Conversation
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 CopilotKit#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 CopilotKit#58 added to the system_prompt.
NathanTarbert
requested review from
MikeRyanDev,
davidmckayv,
guidovizoso and
tylerslaton
as code owners
August 21, 2026 17:13
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
Removes the knowledge retrieval code nothing reaches, and corrects the two documentation claims that describe a connector pipeline as though it were running.
#59 is right that a
knowledgecoworker ships astype: built-inwith only asystem_prompt, and thatcreateKnowledgeAgent,createAgentInvokerandInMemoryKnowledgeRepositoryhave no caller outside their own tests. What that leaves is a second, unreachable design for retrieval sitting beside the one the product actually uses, and a reader has no way to tell which is which. The tests passing on it make it look maintained.It also isn't the design this repo would land now.
InMemoryKnowledgeRepositoryholds documents, chunks and ACLs in aMapin the server process, which is the exact thing #21 took back for the approval registry and the repetition counter, and for the exact reason: a second replica answers from a different Map and nothing says so.canRead(actor, entries[])has the same problem one level down — it filters an array of ACL rows already loaded into the process, and a Postgres read path filtersdocument_aclsin SQL instead. Keeping either as a starting point would point whoever builds the real search at the shape that has to be thrown away.The persistence half of that pipeline is already Postgres and already correct:
createSyncPersistence(server/src/connectors/sync-persistence.ts) writesdocuments,chunks,document_acls,connector_cursorsandsync_runsinside one transaction, with an integration test against a real database. Nothing here touches it. What is missing is a connector adapter to produce changes, a schedule to run one, and a read path over those tables — none of which the deleted files were closer to than an empty directory is.Deleted:
server/src/agents/knowledge-agent.ts,server/src/agents/invocation.ts,server/src/knowledge/(repository.ts,acl.ts,types.ts), and the four test files that were their only callers.The docs changes say what is and isn't wired:
docs/configuration.md— theknowledge.yamlsection now says a declaration is not a connection: Google Drive can be configured from/admin/connectors, OneDrive has no setup screen yet, nothing syncs behind either one, and the retrieval path that works today is an MCP tool granted from/admin/plugins. Built-in Bots are handed granted tools as server-executed tool definitions (builtInAgentConfiguration,server/src/copilot.ts:165), so that path already goes through the grant, the policy engine and the audit row.docs/configuration.md— theagents.yamlexample'srole_descriptionfor Knowledge said "Answer company knowledge questions and cite sources." unhedged, while the same field inexamples/fintech/agents.yamlsays "cite sources when available". Have Knowledge admit no source is connected, instead of citing one #58 fixed thesystem_promptin both files and left this line behind. Same hedge, both places.docs/architecture.md— the tenant package section says the same thing about declarations, connectors and where retrieval actually happens today.This does not close #59. The connector adapter, the worker schedule and the search path are still to build, and the issue should stay open for them.
Where it runs
InMemoryKnowledgeRepositorywas per-process state, and it is gone rather than replaced.Boundary and audit
Changelog
No line. A deployment behaves exactly the same afterwards: the deleted modules had no caller, so nothing that runs today runs differently.
Proof
Reachability, before deleting anything —
createKnowledgeAgent,createAgentInvoker,InMemoryKnowledgeRepository,canRead,KnowledgeActorandKnowledgeAclEntryacross every.ts/.tsxin the repo returned only their own definitions and the four test files removed here. No.md,.jsonor.yamlreferenced them either, andserver/src/knowledge/had no barrel re-exporting it.Ran on this branch:
bun run format:check— clean, 354 files.bun run lint— 27 warnings, 1 info. Identical on a cleanmaincheckout, so all pre-existing.bun run typecheck— clean across app, server and worker.bun run build— clean.bun test— 771 pass, 5 skip, 79 fail, 855 tests across 87 files. Onmainwith no changes: 780 pass, 5 skip, 79 fail, 864 tests across 91 files. The 79 failures are the database integration tests; I have no Postgres available locally and they fail the same way on an unmodifiedmain, which is what the CItestsjob runs pgvector for. The delta is exactly the 9 tests in the 4 deleted files, and the failure count is unchanged, so nothing that was passing stopped.MINIMUM_TESTS = 400inscripts/test-ci.ts, so no change is needed there.CI is the real signal for the database tests.