From c938815405087b630ac4b8356f34342f463b5fec Mon Sep 17 00:00:00 2001 From: Cipher208 <269750686+Cipher208@users.noreply.github.com> Date: Tue, 3 Nov 2026 16:04:43 +0100 Subject: [PATCH] fix(graph): get_neighbors may not leave its owner, and stop rescanning per hop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `get_neighbors` returned a neighbour's content filtered by LAYER only — no user_id on the returned node, none on the start node. So the agent graph of one owner handed back text belonging to another: measured on the live bases, a depth-1 walk from a `default` node returned 14 rows owned by `test_hooks` in every one of the three, and a depth-2 walk in CowAgent returned 301. The owner is now taken from the start node itself, not from a new parameter — forgetting to pass one is exactly the defect being fixed. Every hop, not just the final row, must land inside `member` (same layer AND same user_id), so a foreign node cannot bridge two of the owner's nodes. A missing start node, or one in another layer, returns `[]` rather than raising: if the owner is unknown, nothing is returned. The first version of this fix was 6x slower at depth 2 on the largest user layer (0.22 s -> 1.30 s) because `IN (SELECT node_id FROM member)` was evaluated per hop. `member AS MATERIALIZED` joined on `target_id` brings that to 58.6 ms on Hermes depth 2 against 1105.8 ms for the `IN` form (identical row sets), and depth 3 from 27.2 s to 7.0 s. Without `MATERIALIZED` the join is inlined into the recursive step and re-scans `epi_nodes` — 4.7 s, worse than what it replaced — so the hint is load-bearing, not decoration. Depth 1 pays 3.6 ms for it. Only tests call this method today (`rag/multi_source.py` and `rag/edm.py` use their own SQL and already scope user_id on the returned node), so the leak was latent — but it is a public method and would have returned foreign content the first day anything called it. --- graph/epistemic.py | 43 +++++++++-- .../test_graph/test_neighbors_owner_scope.py | 74 +++++++++++++++++++ 2 files changed, 109 insertions(+), 8 deletions(-) create mode 100644 tests/test_graph/test_neighbors_owner_scope.py diff --git a/graph/epistemic.py b/graph/epistemic.py index 2c98a67..94c00fd 100644 --- a/graph/epistemic.py +++ b/graph/epistemic.py @@ -260,31 +260,58 @@ async def _query_nodes(self, sql: str, params: tuple[Any, ...]) -> list[Epistemi return [self._row_to_node(r) for r in rows] async def get_neighbors(self, node_id: int, depth: int = 1, relation: str | None = None, tag: str | None = None) -> list[dict[str, Any]]: - """Neighbors via recursive CTE. + """Neighbors via recursive CTE, scoped to the start node's owner. A2.4: optional `relation` and edge-`tag` (substring match on the edge's JSON tags — `_inverse`/`_value_regex`). + + D2: the owner is read from the start node itself, never from a + parameter — a caller that forgets a `user_id` argument is exactly the + defect this scoping closes, and a layer-only filter answered with the + text of a node belonging to someone else. A start node that is missing + or lives in another layer leaves `owner` empty, so the CTE yields + nothing: not knowing whose neighbours these are means returning none. """ - edge_filter = "WHERE e.source_id = ?" - params_list: list[Any] = [node_id] + owner_params: list[Any] = [node_id, self.layer, self.layer] + edge_filter = "e.source_id = ?" + edge_params: list[Any] = [node_id] if relation: edge_filter += " AND e.relation = ?" - params_list.append(relation) + edge_params.append(relation) + # `member` is the owner's node set in this layer, and every hop is + # checked against it: filtering only the returned rows would still let + # a foreign node bridge two of the owner's own nodes. + # + # D2 perf: a JOIN against `member` beats `target_id IN (SELECT ...)` + # on the user layer (hermes depth-2: ~1.2 s -> ~0.12 s), but only + # because `MATERIALIZED` pins it: without it SQLite inlines the CTE + # into the recursive step and rescans epi_nodes on every hop (measured + # ~4.7 s at the same depth). The owner+layer guarantee is unchanged — + # a hop still has to land on a node in `member`. sql = f""" - WITH RECURSIVE graph AS ( + WITH RECURSIVE + owner AS ( + SELECT user_id FROM epi_nodes WHERE node_id = ? AND layer = ? + ), + member AS MATERIALIZED ( + SELECT n.node_id FROM epi_nodes n JOIN owner o ON n.user_id = o.user_id + WHERE n.layer = ? + ), + graph AS ( SELECT e.source_id, e.target_id, e.relation, e.weight, e.tags as etags, 1 as d - FROM epi_edges e {edge_filter} + FROM epi_edges e JOIN member m ON m.node_id = e.target_id + WHERE {edge_filter} UNION ALL SELECT e.source_id, e.target_id, e.relation, e.weight, e.tags as etags, g.d + 1 FROM epi_edges e JOIN graph g ON e.source_id = g.target_id + JOIN member m ON m.node_id = e.target_id WHERE g.d < ? ) SELECT n.node_id, n.content, n.node_type, n.tags, g.relation, g.weight, g.etags FROM graph g JOIN epi_nodes n ON g.target_id = n.node_id WHERE n.layer = ? """ - params_list.append(depth) - params_list.append(self.layer) + params_list = owner_params + edge_params + [depth, self.layer] rows = await self._dispatch.query(DB_NAME, sql, tuple(params_list)) out = [ { diff --git a/tests/test_graph/test_neighbors_owner_scope.py b/tests/test_graph/test_neighbors_owner_scope.py new file mode 100644 index 0000000..58cb750 --- /dev/null +++ b/tests/test_graph/test_neighbors_owner_scope.py @@ -0,0 +1,74 @@ +"""D2: get_neighbors must never leave the start node's owner. + +The owner is taken from the start node itself, not from a parameter — a +caller that forgot to pass `user_id` is exactly the defect being fixed. +""" + +import asyncio + +import pytest + +from shared.connection import connection_manager + + +@pytest.fixture() +def hermetic_graph(tmp_path, monkeypatch): + monkeypatch.setattr(connection_manager, "base_dir", tmp_path) + connection_manager._conns.clear() + from shared.migrations import migration_manager + + asyncio.run(migration_manager.migrate()) + from graph.epistemic import EpistemicGraph + + yield EpistemicGraph(layer="user", cm=connection_manager), tmp_path + connection_manager._conns.clear() + + +async def test_get_neighbors_never_returns_another_owners_nodes(hermetic_graph): + """A layer-only filter hands `u1` the text of `u2`'s node.""" + g, _ = hermetic_graph + src = await g.add_node("u1", "u1 hub", "fact") + mine = await g.add_node("u1", "u1 neighbor", "fact") + foreign = await g.add_node("u2", "u2 secret", "fact") + await g.add_edge(src, mine, "knows") + await g.add_edge(src, foreign, "knows") + + nbrs = await g.get_neighbors(src) + ids = {n["id"] for n in nbrs} + assert ids == {mine}, "only the start node's owner may be returned" + assert all("u2 secret" not in n["content"] for n in nbrs) + + +async def test_get_neighbors_does_not_walk_through_another_owner(hermetic_graph): + """A foreign node must not act as a bridge into the owner's own graph.""" + g, _ = hermetic_graph + src = await g.add_node("u1", "u1 start", "fact") + foreign = await g.add_node("u2", "u2 bridge", "fact") + beyond = await g.add_node("u1", "u1 beyond the bridge", "fact") + await g.add_edge(src, foreign, "knows") + await g.add_edge(foreign, beyond, "knows") + + nbrs = await g.get_neighbors(src, depth=2) + assert nbrs == [], "traversal may not cross another owner to reach a node" + + +async def test_get_neighbors_start_node_in_another_layer_returns_nothing(hermetic_graph): + """Fail closed: an unknown owner means nothing is returned, not everything.""" + from graph.epistemic import EpistemicGraph + + g, _ = hermetic_graph + user_node = await g.add_node("u1", "user node", "fact") + agent = EpistemicGraph(layer="agent", cm=connection_manager) + agent_node = await agent.add_node("u1", "agent only", "fact") + # An edge from the other layer's node would leak a user-layer node if the + # layer of the start node were not checked too. + await agent.add_edge(agent_node, user_node, "mentions") + + assert await g.get_neighbors(agent_node) == [] + + +async def test_get_neighbors_missing_start_node_returns_nothing(hermetic_graph): + g, _ = hermetic_graph + await g.add_node("u1", "lone node", "fact") + + assert await g.get_neighbors(10**9) == []