diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 35a61bc..7d44473 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -489,13 +489,20 @@ async def create_rule( order_index: Display order within the topic (default 0). force: Bypass the near-duplicate gate. By default, a title-identical rule already in this topic BLOCKS creation and returns its id so you update - it instead. Set true only for a genuinely distinct rule. + it instead. Set true only for a genuinely distinct rule. A rule or + preference that answers the same MOMENT under another title does + not block: the create goes through and the reply carries + `overlaps` and `overlap_note` — read the top one and decide. """ uid = current_user_id() if not force: dup = await dedup_svc.find_duplicate_rule(title, topic_id=topic_id) if dup is not None: return dedup_svc.duplicate_response(dup, "rule") + # Before the create, so the new rule cannot find itself (#4134). + overlaps = await dedup_svc.find_overlapping_rules( + uid, title, statement, when_to_apply, + ) rule = await rulebooks_svc.create_rule( topic_id=topic_id, user_id=uid, title=title, statement=statement, when_to_apply=when_to_apply, @@ -503,7 +510,9 @@ async def create_rule( why=why, how_to_apply=how_to_apply, order_index=order_index, verify_with=verify_with, expires_when=expires_when, ) - return await rulebooks_svc.rule_detail(uid, rule, system_ids) + data = await rulebooks_svc.rule_detail(uid, rule, system_ids) + data.update(dedup_svc.overlap_response(overlaps, "rule")) + return data async def create_project_rule( @@ -582,7 +591,9 @@ async def create_project_rule( order_index: Display order within the project's rule list (default 0). force: Bypass the near-duplicate gate. By default, a title-identical rule already on this project BLOCKS creation and returns its id so you - update it instead. Set true only for a genuinely distinct rule. + update it instead. Set true only for a genuinely distinct rule. An + overlap by meaning never blocks; it arrives as `overlaps` and + `overlap_note` on the reply — see create_rule. """ uid = current_user_id() derived_title = title.strip() or statement.strip().split(".")[0][:50] @@ -590,6 +601,9 @@ async def create_project_rule( dup = await dedup_svc.find_duplicate_rule(derived_title, project_id=project_id) if dup is not None: return dedup_svc.duplicate_response(dup, "rule") + overlaps = await dedup_svc.find_overlapping_rules( + uid, derived_title, statement, when_to_apply, project_id=project_id, + ) rule = await rulebooks_svc.create_project_rule( project_id=project_id, user_id=uid, title=derived_title, statement=statement, when_to_apply=when_to_apply, @@ -597,7 +611,9 @@ async def create_project_rule( why=why, how_to_apply=how_to_apply, order_index=order_index, verify_with=verify_with, expires_when=expires_when, ) - return await rulebooks_svc.rule_detail(uid, rule, system_ids) + data = await rulebooks_svc.rule_detail(uid, rule, system_ids) + data.update(dedup_svc.overlap_response(overlaps, "rule")) + return data async def update_rule( @@ -784,7 +800,10 @@ async def create_preference( a preference could only be filed after the fact (#4249). force: Bypass the near-duplicate gate. For a genuinely distinct preference, not for one that is "mostly" different — a mostly - different preference is an update. + different preference is an update. A RULE that already answers + this moment comes back as `overlaps` / `overlap_note` on the reply + rather than blocking; if it says the same thing, the preference is + the weaker copy of it and should go. """ uid = current_user_id() if not when_to_apply.strip(): @@ -803,13 +822,18 @@ async def create_preference( dup = await dedup_svc.find_duplicate_rule(title, topic_id=topic_id) if dup is not None: return dedup_svc.duplicate_response(dup, "rule") + overlaps = await dedup_svc.find_overlapping_rules( + uid, title, statement, when_to_apply, + ) rule = await rulebooks_svc.create_rule( topic_id=topic_id, user_id=uid, title=title, statement=statement, when_to_apply=when_to_apply, kind="preference", arose_from_id=arose_from_id, why=why, how_to_apply=how_to_apply, order_index=order_index, ) - return await rulebooks_svc.rule_detail(uid, rule, system_ids) + data = await rulebooks_svc.rule_detail(uid, rule, system_ids) + data.update(dedup_svc.overlap_response(overlaps, "preference")) + return data async def update_preference( diff --git a/src/scribe/services/dedup.py b/src/scribe/services/dedup.py index 24bb184..90101e6 100644 --- a/src/scribe/services/dedup.py +++ b/src/scribe/services/dedup.py @@ -772,10 +772,17 @@ async def find_duplicate_rule( topic_id: int | None = None, project_id: int | None = None, ) -> DuplicateMatch | None: - """Title-based near-duplicate of a rule, scoped to the same topic (a rulebook - rule) or the same project (a project rule). Rules aren't a semantic-retrieval - surface, so a normalized-title match is the right (and only) signal. Fail-open - like find_duplicate_note.""" + """Title-identical rule in the same topic (a rulebook rule) or the same + project (a project rule) — the one signal certain enough to BLOCK on. + Fail-open like find_duplicate_note. + + This is not the only duplicate signal for rules. It said so until #4134 — + "rules aren't a semantic-retrieval surface" — which stopped being true + when rules were embedded (rule_document, semantic_search_rules), and a + title is the field LEAST likely to collide when someone is deliberately + writing a second record about the same moment. find_overlapping_rules is + the meaning half; it surfaces rather than blocks, for the reason recorded + above _RULE_OVERLAP_FLOOR.""" norm = " ".join((title or "").split()).lower() if not norm or (topic_id is None and project_id is None): return None @@ -797,6 +804,128 @@ async def find_duplicate_rule( return None + +# --- rule / preference overlap (#4134) ---------------------------------------- +# Rules and preferences are one table and one ranking: every hook arm searches +# them with no `kind` filter. So a preference that restates a rule is not a +# harmless near-copy — when only the preference places, a session receives +# binding guidance labelled "preference" and treats it as optional. The title +# gate above cannot see it: a second record about the same moment is exactly +# the case where someone chose a different title. +# +# WHY THIS SURFACES INSTEAD OF BLOCKING. Measured 2026-09-21 on bge-small-en- +# v1.5, querying with the gate's own rule_document shape across 16 sampled +# records (10 preferences, 6 rules) and reading the nearest OTHER record (#4134 +# has the ids): +# +# a preference rewording an existing rule 0.850 +# nearest distinct neighbours, 16 samples 0.672 – 0.853 +# "when to delegate" beside "never delegate writing" 0.853 +# "work lands on the working branch" beside "nothing reaches +# the release branch unasked" 0.850 +# "let each action finish" beside "poll CI yourself" 0.847 +# +# The two bands overlap: records that are deliberately distinct about one +# moment — a rule for what must happen beside a rule for what must not — sit +# exactly where a true restatement does. No threshold separates them, so a +# block would refuse legitimate records and teach force=true on every create. +# What the embedding CAN say reliably is "these answer the same moment", and +# whether they say the same THING is a reading, which is the author's. So the +# create goes through and carries the records it overlaps, with what to do if +# they are the same. +# +# 0.80 is the floor because the one measured true duplicate sat at 0.850 and a +# floor at the edge of it would miss the next, slightly looser rewording; +# 6 of the 16 distinct neighbours also clear it, which is the cost, paid in +# one line on the create's reply rather than in a refused write. Retune with +# the embedder, not the corpus. +_RULE_OVERLAP_FLOOR = 0.80 +_RULE_OVERLAP_LIMIT = 3 + + +@dataclass +class RuleOverlap: + """An existing rule or preference that answers the same moment.""" + id: int + title: str + kind: str # "rule" | "preference" + project_id: int | None + similarity: float + + +async def find_overlapping_rules( + user_id: int, + title: str, + statement: str, + when_to_apply: str, + *, + project_id: int | None = None, +) -> list[RuleOverlap]: + """Existing rules AND preferences whose trigger reads as this one's. + + Queried with rule_document — the exact shape the corpus is embedded as — + so the score compares like with like. Both kinds, because the harm is + across them (#4134). + + Scope follows the new record's home. A project rule is compared with + global rules plus that project's own, the set it will rank against. A + global record (project_id None) applies everywhere, so it is compared with + every rule the caller owns: a global rule restating one project's rule is + the same overlap, arriving in that project. + + Run BEFORE the create, so the new record cannot match itself. Never + raises: an overlap is advice, and a create must not depend on it. + """ + doc_title, doc_body = embeddings_svc.rule_document(title, statement, when_to_apply) + query = "\n\n".join(p for p in (doc_title, doc_body) if p) + # The note gate's floor, for the same reason: a short document sits in a + # tight neighbourhood and resembles everything. + if len(query.strip()) < _MIN_BODY_FOR_SEMANTIC: + return [] + try: + hits = await embeddings_svc.semantic_search_rules( + user_id, query, limit=_RULE_OVERLAP_LIMIT, + threshold=_RULE_OVERLAP_FLOOR, + project_id=project_id, everywhere=project_id is None, + ) + except Exception: + logger.debug("rule overlap check skipped", exc_info=True) + return [] + return [ + RuleOverlap( + id=rule.id, title=rule.title, kind=rule.kind or "rule", + project_id=rule.project_id, similarity=round(score, 3), + ) + for score, rule in hits + ] + + +def overlap_response(overlaps: list[RuleOverlap], new_kind: str) -> dict: + """The keys a rule/preference create adds to its reply when the record it + just wrote answers the same moment as an existing one. Empty when none.""" + if not overlaps: + return {} + top = overlaps[0] + named = "; ".join( + f'{o.kind} {o.id} "{o.title}" ({o.similarity})' for o in overlaps + ) + return { + "overlaps": [ + {"id": o.id, "title": o.title, "kind": o.kind, + "project_id": o.project_id, "similarity": o.similarity} + for o in overlaps + ], + "overlap_note": ( + f"Created — and it answers the same moment as: {named}. Read " + f"{top.kind} {top.id} now. If it says the same thing, fold what is " + f"new into it (update_{top.kind}) and delete this {new_kind}: two " + f"records ranked together split one instruction, and the weaker " + f"one can arrive alone. If they say different things about one " + f"moment, keep both — that is common and fine." + ), + } + + # --- the plan gate (milestone 415) ------------------------------------------- # A session asked "what work is open?" that cannot see an existing plan makes a # second one: a new milestone beside the one that already covers the work, or diff --git a/tests/conftest.py b/tests/conftest.py index 24db63d..3058aa3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -147,3 +147,20 @@ def _no_rule_arm(): with patch("scribe.services.plugin_context.semantic_search_rules", AsyncMock(return_value=[])): yield + + +@pytest.fixture(autouse=True) +def _no_rule_overlap(): + """Stub the rule/preference create path's overlap check (#4134). + + The same reason as _no_rule_arm, one door over: every create_rule / + create_project_rule / create_preference now asks semantic_search_rules + whether an existing record answers the same moment, so each existing + rule-tool unit test would load the embedding model through a call it never + meant to make. The check's own behaviour is tested in + tests/test_rule_overlap_gate.py, which binds the real function at import + time — before this patch runs — and stubs the search beneath it instead. + """ + with patch("scribe.services.dedup.find_overlapping_rules", + AsyncMock(return_value=[])): + yield diff --git a/tests/test_rule_overlap_gate.py b/tests/test_rule_overlap_gate.py new file mode 100644 index 0000000..e563660 --- /dev/null +++ b/tests/test_rule_overlap_gate.py @@ -0,0 +1,273 @@ +"""Rule and preference creates surface the records they overlap (#4134). + +Rules and preferences share one table and one ranking, so a preference that +restates a rule splits one instruction in two — and when only the preference +places, binding guidance arrives labelled optional. The title gate could not +see it: a second record about the same moment is exactly the one written +under a different title. + +These tests pin three things: + +- WHAT is compared: the new record's rule_document (the shape the corpus is + embedded as), against BOTH kinds, in the scope the new record will rank in. +- THAT it advises rather than blocks. The measurement above + dedup._RULE_OVERLAP_FLOOR found distinct neighbours scoring as high as a + true restatement, so a block would refuse legitimate records. +- WHERE it runs: all three create doors, before the create, so the new record + cannot match itself. + +`find_overlapping_rules` is bound here at import time, before conftest's +autouse `_no_rule_overlap` replaces the module attribute, so the service tests +exercise the real function and stub the search beneath it. +""" +from __future__ import annotations + +import ast +import inspect +import pathlib +from unittest.mock import AsyncMock, patch + +import pytest + +from scribe.services import dedup +from scribe.services.dedup import ( + RuleOverlap, + find_overlapping_rules, + overlap_response, +) +from tests.helpers import fake_rule +from tests.helpers import plain_rule_detail as _plain_detail + +pytestmark = pytest.mark.usefixtures("_bind_user") + +SEARCH = "scribe.services.dedup.embeddings_svc.semantic_search_rules" +TOOLS = "scribe.mcp.tools.rulebooks" + +# Long enough to clear _MIN_BODY_FOR_SEMANTIC in the rule_document shape. +TRIGGER = ( + "About to push a commit to dev and stop to ask whether pushing is allowed, " + "or ending a turn with a question about landing work that was already " + "committed on the branch the operator treats as home." +) +STATEMENT = "Push to dev after committing without asking first." + + +# ── what is compared ────────────────────────────────────────────────────── + + +@pytest.mark.asyncio +async def test_the_query_is_the_embedded_document_shape(): + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + await find_overlapping_rules(7, "Push without asking", STATEMENT, TRIGGER) + query = search.call_args.args[1] + title, body = dedup.embeddings_svc.rule_document( + "Push without asking", STATEMENT, TRIGGER, + ) + assert query == f"{title}\n\n{body}" + assert "When to apply:" in query + + +@pytest.mark.asyncio +async def test_both_kinds_are_searched(): + """The harm is ACROSS kinds; a kind filter would hide exactly it.""" + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + await find_overlapping_rules(7, "t", STATEMENT, TRIGGER) + assert search.call_args.kwargs.get("kind") is None + + +@pytest.mark.asyncio +async def test_a_global_record_is_compared_with_every_rule_the_caller_owns(): + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + await find_overlapping_rules(7, "t", STATEMENT, TRIGGER) + kw = search.call_args.kwargs + assert kw["everywhere"] is True + assert kw["project_id"] is None + + +@pytest.mark.asyncio +async def test_a_project_rule_is_compared_with_what_it_will_rank_against(): + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + await find_overlapping_rules(7, "t", STATEMENT, TRIGGER, project_id=5) + kw = search.call_args.kwargs + assert kw["project_id"] == 5 + assert kw["everywhere"] is False + + +@pytest.mark.asyncio +async def test_the_floor_is_the_measured_one(): + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + await find_overlapping_rules(7, "t", STATEMENT, TRIGGER) + assert search.call_args.kwargs["threshold"] == dedup._RULE_OVERLAP_FLOOR + + +def test_the_floor_sits_below_the_measured_restatement(): + """A reworded duplicate of a real rule scored 0.850 (the measurement above + _RULE_OVERLAP_FLOOR). A floor at or above that misses the case the check + exists for; this fails if someone raises it there by analogy with the + blocking gates' 0.90+.""" + assert dedup._RULE_OVERLAP_FLOOR < 0.85 + + +@pytest.mark.asyncio +async def test_a_short_document_is_not_searched(): + search = AsyncMock(return_value=[]) + with patch(SEARCH, search): + out = await find_overlapping_rules(7, "t", "s", "when") + assert out == [] + search.assert_not_called() + + +@pytest.mark.asyncio +async def test_a_failed_search_lets_the_create_through(): + with patch(SEARCH, AsyncMock(side_effect=RuntimeError("no embedder"))): + out = await find_overlapping_rules(7, "t", STATEMENT, TRIGGER) + assert out == [] + + +@pytest.mark.asyncio +async def test_hits_carry_their_kind(): + hits = [ + (0.8504, fake_rule(id=1, title="`dev` is home", kind="rule")), + (0.81, fake_rule(id=9, title="Report pushes", kind="preference", project_id=3)), + ] + with patch(SEARCH, AsyncMock(return_value=hits)): + out = await find_overlapping_rules(7, "t", STATEMENT, TRIGGER) + assert out == [ + RuleOverlap(1, "`dev` is home", "rule", None, 0.85), + RuleOverlap(9, "Report pushes", "preference", 3, 0.81), + ] + + +# ── what it says ────────────────────────────────────────────────────────── + + +def test_no_overlap_adds_nothing(): + assert overlap_response([], "rule") == {} + + +def test_the_note_names_the_record_to_read_and_both_outcomes(): + out = overlap_response( + [RuleOverlap(1, "`dev` is home", "rule", None, 0.85)], "preference", + ) + assert out["overlaps"] == [{ + "id": 1, "title": "`dev` is home", "kind": "rule", + "project_id": None, "similarity": 0.85, + }] + note = out["overlap_note"] + assert "Created" in note # it did not block + assert "update_rule" in note # the top match's own door + assert "delete this preference" in note # the new record's kind + assert "keep both" in note # distinct records are legitimate + + +# ── where it runs ───────────────────────────────────────────────────────── + + +def _rule_one_overlaps(): + return [RuleOverlap(1, "`dev` is home", "rule", None, 0.85)] + + +@pytest.mark.asyncio +async def test_create_rule_checks_before_creating_and_reports(): + order: list[str] = [] + find = AsyncMock(side_effect=lambda *a, **k: order.append("find") or _rule_one_overlaps()) + create = AsyncMock(side_effect=lambda **k: order.append("create") or fake_rule(id=50)) + with patch(f"{TOOLS}.dedup_svc.find_duplicate_rule", AsyncMock(return_value=None)), \ + patch(f"{TOOLS}.dedup_svc.find_overlapping_rules", find), \ + patch(f"{TOOLS}.rulebooks_svc.create_rule", create), _plain_detail(): + from scribe.mcp.tools.rulebooks import create_rule + out = await create_rule( + topic_id=10, title="Push without asking", statement=STATEMENT, + when_to_apply=TRIGGER, + ) + assert order == ["find", "create"] + assert find.call_args.args == (7, "Push without asking", STATEMENT, TRIGGER) + assert out["id"] == 50 + assert out["overlaps"][0]["id"] == 1 + + +@pytest.mark.asyncio +async def test_create_project_rule_checks_in_its_project(): + find = AsyncMock(return_value=_rule_one_overlaps()) + with patch(f"{TOOLS}.dedup_svc.find_duplicate_rule", AsyncMock(return_value=None)), \ + patch(f"{TOOLS}.dedup_svc.find_overlapping_rules", find), \ + patch(f"{TOOLS}.rulebooks_svc.create_project_rule", + AsyncMock(return_value=fake_rule(id=51, project_id=5, topic_id=None))), \ + _plain_detail(): + from scribe.mcp.tools.rulebooks import create_project_rule + out = await create_project_rule( + project_id=5, title="Push without asking", statement=STATEMENT, + when_to_apply=TRIGGER, + ) + assert find.call_args.kwargs == {"project_id": 5} + assert "overlap_note" in out + + +@pytest.mark.asyncio +async def test_create_preference_checks_and_names_itself_a_preference(): + find = AsyncMock(return_value=_rule_one_overlaps()) + with patch(f"{TOOLS}.dedup_svc.find_duplicate_rule", AsyncMock(return_value=None)), \ + patch(f"{TOOLS}.dedup_svc.find_overlapping_rules", find), \ + patch(f"{TOOLS}.rulebooks_svc.create_rule", + AsyncMock(return_value=fake_rule(id=52, kind="preference"))), \ + _plain_detail(): + from scribe.mcp.tools.rulebooks import create_preference + out = await create_preference( + topic_id=10, title="Push without asking", statement=STATEMENT, + when_to_apply=TRIGGER, arose_from_id=42, + ) + find.assert_awaited_once() + assert "delete this preference" in out["overlap_note"] + + +@pytest.mark.asyncio +async def test_a_create_with_no_overlap_reads_as_before(): + with patch(f"{TOOLS}.dedup_svc.find_duplicate_rule", AsyncMock(return_value=None)), \ + patch(f"{TOOLS}.dedup_svc.find_overlapping_rules", AsyncMock(return_value=[])), \ + patch(f"{TOOLS}.rulebooks_svc.create_rule", + AsyncMock(return_value=fake_rule(id=53))), _plain_detail(): + from scribe.mcp.tools.rulebooks import create_rule + out = await create_rule( + topic_id=10, title="t", statement=STATEMENT, when_to_apply=TRIGGER, + ) + assert "overlaps" not in out and "overlap_note" not in out + + +CREATE_DOORS = ("create_rule", "create_project_rule", "create_preference") + + +def test_every_rule_create_door_asks(): + """Structural, so a fourth create door fails here rather than shipping + with the title gate alone. Keyed on the doors that call + find_duplicate_rule: any door gated by title must also be checked by + meaning, because the title gate is the one that cannot see this.""" + root = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe" + tree = ast.parse((root / "mcp" / "tools" / "rulebooks.py").read_text()) + title_gated, overlap_checked = set(), set() + for fn in tree.body: + if not isinstance(fn, ast.AsyncFunctionDef): + continue + for node in ast.walk(fn): + if isinstance(node, ast.Attribute): + if node.attr == "find_duplicate_rule": + title_gated.add(fn.name) + elif node.attr == "find_overlapping_rules": + overlap_checked.add(fn.name) + assert title_gated >= set(CREATE_DOORS), "registry drifted from the module" + assert title_gated <= overlap_checked, ( + f"title-gated but never checked by meaning: " + f"{sorted(title_gated - overlap_checked)}" + ) + + +def test_the_stale_premise_is_gone(): + """find_duplicate_rule claimed rules were not a semantic-retrieval surface + — false since rules were embedded, and the reason this gap survived.""" + doc = inspect.getdoc(dedup.find_duplicate_rule) or "" + assert "aren't a semantic-retrieval" not in doc + assert "find_overlapping_rules" in doc