From ef1dbdfc861c06bd327421c49eb4523fc6ed13a0 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 25 Jul 2026 23:06:52 -0400 Subject: [PATCH] feat(acl): unify the retrieval scopes; mark shared rows in Knowledge browse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #2092 and the Knowledge-browse provenance gap. The two halves of a hybrid search disagreed: the keyword half honoured shares while the semantic half was pinned to NoteEmbedding.user_id, so a shared record was findable by wording and invisible by meaning — the case a semantic search exists to serve. semantic_search_notes now scopes on Note via a `scope` parameter, and each of its five callers declares which kind of act it is: mcp/tools/search.py read the agent asked routes/search.py read the user typed it knowledge.py (semantic) read matches the keyword half beside it plugin_context.py browse nobody asked; never a one-to-one share dedup.py own a verdict that blocks a write must not hinge on another person's notes That last one is the reason this isn't a single global widening: the dedup gate returns "update the existing one instead", so matching a stranger's record would refuse a legitimate create and point at something the caller can't edit. Scope defaults to "own" so a caller that forgets is wrong in the safe direction, and an unknown scope raises rather than falling back — a typo there would be a data-exposure bug. Auto-inject keeps the browse scope, which still admits a collaborator's note via a shared project. Its menu line is the only provenance an agent sees, so a foreign hit now reads: #12 "Title" (0.71) - shared by alex, treat as a suggestion. MCP and REST search results carry shared/owner too. Knowledge browse: the feed hydrates cards from /api/knowledge/batch rather than the list route, so both paths label rows now, and KnowledgeView shows "by " on records the viewer doesn't own. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt --- frontend/src/views/KnowledgeView.vue | 19 +++ src/scribe/mcp/tools/search.py | 15 +++ src/scribe/routes/knowledge.py | 9 +- src/scribe/routes/search.py | 12 +- src/scribe/services/access.py | 47 +++++-- src/scribe/services/dedup.py | 5 + src/scribe/services/embeddings.py | 18 ++- src/scribe/services/knowledge.py | 16 +-- src/scribe/services/plugin_context.py | 20 ++- tests/test_retrieval_scopes.py | 148 +++++++++++++++++++++++ tests/test_services_access_visibility.py | 32 ++++- 11 files changed, 319 insertions(+), 22 deletions(-) create mode 100644 tests/test_retrieval_scopes.py diff --git a/frontend/src/views/KnowledgeView.vue b/frontend/src/views/KnowledgeView.vue index 307813b..7d5d54d 100644 --- a/frontend/src/views/KnowledgeView.vue +++ b/frontend/src/views/KnowledgeView.vue @@ -27,6 +27,10 @@ interface KnowledgeItem { project_id: number | null; created_at: string; updated_at: string; + // Set only when another user owns this record — their suggestion, not one of + // yours. Absent means it's yours. + shared?: boolean; + owner?: string | null; // Task-specific status?: string; priority?: string; @@ -437,6 +441,11 @@ onUnmounted(() => {
{{ tag }}
+ by {{ item.owner ?? "another user" }} {{ formatDate(item.updated_at) }} @@ -839,6 +848,16 @@ onUnmounted(() => { color: var(--color-muted); } .k-card-date { font-size: 0.72rem; color: var(--color-text-secondary); white-space: nowrap; opacity: 0.7; } +/* Only rendered for a record another user owns, so an unmarked card is + unambiguously the viewer's own. */ +.shared-tag { + font-size: 0.68rem; + padding: 0.08rem 0.35rem; + border-radius: 4px; + white-space: nowrap; + background: color-mix(in srgb, var(--color-text-secondary) 15%, transparent); + color: var(--color-text-secondary); +} /* ── Task card ──────────────────────────────────────────── */ .k-card-task { diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index 12bc32b..0c317a4 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -10,6 +10,7 @@ from __future__ import annotations import time from scribe.mcp._context import current_user_id +from scribe.services.access import owner_names_for from scribe.services.embeddings import DEFAULT_SIMILARITY_THRESHOLD, semantic_search_notes from scribe.services.retrieval_telemetry import record_retrieval @@ -42,6 +43,10 @@ async def search( Returns: {"results": [{"id", "title", "body", "is_task", "tags", "similarity"}], "total": int} + + A result marked `shared: true` with an `owner` belongs to another user — + that person's suggestion, not the operator's own record or settled practice. + Weigh it on its merits and say whose it is when you use it. """ uid = current_user_id() limit = max(1, min(limit, 50)) @@ -50,6 +55,9 @@ async def search( raw = await semantic_search_notes( uid, q, limit=limit, is_task=is_task, project_id=project_id or None, + # An explicit search reaches everything the operator may read, including + # records shared with them one-to-one. + scope="read", ) record_retrieval( user_id=uid, source="mcp_search", query=q, @@ -57,6 +65,9 @@ async def search( project_id=project_id or None, is_task=is_task, results=raw, duration_ms=(time.perf_counter() - t0) * 1000.0, ) + owners = await owner_names_for( + {int(note.user_id) for _s, note in raw if note.user_id != uid} + ) return { "results": [ { @@ -66,6 +77,10 @@ async def search( "is_task": bool(note.is_task), "tags": list(note.tags or []), "similarity": float(score), + **( + {"shared": True, "owner": owners.get(int(note.user_id))} + if note.user_id != uid else {} + ), } for score, note in raw ], diff --git a/src/scribe/routes/knowledge.py b/src/scribe/routes/knowledge.py index 901693f..3127c1a 100644 --- a/src/scribe/routes/knowledge.py +++ b/src/scribe/routes/knowledge.py @@ -5,6 +5,7 @@ from quart import Blueprint, jsonify, request from scribe.auth import get_current_user_id, login_required from scribe.routes.utils import parse_pagination +from scribe.services.access import label_shared_items logger = logging.getLogger(__name__) @@ -54,7 +55,9 @@ async def list_knowledge(): ) return jsonify({ - "items": items, + # Mark rows another user owns: this feed can be mixed-ownership, and an + # unmarked card reads as one the viewer wrote. + "items": await label_shared_items(uid, items), "total": total, "page": page, "per_page": limit, @@ -116,7 +119,9 @@ async def get_knowledge_batch(): from scribe.services.knowledge import get_knowledge_by_ids items = await get_knowledge_by_ids(uid, ids) - return jsonify({"items": items}) + # The scrolling feed hydrates its cards here, not from the list route, so the + # ownership markers have to be applied on this path too. + return jsonify({"items": await label_shared_items(uid, items)}) @knowledge_bp.route("/tags", methods=["GET"]) diff --git a/src/scribe/routes/search.py b/src/scribe/routes/search.py index 6e5254e..8d56583 100644 --- a/src/scribe/routes/search.py +++ b/src/scribe/routes/search.py @@ -3,6 +3,7 @@ import time from quart import Blueprint, jsonify, request from scribe.auth import login_required, get_current_user_id +from scribe.services.access import owner_names_for from scribe.services.embeddings import semantic_search_notes from scribe.services.retrieval_telemetry import record_retrieval @@ -36,7 +37,9 @@ async def search_route(): t0 = time.perf_counter() results = await semantic_search_notes( - uid, q, limit=limit, is_task=is_task, threshold=_REST_SEARCH_THRESHOLD + uid, q, limit=limit, is_task=is_task, threshold=_REST_SEARCH_THRESHOLD, + # The user typed this, so it reaches everything they may read. + scope="read", ) record_retrieval( user_id=uid, source="rest_search", query=q, @@ -44,6 +47,9 @@ async def search_route(): project_id=None, is_task=is_task, results=results, duration_ms=(time.perf_counter() - t0) * 1000.0, ) + owners = await owner_names_for( + {int(note.user_id) for _s, note in results if note.user_id != uid} + ) return jsonify({ "results": [ { @@ -53,6 +59,10 @@ async def search_route(): "is_task": note.is_task, "tags": note.tags or [], "similarity": score, + **( + {"shared": True, "owner": owners.get(int(note.user_id))} + if note.user_id != uid else {} + ), } for score, note in results # semantic_search_notes returns list[tuple[float, Note]] ], diff --git a/src/scribe/services/access.py b/src/scribe/services/access.py index 413b8b4..2598215 100644 --- a/src/scribe/services/access.py +++ b/src/scribe/services/access.py @@ -182,6 +182,45 @@ async def can_write_note(user_id: int, note_id: int) -> bool: # you has not earned that standing, so it waits until you go looking for it. # --------------------------------------------------------------------------- +def notes_visibility_clause(user_id: int, scope: str = "own"): + """The one place a retrieval declares how far it may see. + + Every path that returns notes picks a scope by what KIND of act it is: + + "own" — the caller's records only. For machinery whose answer must not + depend on other people: the near-duplicate gate can't block a + create because a stranger wrote something similar, and can't + point at a record the caller cannot edit. + "browse" — own + project-reachable. For passive surfaces, where an + unrequested record would read as endorsed. + "read" — everything the ACL permits. For explicit acts: a typed search, + a fetch by id. + + Defaults to the narrowest, so a new caller that forgets to choose is wrong in + the safe direction. + """ + if scope == "own": + return Note.user_id == user_id + if scope == "browse": + return browsable_notes_clause(user_id) + if scope == "read": + return readable_notes_clause(user_id) + raise ValueError(f"unknown note scope {scope!r} (own | browse | read)") + + +async def owner_names_for(user_ids: set[int]) -> dict[int, str]: + """{user_id: username} in one query. Empty input costs nothing.""" + if not user_ids: + return {} + async with async_session() as session: + rows = ( + await session.execute( + select(User.id, User.username).where(User.id.in_(user_ids)) + ) + ).all() + return {uid: name for uid, name in rows} + + def _my_group_ids(user_id: int): """The caller's group ids as a SUBQUERY, not a fetched list. @@ -267,13 +306,7 @@ async def label_shared_items(user_id: int, items: list[dict]) -> list[dict]: } if not foreign: return items - async with async_session() as session: - rows = ( - await session.execute( - select(User.id, User.username).where(User.id.in_(foreign)) - ) - ).all() - names = {uid: name for uid, name in rows} + names = await owner_names_for(foreign) for it in items: owner_id = it.get("user_id") if owner_id is not None and owner_id != user_id: diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index 980efed..252f42e 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -124,6 +124,11 @@ async def find_duplicate_note( user_id, query, project_id=project_id, is_task=is_task, orphan_only=(project_id is None), limit=3, threshold=_SEMANTIC_THRESHOLD, + # Owner-only, deliberately: this gate BLOCKS a create and tells the + # caller to update the match instead. Matching someone else's record + # would refuse their write and point them at something they may not + # be able to edit. + scope="own", ) for score, note in hits: # semantic_search_notes doesn't filter note_type — enforce it here so diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index b893345..b88e2a4 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -19,6 +19,7 @@ from sqlalchemy import delete, select from scribe.models import async_session from scribe.models.embedding import NoteEmbedding from scribe.models.note import Note +from scribe.services.access import notes_visibility_clause logger = logging.getLogger(__name__) @@ -114,12 +115,21 @@ async def semantic_search_notes( project_id: int | None = None, is_task: bool | None = None, orphan_only: bool = False, + scope: str = "own", ) -> list[tuple[float, Note]]: """Return up to *limit* (score, note) pairs most relevant to *query*. Scores are cosine similarities in [-1, 1]; only notes at or above *threshold* are returned, sorted highest-first. + `scope` ("own" | "browse" | "read", see access.notes_visibility_clause) + decides how far this may see. It exists because this one function serves + three different kinds of act: an explicit search, which should reach + everything the caller may read; passive auto-injection, which must not pull + an unrequested record into their context; and the near-duplicate gate, whose + verdict must not depend on other people's notes at all. Defaults to "own" so + a caller that forgets is wrong in the safe direction. + Ranking and the top-k cut happen in Postgres via pgvector's cosine-distance operator (`<=>`, exposed as ``Vector.cosine_distance``) backed by the HNSW index from migration 0067 — so this is an indexed ``ORDER BY ... LIMIT k`` @@ -145,11 +155,17 @@ async def semantic_search_notes( try: async with async_session() as session: + # Scope on Note, not NoteEmbedding.user_id: the embedding row belongs + # to the note's owner, so filtering it would pin every scope to "own" + # and leave shared records unreachable by meaning. stmt = ( select(Note, distance.label("distance")) .select_from(NoteEmbedding) .join(Note, NoteEmbedding.note_id == Note.id) - .where(NoteEmbedding.user_id == user_id, Note.deleted_at.is_(None)) + .where( + notes_visibility_clause(user_id, scope), + Note.deleted_at.is_(None), + ) ) if orphan_only: stmt = stmt.where(Note.project_id.is_(None)) diff --git a/src/scribe/services/knowledge.py b/src/scribe/services/knowledge.py index 1fddc30..71d987f 100644 --- a/src/scribe/services/knowledge.py +++ b/src/scribe/services/knowledge.py @@ -5,7 +5,9 @@ ACL (rules #47/#78, decision note 2094): these queries were owner-only until *found*. They now honour shares — at two different widths: - **searching** (a `q` the caller typed) uses the full read scope, so a record - shared directly with you is findable when you go looking for it; + shared directly with you is findable when you go looking for it. BOTH halves + of the hybrid search — keyword and semantic — see equally, or a record would + be findable by wording and invisible by meaning; - **browsing** (no `q`) and the facet counts beside it use the narrower browse scope: your own records plus anything in a project you can reach. @@ -174,19 +176,17 @@ async def _semantic_knowledge_search( except Exception: logger.warning("Keyword search failed", exc_info=True) - # 2. Semantic search — conceptual similarity. - # NOTE: this half is still OWNER-ONLY. `semantic_search_notes` scopes by - # `NoteEmbedding.user_id`, and it also backs auto-inject and the `search` - # MCP tool — so widening it would put another user's shared content into - # your agent context automatically. That's a product decision, not a bug - # fix, and it's tracked separately. Consequence until then: a shared record - # is findable here by WORDING (the keyword half above) but not by MEANING. + # 2. Semantic search — conceptual similarity, at the SAME scope as the + # keyword half above. Both halves of one search must see equally, or a shared + # record would be findable by wording and invisible by meaning — which is the + # case a semantic search exists to serve. semantic_notes: list[Note] = [] try: from scribe.services.embeddings import semantic_search_notes is_task_filter = True if note_type in ("task", "plan") else (False if note_type else None) candidates = await semantic_search_notes( user_id=user_id, + scope="read", query=q, limit=min(200, limit * 4), threshold=0.3, diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index 36c7191..7241838 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -25,7 +25,7 @@ from scribe.services import knowledge as knowledge_svc from scribe.services import notes as notes_svc from scribe.services import projects as projects_svc from scribe.services import rulebooks as rulebooks_svc -from scribe.services.access import label_shared_items +from scribe.services.access import label_shared_items, owner_names_for from scribe.services.embeddings import semantic_search_notes from scribe.services.retrieval_telemetry import record_retrieval from scribe.services.settings import get_setting @@ -196,6 +196,11 @@ async def build_autoinject_hint( threshold=cfg["threshold"], project_id=(project_id or None), exclude_ids=set(exclude_ids or []), + # Injection is the one retrieval nobody asked for, so it takes the BROWSE + # scope: never a record shared one-to-one with the operator. What can + # still appear is a collaborator's note inside a shared project — legible + # only because the line below names its owner. + scope="browse", ) record_retrieval( user_id=user_id, source="auto_inject", query=q, @@ -210,6 +215,13 @@ async def build_autoinject_hint( top_score = hits[0][0] kept = [(s, n) for s, n in hits if s >= top_score - _AUTOINJECT_BAND] + # A collaborator's note can reach this menu via a shared project, and the + # operator never asked for it — so say whose it is. Unattributed, it reads as + # something they wrote and settled. + owners = await owner_names_for({ + int(n.user_id) for _s, n in kept if n.user_id != user_id + }) + lines = [ "> Possibly relevant from your Scribe notes — call `get_note(id)` to " "open any in full (titles only; injected once per session):", @@ -218,7 +230,11 @@ async def build_autoinject_hint( for score, note in kept: note_ids.append(int(note.id)) title = (note.title or "(untitled)").replace("\n", " ").strip() - lines.append(f"> - #{note.id} \"{title}\" ({score:.2f})") + line = f"> - #{note.id} \"{title}\" ({score:.2f})" + if note.user_id != user_id: + who = owners.get(int(note.user_id)) or "another user" + line += f" — shared by {who}, treat as a suggestion" + lines.append(line) return {"context": "\n".join(lines), "note_ids": note_ids, "config": cfg} diff --git a/tests/test_retrieval_scopes.py b/tests/test_retrieval_scopes.py new file mode 100644 index 0000000..5eeaf29 --- /dev/null +++ b/tests/test_retrieval_scopes.py @@ -0,0 +1,148 @@ +"""Every retrieval path must declare the right visibility scope. + +`semantic_search_notes` serves three different kinds of act, and the correct +scope differs for each. Getting one wrong is silent — the code still works, it +just sees too much or too little — so each caller is pinned here: + + explicit search → "read" the caller asked; reach everything they may read + auto-injection → "browse" nobody asked; never a one-to-one shared record + near-duplicate → "own" a verdict that blocks a write can't hinge on + someone else's notes + +The keyword and semantic halves of one hybrid search must also agree, or a +shared record would be findable by wording and invisible by meaning. +""" +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest + + +def _note(id=1, user_id=7, title="A note"): + n = MagicMock() + n.id = id + n.user_id = user_id + n.title = title + n.body = "body" + n.tags = [] + n.is_task = False + n.note_type = "note" + return n + + +@pytest.fixture +def spy(): + """Patch semantic_search_notes everywhere it's imported and capture kwargs.""" + mock = AsyncMock(return_value=[]) + targets = [ + "scribe.services.embeddings.semantic_search_notes", + "scribe.services.plugin_context.semantic_search_notes", + "scribe.mcp.tools.search.semantic_search_notes", + "scribe.routes.search.semantic_search_notes", + ] + patches = [patch(t, mock) for t in targets] + for p in patches: + p.start() + yield mock + for p in patches: + p.stop() + + +@pytest.mark.asyncio +async def test_mcp_search_uses_read_scope(spy): + from scribe.mcp._context import _user_id_ctx + token = _user_id_ctx.set(7) + try: + with patch("scribe.services.retrieval_telemetry.record_retrieval", MagicMock()): + from scribe.mcp.tools.search import search + await search(q="debounce") + finally: + _user_id_ctx.reset(token) + assert spy.await_args.kwargs["scope"] == "read" + + +@pytest.mark.asyncio +async def test_auto_inject_uses_browse_scope(spy): + """The one retrieval nobody requested. A record shared one-to-one with the + operator must never arrive this way.""" + from scribe.services import plugin_context + with patch.object(plugin_context, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.55, + "top_k": 5})), \ + patch.object(plugin_context, "record_retrieval", MagicMock()): + await plugin_context.build_autoinject_hint(7, "how do I debounce") + assert spy.await_args.kwargs["scope"] == "browse" + + +@pytest.mark.asyncio +async def test_dedup_gate_uses_own_scope(spy): + """An empty title skips the title signal and goes straight to the semantic + one — no session needed. The gate blocks a create and tells the caller to + update the match instead, so matching another user's record would refuse + their write and point them at something they may not be able to edit.""" + from scribe.services import dedup + await dedup.find_duplicate_note( + 7, "", body="x" * 400, project_id=None, is_task=False, + ) + assert spy.await_args.kwargs["scope"] == "own" + + +@pytest.mark.asyncio +async def test_hybrid_search_halves_agree_on_scope(): + """The keyword half and the semantic half of one search must see equally.""" + from scribe.services import knowledge + + captured: dict[str, object] = {} + + async def fake_semantic(**kwargs): + captured["scope"] = kwargs.get("scope") + return [] + + from scribe.models.note import Note + with patch("scribe.services.embeddings.semantic_search_notes", + AsyncMock(side_effect=fake_semantic)), \ + patch.object(knowledge, "readable_notes_clause", + MagicMock(return_value=(Note.user_id == 7))) as read_clause, \ + patch.object(knowledge, "async_session") as sess: + session = AsyncMock() + session.__aenter__ = AsyncMock(return_value=session) + session.__aexit__ = AsyncMock(return_value=False) + result = MagicMock() + result.scalars.return_value.all.return_value = [] + session.execute = AsyncMock(return_value=result) + sess.return_value = session + + await knowledge.query_knowledge( + user_id=7, note_type=None, tags=[], sort="modified", + q="debounce", limit=10, offset=0, + ) + + # Keyword half took the read scope... + read_clause.assert_called_once_with(7) + # ...and so did the semantic half. + assert captured["scope"] == "read" + + +@pytest.mark.asyncio +async def test_injected_menu_attributes_another_users_note(): + """Auto-inject can surface a collaborator's note via a shared project. The + menu line is the only provenance the agent sees, so it has to name them.""" + from scribe.services import plugin_context + + mine, theirs = _note(1, user_id=7, title="Mine"), _note(2, user_id=9, title="Theirs") + with patch.object(plugin_context, "semantic_search_notes", + AsyncMock(return_value=[(0.9, theirs), (0.88, mine)])), \ + patch.object(plugin_context, "get_autoinject_config", + AsyncMock(return_value={"enabled": True, "threshold": 0.5, + "top_k": 5})), \ + patch.object(plugin_context, "record_retrieval", MagicMock()), \ + patch.object(plugin_context, "owner_names_for", + AsyncMock(return_value={9: "alex"})): + out = await plugin_context.build_autoinject_hint(7, "anything") + + lines = out["context"].splitlines() + theirs_line = next(ln for ln in lines if "#2" in ln) + mine_line = next(ln for ln in lines if "#1" in ln) + assert "shared by alex" in theirs_line + assert "suggestion" in theirs_line + # The operator's own note stays unadorned — absence of a marker is the signal. + assert "shared by" not in mine_line diff --git a/tests/test_services_access_visibility.py b/tests/test_services_access_visibility.py index d2845d2..1b9a6d0 100644 --- a/tests/test_services_access_visibility.py +++ b/tests/test_services_access_visibility.py @@ -15,7 +15,11 @@ Assertions compile each clause to SQL and inspect its shape. """ import pytest -from scribe.services.access import browsable_notes_clause, readable_notes_clause +from scribe.services.access import ( + browsable_notes_clause, + notes_visibility_clause, + readable_notes_clause, +) def _sql(clause) -> str: @@ -92,6 +96,32 @@ def test_browse_scope_is_never_the_whole_table(): assert "1 = 1" not in sql +# --- the scope resolver ------------------------------------------------------ + +def test_own_scope_is_ownership_alone(): + """The near-duplicate gate depends on this: its verdict must not turn on + another person's records, or it would refuse a write and point the caller at + something they may not be able to edit.""" + sql = _sql(notes_visibility_clause(7, "own")) + assert sql == "notes.user_id = 7" + + +def test_scope_resolver_maps_to_the_right_clauses(): + assert _sql(notes_visibility_clause(7, "browse")) == _browse() + assert _sql(notes_visibility_clause(7, "read")) == _read() + + +def test_scope_defaults_to_the_narrowest(): + """A caller that forgets to choose must be wrong in the safe direction.""" + assert _sql(notes_visibility_clause(7)) == _sql(notes_visibility_clause(7, "own")) + + +def test_unknown_scope_is_rejected_loudly(): + """Silently falling back would turn a typo into a data-exposure bug.""" + with pytest.raises(ValueError): + notes_visibility_clause(7, "everything") + + @pytest.mark.parametrize("clause_fn", [readable_notes_clause, browsable_notes_clause]) def test_clauses_are_pure_builders(clause_fn): """Synchronous and side-effect free — no coroutine, no session of their own.