diff --git a/.claude/rules/file-map.md b/.claude/rules/file-map.md index cc96aa11..b636df19 100644 --- a/.claude/rules/file-map.md +++ b/.claude/rules/file-map.md @@ -135,6 +135,7 @@ Full module index for LocalChat. **Keep this current** — update in the same co | `docs/CONFIGURATION.md` | Configuration reference — every env var, default and effect (lifted out of README) | | `docs/MIGRATIONS.md` | Migration docs — how to apply, write, and roll back | | `tests/integration/test_object_authorization_over_the_wire.py` | P2-2b — a plain user holding workspace A addresses workspace B's objects over HTTP against a real Postgres, and every route must refuse. The route list is derived from `app.openapi()` (not `app.routes`, which this FastAPI version leaves unflattened as `_IncludedRouter` with `path=None`), an unclassified path parameter fails rather than silently dropping its routes, and the fixture proves its own objects are reachable by their owner — a 404 is a pass here, so an empty fixture would be a perfect green run testing nothing. Runs as an editor and as an owner of A (R-5), and probes ids named in a body or header beside a control with the caller's own | +| `tests/integration/test_graph_expansion_scope.py` | R-12 (QP-9 review) — GraphRAG's 1-hop lookup returns a name that co-occurs in the caller's workspace, not one from another workspace's document or from a retired one | | `tests/integration/test_ontology_reads_live_documents.py` | A workspace's ontology counts only its live documents and live chunks: a retired document and a chunk retired by a re-ingest leave it | | `tests/integration/test_chunk_statistics_scope.py` | R-1 (QP-9 review) — document statistics count and sample only the caller's workspace's live chunks, with a second workspace and a retired document beside it | | `tests/integration/test_migrations_apply.py` | TQ-5b — the migration chain applies against a real database, lands on the single head, and a second run does nothing | diff --git a/CHANGELOG.md b/CHANGELOG.md index 77612f86..f49119ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ OpenAI, `https://api.openai.com/v1`) and drop any `provider/` prefix from `CLOUD `CLOUD_PRICE_OUTPUT_PER_MTOK`; without both, only tokens are counted. A provider that is not OpenAI-shaped (Anthropic's native API, Vertex) is no longer reachable, as ADR-4 accepted. +- **GraphRAG's query expansion reads only the caller's workspace (R-12).** Entities are shared across workspaces, so with `GRAPH_RAG_ENABLED` on, the 1-hop lookup added names that co-occurred in any workspace's documents, retired ones included, to the caller's query. A co-occurrence now counts only through a live document of the scope. GraphRAG is off by default. - **A password change ends the account's other sessions (`SECURITY.md` item 13).** A session opened before the change, on another device or by whoever knew the old password, kept working until its token expired. Changing your own password, or an admin resetting diff --git a/SECURITY.md b/SECURITY.md index b048984e..b3797b9e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -337,7 +337,7 @@ fails on the code before it. | R-9 | Low | Single-object purges do not require the row to be retired first | **Open** (admin-only) | | R-10 | Low | Admin user routes take raw dicts; the last-admin count ignores `is_active` | **Open** (admin-only) | | R-11 | Low | The image's source is owned by the runtime user | **Open** | -| R-12 | Low | GraphRAG's entity expansion is not scoped (off by default) | **Open** | +| R-12 | Low | GraphRAG's entity expansion is not scoped (off by default) | **Fixed** — a co-occurrence counts only through a live document of the caller's workspace; retired documents contributed too. `tests/integration/test_graph_expansion_scope.py` | | — | — | `GET /api/workspaces/active` and `POST /api/workspaces/switch` returned any named workspace, its system prompt included, to anyone signed in; found by R-5's new probes | **Fixed** (#510) — `tests/unit/test_active_workspace_is_authorised.py` | ## October 2026 audit: findings register diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 75efec05..448b3905 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -1925,7 +1925,7 @@ From `SECURITY.md`'s QP-9 register, in this order: migration 0024). 3. R-10: admin user routes take raw dicts; the last-admin count ignores `is_active`. 4. R-5b: a ratchet over the `ALL_WORKSPACES` call sites, each with a recorded reason. -5. R-9, R-11, R-12: purge preconditions, the image's file ownership, GraphRAG's scope. +5. R-9, R-11, R-12 ✅: purge preconditions, the image's file ownership, GraphRAG's scope. #### QP-11 — Replace `litellm` (ADR-4) ✅ (built 2026-11-07, taken ahead of the rest of QP-10) diff --git a/src/db/entities.py b/src/db/entities.py index 42a9d212..999acc3f 100644 --- a/src/db/entities.py +++ b/src/db/entities.py @@ -10,7 +10,11 @@ class EntitiesRepository: - """GraphRAG entities and their relations.""" + """GraphRAG entities and their relations. + + ``entities`` rows belong to no workspace: read a name only through a relation whose + document is in the caller's scope, never from the table directly. + """ def __init__(self, db: DatabaseConnection) -> None: self._db = db @@ -82,30 +86,39 @@ def related_names( self, entity_names: list[str], max_results: int = 20, + *, + scope: Scope, ) -> list[str]: """ Return names of entities co-occurring with any of the given entity names. - Used by the query expander to broaden lexical search terms. + Used by the query expander to broaden lexical search terms. Only co-occurrences + in *scope*'s live documents count: entities are shared across workspaces, so the + relation's document is the only thing that says whose they are (R-12). The chunk + must be live too, or a re-ingest would leave the old version's names behind. """ if not entity_names or not self._db.is_connected: return [] - with self._db.get_connection() as conn: + ws_filter, params_ws = scope_predicate(scope, "d.workspace_id") + with self._db.get_connection(scope=scope) as conn: with conn.cursor() as cursor: cursor.execute( - """ + f""" SELECT DISTINCT e2.name FROM entities e1 JOIN entity_relations er ON er.source_id = e1.id OR er.target_id = e1.id + JOIN documents d ON d.id = er.doc_id AND d.deleted_at IS NULL + JOIN document_chunks c ON c.id = er.chunk_id AND c.deleted_at IS NULL JOIN entities e2 ON ( CASE WHEN er.source_id = e1.id THEN er.target_id ELSE er.source_id END = e2.id ) WHERE e1.name = ANY(%s) AND e2.name != ALL(%s) + {ws_filter} ORDER BY e2.name LIMIT %s - """, - (entity_names, entity_names, max_results), + """, # nosec B608 # scope_predicate fragment: literal column, value is %s + (entity_names, entity_names, *params_ws, max_results), ) return [r[0] for r in cursor.fetchall()] diff --git a/src/graph/expander.py b/src/graph/expander.py index 0aef59b6..f30ddd14 100644 --- a/src/graph/expander.py +++ b/src/graph/expander.py @@ -16,6 +16,7 @@ from typing import Any from ..utils.logging_config import get_logger +from ..utils.scope import Scope from .extractor import _KEEP_TYPES, _get_nlp logger = get_logger(__name__) @@ -30,6 +31,8 @@ def expand( db: Any, max_extra: int = 5, graph_store: Any | None = None, + *, + scope: Scope, ) -> list[str]: """ Return additional entity-name strings for query expansion. @@ -41,6 +44,7 @@ def expand( max_extra: Maximum additional names to return. graph_store: Optional GraphStore — if provided, used instead of db for graph lookups. + scope: The workspace whose documents the co-occurrences come from. Returns: List of extra entity name strings (may be empty). @@ -65,7 +69,7 @@ def expand( store = graph_store or _wrap_db(db) try: - related = store.get_related_entity_names(query_entities, max_results=max_extra) + related = store.get_related_entity_names(query_entities, max_results=max_extra, scope=scope) if related: logger.info(f"[GraphRAG] Expanded query with {len(related)} entity terms") return related diff --git a/src/graph/store.py b/src/graph/store.py index 1734c20d..d30e020a 100644 --- a/src/graph/store.py +++ b/src/graph/store.py @@ -19,6 +19,8 @@ from abc import ABC, abstractmethod from typing import Any +from ..utils.scope import Scope + class GraphStore(ABC): """Abstract interface for the entity / relation graph store.""" @@ -40,9 +42,9 @@ def insert_relation( @abstractmethod def get_related_entity_names( - self, entity_names: list[str], max_results: int = 20 + self, entity_names: list[str], max_results: int = 20, *, scope: Scope ) -> list[str]: - """Return entity names that co-occur with any of *entity_names*.""" + """Return entity names that co-occur with any of *entity_names* in *scope*'s documents.""" class PostgresGraphStore(GraphStore): @@ -65,8 +67,8 @@ def insert_relation( self._db.entities.insert_relation(source_id, target_id, doc_id, chunk_id, relation) def get_related_entity_names( - self, entity_names: list[str], max_results: int = 20 + self, entity_names: list[str], max_results: int = 20, *, scope: Scope ) -> list[str]: - return self._db.entities.related_names(entity_names, max_results=max_results) + return self._db.entities.related_names(entity_names, max_results=max_results, scope=scope) diff --git a/src/rag/retrieval.py b/src/rag/retrieval.py index 03dac4fb..a4f86742 100644 --- a/src/rag/retrieval.py +++ b/src/rag/retrieval.py @@ -416,7 +416,7 @@ def retrieve_context( if config.GRAPH_RAG_ENABLED: try: from ..graph.expander import QueryExpander - extra_terms = QueryExpander().expand(query_clean, db) + extra_terms = QueryExpander().expand(query_clean, db, scope=scope) if extra_terms: query_clean = query_clean + " " + " ".join(extra_terms) logger.debug(f"[GraphRAG] Expanded query with: {extra_terms}") diff --git a/tests/integration/test_graph_expansion_scope.py b/tests/integration/test_graph_expansion_scope.py new file mode 100644 index 00000000..8029f677 --- /dev/null +++ b/tests/integration/test_graph_expansion_scope.py @@ -0,0 +1,75 @@ +"""R-12 (QP-9 review) against Postgres — GraphRAG's expansion reads only the caller's workspace. + +Entities are shared across workspaces, keyed on name and type alone, so the 1-hop lookup +returned names that co-occurred in any workspace's documents, retired ones included. With +GraphRAG on, those names were appended to the caller's query. Now a co-occurrence counts +only through a live document of the scope. +""" + +from __future__ import annotations + +import uuid +from collections.abc import Iterator +from typing import Any + +import pytest + +pytestmark = [pytest.mark.integration, pytest.mark.db] + + +@pytest.fixture(scope="module") +def database() -> Any: + from src.db import Database + + db = Database() + ok, message = db.initialize() + if not ok or not db.is_connected: + pytest.skip(f"PostgreSQL is not available: {message}") + from src.app_bootstrap import _run_alembic_migrations + + _run_alembic_migrations() + return db + + +@pytest.fixture() +def workspaces(database: Any) -> Iterator[tuple[str, str]]: + a = database.workspaces.create(f"r12-a-{uuid.uuid4().hex[:8]}", owner_id=None) + b = database.workspaces.create(f"r12-b-{uuid.uuid4().hex[:8]}", owner_id=None) + yield a, b + database.workspaces.retire(a) + database.workspaces.retire(b) + + +def _relate(database: Any, ws: str, anchor: str, other: str) -> int: + """A document in *ws* in which *anchor* and *other* co-occur; returns its id.""" + tag = uuid.uuid4().hex[:8] + doc = database.documents.insert(f"r12-{tag}.txt", f"{anchor} {other}", workspace_id=ws, scope=ws) + chunk = database.documents.insert_chunks([(doc, f"{anchor} {other}", 0, [0.02] * 768)], scope=ws)[0] + database.entities.insert_relation( + database.entities.upsert(anchor, "ORG"), database.entities.upsert(other, "ORG"), doc, chunk) + return doc + + +def test_a_name_from_another_workspace_is_not_returned(database, workspaces): + a, b = workspaces + anchor = f"Anchor{uuid.uuid4().hex[:6]}" + _relate(database, a, anchor, f"{anchor}Own") + _relate(database, b, anchor, f"{anchor}Foreign") + assert database.entities.related_names([anchor], scope=a) == [f"{anchor}Own"] + + +def test_a_retired_document_contributes_nothing(database, workspaces): + a, _ = workspaces + anchor = f"Anchor{uuid.uuid4().hex[:6]}" + doc = _relate(database, a, anchor, f"{anchor}Retired") + database.documents.retire(doc, scope=a) + assert database.entities.related_names([anchor], scope=a) == [] + + +def test_a_chunk_retired_by_a_reingest_contributes_nothing(database, workspaces): + """A name removed from a document must not outlive the re-ingest that removed it.""" + a, _ = workspaces + anchor = f"Anchor{uuid.uuid4().hex[:6]}" + doc = _relate(database, a, anchor, f"{anchor}Old") + database.documents.retire_chunks(doc, scope=a) + assert database.entities.related_names([anchor], scope=a) == [] diff --git a/tests/integration/test_list_params_survive_vector_adapters.py b/tests/integration/test_list_params_survive_vector_adapters.py index ed63fd32..cdf89eb5 100644 --- a/tests/integration/test_list_params_survive_vector_adapters.py +++ b/tests/integration/test_list_params_survive_vector_adapters.py @@ -117,7 +117,7 @@ def test_entity_lookup_returns_the_co_occurring_name(self, database): a = db.entities.upsert('Ollama', 'PRODUCT') b = db.entities.upsert('Postgres', 'ORG') db.entities.insert_relation(a, b, doc, chunk_ids[0]) -names = db.entities.related_names(['Ollama'], max_results=5) +names = db.entities.related_names(['Ollama'], max_results=5, scope=ALL_WORKSPACES) assert names == ['Postgres'], names db.close() print('OK') @@ -129,9 +129,10 @@ def test_a_name_with_no_relations_returns_empty_not_an_error(self, database): """The negative case: empty is a real answer here, and must not be an exception.""" result = _run(database, """ from src.db import Database +from src.utils.scope import ALL_WORKSPACES db = Database(); db.initialize() db.entities.upsert('Solitary', 'ORG') -assert db.entities.related_names(['Solitary'], max_results=5) == [] +assert db.entities.related_names(['Solitary'], max_results=5, scope=ALL_WORKSPACES) == [] db.close() print('OK') """) diff --git a/tests/unit/test_db_entities_memories.py b/tests/unit/test_db_entities_memories.py index 6d44c393..0d638a13 100644 --- a/tests/unit/test_db_entities_memories.py +++ b/tests/unit/test_db_entities_memories.py @@ -32,6 +32,8 @@ from src.utils.scope import ALL_WORKSPACES +WS = "11111111-1111-1111-1111-111111111111" + # --------------------------------------------------------------------------- # Helpers — build a mixin instance with a mocked get_connection # --------------------------------------------------------------------------- @@ -131,20 +133,37 @@ def test_custom_relation_and_weight(self): class TestGetRelatedEntityNames: def test_returns_empty_for_empty_input(self): m, _, _ = _entities_mixin() - result = m.entities.related_names([]) + result = m.entities.related_names([], scope=WS) assert result == [] m.get_connection.assert_not_called() def test_returns_empty_when_db_unavailable(self): m, _, _ = _entities_mixin(connected=False) - result = m.entities.related_names(["Apple"]) + result = m.entities.related_names(["Apple"], scope=WS) assert result == [] def test_returns_names_from_db(self): m, _, cur = _entities_mixin(fetchall_return=[("Google",), ("Microsoft",)]) - result = m.entities.related_names(["Apple"]) + result = m.entities.related_names(["Apple"], scope=WS) assert result == ["Google", "Microsoft"] + def test_only_the_scopes_live_documents_are_read(self): + """R-12: a co-occurrence counts only through a live document of the scope.""" + m, _, cur = _entities_mixin(fetchall_return=[]) + m.entities.related_names(["Apple"], max_results=7, scope=WS) + statement, params = cur.execute.call_args.args + assert ( + m.get_connection.call_args.kwargs["scope"], + "d.deleted_at IS NULL" in statement and "c.deleted_at IS NULL" in statement + and "AND d.workspace_id = %s" in statement, + params, + ) == (WS, True, (["Apple"], ["Apple"], WS, 7)) + + def test_a_missing_scope_is_refused(self): + m, _, _ = _entities_mixin() + with pytest.raises(ValueError): + m.entities.related_names(["Apple"], scope=None) + # =========================================================================== # MemoriesRepository diff --git a/tests/unit/test_graph_modules.py b/tests/unit/test_graph_modules.py index 54ad8d1b..291b3952 100644 --- a/tests/unit/test_graph_modules.py +++ b/tests/unit/test_graph_modules.py @@ -17,6 +17,9 @@ # Helpers # --------------------------------------------------------------------------- +WS = "11111111-1111-1111-1111-111111111111" + + def _make_db(connected=True): db = MagicMock() db.is_connected = connected @@ -256,21 +259,21 @@ def _expander(self): def test_returns_empty_when_nlp_unavailable(self): e = self._expander() with patch("src.graph.expander._get_nlp", return_value=None): - result = e.expand("Apple acquires startup", _make_db()) + result = e.expand("Apple acquires startup", _make_db(), scope=WS) assert result == [] def test_returns_empty_when_db_not_connected(self): e = self._expander() mock_nlp = _make_mock_nlp([("Apple", "ORG")]) with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("Apple acquires startup", _make_db(connected=False)) + result = e.expand("Apple acquires startup", _make_db(connected=False), scope=WS) assert result == [] def test_returns_empty_when_no_entities_in_query(self): e = self._expander() mock_nlp = _make_mock_nlp([]) # no entities with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("what is this?", _make_db()) + result = e.expand("what is this?", _make_db(), scope=WS) assert result == [] def test_returns_related_entity_names(self): @@ -279,7 +282,7 @@ def test_returns_related_entity_names(self): db = _make_db() db.entities.related_names.return_value = ["iPhone", "Tim Cook"] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("Apple acquires startup", db) + result = e.expand("Apple acquires startup", db, scope=WS) assert result == ["iPhone", "Tim Cook"] def test_db_failure_returns_empty(self): @@ -288,14 +291,14 @@ def test_db_failure_returns_empty(self): db = _make_db() db.entities.related_names.side_effect = Exception("DB error") with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("Apple acquires startup", db) + result = e.expand("Apple acquires startup", db, scope=WS) assert result == [] def test_nlp_exception_returns_empty(self): e = self._expander() mock_nlp = MagicMock(side_effect=RuntimeError("NLP crash")) with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("some query", _make_db()) + result = e.expand("some query", _make_db(), scope=WS) assert result == [] def test_respects_max_extra(self): @@ -304,7 +307,7 @@ def test_respects_max_extra(self): db = _make_db() db.entities.related_names.return_value = ["A", "B"] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - e.expand("Apple text", db, max_extra=3) + e.expand("Apple text", db, max_extra=3, scope=WS) # Verify max_extra is forwarded to the DB call call_kwargs = db.entities.related_names.call_args assert call_kwargs.kwargs.get("max_results") == 3 or 3 in call_kwargs.args @@ -317,7 +320,7 @@ def test_filters_entities_by_length(self): db = _make_db() db.entities.related_names.return_value = [] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("X Corp", db) + result = e.expand("X Corp", db, scope=WS) # No entities passed the length filter → no DB call assert result == [] @@ -328,7 +331,7 @@ def test_filters_entity_at_lower_boundary_2_chars_kept(self): db = _make_db() db.entities.related_names.return_value = ["related"] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("Hi Corp", db) + result = e.expand("Hi Corp", db, scope=WS) db.entities.related_names.assert_called_once() assert db.entities.related_names.call_args.args[0] == ["Hi"] assert result == ["related"] @@ -341,7 +344,7 @@ def test_filters_entity_at_upper_boundary_200_chars_kept(self): db = _make_db() db.entities.related_names.return_value = ["related"] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("query", db) + result = e.expand("query", db, scope=WS) db.entities.related_names.assert_called_once() assert db.entities.related_names.call_args.args[0] == [entity_text] assert result == ["related"] @@ -353,7 +356,7 @@ def test_filters_entity_over_201_chars_excluded(self): mock_nlp = _make_mock_nlp([(entity_text, "ORG")]) db = _make_db() with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("query", db) + result = e.expand("query", db, scope=WS) db.entities.related_names.assert_not_called() assert result == [] @@ -365,10 +368,19 @@ def test_and_not_or_between_label_and_length_checks(self): mock_nlp = _make_mock_nlp([("Quickly Indeed", "ADV")]) db = _make_db() with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - result = e.expand("query", db) + result = e.expand("query", db, scope=WS) db.entities.related_names.assert_not_called() assert result == [] + def test_the_lookup_is_confined_to_the_callers_workspace(self): + """R-12: the scope reaches the graph lookup unchanged.""" + e = self._expander() + db = _make_db() + db.entities.related_names.return_value = [] + with patch("src.graph.expander._get_nlp", return_value=_make_mock_nlp([("Apple", "ORG")])): + e.expand("Apple text", db, scope=WS) + assert db.entities.related_names.call_args.kwargs["scope"] == WS + def test_default_max_extra_is_five(self): """No explicit max_extra override — the default value (5) must be forwarded.""" e = self._expander() @@ -376,6 +388,6 @@ def test_default_max_extra_is_five(self): db = _make_db() db.entities.related_names.return_value = [] with patch("src.graph.expander._get_nlp", return_value=mock_nlp): - e.expand("Apple text", db) + e.expand("Apple text", db, scope=WS) call_kwargs = db.entities.related_names.call_args assert call_kwargs.kwargs.get("max_results") == 5 or 5 in call_kwargs.args diff --git a/tests/unit/test_object_authorization_matrix.py b/tests/unit/test_object_authorization_matrix.py index 2259736e..a89b139e 100644 --- a/tests/unit/test_object_authorization_matrix.py +++ b/tests/unit/test_object_authorization_matrix.py @@ -80,6 +80,7 @@ "conversations.low_confidence_queries", "connectors.list_all", "entities.workspace_ontology", + "entities.related_names", "memories.search", "memories.is_duplicate", "memories.list_page",