Skip to content

Remove the knowledge path until it's supported. - #89

Closed
NathanTarbert wants to merge 1 commit into
CopilotKit:mainfrom
NathanTarbert:fix/issue-59-unreachable-knowledge-path
Closed

Remove the knowledge path until it's supported.#89
NathanTarbert wants to merge 1 commit into
CopilotKit:mainfrom
NathanTarbert:fix/issue-59-unreachable-knowledge-path

Conversation

@NathanTarbert

Copy link
Copy Markdown
Contributor

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 knowledge coworker ships as type: built-in with only a system_prompt, and that createKnowledgeAgent, createAgentInvoker and InMemoryKnowledgeRepository have 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. InMemoryKnowledgeRepository holds documents, chunks and ACLs in a Map in 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 filters document_acls in 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) writes documents, chunks, document_acls, connector_cursors and sync_runs inside 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 — the knowledge.yaml section 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 — the agents.yaml example's role_description for Knowledge said "Answer company knowledge questions and cite sources." unhedged, while the same field in examples/fintech/agents.yaml says "cite sources when available". Have Knowledge admit no source is connected, instead of citing one #58 fixed the system_prompt in 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

  • New state that outlives a request? None. This removes some: InMemoryKnowledgeRepository was per-process state, and it is gone rather than replaced.
  • What happens on the second replica? Identical behavior on every replica, and one fewer thing that would have behaved differently on each. No runtime code path changes, because none of the deleted code was on one.
  • Anything serialised? N/A, no writes.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No.

Boundary and audit

  • N/A. No gateway, policy, or audit path is touched. The MCP retrieval path the docs now name is the existing one and is unchanged.

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, KnowledgeActor and KnowledgeAclEntry across every .ts/.tsx in the repo returned only their own definitions and the four test files removed here. No .md, .json or .yaml referenced them either, and server/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 clean main checkout, so all pre-existing.
  • bun run typecheck — clean across app, server and worker.
  • bun run build — clean.
  • bun test771 pass, 5 skip, 79 fail, 855 tests across 87 files. On main with 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 unmodified main, which is what the CI tests job 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.
  • Test floor: 855 is well above MINIMUM_TESTS = 400 in scripts/test-ci.ts, so no change is needed there.

CI is the real signal for the database tests.

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 NathanTarbert changed the title Remove the knowledge path nothing reaches, and say what is not wired Remove the knowledge path until it's supported. Aug 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Knowledge ships as a built-in coworker; the search-and-cite path behind it is never called

1 participant