diff --git a/app.py b/app.py index 2ae5ec761..ca51f54bb 100644 --- a/app.py +++ b/app.py @@ -666,6 +666,13 @@ app.include_router(setup_session_routes( from routes.admin_wipe.admin_wipe_routes import setup_admin_wipe_routes app.include_router(setup_admin_wipe_routes(session_manager)) +# Memory Graph View (beta) — MUST be included before memory_router below. +# memory_routes.py's GET/PUT/DELETE /api/memory/{memory_id} wildcard would +# otherwise swallow GET /api/memory/graph, since Starlette matches routes in +# registration order across the whole app, not by specificity. +from routes.memory.memory_graph_routes import setup_memory_graph_routes +app.include_router(setup_memory_graph_routes(memory_manager, memory_vector=memory_vector)) + # Memory from routes.memory.memory_routes import setup_memory_routes memory_router = setup_memory_routes(memory_manager, session_manager, memory_vector=memory_vector) diff --git a/routes/memory/memory_graph_routes.py b/routes/memory/memory_graph_routes.py new file mode 100644 index 000000000..331f7e426 --- /dev/null +++ b/routes/memory/memory_graph_routes.py @@ -0,0 +1,148 @@ +# routes/memory/memory_graph_routes.py +"""Memory Graph View endpoints: read-only graph derivation plus manual +relationship (link) editing between a user's own memories. + +Kept as a separate router (not folded into memory_routes.py's wildcard-heavy +router) but MUST be included in app.py before that router — see the comment +at the include_router call site. `GET /api/memory/graph` would otherwise be +swallowed by memory_routes.py's `GET /api/memory/{memory_id}` wildcard if +that router's routes were checked first. +""" +from typing import Dict, List, Optional +import logging + +from fastapi import APIRouter, HTTPException, Query, Request + +from services.memory import MemoryManager +from src.auth_helpers import get_current_user, require_privilege, require_user +from src.memory_graph import ( + DEFAULT_MAX_EDGES_PER_NODE, + DEFAULT_MIN_SIMILARITY, + build_graph, +) + +logger = logging.getLogger(__name__) + + +def setup_memory_graph_routes(memory_manager: MemoryManager, memory_vector=None): + """Set up Memory Graph View routes.""" + router = APIRouter(prefix="/api/memory", tags=["memory-graph"]) + + def _owner(request: Request) -> Optional[str]: + return get_current_user(request) + + def _verify_memory_owner(memory: dict, user: Optional[str]): + """Raise 404 if user doesn't own this memory. Mirrors + memory_routes.py's _verify_memory_owner: strict ownership so a + legacy/null-owner memory never leaks across accounts.""" + if user is None: + return # Auth disabled + if memory.get("owner") != user: + raise HTTPException(404, "Memory not found") + + @router.get("/graph") + def get_memory_graph( + request: Request, + category: Optional[List[str]] = Query(None), + min_similarity: float = Query(DEFAULT_MIN_SIMILARITY, ge=0.0, le=1.0), + max_edges_per_node: int = Query(DEFAULT_MAX_EDGES_PER_NODE, ge=1, le=50), + include_session_edges: bool = Query(True), + include_manual_edges: bool = Query(True), + limit: int = Query(1000, ge=1, le=5000), + ): + """Return the caller's own memories as a derived node/edge graph.""" + user = require_user(request) + memories = memory_manager.load(owner=user) + return build_graph( + memories, + memory_vector, + categories=category, + min_similarity=min_similarity, + max_edges_per_node=max_edges_per_node, + include_session_edges=include_session_edges, + include_manual_edges=include_manual_edges, + limit=limit, + ) + + @router.get("/graph/{memory_id}/neighbors") + def get_memory_graph_neighbors( + request: Request, + memory_id: str, + min_similarity: float = Query(DEFAULT_MIN_SIMILARITY, ge=0.0, le=1.0), + max_edges_per_node: int = Query(DEFAULT_MAX_EDGES_PER_NODE, ge=1, le=50), + ): + """Lazy drill-down: one node plus its immediate derived neighbors. + + For graphs too large to render whole (see build_graph's `limit`/ + `truncated`), the frontend can expand a single node on demand instead + of the server ever needing to compute/return the entire graph. + """ + user = require_user(request) + memories = memory_manager.load(owner=user) + target = next((m for m in memories if m.get("id") == memory_id), None) + if target is None: + raise HTTPException(404, "Memory not found") + _verify_memory_owner(target, user) + + full = build_graph( + memories, + memory_vector, + min_similarity=min_similarity, + max_edges_per_node=max_edges_per_node, + limit=len(memories) or 1, + ) + neighbor_ids = { + (e["target"] if e["source"] == memory_id else e["source"]) + for e in full["edges"] + if memory_id in (e["source"], e["target"]) + } + neighbor_ids.add(memory_id) + nodes = [n for n in full["nodes"] if n["id"] in neighbor_ids] + edges = [e for e in full["edges"] if e["source"] in neighbor_ids and e["target"] in neighbor_ids] + return {"nodes": nodes, "edges": edges, "meta": {"node_count": len(nodes), "edge_count": len(edges)}} + + @router.post("/{memory_id}/links") + def add_memory_link(request: Request, memory_id: str, target_id: str = Query(...)): + """Create an explicit manual relationship between two of the caller's + own memories (the Memory Graph View's "draw a link" affordance).""" + user = require_privilege(request, "can_manage_memory") + if target_id == memory_id: + raise HTTPException(400, "A memory cannot link to itself") + + all_mem = memory_manager.load_all() + source = next((m for m in all_mem if m.get("id") == memory_id), None) + if source is None: + raise HTTPException(404, "Memory not found") + _verify_memory_owner(source, user) + target = next((m for m in all_mem if m.get("id") == target_id), None) + if target is None: + raise HTTPException(404, "Target memory not found") + _verify_memory_owner(target, user) + + links = list(source.get("links") or []) + if target_id not in links: + links.append(target_id) + source["links"] = links + memory_manager.save(all_mem) + return {"ok": True, "links": links} + + @router.delete("/{memory_id}/links/{target_id}") + def remove_memory_link(request: Request, memory_id: str, target_id: str): + """Remove a manual relationship. Idempotent — removing a link that + doesn't exist is not an error, matching how memory delete/pin already + treat repeat calls as harmless in this codebase.""" + user = require_privilege(request, "can_manage_memory") + all_mem = memory_manager.load_all() + source = next((m for m in all_mem if m.get("id") == memory_id), None) + if source is None: + raise HTTPException(404, "Memory not found") + _verify_memory_owner(source, user) + + links = list(source.get("links") or []) + if target_id in links: + links = [l for l in links if l != target_id] + source["links"] = links + memory_manager.save(all_mem) + return {"ok": True, "links": links} + + return router diff --git a/tests/test_memory_graph_route_ordering.py b/tests/test_memory_graph_route_ordering.py new file mode 100644 index 000000000..276ce6e1f --- /dev/null +++ b/tests/test_memory_graph_route_ordering.py @@ -0,0 +1,59 @@ +"""Regression guard for the one real collision risk in this feature. + +routes/memory/memory_routes.py registers `GET/PUT/DELETE /api/memory/{memory_id}` +as a single-segment wildcard. Starlette matches routes in registration order +across the whole app, not by specificity, so `GET /api/memory/graph` would be +silently swallowed by that wildcard (memory_id="graph") if memory_router were +ever included before memory_graph_router. app.py documents and enforces the +required order; this test builds a minimal app the same way and proves a real +HTTP request resolves to the graph handler, not the wildcard 404 path — a +plain "call the endpoint function directly" test (the repo's usual route-test +style) can't catch this class of bug because it looks up routes by exact +path-string equality, not by simulating Starlette's request matching. +""" +from unittest.mock import MagicMock + +from fastapi import FastAPI +from fastapi.testclient import TestClient +from starlette.requests import Request + +from routes.memory.memory_graph_routes import setup_memory_graph_routes +from routes.memory.memory_routes import setup_memory_routes + + +def _build_app(): + app = FastAPI() + + memory_manager = MagicMock() + memory_manager.load.return_value = [] + session_manager = MagicMock() + + # Mirrors app.py: memory_graph_router included BEFORE memory_router. + app.include_router(setup_memory_graph_routes(memory_manager, memory_vector=None)) + app.include_router(setup_memory_routes(memory_manager, session_manager, memory_vector=None)) + + @app.middleware("http") + async def _fake_auth(request: Request, call_next): + request.state.current_user = "alice" + request.state.api_token = False + return await call_next(request) + + return app + + +def test_graph_route_is_not_swallowed_by_memory_id_wildcard(): + client = TestClient(_build_app()) + resp = client.get("/api/memory/graph") + assert resp.status_code == 200 + body = resp.json() + assert "nodes" in body and "edges" in body + + +def test_memory_id_wildcard_still_works_for_real_ids(): + client = TestClient(_build_app()) + resp = client.get("/api/memory/some-real-id") + # Not found (empty memory store), but resolved by the wildcard handler, + # not a 422/other error — proves the wildcard route still works normally + # once the graph route (checked first) doesn't match. + assert resp.status_code == 404 + assert resp.json()["detail"] == "Memory not found" diff --git a/tests/test_memory_graph_routes.py b/tests/test_memory_graph_routes.py new file mode 100644 index 000000000..4604f46ff --- /dev/null +++ b/tests/test_memory_graph_routes.py @@ -0,0 +1,210 @@ +"""Memory Graph View route tests. + +Follows the repo's established convention (see +tests/test_memory_routes_session_owner.py): build the router via its +setup_*_routes factory directly, monkeypatch auth helpers, look up the +target endpoint by path, and call it directly with a hand-built Request +stand-in. No TestClient/ASGI app — except in +test_memory_graph_route_ordering.py, which specifically needs real Starlette +path matching. +""" +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from fastapi import HTTPException + +import routes.memory.memory_graph_routes as mgr + + +def _route(router, path, method): + for r in router.routes: + if r.path == path and method in getattr(r, "methods", set()): + return r.endpoint + raise AssertionError(path) + + +def _request(user): + return SimpleNamespace( + state=SimpleNamespace(current_user=user, api_token=False), + app=SimpleNamespace(state=SimpleNamespace(auth_manager=None)), + client=SimpleNamespace(host="127.0.0.1"), + ) + + +def _allow_memory_management(monkeypatch, caller): + monkeypatch.setattr(mgr, "require_privilege", lambda request, key: caller) + + +def test_graph_only_returns_callers_own_memories(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + monkeypatch.setattr(mgr, "require_user", lambda request: "alice", raising=False) + memory_manager = MagicMock() + memory_manager.load.side_effect = lambda owner=None: ( + [{"id": "m1", "text": "alice's note", "owner": "alice", "category": "fact", "uses": 0, "timestamp": 1}] + if owner == "alice" else [] + ) + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + get_graph = _route(router, "/api/memory/graph", "GET") + + out = get_graph(request=_request("alice"), category=None, min_similarity=0.75, + max_edges_per_node=5, include_session_edges=True, + include_manual_edges=True, limit=1000) + + assert [n["id"] for n in out["nodes"]] == ["m1"] + memory_manager.load.assert_called_with(owner="alice") + + +def test_graph_neighbors_rejects_foreign_owned_memory(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "bob", raising=False) + monkeypatch.setattr(mgr, "require_user", lambda request: "bob", raising=False) + memory_manager = MagicMock() + memory_manager.load.return_value = [ + {"id": "victim-mem", "text": "alice secret", "owner": "alice", "category": "fact", "uses": 0, "timestamp": 1}, + ] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + neighbors = _route(router, "/api/memory/graph/{memory_id}/neighbors", "GET") + + with pytest.raises(HTTPException) as exc: + neighbors(request=_request("bob"), memory_id="victim-mem", min_similarity=0.75, max_edges_per_node=5) + assert exc.value.status_code == 404 + + +def test_graph_neighbors_returns_404_for_unknown_id(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + monkeypatch.setattr(mgr, "require_user", lambda request: "alice", raising=False) + memory_manager = MagicMock() + memory_manager.load.return_value = [] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + neighbors = _route(router, "/api/memory/graph/{memory_id}/neighbors", "GET") + + with pytest.raises(HTTPException) as exc: + neighbors(request=_request("alice"), memory_id="nope", min_similarity=0.75, max_edges_per_node=5) + assert exc.value.status_code == 404 + + +def test_graph_neighbors_scopes_to_connected_subgraph(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + monkeypatch.setattr(mgr, "require_user", lambda request: "alice", raising=False) + memory_manager = MagicMock() + memory_manager.load.return_value = [ + {"id": "a", "text": "t", "owner": "alice", "category": "fact", "uses": 0, "timestamp": 1, "session_id": "s1"}, + {"id": "b", "text": "t", "owner": "alice", "category": "fact", "uses": 0, "timestamp": 1, "session_id": "s1"}, + {"id": "c", "text": "t", "owner": "alice", "category": "fact", "uses": 0, "timestamp": 1}, + ] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + neighbors = _route(router, "/api/memory/graph/{memory_id}/neighbors", "GET") + + out = neighbors(request=_request("alice"), memory_id="a", min_similarity=0.75, max_edges_per_node=5) + + assert {n["id"] for n in out["nodes"]} == {"a", "b"} + + +def test_add_link_requires_can_manage_memory_privilege(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + + def deny(request, key): + raise HTTPException(403, "nope") + monkeypatch.setattr(mgr, "require_privilege", deny) + + memory_manager = MagicMock() + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + add_link = _route(router, "/api/memory/{memory_id}/links", "POST") + + with pytest.raises(HTTPException) as exc: + add_link(request=_request("alice"), memory_id="a", target_id="b") + assert exc.value.status_code == 403 + memory_manager.save.assert_not_called() + + +def test_add_link_rejects_foreign_owned_target(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "bob", raising=False) + _allow_memory_management(monkeypatch, "bob") + memory_manager = MagicMock() + memory_manager.load_all.return_value = [ + {"id": "bob-mem", "text": "t", "owner": "bob"}, + {"id": "alice-mem", "text": "t", "owner": "alice"}, + ] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + add_link = _route(router, "/api/memory/{memory_id}/links", "POST") + + with pytest.raises(HTTPException) as exc: + add_link(request=_request("bob"), memory_id="bob-mem", target_id="alice-mem") + assert exc.value.status_code == 404 + memory_manager.save.assert_not_called() + + +def test_add_link_rejects_self_link(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + _allow_memory_management(monkeypatch, "alice") + memory_manager = MagicMock() + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + add_link = _route(router, "/api/memory/{memory_id}/links", "POST") + + with pytest.raises(HTTPException) as exc: + add_link(request=_request("alice"), memory_id="a", target_id="a") + assert exc.value.status_code == 400 + memory_manager.save.assert_not_called() + + +def test_add_link_persists_bidirectionally_addressable_link(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + _allow_memory_management(monkeypatch, "alice") + memory_manager = MagicMock() + entries = [ + {"id": "a", "text": "t", "owner": "alice"}, + {"id": "b", "text": "t", "owner": "alice"}, + ] + memory_manager.load_all.return_value = entries + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + add_link = _route(router, "/api/memory/{memory_id}/links", "POST") + + out = add_link(request=_request("alice"), memory_id="a", target_id="b") + + assert out == {"ok": True, "links": ["b"]} + memory_manager.save.assert_called_once_with(entries) + assert entries[0]["links"] == ["b"] + + +def test_add_link_is_idempotent_when_link_already_present(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + _allow_memory_management(monkeypatch, "alice") + memory_manager = MagicMock() + entries = [ + {"id": "a", "text": "t", "owner": "alice", "links": ["b"]}, + {"id": "b", "text": "t", "owner": "alice"}, + ] + memory_manager.load_all.return_value = entries + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + add_link = _route(router, "/api/memory/{memory_id}/links", "POST") + + out = add_link(request=_request("alice"), memory_id="a", target_id="b") + + assert out["links"] == ["b"] + + +def test_remove_link_is_idempotent_when_link_absent(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "alice", raising=False) + _allow_memory_management(monkeypatch, "alice") + memory_manager = MagicMock() + memory_manager.load_all.return_value = [{"id": "a", "text": "t", "owner": "alice"}] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + remove_link = _route(router, "/api/memory/{memory_id}/links/{target_id}", "DELETE") + + out = remove_link(request=_request("alice"), memory_id="a", target_id="never-linked") + + assert out == {"ok": True, "links": []} + memory_manager.save.assert_not_called() + + +def test_remove_link_rejects_foreign_owned_source(monkeypatch): + monkeypatch.setattr(mgr, "get_current_user", lambda request: "bob", raising=False) + _allow_memory_management(monkeypatch, "bob") + memory_manager = MagicMock() + memory_manager.load_all.return_value = [{"id": "alice-mem", "text": "t", "owner": "alice", "links": ["x"]}] + router = mgr.setup_memory_graph_routes(memory_manager, memory_vector=None) + remove_link = _route(router, "/api/memory/{memory_id}/links/{target_id}", "DELETE") + + with pytest.raises(HTTPException) as exc: + remove_link(request=_request("bob"), memory_id="alice-mem", target_id="x") + assert exc.value.status_code == 404