diff --git a/src/scribe/mcp/tools/rulebooks.py b/src/scribe/mcp/tools/rulebooks.py index 6fc4f00..6d3c094 100644 --- a/src/scribe/mcp/tools/rulebooks.py +++ b/src/scribe/mcp/tools/rulebooks.py @@ -236,6 +236,13 @@ async def list_always_on_rules(project_id: int = 0) -> dict: Call this at session start. Treat the returned rules as binding for the session — they apply regardless of which project (if any) is in scope. + + Returns the ALWAYS-ON tier only (milestone 307). A `conditional` rule is + still binding when it applies; it just is not resident — it reaches a + session through enter_project (when the project works in an area the rule + is tagged to) or through search(content_type="rule"). Nothing here is a + behaviour change until rules are actually re-tiered: `tier` defaults to + always_on, so an existing rulebook returns exactly what it always did. Pair with get_project(id).applicable_rules when working on a specific project to also load that project's subscription-derived rules. diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index 1c5cde7..c24020a 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -13,6 +13,7 @@ from typing import Optional from sqlalchemy import delete as sql_delete, insert, select from scribe.models import async_session +from scribe.models.system import System from scribe.models.rulebook import Rulebook logger = logging.getLogger(__name__) @@ -223,7 +224,7 @@ async def delete_topic(topic_id: int, user_id: int) -> None: # ── Rule CRUD ────────────────────────────────────────────────────────── -from scribe.models.rulebook import Rule, RuleRelation +from scribe.models.rulebook import Rule, RuleRelation, rule_systems async def _assert_topic_owned(session, topic_id: int, user_id: int) -> None: @@ -358,6 +359,54 @@ def _refresh_rule_embedding(rule: Rule) -> None: logger.exception("embedding refresh failed for rule %s", rule.id) +async def co_surfaced_partners( + user_id: int, rule_ids: list[int], exclude_ids: set[int] | None = None, +) -> list[Rule]: + """Rules that must arrive WITH the given ones, because they fail together. + + This is the whole reason `co_surfaces` exists. Rule 144 was split off rule + 46 and folded back into it the same day, on the correct observation that + "either rule could surface without the other and miss exposing a project to + what the entire shape is intended to be." Merging was the only fix + available; this is the fix that should have been available. + + Two limits, both deliberate: + + - Only rules the caller OWNS. An edge is not a back door into someone + else's rulebook. + - `exclude_ids` is honoured, and callers pass the project's SUPPRESSIONS. + A project that explicitly muted a rule should not have it dragged back in + by an edge — the suppression is a decision, and the edge does not + outrank it. + """ + if not rule_ids: + return [] + known = set(rule_ids) | (exclude_ids or set()) + async with async_session() as session: + edges = (await session.execute( + select(RuleRelation).where( + RuleRelation.kind == "co_surfaces", + or_( + RuleRelation.from_rule_id.in_(rule_ids), + RuleRelation.to_rule_id.in_(rule_ids), + ), + ) + )).scalars().all() + partners = { + (edge.to_rule_id if edge.from_rule_id in known else edge.from_rule_id) + for edge in edges + } - known + if not partners: + return [] + # Ownership re-checked per partner rather than assumed from the edge. + out = [] + for partner_id in sorted(partners): + rule = await _fetch_owned_rule(session, partner_id, user_id) + if rule is not None: + out.append(rule) + return out + + async def rule_detail(user_id: int, rule: Rule, system_ids: list[int] | None = None) -> dict: """The full record, with its areas and edges attached. @@ -567,6 +616,17 @@ async def list_always_on_rules( Rule.deleted_at.is_(None), RulebookTopic.deleted_at.is_(None), Rulebook.deleted_at.is_(None), + # TIER (milestone 307). This is the SESSION-START call, made + # before any project is in scope — there is no area vocabulary + # to match a conditional rule against yet, so only the + # unconditional tier belongs here. A conditional rule reaches a + # session through enter_project (by area) or search (by + # meaning), not by being resident. + # + # Behaviour is unchanged until rules are actually re-tiered: + # `tier` defaults to always_on, so every existing rule still + # arrives exactly as it did. + Rule.tier == "always_on", ) ) if project_id: @@ -1110,6 +1170,30 @@ async def get_applicable_rules( rules_q = rules_q.where(Rule.id.notin_(suppressed_rule_ids)) if suppressed_topic_ids: rules_q = rules_q.where(Rule.topic_id.notin_(suppressed_topic_ids)) + # TIER (milestone 307). always_on rules are resident, as every rule was + # before tiers existed. A conditional rule is REACHABLE, and reaches + # this project only when it is tagged to an area this project actually + # works in — a deterministic tag match, never a similarity score, so + # bindingness never depends on a ranking (D7). + # + # Applied in SQL rather than by filtering afterwards, so `limit` counts + # the rules that will actually be surfaced instead of counting rules + # that are about to be dropped. + project_area_ids = (await session.execute( + select(System.canonical_id).where( + System.project_id == project_id, + System.canonical_id.is_not(None), + System.deleted_at.is_(None), + System.status == "active", + ).distinct() + )).scalars().all() + reachable = select(rule_systems.c.rule_id).where( + rule_systems.c.canonical_id.in_(project_area_ids) + ) if project_area_ids else None + tier_clause = (Rule.tier == "always_on") + if reachable is not None: + tier_clause = or_(tier_clause, Rule.id.in_(reachable)) + rules_q = rules_q.where(tier_clause) rule_rows = (await session.execute(rules_q)).all() truncated = len(rule_rows) > limit rules = [ @@ -1130,9 +1214,40 @@ async def get_applicable_rules( ) .order_by(Rule.order_index, Rule.title) ) + if reachable is not None: + proj_rules_q = proj_rules_q.where( + or_(Rule.tier == "always_on", Rule.id.in_(reachable)) + ) + else: + proj_rules_q = proj_rules_q.where(Rule.tier == "always_on") proj_rule_rows = (await session.execute(proj_rules_q)).all() project_rules = [rule_brief(rule) for (rule,) in proj_rule_rows] + # Edges travel with the rules they belong to (milestone 307). + # + # A co_surfaces partner that was not otherwise selected is ADDED, because a + # rule that arrives without the half it fails with is the failure the edge + # was created to prevent. Suppressions are passed as exclusions so an + # explicit mute still wins over an edge. + surfaced_ids = [r["id"] for r in rules] + [r["id"] for r in project_rules] + partners = await co_surfaced_partners( + user_id, surfaced_ids, exclude_ids=set(suppressed_rule_ids), + ) + for partner in partners: + rules.append(rule_brief(partner, via="co_surfaces")) + surfaced_ids.append(partner.id) + + # Relations on every surfaced rule, so a reader can see that an override + # exists rather than discovering the contradiction by acting on the wrong + # one. Areas too — they are why a conditional rule is here at all. + edges = await list_rule_relations(surfaced_ids) + areas = await list_rule_systems(surfaced_ids) + for brief in (*rules, *project_rules): + if edges.get(brief["id"]): + brief["relations"] = edges[brief["id"]] + if areas.get(brief["id"]): + brief["systems"] = areas[brief["id"]] + return { "rules": rules, "project_rules": project_rules, diff --git a/tests/test_integration_rule_surfacing.py b/tests/test_integration_rule_surfacing.py new file mode 100644 index 0000000..661eced --- /dev/null +++ b/tests/test_integration_rule_surfacing.py @@ -0,0 +1,175 @@ +"""Real-Postgres tests for WHICH rules reach a session (milestone 307 step 5). + +What mocks can't prove, and what this milestone must not get wrong: + +1. **Nothing stops binding.** A rule with no tier, no areas and no edges + behaves exactly as it did before tiers existed. That is the one failure this + whole design must not produce, and it is asserted first. +2. A conditional rule is invisible to a project that doesn't work in its area, + and arrives — binding, not suggested — to one that does. +3. A `co_surfaces` partner arrives with its other half, which is the failure + that made merging rule 144 into rule 46 look like the only fix. +4. An explicit suppression outranks an edge. +""" +import pytest +import pytest_asyncio + +from scribe.models import async_session +from scribe.models.project import Project +from scribe.models.rulebook import Rulebook +from scribe.services import canonical_systems as canonical_svc +from scribe.services import rulebooks as rulebooks_svc +from scribe.services import systems as systems_svc +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + + +@pytest_asyncio.fixture +async def world(): + """A project with TWO rulebooks, because the two payloads are different sets. + + `list_always_on_rules` covers always-on rulebooks; `get_applicable_rules` + covers SUBSCRIBED ones. Conflating them is easy and would make these tests + assert nothing, so the fixture carries one of each and every test says + which payload it is about. + """ + async with async_session() as s: + owner = await ensure_user(s, "surfacing_owner") + project = Project(user_id=owner.id, title="Surfacing target") + s.add(project) + await s.flush() + ids = {"owner": owner.id, "pid": project.id} + await s.commit() + + always = await rulebooks_svc.create_rulebook(ids["owner"], "Family standards") + async with async_session() as s: + rb = await s.get(Rulebook, always.id) + rb.always_on = True + await s.commit() + always_topic = await rulebooks_svc.create_topic(always.id, ids["owner"], "git") + await rulebooks_svc.create_rule( + always_topic.id, ids["owner"], "dev is home", "Work on dev.", + ) + + book = await rulebooks_svc.create_rulebook(ids["owner"], "Subscribed practices") + topic = await rulebooks_svc.create_topic(book.id, ids["owner"], "release") + plain = await rulebooks_svc.create_rule( + topic.id, ids["owner"], "Between batches, keep stacking", "Keep going.", + ) + await rulebooks_svc.subscribe_project( + project_id=ids["pid"], rulebook_id=book.id, user_id=ids["owner"], + ) + ids.update({ + "always": always.id, "always_topic": always_topic.id, + "book": book.id, "topic": topic.id, "plain": plain.id, + }) + return ids + + +async def _titles(ids) -> set[str]: + applicable = await rulebooks_svc.get_applicable_rules(ids["pid"], ids["owner"]) + return {r["title"] for r in applicable["rules"]} + + +@pytest.mark.integration +async def test_a_rule_with_no_tier_no_areas_and_no_edges_binds_exactly_as_before(world): + """THE compatibility guarantee. An install upgrades and every rule it + already had keeps arriving — no tier set, no areas, no edges, still bound. + Getting this wrong would silently stop enforcing rules people rely on, + which is worse than any amount of payload bloat.""" + always_on = await rulebooks_svc.list_always_on_rules(world["owner"]) + assert "dev is home" in {r.title for r in always_on} + assert "Between batches, keep stacking" in await _titles(world) + + +@pytest.mark.integration +async def test_a_conditional_rule_is_reachable_not_resident(world): + """It leaves the session-start payload entirely — that is the point of the + tier — and it does NOT reach a project with no matching area.""" + # In the ALWAYS-ON book: the tier alone keeps it out of the session-start + # payload, which is the whole point of the tier. + resident = await rulebooks_svc.create_rule( + world["always_topic"], world["owner"], "Release tagging", "Derive the tag.", + when_to_apply="when cutting a release", tier="conditional", + ) + assert resident.tier == "conditional" + always_on = await rulebooks_svc.list_always_on_rules(world["owner"]) + assert "Release tagging" not in {r.title for r in always_on} + + # In the SUBSCRIBED book, untagged: the project has no area to reach it by, + # so it stays out of the project payload too. Absent for a DIFFERENT reason + # than above, which is why both are asserted. + await rulebooks_svc.create_rule( + world["topic"], world["owner"], "Untagged conditional", "No area yet.", + when_to_apply="sometime", tier="conditional", + ) + assert "Untagged conditional" not in await _titles(world) + + +@pytest.mark.integration +async def test_a_conditional_rule_binds_a_project_that_works_in_its_area(world): + """The payoff: the tag match carries it in deterministically. The project + reaches the area through its own System's canonical_id — its local NAME is + irrelevant, which is the whole reason the catalog exists.""" + area = await canonical_svc.find_by_name("CI & Release") + assert area is not None, "migration 0087 seeds the standard vocabulary" + + rule = await rulebooks_svc.create_rule( + world["topic"], world["owner"], "Release tagging", "Derive the tag.", + when_to_apply="when cutting a release", tier="conditional", + ) + await rulebooks_svc.set_rule_systems(rule.id, world["owner"], [area.id]) + + # Still absent: the project has no Systems at all yet. + assert "Release tagging" not in await _titles(world) + + # The project names the area with its OWN word, mapped to the same canon. + local = await systems_svc.create_system( + world["owner"], world["pid"], "CI & runners", description="ours", + ) + await canonical_svc.set_system_canonical(world["owner"], local.id, area.id) + + surfaced = await rulebooks_svc.get_applicable_rules(world["pid"], world["owner"]) + hit = [r for r in surfaced["rules"] if r["title"] == "Release tagging"] + assert hit, "a tagged conditional rule must bind a project working in that area" + assert [s["name"] for s in hit[0]["systems"]] == ["CI & Release"] + + +@pytest.mark.integration +async def test_co_surfaces_drags_in_the_half_that_would_have_been_missed(world): + """Rule 144 was split off rule 46 and folded back the same day because + "either rule could surface without the other". This is the edge that makes + that unnecessary: the partner arrives even though nothing else selected + it, and says why it is here.""" + partner = await rulebooks_svc.create_rule( + world["topic"], world["owner"], "Version names are labels", + "A name decides nothing.", + when_to_apply="when naming a build", tier="conditional", + ) + await rulebooks_svc.add_rule_relation( + world["owner"], world["plain"], partner.id, "co_surfaces", + note="they fail together", + ) + surfaced = await rulebooks_svc.get_applicable_rules(world["pid"], world["owner"]) + hit = [r for r in surfaced["rules"] if r["title"] == "Version names are labels"] + assert hit, "a co_surfaces partner must arrive with its other half" + assert hit[0]["via"] == "co_surfaces" + + +@pytest.mark.integration +async def test_a_suppression_outranks_an_edge(world): + """The edge says these belong together; the suppression says this project + does not want that one. An explicit decision beats an inferred one.""" + partner = await rulebooks_svc.create_rule( + world["topic"], world["owner"], "Muted partner", "Should not arrive.", + tier="conditional", + ) + await rulebooks_svc.add_rule_relation( + world["owner"], world["plain"], partner.id, "co_surfaces", + ) + await rulebooks_svc.suppress_rule_for_project( + world["pid"], partner.id, world["owner"], + ) + surfaced = await rulebooks_svc.get_applicable_rules(world["pid"], world["owner"]) + assert "Muted partner" not in {r["title"] for r in surfaced["rules"]} diff --git a/tests/test_services_rulebooks.py b/tests/test_services_rulebooks.py index 6d52944..b7de1ad 100644 --- a/tests/test_services_rulebooks.py +++ b/tests/test_services_rulebooks.py @@ -198,6 +198,25 @@ def _empty(): return r +def _no_edges(): + """Silence the three post-query lookups get_applicable_rules now makes. + + They are separate service functions with their own coverage (and the real + wiring is proven against Postgres in test_integration_rule_surfacing), so + stubbing them here keeps each of these tests about the one projection it + was written to check — rather than about the order a mocked session's + execute() calls happen to arrive in. + """ + return ( + patch("scribe.services.rulebooks.co_surfaced_partners", + AsyncMock(return_value=[])), + patch("scribe.services.rulebooks.list_rule_relations", + AsyncMock(return_value={})), + patch("scribe.services.rulebooks.list_rule_systems", + AsyncMock(return_value={})), + ) + + @pytest.mark.asyncio async def test_get_applicable_rules_returns_shape(): """get_applicable_rules returns the full projection — including the @@ -213,12 +232,14 @@ async def test_get_applicable_rules_returns_shape(): "git-workflow", 1, "FabledSword family") for i in range(50) ] - # Execute order: sub_q, suppressed_rules_q, suppressed_topics_q, rules_q, proj_rules_q + # Execute order: sub_q, suppressed_rules_q, suppressed_topics_q, + # project-areas_q (milestone 307), rules_q, proj_rules_q mock_session.execute = AsyncMock(side_effect=[ - sub_result, _empty(), _empty(), rules_result, _empty(), + sub_result, _empty(), _empty(), _empty(), rules_result, _empty(), ]) - with patch("scribe.services.rulebooks.async_session") as mock_cls: + _p1, _p2, _p3 = _no_edges() + with patch("scribe.services.rulebooks.async_session") as mock_cls, _p1, _p2, _p3: mock_cls.return_value = mock_session from scribe.services.rulebooks import get_applicable_rules result = await get_applicable_rules(project_id=3, user_id=7, limit=50) @@ -250,10 +271,11 @@ async def test_get_applicable_rules_truncates_when_over_limit(): (fake_rule(id=i, title=f"r{i}"), "topic", 1, "rb") for i in range(51) ] mock_session.execute = AsyncMock(side_effect=[ - sub_result, _empty(), _empty(), rules_result, _empty(), + sub_result, _empty(), _empty(), _empty(), rules_result, _empty(), ]) - with patch("scribe.services.rulebooks.async_session") as mock_cls: + _p1, _p2, _p3 = _no_edges() + with patch("scribe.services.rulebooks.async_session") as mock_cls, _p1, _p2, _p3: mock_cls.return_value = mock_session from scribe.services.rulebooks import get_applicable_rules result = await get_applicable_rules(project_id=3, user_id=7, limit=50) @@ -274,10 +296,11 @@ async def test_get_applicable_rules_includes_project_scoped_rules(): statement="Land schema changes in their own PR."),), ] mock_session.execute = AsyncMock(side_effect=[ - _empty(), _empty(), _empty(), _empty(), proj_rules_result, + _empty(), _empty(), _empty(), _empty(), _empty(), proj_rules_result, ]) - with patch("scribe.services.rulebooks.async_session") as mock_cls: + _p1, _p2, _p3 = _no_edges() + with patch("scribe.services.rulebooks.async_session") as mock_cls, _p1, _p2, _p3: mock_cls.return_value = mock_session from scribe.services.rulebooks import get_applicable_rules result = await get_applicable_rules(project_id=3, user_id=7) @@ -303,10 +326,14 @@ async def test_get_applicable_rules_surfaces_suppressed_with_context(): (22, "design-system", 1, "FabledSword family"), ] mock_session.execute = AsyncMock(side_effect=[ - _empty(), suppressed_rules_result, suppressed_topics_result, _empty(), _empty(), + # sub_q, suppressed_rules_q, suppressed_topics_q, project-areas_q, + # rules_q, proj_rules_q + _empty(), suppressed_rules_result, suppressed_topics_result, + _empty(), _empty(), _empty(), ]) - with patch("scribe.services.rulebooks.async_session") as mock_cls: + _p1, _p2, _p3 = _no_edges() + with patch("scribe.services.rulebooks.async_session") as mock_cls, _p1, _p2, _p3: mock_cls.return_value = mock_session from scribe.services.rulebooks import get_applicable_rules result = await get_applicable_rules(project_id=3, user_id=7)