Rules become findable: canon catalog, triggers, tiers, edges, retrieval, surfacing (milestone 307, steps 1–5) #131
@@ -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.
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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"]}
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user