From 188e78bbcd74dc92902a666d50acc53e954e65c4 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Tue, 15 Sep 2026 12:06:34 -0400 Subject: [PATCH] =?UTF-8?q?feat(rules):=20retrieval=20honours=20a=20rule's?= =?UTF-8?q?=20home=20=E2=80=94=20global=20everywhere,=20a=20project's=20ru?= =?UTF-8?q?les=20only=20in=20that=20project=20(#4074)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit semantic_search_rules searched every rule the user owned, and every hook arm called it without a project, so each project's rules were injected into every other project's sessions and a project rule meant nothing a session could feel. The search now takes a scope: global rules by default (an unbound session, or a caller that forgets to say), global plus project N when given project_id (N's rules only if the caller can read that project, through access.can_read_project), and every owned rule with everywhere=True. The four hook arms and the report preference lookup pass the session's project; an explicit search(content_type="rule") scopes to its project_id, or asks the whole rulebook without one. Milestone 414 step 1. Guarded by an AST walk that every hook call site passes project_id, and an integration test on real Postgres that a rule is reached only from its home. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy --- src/scribe/mcp/tools/search.py | 21 +++-- src/scribe/services/embeddings.py | 48 ++++++++--- src/scribe/services/plugin_context.py | 19 ++-- src/scribe/services/reply_preferences.py | 2 +- tests/test_integration_rule_scope.py | 105 +++++++++++++++++++++++ tests/test_mcp_tool_search.py | 19 ++++ tests/test_rule_usage_wiring.py | 69 +++++++++++++++ 7 files changed, 255 insertions(+), 28 deletions(-) create mode 100644 tests/test_integration_rule_scope.py diff --git a/src/scribe/mcp/tools/search.py b/src/scribe/mcp/tools/search.py index ace9009..8a5b6d9 100644 --- a/src/scribe/mcp/tools/search.py +++ b/src/scribe/mcp/tools/search.py @@ -18,7 +18,7 @@ from scribe.services import rulebooks as rulebooks_svc from scribe.services.retrieval_telemetry import record_retrieval, retrieval_summary -async def _search_rules(uid: int, q: str, limit: int) -> dict: +async def _search_rules(uid: int, q: str, limit: int, project_id: int) -> dict: """Rules by meaning — a separate result shape because a rule IS different. A rule hit carries `why` and `how_to_apply`: they are the operational half @@ -29,10 +29,17 @@ async def _search_rules(uid: int, q: str, limit: int) -> dict: the moment someone is about to act on a rule, and "this asserts a fact nobody has confirmed" is part of what the rule says. - Rules are not project-scoped the way notes are (a family rule belongs to no - project), so `project_id` and `system_id` do not apply here. + `project_id` scopes the way it does for notes, with one difference: a + GLOBAL rule (one in a rulebook) belongs to no project and applies in every + one, so a scoped search returns global rules plus that project's own. + Without a project it asks the whole rulebook — every rule, whatever its + home — because that is the question an unscoped "is there a rule about + this" is asking. `system_id` does not apply to rules. """ - raw = await semantic_search_rules(uid, q, limit=limit) + if project_id: + raw = await semantic_search_rules(uid, q, limit=limit, project_id=project_id) + else: + raw = await semantic_search_rules(uid, q, limit=limit, everywhere=True) return { "results": [ { @@ -84,7 +91,9 @@ async def search( Reach for 'rule' when you want to know whether a standing instruction covers something: "is there a rule about release tagging?". A hit carries the rule's `why` and `how_to_apply`, - which the session-start payload does not. + which the session-start payload does not. With a project_id, + rules come back as the global rules plus that project's own; + with 0, every rule in the rulebook. limit: maximum number of results (1-50). project_id: Scope results to one project. PASS THE ACTIVE PROJECT'S ID whenever a project is in scope (the one you entered with @@ -108,7 +117,7 @@ async def search( uid = current_user_id() limit = max(1, min(limit, 50)) if content_type == "rule": - return await _search_rules(uid, q, limit) + return await _search_rules(uid, q, limit, project_id) is_task = {"note": False, "task": True}.get(content_type) # None => any t0 = time.perf_counter() report: dict = {} diff --git a/src/scribe/services/embeddings.py b/src/scribe/services/embeddings.py index b1f1c90..6365dd6 100644 --- a/src/scribe/services/embeddings.py +++ b/src/scribe/services/embeddings.py @@ -23,7 +23,7 @@ from sqlalchemy import delete, or_, select from scribe.models import async_session from scribe.models.embedding import NoteEmbedding, RuleEmbedding from scribe.models.note import Note -from scribe.services.access import notes_visibility_clause +from scribe.services.access import can_read_project, notes_visibility_clause if TYPE_CHECKING: # resolves the Rule forward ref without importing at runtime from scribe.models.rulebook import Rule @@ -819,6 +819,9 @@ async def semantic_search_rules( threshold: float = _SIMILARITY_THRESHOLD, kind: str | None = None, report: dict | None = None, + *, + project_id: int | None = None, + everywhere: bool = False, ) -> list[tuple[float, "Rule"]]: """Return up to *limit* (score, rule) pairs most relevant to *query*. @@ -836,12 +839,26 @@ async def semantic_search_rules( reports a decline the ranker never made (#3765). ABSENT means no search touched the dict at all, which is a stand-in in a test, not a real call. - Scoped by OWNERSHIP — a rule is the caller's if they own its rulebook or - its project. Deliberately not filtered to what currently BINDS a given - project: this answers "is there a rule about this", which a person asking - wants answered across their whole rulebook. Deciding which rules bind where - is the surfacing question, and it has its own machinery - (get_applicable_rules) rather than a second, subtly different copy here. + SCOPED, and the scope is a rule's home (milestone 414). A rule lives in a + rulebook topic — GLOBAL, it applies wherever its owner works — or on one + project, where it applies to that project and nowhere else: + + - default (`project_id=None`): global rules only. A hook with no bound + project gets these, and so does any caller that forgets to say; the + safe failure is surfacing less, not another project's rules. + - `project_id=N`: global rules plus project N's own, and N's only when + the caller can read that project (access.can_read_project, so a shared + project's rules reach its collaborators too). + - `everywhere=True`: every rule the caller owns, in any home. Only for an + explicit whole-rulebook question — `search(content_type="rule")` with no + project — where "is there a rule about this" is asked across everything. + + This used to be scoped by OWNERSHIP alone, on the argument that "is there + a rule about this" wants the whole rulebook. That is still right for the + explicit ask. It was wrong for the hooks, which inject unasked: every + project's rules surfaced in every other project's sessions — one repo's + template conventions arriving while editing an unrelated one — and a + project rule meant nothing a session could feel. THERE IS NO TIER TO NARROW BY ANY MORE (milestone 394). This carried a `tier` parameter, and the arms deliberately passed nothing: filtering on it @@ -883,6 +900,17 @@ async def semantic_search_rules( distance = RuleEmbedding.embedding.cosine_distance(query_vec) try: + # topic_id XOR project_id (migration 0059), so a rule matches exactly + # one arm of whichever clause applies. Inside the try: the access + # check reads the database too, and this function fails open. + global_rule = Rulebook.owner_user_id == user_id + if everywhere: + home = or_(global_rule, Project.user_id == user_id) + elif project_id and await can_read_project(user_id, project_id): + home = or_(global_rule, Rule.project_id == project_id) + else: + home = global_rule + async with async_session() as session: rows = (await session.execute( select(Rule, distance.label("distance")) @@ -895,11 +923,7 @@ async def semantic_search_rules( Rule.deleted_at.is_(None), # No threshold predicate — see the note above # semantic_search_notes. Applied below, after the collapse. - # topic_id XOR project_id, so exactly one arm can match. - or_( - Rulebook.owner_user_id == user_id, - Project.user_id == user_id, - ), + home, *( [Rule.kind == kind] if kind else [] ), ) # Overfetch so collapsing chunks to their best row still fills diff --git a/src/scribe/services/plugin_context.py b/src/scribe/services/plugin_context.py index da3619a..63e82a6 100644 --- a/src/scribe/services/plugin_context.py +++ b/src/scribe/services/plugin_context.py @@ -855,7 +855,7 @@ async def _reserve_slot_for_preference( # be indistinguishable from one that earned its place. found = await semantic_search_rules( user_id, query, limit=1, threshold=threshold, - kind="preference", report=_rep, + kind="preference", report=_rep, project_id=project_id or None, ) fresh = [(s, r) for s, r in found if r.id not in already] # ITS OWN SOURCE, and both sides of the trade logged. #2463's own finding @@ -953,14 +953,15 @@ async def build_prompt_rule_hint( t0 = time.perf_counter() _rep: dict = {} - # NOT scoped to the project, and that is the corpus's own decision - # rather than an omission here — semantic_search_rules is scoped by - # OWNERSHIP on purpose, because "is there a rule about this" is asked - # across a whole rulebook. `project_id` below reaches the log row and - # nothing else. + # SCOPED TO THIS SESSION'S PROJECT (milestone 414): global rules plus + # the bound project's own. An unbound session (project_id 0) gets + # global rules only. This arm used to search every rule the user owned, + # so each project's rules were injected into every other project's + # sessions — this surface speaks unasked, and a whole-rulebook answer + # is only right for someone who asked the whole rulebook. hits = await semantic_search_rules( user_id, q, limit=PROMPTRULE_LIMIT, threshold=threshold, - report=_rep, + report=_rep, project_id=project_id or None, ) duration_ms = (time.perf_counter() - t0) * 1000.0 @@ -1831,7 +1832,7 @@ async def build_write_path_hint( hits = await semantic_search_rules( user_id, code or path, limit=RULEHINT_LIMIT, threshold=cfg["rule_threshold"], - report=_rep_wpr, + report=_rep_wpr, project_id=project_id or None, ) rule_ms = (time.perf_counter() - rule_t0) * 1000.0 # BAND FIRST, dedup second, and the order is the whole point (#3851). @@ -1986,7 +1987,7 @@ async def build_tool_rule_hint( hits = await semantic_search_rules( user_id, query, limit=RULEHINT_LIMIT, threshold=cfg["tool_rule_threshold"], - report=_rep_ptr, + report=_rep_ptr, project_id=project_id or None, ) duration_ms = (time.perf_counter() - t0) * 1000.0 diff --git a/src/scribe/services/reply_preferences.py b/src/scribe/services/reply_preferences.py index 7065a0d..92082b4 100644 --- a/src/scribe/services/reply_preferences.py +++ b/src/scribe/services/reply_preferences.py @@ -107,7 +107,7 @@ async def completion_preferences(user_id: int, *, project_id: int | None = None) t0 = time.perf_counter() hits = await semantic_search_rules( user_id, COMPLETION_QUERY, limit=LIMIT, threshold=threshold, - kind="preference", report=report, + kind="preference", report=report, project_id=project_id, ) hits = [(score, rule) for score, rule in hits if rule.kind == "preference"] record_retrieval( diff --git a/tests/test_integration_rule_scope.py b/tests/test_integration_rule_scope.py new file mode 100644 index 0000000..27ca02d --- /dev/null +++ b/tests/test_integration_rule_scope.py @@ -0,0 +1,105 @@ +"""Real-Postgres tests for WHERE a rule reaches (milestone 414, step 1). + +A rule lives in a rulebook topic (global) or on one project. Retrieval used to +ignore that and search every rule the user owned, so each project's rules were +injected into every other project's sessions. What a mock cannot show is the +join doing the scoping: these seed real rules with hand-made vectors and stub +only the embedder, so every rule is an equally good match and the home alone +decides what comes back. +""" +import uuid +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +import pytest_asyncio + +from scribe.models import async_session +from scribe.models.embedding import EMBEDDING_DIM, RuleEmbedding +from scribe.models.project import Project +from scribe.models.share import ProjectShare +from scribe.services import rulebooks as rulebooks_svc +from scribe.services.embeddings import CHUNKER_VERSION, semantic_search_rules +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +QUERY_VEC = [1.0] + [0.0] * (EMBEDDING_DIM - 1) + + +@pytest_asyncio.fixture +async def homes(): + """One global rule, one rule on project A, one on project B, and a + collaborator A is shared with. Every rule embeds identically to the query.""" + # Fresh users per test: every rule matches the query equally, so a rule + # left by another test would be indistinguishable from a scoping leak. + tag = uuid.uuid4().hex[:8] + async with async_session() as s: + owner = await ensure_user(s, f"rule_scope_owner_{tag}") + collaborator = await ensure_user(s, f"rule_scope_collaborator_{tag}") + a = Project(user_id=owner.id, title="Scope A") + b = Project(user_id=owner.id, title="Scope B") + s.add_all([a, b]) + await s.flush() + s.add(ProjectShare(project_id=a.id, shared_with_user_id=collaborator.id, + permission="viewer", invited_by=owner.id)) + ids = {"owner": owner.id, "collaborator": collaborator.id, "a": a.id, "b": b.id} + await s.commit() + + with patch("scribe.services.rulebooks._refresh_rule_embedding", MagicMock()): + book = await rulebooks_svc.create_rulebook(ids["owner"], "Scope house style") + topic = await rulebooks_svc.create_topic(book.id, ids["owner"], "everywhere") + glob = await rulebooks_svc.create_rule( + topic.id, ids["owner"], "Global scope rule", "Applies in every project.", + when_to_apply="always", + ) + on_a = await rulebooks_svc.create_project_rule( + ids["a"], ids["owner"], "Project A rule", "Applies to A only.", + when_to_apply="working on A", + ) + on_b = await rulebooks_svc.create_project_rule( + ids["b"], ids["owner"], "Project B rule", "Applies to B only.", + when_to_apply="working on B", + ) + + async with async_session() as s: + for rule in (glob, on_a, on_b): + s.add(RuleEmbedding( + rule_id=rule.id, chunk_index=0, embedding=QUERY_VEC, + chunk_text=rule.title, chunker_version=CHUNKER_VERSION, + )) + await s.commit() + ids.update(glob=glob.id, on_a=on_a.id, on_b=on_b.id) + return ids + + +async def _found(user_id: int, **scope) -> set[int]: + with patch("scribe.services.embeddings.get_embedding", + AsyncMock(return_value=QUERY_VEC)): + hits = await semantic_search_rules(user_id, "anything", limit=10, + threshold=0.5, **scope) + return {rule.id for _score, rule in hits} + + +async def test_retrieval_reaches_a_rule_only_from_its_home(homes): + owner = homes["owner"] + glob, on_a, on_b = homes["glob"], homes["on_a"], homes["on_b"] + + # A session bound to A: the global rule and A's own, never B's. + assert await _found(owner, project_id=homes["a"]) == {glob, on_a} + assert await _found(owner, project_id=homes["b"]) == {glob, on_b} + + # No bound project, and the default: global only. A caller that forgets + # to pass a scope surfaces less, not another project's rules. + assert await _found(owner) == {glob} + + # The explicit whole-rulebook question still reaches everything. + assert await _found(owner, everywhere=True) == {glob, on_a, on_b} + + +async def test_a_shared_project_brings_its_rules_to_a_collaborator(homes): + """Readability goes through access.can_read_project (rule 78): a viewer on + A gets A's rules. Not the owner's global rules — rulebooks are the + owner's — and not B's, which is not shared.""" + collaborator = homes["collaborator"] + assert await _found(collaborator, project_id=homes["a"]) == {homes["on_a"]} + assert await _found(collaborator, project_id=homes["b"]) == set() diff --git a/tests/test_mcp_tool_search.py b/tests/test_mcp_tool_search.py index dac241f..39f023b 100644 --- a/tests/test_mcp_tool_search.py +++ b/tests/test_mcp_tool_search.py @@ -87,3 +87,22 @@ async def test_fable_search_limit_is_clamped(): mock_search.reset_mock() await search(q="x", limit=0) assert mock_search.call_args.kwargs["limit"] == 1 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("project_id, scope", [ + (5, {"project_id": 5}), + # No project: the explicit question is asked of the whole rulebook. + (0, {"everywhere": True}), +]) +async def test_rule_search_scopes_to_the_project_it_is_given(project_id, scope): + """With a project: global rules plus that project's (milestone 414). + Without one: every rule, because an unscoped "is there a rule about this" + is asking the whole rulebook — unlike a hook, which speaks unasked.""" + _user_id_ctx.set(7) + found = AsyncMock(return_value=[]) + with patch("scribe.mcp.tools.search.semantic_search_rules", found): + await search(q="release tagging", content_type="rule", project_id=project_id) + kwargs = found.await_args.kwargs + assert {k: kwargs[k] for k in scope} == scope + assert set(kwargs) & {"project_id", "everywhere"} == set(scope) diff --git a/tests/test_rule_usage_wiring.py b/tests/test_rule_usage_wiring.py index c57fa81..7058ef3 100644 --- a/tests/test_rule_usage_wiring.py +++ b/tests/test_rule_usage_wiring.py @@ -1625,3 +1625,72 @@ async def test_an_act_arm_reports_the_bar_it_actually_searched_at(): if c.kwargs.get("source") == "pre_tool_rule"] assert len(rows) == 1 assert rows[0].kwargs["threshold"] == search.await_args.kwargs["threshold"] + + +# ── scope: a session gets global rules plus its own project's (milestone 414) ── + + +def test_every_hook_rule_search_says_which_project_it_is_for(): + """Every call site passes `project_id`, walked rather than grepped (rule 167). + + semantic_search_rules defaults to GLOBAL rules only, so an arm that forgets + the keyword does not leak another project's rules — it quietly stops + surfacing its own project's. That is the failure this pins, and it is + silent in a session: nothing errors, a project rule just never arrives. + `everywhere` is not an acceptable answer in a hook, which speaks unasked. + """ + sources = { + "src/scribe/services/plugin_context.py": 4, + "src/scribe/services/reply_preferences.py": 1, + } + for path, expected in sources.items(): + calls = [ + n for n in ast.walk(ast.parse(Path(path).read_text())) + if isinstance(n, ast.Call) + and getattr(n.func, "id", None) == "semantic_search_rules" + ] + # The count is what lets this fail: a new arm is a new call site, and + # it must be looked at rather than slip past a guard that only checks + # the calls it already knew about. + assert len(calls) == expected, ( + f"{path} has {len(calls)} rule searches, expected {expected} — a new " + f"arm must decide its scope; update this count once it passes project_id" + ) + for call in calls: + keywords = {k.arg for k in call.keywords} + assert "project_id" in keywords, ( + f"{path}:{call.lineno} searches rules without project_id, so it " + f"gets global rules only and never its own project's" + ) + assert "everywhere" not in keywords, ( + f"{path}:{call.lineno} searches every project's rules from a hook" + ) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("bound, scope", [(7, 7), (0, None)]) +async def test_the_act_arms_scope_their_search_to_the_bound_project(bound, scope): + """A bound session searches its own project; an unbound one (0) searches + global rules only, which the search spells as `project_id=None`.""" + from scribe.services import plugin_context as pc + + cfg = {"enabled": True, "threshold": 0.6, "top_k": 3, + "rule_threshold": 0.6, "tool_rule_threshold": 0.6} + tool_search = AsyncMock(return_value=[]) + with ExitStack() as stack: + stack.enter_context(patch.object( + pc, "get_writepath_config", AsyncMock(return_value=cfg))) + stack.enter_context(patch.object(pc, "semantic_search_rules", tool_search)) + stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) + stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) + await pc.build_tool_rule_hint(1, "Bash", "git push origin dev", project_id=bound) + assert tool_search.await_args.kwargs["project_id"] == scope + + prompt_search = AsyncMock(return_value=[]) + with ExitStack() as stack: + stack.enter_context(patch.object(pc, "get_setting", AsyncMock(return_value="0.6"))) + stack.enter_context(patch.object(pc, "semantic_search_rules", prompt_search)) + stack.enter_context(patch.object(pc, "record_retrieval", MagicMock())) + stack.enter_context(patch.object(pc, "record_rule_surfaced", MagicMock())) + await pc.build_prompt_rule_hint(1, "please merge to main", project_id=bound) + assert prompt_search.await_args.kwargs["project_id"] == scope