feat(rules): retrieval honours a rule's home — global everywhere, a project's rules only in that project (#4074)
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / integration (push) Successful in 59s
CI & Build / Python tests (push) Successful in 1m37s
CI & Build / Build & push image (push) Successful in 31s
CI & Build / Python lint (push) Successful in 4s
CI & Build / Plugin hooks (push) Successful in 9s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / integration (push) Successful in 59s
CI & Build / Python tests (push) Successful in 1m37s
CI & Build / Build & push image (push) Successful in 31s
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01821k5B3Ysecp9fNYs92Kuy
This commit is contained in:
@@ -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 = {}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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()
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user