[jwvanderstam]

What

A follow-up from the separate security review of #527 (Info: a dormant hazard, not reachable today).

Retriever.retrieve_context looked the query up in QueryCache before searching, and stored the result in it afterwards. The cache's key is (query, top_k, min_similarity, use_hybrid). It names no workspace, knowledge tier, document filter or additional workspaces, so a hit would serve one workspace's results to another. The path was dead only because _get_app_cache() always returns None.

Change

• The dead read and write in retrieve_context and _store_query_cache are removed. The embedding cache stays: an embedding depends on text and model only.
• QueryCache's docstring says its key carries no scope, so it must never hold a scoped retrieval's results. The class itself, its stats and its initialisation are untouched.

Removed rather than given a scoped key: the key would need every input that shapes the result, and nothing uses the cache, so a correct key would be speculative code.

Tests

• test_no_cached_result_is_served_in_place_of_a_scoped_search hands retrieval a cache answering with another workspace's chunk. It fails on the old code and passes now.
• Retrieval and cache unit tests: 58 passed. ruff and mypy are clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01KkWPqAcWbxfqz1aCP6Qsdx

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[jwvanderstam]

Security review (separate agent session, CONTRIBUTING.md)

Reviewed the diff of #533 (head c13584a) against a04da4d, plus the surrounding code in src/rag/retrieval.py, src/cache/managers.py, src/graph/expander.py and src/db/entities.py.

Verdict: the change does what it says. It removes a scope-blind read and write and adds no new access path. I found no Medium or higher finding in the diff. There are two Low findings below: one about the new test, and one pre-existing scope gap in the same function that the diff did not touch.

What I verified

• After the change, nothing in src/ calls QueryCache.get/set. app.state.query_cache is still built (src/app_bootstrap.py:458-466), but only api_routes.py:140 and services/admin_stats.py:41 read it, and only for stats. With the retrieval read and write gone, no other path can serve a cached retrieval across workspaces.
• The embedding cache that remains (retrieval.py:426,433, _get_cached_embedding at :480-514) is keyed on (text, model). An embedding depends only on those two, so sharing it across workspaces leaks no data.
• The other in-process caches on the request path are either keyed by scope (services/chat.py:22,194-198, _status_doc_count_cache: dict[Scope, ...]) or hold no workspace data (_ollama_status_cache).
• The new test fails on the old code: I restored src/rag/retrieval.py from a04da4d, ran it, saw it fail, and reverted. On the PR head, test_global_knowledge_retrieval.py and the two cache test files pass (39 tests).

Checklist

1. Identity. Not touched.
2. Authorisation. Not touched. No route changes.
3. Scope. Improved. retrieve_context no longer returns a result looked up without scope, knowledge_scope, filename_filter, source_ids or additional_workspace_ids (the lines removed around retrieval.py:426). Every remaining retrieval path takes the caller's scope (retrieval.py:442-463). No ALL_WORKSPACES is added. See Low-2 for a pre-existing unscoped read in the same function.
4. Row-level security. Not touched. No migration.
5. Data integrity. Not touched.
6. Input. Not touched.
7. Secrets. Not touched. No new configuration. The only logging removed is the "[RAG] Query cache hit" line.
8. Image. Not touched.
9. Tests. Yes. test_no_cached_result_is_served_in_place_of_a_scoped_search (tests/unit/test_global_knowledge_retrieval.py:170-176) fails before the fix and passes after it. It also asserts the permitted outcome, the caller's own chunk {1}, so it is more than a refusal. One weakness, in Low-1.

Findings

Low-1: the test's mock is also the embedding cache, so the search runs with a bogus embedding.
_get_app_cache() feeds both caches (retrieval.py:426 → _get_cached_embedding at :494-498). The test patches it to return foreign, so foreign.get(text, model) answers ["another workspace's chunk"], and that string list becomes the query embedding. The test passes only because FakeDb.search_similar ignores its embedding (test_global_knowledge_retrieval.py:32-34).
Failure scenario: a later change that validates the embedding, or a fake that uses it, makes the test fail or skip the search for reasons unrelated to the cache. A reader would then misread what the test is guarding.
Suggested fix: make the mock answer the 4-argument query-cache lookup only, for example foreign.get.side_effect = lambda *a: ["another workspace's chunk"] if len(a) == 4 else None. Also assert that the search ran in the caller's scope: ("semantic", WORKSPACE, False) in retriever._db.calls.

Low-2 (pre-existing, outside the diff): GraphRAG query expansion in the same retrieve_context reads entities from every workspace.
retrieval.py:416-422 calls QueryExpander().expand(query_clean, db) with no scope. That reaches EntitiesRepository.related_names (src/db/entities.py:81-110), which opens get_connection() with no scope and selects co-occurring entity names without joining documents. That means no workspace predicate and no deleted_at IS NULL. entities and entity_relations are not among the ten RLS tables (migrations/versions/0023_qp5_force_row_level_security.py:27-38), so the database does not stop it either, and the QP-5 ratchet does not count it.
Failure scenario: with GRAPH_RAG_ENABLED=true (it is off by default), workspace A's question naming an entity is extended with names that co-occur with it only in workspace B's documents, or in retired ones. Those terms change A's lexical and semantic ranking and are written to the debug log (retrieval.py:422). The chunks returned are still scoped, so no document content crosses workspaces. The leak is a side channel: B's entity names, through ranking and the logs. This is the same kind of issue this PR fixes: a lookup inside retrieval whose key carries no scope.
Suggested fix: pass scope through expand() into related_names(..., scope=...). Restrict entity_relations.doc_id to live documents in the scope with scope_predicate, as workspace_ontology already does at entities.py:112-119. Record it as its own ticket if it is not fixed here.

Info (no action needed for this PR):

• _get_app_cache() (retrieval.py:184-186) now feeds only the embedding cache, and always with None.
• QueryCache is still built and reported on the settings page with permanently zero hits.

Both are harmless. The new docstring (src/cache/managers.py:40-45) correctly warns against re-wiring the cache.

🤖 Generated with Claude Code

[jwvanderstam]

Thanks for the review. On the findings:

• Low-1 (the test): fixed in the latest commit. The mock answers only the query cache's four-argument lookup and misses on the embedding cache's (text, model). The test also asserts that every search ran in WORKSPACE. It still fails with the old retrieval.py restored.
• Low-2 (GraphRAG expansion unscoped): this is R-12 from the QP-9 review. fix(security): R-12 — GraphRAG's expansion reads only the caller's workspace #527 fixes it: related_names takes scope=, joins the relation's document and chunk, and filters with scope_predicate and deleted_at.
• Info: agreed, harmless. The QueryCache stats panel is left as it is.

🤖 Generated with Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud