feat(acl): unify the retrieval scopes; mark shared rows in Knowledge browse
CI & Build / Python lint (push) Successful in 3s
CI & Build / Python tests (push) Failing after 31s
CI & Build / TypeScript typecheck (push) Successful in 34s
CI & Build / integration (push) Successful in 34s
CI & Build / Build & push image (push) Has been skipped
CI & Build / Python lint (push) Successful in 3s
CI & Build / Python tests (push) Failing after 31s
CI & Build / TypeScript typecheck (push) Successful in 34s
CI & Build / integration (push) Successful in 34s
CI & Build / Build & push image (push) Has been skipped
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 <owner>" on records the viewer doesn't own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLwAaV4DQEmVyn496HnEvt
This commit is contained in:
@@ -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
|
||||
Reference in New Issue
Block a user