Do not search documents out of our own database - #118
Merged
Conversation
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.
davidmckayv
requested review from
MikeRyanDev,
guidovizoso and
tylerslaton
as code owners
August 21, 2026 22:56
This was referenced Aug 21, 2026
davidmckayv
pushed a commit
that referenced
this pull request
Aug 22, 2026
#118 asked for the knowledge/ modules and the three tables to be dropped in a change that says so. This is the half that can be done now. server/src/knowledge/acl.ts, repository.ts and types.ts have no importer outside their own two test files. InMemoryKnowledgeRepository holds documents, chunks and ACLs in a Map in the process, which is the shape #21 took back, and canRead(actor, entries[]) filters ACL rows already pulled into memory rather than in SQL. Neither is a starting point for anything #119 describes. The three tables stay for now. PR #97 has not merged, so connectors/sync-persistence.ts still imports documents, chunks, document_acls, connector_cursors and sync_runs and writes to all of them; dropping the tables breaks typecheck there. It is already orphaned at runtime, its only caller being its own integration test, so this is a typecheck dependency rather than a live one, but it is #97's to remove. Left alone deliberately: connector_instances, which server/src/connectors.ts writes to from production code; connector_cursors and sync_runs, which are connector-side and go with #97; and webhook_subscriptions, which is referenced nowhere at all and wants its own change. Docs that describe pgvector holding knowledge records stay accurate while the tables exist and belong in the commit that drops them.
This was referenced Aug 22, 2026
Merged
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
1 participant
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.
Reverts #113, which was merged by accident, and states the direction so the next person does not
rebuild it.
What it did, and why it goes
#113 added a knowledge search over
documents,chunksanddocument_acls— our own copy of acustomer's corpus, chunked, ranked here, with an ACL predicate of our own writing deciding who may
see what. The code is careful and putting the ACL filter in SQL rather than in JavaScript is the
right instinct. The design is what is being taken back, not the execution.
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 second permission model that has to be kept in step with theirs, and every gap between the
two is an answer assembled confidently out of documents the person cannot open. It is also a second
copy of somebody's data to secure, to keep current, and to remember to delete when they leave — which
is exactly the failure #97 had to go and fix on the connector side.
This is the same conclusion #97 reached from the other end: it removes the sync that was filling these
tables, because one service-account credential reading everybody's documents gives every person the
same answer regardless of what they may see.
Where retrieval does belong
Over the tool catalogue, not over documents. When a deployment has a hundred tools across a dozen
vendors, choosing which one to call is a search problem — and it is a search over our own metadata,
tool names and descriptions and parameters, rather than over a customer's files. Same word, different
thing. Written up so it is a decision rather than folklore.
Scope
The write half goes with #97, which deletes
connectors/sync-persistence.tsand the worker runner.After both, the three tables and the remaining
knowledge/modules are read by nothing. Droppingthem is not in here — that is a schema change and it should be its own commit that says so, once
#97 has landed, rather than a side effect of a revert.
Proof
bun run test:ci: 1076 tests, 0 fail. Typecheck andformat:checkclean. The revert is clean; noconflict resolution was needed.