CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 8s
CI & Build / TypeScript typecheck (push) Successful in 12s
CI & Build / integration (push) Successful in 18s
CI & Build / Python tests (push) Successful in 48s
CI & Build / Build & push image (push) Successful in 28s
#2462, both decided halves. ## The ACL defect list_notes filtered on Note.user_id == user_id with no scope parameter at all — never a per-call decision, the capability was absent. query_knowledge beside it was deliberately browse-scoped with a comment saying why. So a task in a shared project was invisible in list_tasks, enter_project's open-task list, the SessionStart todo count and the web UI's task views, while the same project's notes and snippets appeared. Now the shared clause: notes_visibility_clause(user_id, "browse"). Browse and not read, per decision #2094 — an ambient list must never surface a record someone shared one-to-one; those stay search-only. This is the remaining half of a fix made twice before (#2159 widened fetch, #2092 widened meaning), and the guard below is what stops a fourth half appearing. Guarded by source inspection in test_retrieval_scopes: every list-shaped service references a shared visibility clause AND carries no bare Note.user_id comparison that would quietly re-narrow it. An owner-only list returns correct-looking rows and simply omits the shared ones — the shape no behavioural test catches. ## q is semantic (operator: "make it match") The UI's note list keyword-matched while the UI's Browse search semantic-matched, over the same records, with nothing saying so. Now one meaning: q joins the embedding index and orders by cosine distance at the interactive floor, with every lifecycle filter still applied in the same indexed query. Relevance ordering wins over `sort` when q is present — a query is a relevance claim, and sorting its results by date would shuffle the answer. ILIKE survives only as the embedder-down fallback: degraded, never empty. Stated position: superseded records are NOT demoted in list-q. The penalty reorders a top-k; reordering a paginated, counted list would make page boundaries lie. The search surfaces carry the demotion. ## The interactive floor becomes one constant INTERACTIVE_SEARCH_THRESHOLD = 0.3 in embeddings.py, consumed by routes/search.py (was a commented constant), knowledge.py (was a bare literal), and the new list-q. This closes #2463's finding 3 early — the same number lived in two files with the reasoning attached to only one. ## Deferred, deliberately The return-shape unification (ORM objects vs dicts) stays undone. It is a 14-caller refactor whose motivation — callers being unable to swap paths — shrinks now that both paths share the clause and the meaning of q. If swap pressure recurs it deserves its own change, not a rider on an ACL fix. Refs #2462, #2463
248 lines
10 KiB
Python
248 lines
10 KiB
Python
"""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
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def _no_supersession():
|
|
"""The auto-inject menu now asks which of its lines are superseded (#278).
|
|
|
|
That is a real database call on a path these tests exercise without one.
|
|
Stubbed to "nothing superseded" — the ordinary state — rather than hidden
|
|
behind a try/except in the product, which would make the code lie about
|
|
what it does. The label's own behaviour is covered in
|
|
tests/test_supersession_ranking.py.
|
|
"""
|
|
with patch("scribe.services.plugin_context.superseded_ids",
|
|
AsyncMock(return_value=set())):
|
|
yield
|
|
|
|
|
|
|
|
def _note(id=1, user_id=7, title="A note", note_type="note",
|
|
is_task=False, task_kind="work"):
|
|
n = MagicMock()
|
|
n.id = id
|
|
n.user_id = user_id
|
|
n.title = title
|
|
n.body = "body"
|
|
n.tags = []
|
|
# Real values, not auto-attributes: the menu reads these to label each line,
|
|
# and a MagicMock is truthy — every record would render as a task (note 2109).
|
|
n.is_task = is_task
|
|
n.task_kind = task_kind
|
|
n.note_type = note_type
|
|
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
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_injected_menu_labels_the_record_kind():
|
|
"""Every injected line renders the same shape, so without a kind marker a
|
|
recorded snippet is indistinguishable from a stray dev-log — exactly where
|
|
prior art most needs to stand out."""
|
|
from scribe.services import plugin_context
|
|
|
|
hits = [
|
|
(0.92, _note(1, title="debounce — rate-limit a callback", note_type="snippet")),
|
|
(0.91, _note(2, title="Release checklist", note_type="process")),
|
|
(0.90, _note(3, title="Auth token expiry", is_task=True, task_kind="issue")),
|
|
(0.89, _note(4, title="Ship the drafter", is_task=True)),
|
|
(0.88, _note(5, title="Why we dropped CalDAV")),
|
|
]
|
|
with patch.object(plugin_context, "semantic_search_notes",
|
|
AsyncMock(return_value=hits)), \
|
|
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()):
|
|
out = await plugin_context.build_autoinject_hint(7, "anything")
|
|
|
|
lines = out["context"].splitlines()
|
|
by_id = {n: next(ln for ln in lines if f"#{n} " in ln) for n in (1, 2, 3, 4, 5)}
|
|
assert "[snippet]" in by_id[1]
|
|
assert "[process]" in by_id[2]
|
|
# Task-ness wins over note_type, and an issue says so rather than "task".
|
|
assert "[issue]" in by_id[3]
|
|
assert "[task]" in by_id[4]
|
|
assert "[note]" in by_id[5]
|
|
# Still title-first: the marker is metadata, not an excuse to carry bodies.
|
|
assert "body" not in out["context"]
|
|
# The header can't claim they're all notes when the markers say otherwise.
|
|
assert "Scribe records" in lines[0]
|
|
|
|
|
|
def test_list_shaped_services_use_the_shared_visibility_clause():
|
|
"""Every service that LISTS notes must declare its reach through
|
|
notes_visibility_clause / browsable_notes_clause — never a bare owner
|
|
filter (#47, #2462).
|
|
|
|
Source inspection, because this is the shape no behavioural test catches:
|
|
an owner-only list returns correct-looking rows and simply omits the shared
|
|
ones, which is how list_notes shipped owner-only for months while
|
|
query_knowledge beside it was deliberately browse-scoped. A task in a
|
|
shared project was invisible in list_tasks, enter_project and the web UI
|
|
while the same project's notes appeared.
|
|
"""
|
|
import ast
|
|
import inspect
|
|
import textwrap
|
|
|
|
from scribe.services import knowledge, notes
|
|
|
|
for fn, label in ((notes.list_notes, "notes.list_notes"),
|
|
(knowledge.query_knowledge, "knowledge.query_knowledge")):
|
|
source = textwrap.dedent(inspect.getsource(fn))
|
|
assert (
|
|
"notes_visibility_clause" in source
|
|
or "browsable_notes_clause" in source
|
|
), (
|
|
f"{label} does not reference a shared visibility clause — a bare "
|
|
f"owner filter silently hides shared-project records (#2462)"
|
|
)
|
|
# And it must not ALSO carry a bare owner filter on the main query,
|
|
# which would quietly re-narrow whatever the clause granted.
|
|
tree = ast.parse(source)
|
|
for node in ast.walk(tree):
|
|
if (isinstance(node, ast.Compare)
|
|
and isinstance(node.left, ast.Attribute)
|
|
and node.left.attr == "user_id"
|
|
and isinstance(node.left.value, ast.Name)
|
|
and node.left.value.id == "Note"):
|
|
raise AssertionError(
|
|
f"{label} compares Note.user_id directly — reach must come "
|
|
f"from the shared clause, not a bare owner filter"
|
|
)
|