feat(moments): rules mount on moments, through every rule door (milestone 458 step 3, #4921)

rule_moments (migration 0117) records which moments a rule arrives at, by
catalog name, cascading with the rule. rule_detail, the one seam every
rule door already returns through, gains moments beside system_ids: None
leaves the mounts alone, a list replaces them. get_rule and both
list_rules doors read them back, batched per page.

All five MCP rule/preference writes and the three REST ones take moments
and validate them before their create or update. An unknown name is
refused with the catalog listed and leaves no half-made rule behind; a
parity test pins that ordering on every door.

Backup v22 carries the mounts as a join table remapped through the rule
map; a real-Postgres round trip checks they land on the restored rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

tests/helpers.py

@@ -315,7 +315,7 @@ def plain_rule_detail():
     stubbing the seam slightly differently is a test asserting something
     slightly different than it appears to.
     """
-    async def _detail(_uid, rule, _system_ids=None):
+    async def _detail(_uid, rule, _system_ids=None, _moments=None):
         return rule.to_dict()
     return patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail", _detail)
 

tests/test_integration_rule_moments.py

@@ -0,0 +1,157 @@
+"""Real-Postgres tests for rule mounts (milestone 458 step 3).
+
+A mount is the claim "this rule arrives at this moment". These pin what that
+claim depends on:
+- the set given is the set stored, and a refused name stores nothing;
+- only the owner can mount;
+- the rule's detail reads its mounts back;
+- a backup carries them, attached to the RESTORED rule's new id rather than
+  the old number.
+"""
+import pytest
+import pytest_asyncio
+from sqlalchemy import select
+
+from scribe.models import async_session
+from scribe.models.rulebook import Rule, Rulebook, RulebookTopic
+from scribe.models.user import User
+from scribe.services import backup
+from scribe.services import rulebooks as rulebooks_svc
+from tests.helpers import ensure_user
+
+pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")]
+
+OWNER_USERNAME = "rule_moments_owner"
+STRANGER_USERNAME = "rule_moments_stranger"
+RESTORED_USERNAME = "rule_moments_restored"
+TRIGGER = "about to tell the operator a piece of work is finished"
+
+
+async def _purge_books(username: str) -> None:
+    """At SETUP: rule writes fire a detached embedding refresh, and a teardown
+    delete races it (test_integration_rule_versions records why)."""
+    async with async_session() as s:
+        for user in (await s.execute(
+            select(User).where(User.username == username)
+        )).scalars().all():
+            for book in (await s.execute(
+                select(Rulebook).where(Rulebook.owner_user_id == user.id)
+            )).scalars().all():
+                await s.delete(book)
+        await s.commit()
+
+
+@pytest_asyncio.fixture
+async def world():
+    for name in (OWNER_USERNAME, STRANGER_USERNAME, RESTORED_USERNAME):
+        await _purge_books(name)
+    # The restore below creates a user under a unique username; a previous
+    # run's must be gone first, as in the sibling roundtrip files.
+    async with async_session() as s:
+        for user in (await s.execute(
+            select(User).where(User.username == RESTORED_USERNAME)
+        )).scalars().all():
+            await s.delete(user)
+        await s.commit()
+    async with async_session() as s:
+        owner = await ensure_user(s, OWNER_USERNAME)
+        stranger = await ensure_user(s, STRANGER_USERNAME)
+        await s.commit()
+        uid, sid = owner.id, stranger.id
+    book = await rulebooks_svc.create_rulebook(uid, "Moment fixtures")
+    topic = await rulebooks_svc.create_topic(book.id, uid, "verification")
+    rule = await rulebooks_svc.create_rule(
+        topic.id, uid, "Done means delivered and checked",
+        "Work is done when it is delivered and its automated checks pass.",
+        when_to_apply=TRIGGER,
+    )
+    return {"uid": uid, "sid": sid, "book_id": book.id, "rule": rule}
+
+
+async def test_the_set_given_is_the_set_stored(world):
+    uid, rule = world["uid"], world["rule"]
+    assert await rulebooks_svc.set_rule_moments(
+        rule.id, uid, ["reply.report", " Work.Finish"],
+    ) == ["reply.report", "work.finish"]
+    # Read back in CATALOG order, whatever order they were given in.
+    assert (await rulebooks_svc.list_rule_moments([rule.id]))[rule.id] == [
+        "work.finish", "reply.report",
+    ]
+
+    await rulebooks_svc.set_rule_moments(rule.id, uid, ["work.deliver"])
+    assert (await rulebooks_svc.list_rule_moments([rule.id]))[rule.id] == ["work.deliver"]
+
+    await rulebooks_svc.set_rule_moments(rule.id, uid, [])
+    assert rule.id not in await rulebooks_svc.list_rule_moments([rule.id])
+
+
+async def test_a_refused_name_stores_nothing(world):
+    uid, rule = world["uid"], world["rule"]
+    await rulebooks_svc.set_rule_moments(rule.id, uid, ["work.finish"])
+    with pytest.raises(ValueError, match="unknown moment"):
+        await rulebooks_svc.set_rule_moments(rule.id, uid, ["work.deliver", "work.shipped"])
+    assert (await rulebooks_svc.list_rule_moments([rule.id]))[rule.id] == ["work.finish"]
+
+
+async def test_only_the_owner_can_mount(world):
+    rule = world["rule"]
+    assert await rulebooks_svc.set_rule_moments(rule.id, world["sid"], ["work.finish"]) is None
+    assert rule.id not in await rulebooks_svc.list_rule_moments([rule.id])
+
+
+async def test_the_detail_carries_the_mounts_and_omits_an_empty_set(world):
+    uid, rule = world["uid"], world["rule"]
+    bare = await rulebooks_svc.rule_detail(uid, rule)
+    assert "moments" not in bare
+    mounted = await rulebooks_svc.rule_detail(uid, rule, moments=["work.finish"])
+    assert mounted["moments"] == ["work.finish"]
+    # None leaves them alone: a later read without the argument still has them.
+    assert (await rulebooks_svc.rule_detail(uid, rule))["moments"] == ["work.finish"]
+
+
+async def test_a_backup_carries_the_mounts_to_the_restored_rule(world):
+    uid, rule = world["uid"], world["rule"]
+    await rulebooks_svc.set_rule_moments(rule.id, uid, ["work.finish", "reply.report"])
+
+    async with async_session() as s:
+        payload = {
+            "version": backup.BACKUP_VERSION,
+            "users": backup._user_rows(
+                [(await s.execute(select(User).where(User.id == uid))).scalars().one()]
+            ),
+            "rulebooks": backup._rulebook_rows(
+                [(await s.execute(select(Rulebook).where(
+                    Rulebook.id == world["book_id"]))).scalars().one()]
+            ),
+            "rulebook_topics": backup._topic_rows((await s.execute(
+                select(RulebookTopic).where(RulebookTopic.rulebook_id == world["book_id"])
+            )).scalars().all()),
+            "rules": backup._rule_rows(
+                [(await s.execute(select(Rule).where(Rule.id == rule.id))).scalars().one()]
+            ),
+        }
+    exported = await backup.export_user_backup(uid)
+    payload["rule_moments"] = [
+        row for row in exported["rule_moments"] if row["rule_id"] == rule.id
+    ]
+    assert sorted(r["moment"] for r in payload["rule_moments"]) == [
+        "reply.report", "work.finish",
+    ]
+    payload["users"][0]["username"] = RESTORED_USERNAME
+
+    stats = await backup.restore_full_backup(payload)
+    assert stats["rule_moments"] == 2
+
+    async with async_session() as s:
+        restored_user = (await s.execute(
+            select(User).where(User.username == RESTORED_USERNAME)
+        )).scalars().one()
+        restored_rule = (await s.execute(
+            select(Rule).join(RulebookTopic, RulebookTopic.id == Rule.topic_id)
+            .join(Rulebook, Rulebook.id == RulebookTopic.rulebook_id)
+            .where(Rulebook.owner_user_id == restored_user.id)
+        )).scalars().one()
+    assert restored_rule.id != rule.id
+    assert (await rulebooks_svc.list_rule_moments([restored_rule.id]))[restored_rule.id] == [
+        "work.finish", "reply.report",
+    ]

tests/test_moments.py

@@ -132,3 +132,20 @@ def test_both_doors_read_one_catalog():
     assert routes.moments_svc is moments
     assert tool.actions_svc is moment_actions
     assert routes.moment_actions_svc is moment_actions
+
+
+def test_a_rules_moments_are_normalised_and_deduplicated():
+    assert moments.require_moments([" Work.Finish", "reply.report", "work.finish"]) == [
+        "work.finish", "reply.report",
+    ]
+    assert moments.require_moments("work.finish") == ["work.finish"]
+    assert moments.require_moments([]) == []
+
+
+def test_absent_moments_mean_leave_them_alone():
+    assert moments.require_moments(None) is None
+
+
+def test_one_unknown_moment_refuses_the_whole_set():
+    with pytest.raises(ValueError, match="work.finished"):
+        moments.require_moments(["work.finish", "work.finished"])

tests/test_rule_moment_doors.py

@@ -0,0 +1,132 @@
+"""Every door that writes a rule can mount it on moments (milestone 458 step 3).
+
+THE SHAPE, NOT ONE FUNCTION. test_system_tagging_door_parity records how a
+capability goes missing: whichever door nobody exercised for a kind never
+grows the parameter. So this file asserts the whole table — every rule and
+preference write, on both doors, takes `moments` — and that each one checks
+the names BEFORE its create or update. A refusal after the write would leave
+a rule created without the mounts it asked for, and the caller reading the
+error would reasonably believe nothing happened.
+"""
+from __future__ import annotations
+
+import ast
+import pathlib
+from unittest.mock import AsyncMock, patch
+
+import pytest
+
+from tests.helpers import fake_rule
+from tests.helpers import plain_rule_detail as _plain_detail
+
+ROOT = pathlib.Path(__file__).resolve().parents[1] / "src" / "scribe"
+
+# (tool, the service write it must validate before)
+MCP_DOORS = [
+    ("create_rule", "create_rule"),
+    ("create_project_rule", "create_project_rule"),
+    ("update_rule", "update_rule"),
+    ("create_preference", "create_rule"),
+    ("update_preference", "update_rule"),
+]
+# (route handler, the service write it must validate before)
+REST_DOORS = [
+    ("create_rule", "create_rule"),
+    ("update_rule", "update_rule"),
+    ("create_project_rule", "create_project_rule"),
+]
+
+
+def _fn(path: pathlib.Path, name: str):
+    for node in ast.parse(path.read_text()).body:
+        if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name == name:
+            return node
+    raise AssertionError(f"{path.name} has no {name}")
+
+
+def _first_line_calling(fn, attr: str) -> int | None:
+    lines = [
+        n.lineno for n in ast.walk(fn)
+        if isinstance(n, ast.Call) and (
+            (isinstance(n.func, ast.Attribute) and n.func.attr == attr)
+            or (isinstance(n.func, ast.Name) and n.func.id == attr)
+        )
+    ]
+    return min(lines) if lines else None
+
+
+@pytest.mark.parametrize("tool,write", MCP_DOORS)
+def test_every_mcp_rule_door_takes_moments_and_checks_them_first(tool, write):
+    fn = _fn(ROOT / "mcp" / "tools" / "rulebooks.py", tool)
+    params = {a.arg for a in fn.args.args + fn.args.kwonlyargs}
+    assert "moments" in params, f"{tool} cannot mount a rule"
+    check = _first_line_calling(fn, "require_moments")
+    wrote = _first_line_calling(fn, write)
+    assert check is not None and wrote is not None, (tool, check, wrote)
+    assert check < wrote, f"{tool} validates moments after it has already written"
+    detail = [n for n in ast.walk(fn) if isinstance(n, ast.Call)
+              and isinstance(n.func, ast.Attribute) and n.func.attr == "rule_detail"]
+    assert any(any(isinstance(a, ast.Name) and a.id == "moments" for a in c.args)
+               for c in detail), f"{tool} never hands its moments to rule_detail"
+
+
+@pytest.mark.parametrize("handler,write", REST_DOORS)
+def test_every_rest_rule_door_takes_moments_and_checks_them_first(handler, write):
+    fn = _fn(ROOT / "routes" / "rulebooks.py", handler)
+    check = _first_line_calling(fn, "_moments_or_refusal")
+    wrote = _first_line_calling(fn, write)
+    assert check is not None and wrote is not None, (handler, check, wrote)
+    assert check < wrote, f"{handler} validates moments after it has already written"
+
+
+def test_the_guard_can_fail():
+    """A door with no check at all must be reported, not skipped (rule 167)."""
+    fn = ast.parse("async def f():\n    await svc.create_rule()\n").body[0]
+    assert _first_line_calling(fn, "require_moments") is None
+
+
+async def test_an_unknown_moment_is_refused_before_anything_is_written():
+    create = AsyncMock(return_value=fake_rule(id=5))
+    with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.create_rule", create), \
+         patch("scribe.mcp.tools.rulebooks.dedup_svc.find_duplicate_rule", AsyncMock()), \
+         _plain_detail(), patch("scribe.mcp.tools.rulebooks.current_user_id", lambda: 7):
+        from scribe.mcp.tools.rulebooks import create_rule
+        with pytest.raises(ValueError, match="unknown moment"):
+            await create_rule(
+                topic_id=10, title="t", statement="s",
+                when_to_apply="when the moment this fixture stands in for arises",
+                moments=["work.finished"],
+            )
+    create.assert_not_awaited()
+
+
+async def test_the_door_hands_rule_detail_the_normalised_moments():
+    seen = {}
+
+    async def detail(_uid, rule, _system_ids=None, moments=None):
+        seen["moments"] = moments
+        return rule.to_dict()
+
+    with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule",
+               AsyncMock(return_value=fake_rule(id=5))), \
+         patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail", detail), \
+         patch("scribe.mcp.tools.rulebooks.current_user_id", lambda: 7):
+        from scribe.mcp.tools.rulebooks import update_rule
+        await update_rule(rule_id=5, moments=[" Work.Finish", "reply.report", "work.finish"])
+    assert seen["moments"] == ["work.finish", "reply.report"]
+
+
+async def test_omitting_moments_leaves_the_mounts_alone():
+    seen = {}
+
+    async def detail(_uid, rule, _system_ids=None, moments="unset"):
+        seen["moments"] = moments
+        return rule.to_dict()
+
+    with patch("scribe.mcp.tools.rulebooks.rulebooks_svc.update_rule",
+               AsyncMock(return_value=fake_rule(id=5))), \
+         patch("scribe.mcp.tools.rulebooks.rulebooks_svc.rule_detail", detail), \
+         patch("scribe.mcp.tools.rulebooks.current_user_id", lambda: 7):
+        from scribe.mcp.tools.rulebooks import update_rule
+        await update_rule(rule_id=5, title="renamed")
+    assert seen["moments"] is None

tests/test_services_backup.py

@@ -27,7 +27,7 @@ def test_backup_version_is_current():
 
     (Named for the number it asserted until v10, which is exactly the drift a
     name-carrying-a-value invites; it now says what it checks.)"""
-    assert backup.BACKUP_VERSION == 21
+    assert backup.BACKUP_VERSION == 22
 
 
 def _exportable_note(**over):
@@ -452,7 +452,7 @@ def test_the_column_guard_covers_every_table_with_a_row_helper():
     # REAL table names, as _BACKED_UP holds them — not the shorter keys the
     # payload uses for the same sections. Getting this wrong is what the guard
     # caught on its own first run.
-    join_tables = {"rule_systems"}
+    join_tables = {"rule_systems", "rule_moments"}
     covered = set(_column_guard_targets()) | join_tables
     assert set(backup._BACKED_UP) - covered == set()
     # And no stale entries: every declaration must name a real target.