fix(rag): scope per-collection embedding resolution to the organization - #1425
Merged
Conversation
`collection_name` on `knowledge_bases` is indexed but not unique: two organizations can name a collection the same string and share one vector table. `embeddings_for_collection` resolved the knowledge base with `get_by_collection_name(...).first()` - an arbitrary tenant's row - so an embedding call for org A could resolve org B's knowledge base, unseal B's vault key under B's own scope and bill B, and run A's text through B's credential (#913). It applies to ingestion and search alike, and predates the rerank work the reviewer raised it against. Resolve within the organization the embedding is for: - `knowledge_base_repo.get_for_collection(db, name, organization_id)` is the org-scoped lookup - the caller's own row, then an app-scoped one, never a third tenant's. The worker's `_knowledge_base_for` carried the same two-pass logic and now delegates to it, so the rule lives once. - `embeddings_for_collection(name, organization_id=None)` takes the org; the store threads it (`EmbeddingResolver` is now `(str, UUID | None)`, `search`/`get_collection_info` -> `_for_collection` -> resolver). Search, the collection-info route and the route search all pass the caller's org (`deps.organization_id` for an agent, `ctx.organization_id` for a route); ingestion binds the flow's org in the resolver closure, the store passing none on the insert path. `None` stays first-match for a caller with no organization in hand - a local-directory sync. The physical vector table is still keyed by `collection_name`, so two tenants sharing a name still share a table - the store-level isolation question #913 flags separately, which this does not close. Verified with a two-tenant integration test proving each organization resolves its own key and never the other's, and the resolver's unit tests re-pointed at the org-scoped lookup. The rerank resolver #913 also names does not exist in the tree yet (unmerged #911); its identical fix belongs there, on the same `get_for_collection`. Closes #913
OchnikBartek
added
bug
Member
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8354aeb6f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Security Review
Here are some automated security review suggestions for this pull request.
Reviewed commit: 8354aeb6f8
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
`_for_collection` now calls its resolver `(name, organization_id)` (#913); the integration stubs `_no_resolution` and `_no_collection_of_its_own` still took one argument, so `_for_collection` calling them with two raised `TypeError` under a real database - a path the local run cannot reach, so CI caught it. Both now accept the organization and ignore it, as they ignore the name. Refs #913
Scoping per-collection resolution to the caller's organization (#913) is only safe if every knowledge base on one collection name agrees on how it embeds. Nothing made them agree. `collection_name` is not unique and one name is one physical table, so two rows could index the same vectors at different widths - pgvector then refuses the comparison outright - or at the same width with different models, where it ranks one embedding space against another and answers with plausible nonsense. The credential had the same shape of problem: whichever sibling the resolver happened to read is the vault key that got billed, so an older private row's key could pay for chunks uploaded through a newer shared one. A row created against a name that already exists now adopts that collection's model, width, provider and vault key, and a caller who named a *different* one is refused rather than silently overridden - being given another embedding space with nothing on screen to say so is the worse half. Which sibling a per-collection lookup picks then stops mattering, which is what makes resolving one by organization correct rather than merely narrower. Cross-organization sharing was already refused at claim time (#367), so this closes the same-organization half of it. No backfill: a deployment with rows predating the invariant would need one, and there are none. Verified with tests/test_shared_collection_embedding.py - adoption of each field, the oldest holder winning, a disagreeing choice refused per field, a restating choice accepted, and a first row on a free name keeping the caller's own. Refs #913
Merged
DEENUU1
added a commit
that referenced
this pull request
Sep 5, 2026
Release 0.0.367. Ships #1425 — per-collection embedding resolution scoped to the organization, and the invariant that makes it correct: one collection name is one embedding space.
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
2 participants
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 fixes
knowledge_bases.collection_nameis indexed but not unique — twoorganizations can name a collection the same string and share one vector table.
embeddings_for_collectionresolved the knowledge base by name alone(
get_by_collection_name(...).first()), returning an arbitrary tenant's row. Soan embedding call for org A could resolve org B's knowledge base: unsealing
B's vault key under B's scope, billing B, and running A's text through B's
credential (#913). It affects ingestion and search alike, and predates the
rerank work the reviewer raised it against.
The fix — resolve within the caller's organization
knowledge_base_repo.get_for_collection(db, name, organization_id)— theorg-scoped lookup: the caller's own row first, then an app-scoped
(deployment-wide) one, and never a third tenant's. The worker's
_knowledge_base_forheld the same two-pass logic and now delegates to it, sothe tenant-narrowing rule lives in one place.
embeddings_for_collection(name, organization_id=None)takes the org. Theresolver interface is now
(str, UUID | None)and the store threads it:search/get_collection_info→_for_collection→ resolver. The agent searchpasses
deps.organization_id; the route search and the…/inforoute passctx.organization_id; ingestion binds the flow's own org in the resolverclosure (the store passes none on the insert path).
Nonekeeps the oldfirst-match for a caller with no organization in hand — a local-directory sync.
The adversarial self-review (the automated reviewer is off, #311) flagged
get_collection_infoas the one metadata path still resolving by name alone —it unsealed a foreign tenant's key (for
dimonly; no billing, nothingreturned). It is threaded here too. The remaining org-
Nonepath is theapp-admin-only local-directory sync, unchanged from before.
Verification
collection_name; each resolves its own knowledge base, a third org getsNonerather than a foreign row, an app-scoped row is the fallback, and theembedding resolver never unseals another tenant's vault key.
embedding_resolution.pystays at 100%.make lint-backendgreen.Scope — what this does not close
collection_name, so twotenants sharing a name still share a table — the store-level isolation question
rag: per-collection resolution picks a KB by non-unique collection_name #913 flags separately. Not closed here.
rerank_resolution.py) is not inthe tree yet — it is part of unmerged PR feat(rag): add a real reranker to RAG retrieval #911, where this was first raised.
Its identical fix belongs there, on the same
get_for_collection.Closes #913