feat(rules): an edit leaves behind what it replaced (#3241, milestone 323 step 2)

`update_rule` now snapshots the rule's text before it writes. Rescoping rule
79 meant hand-copying the superseded statement into a task log to keep it
(#3237); the history is that, done by the write path instead of by somebody
remembering.

The snapshot is taken BEFORE the field loop, which is the one ordering that
matters. `update_rule` drops `verified_at` when `verify_with` changes, and
rewriting a check is exactly the edit whose history is worth most — a
snapshot taken afterwards would file the NEW check against the OLD wording.
Taking it up front also covers `clear`, which is a separate argument from the
field loop and is how a rule that stops being a constraint loses its check
entirely.

Session-bound rather than opening its own like note_versions.create_version:
the version and the edit that caused it commit together, so a failed update
cannot leave a history entry for an edit that never happened.

Two guards from the sibling are deliberately absent, and both are now pinned
by tests rather than only by comments — "make it consistent with
note_versions" is a plausible-sounding change that would silently start
dropping history:

- No MIN_VERSION_INTERVAL_SECONDS. That 300-second gate exists because note
  autosave fires every 60. Every version here comes from a deliberate
  update_rule, so three edits in one second are three edits.
- No MAX_VERSIONS and no pruning. A rule is edited a handful of times in its
  life; a cap could only ever discard the one edit somebody went looking for.

Kept from the sibling: the identical-content skip. Both doors resend every
field, so without it a form saved twice would file an identical snapshot.
`order_index` is excluded from the snapshot fields for the same reason —
reordering a rulebook is not an edit to what any rule says.

No delete-time snapshot, against the task's original scope and on the
operator's call. A delete goes through trash_svc and is SOFT: the rule row
keeps its full text and restores untouched, so there is nothing for a
snapshot to preserve. Anything that survived a purge would be data the
operator explicitly asked to be gone.

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

src/scribe/services/rule_versions.py

@@ -0,0 +1,82 @@
+"""A rule's edit history — the snapshot taken when an edit overwrites text.
+
+Sibling of `note_versions`, and the differences are the whole design.
+
+WHAT A VERSION IS FOR. It records what a rule USED TO SAY, because an update
+overwrites that and nothing can recompute it. It is not a record of the rule's
+lifecycle: a soft delete keeps the rule row whole and restorable, so there is
+nothing to preserve at delete time, and anything that outlived a purge would be
+data the operator explicitly asked to be gone.
+
+WHAT IS DELIBERATELY NOT COPIED FROM note_versions (milestone 323). Both of its
+guards defend against note autosave, which fires every 60 seconds and would
+otherwise burn every slot. Rules have no autosave — every version here comes
+from a deliberate `update_rule` — so both guards would only ever discard a real
+edit:
+
+- No `MIN_VERSION_INTERVAL_SECONDS`. Two deliberate edits four minutes apart
+  are two edits.
+- No `MAX_VERSIONS` and no pruning. A rule is edited a handful of times in its
+  life; a cap invites losing the one edit somebody needed.
+
+TEXT ONLY. A rule's topic, its Systems tags and its typed edges are not here.
+They have their own lifecycle, and folding them in would make one word,
+"version", mean two things — the rule's wording, and the rule's place in the
+graph.
+"""
+from sqlalchemy import select
+
+from scribe.models import async_session
+from scribe.models.rule_version import RuleVersion
+
+# The columns a version carries, and therefore the ones whose change is worth
+# a snapshot. `order_index` and `arose_from_id` are editable through
+# update_rule and are deliberately absent: reordering a rulebook is not an
+# edit to what any rule says, and a run of "moved rule 4 above rule 3"
+# snapshots would bury the edits somebody is actually looking for.
+SNAPSHOT_FIELDS = (
+    "title", "statement", "why", "how_to_apply", "when_to_apply",
+    "tier", "verify_with", "expires_when",
+)
+
+
+def snapshot(rule) -> dict:
+    """The rule's text right now, as a plain dict.
+
+    Plain values rather than the ORM object because the caller has to hold
+    this ACROSS the mutation — a live `rule` would show the new text by the
+    time it was read, which is the one thing the snapshot must not do.
+    """
+    return {f: getattr(rule, f) for f in SNAPSHOT_FIELDS}
+
+
+def record_if_changed(session, rule, actor_user_id: int | None, before: dict):
+    """Add a version holding `before`, unless the edit changed no text.
+
+    Git semantics, and not a nicety: `update_rule` is reached by callers that
+    resend every field, so without this a form saved twice would file a second
+    identical snapshot and the history would stop reading as a list of edits.
+
+    Session-BOUND, unlike `note_versions.create_version` which opens its own.
+    The version and the edit that caused it commit together or not at all;
+    a separate session could leave a snapshot behind after the update it
+    describes had failed, which is a history entry for an edit that never
+    happened.
+    """
+    if snapshot(rule) == before:
+        return None
+    version = RuleVersion(
+        rule_id=rule.id, user_id=actor_user_id, **before,
+    )
+    session.add(version)
+    return version
+
+
+async def list_versions(rule_id: int) -> list[RuleVersion]:
+    """Newest first — the reader's question is "what did this just say?"."""
+    async with async_session() as session:
+        return list((await session.execute(
+            select(RuleVersion)
+            .where(RuleVersion.rule_id == rule_id)
+            .order_by(RuleVersion.created_at.desc(), RuleVersion.id.desc())
+        )).scalars().all())

src/scribe/services/rulebooks.py

@@ -20,6 +20,7 @@ from scribe.services.verification import (
     days_since_verified as _days_since_verified,
     last_verified_label as _last_verified_label,
 )
+from scribe.services import rule_versions
 
 logger = logging.getLogger(__name__)
 
@@ -748,6 +749,10 @@ async def update_rule(
             "verify_with", "expires_when",
         }
         check_before = rule.verify_with
+        # Captured BEFORE anything is written, and as plain values — this has
+        # to survive the mutation below. A rule's history is the only record
+        # of what it used to say; the edit itself destroys that.
+        text_before = rule_versions.snapshot(rule)
         for key in clear:
             if key in allowed and key in NULLABLE_RULE_TEXT:
                 setattr(rule, key, None)
@@ -771,6 +776,11 @@ async def update_rule(
         # wrongly vouched for costs the thing the sweep exists to catch.
         if rule.verify_with != check_before:
             rule.verified_at = None
+        # Same session as the edit, so the two commit together. The snapshot
+        # holds the OLD verify_with — the check that was in force when that
+        # wording was written — which is why it is taken before the loop and
+        # not here.
+        rule_versions.record_if_changed(session, rule, user_id, text_before)
         await session.commit()
         await session.refresh(rule)
         _refresh_rule_embedding(rule)

tests/test_integration_rule_versions.py

@@ -0,0 +1,201 @@
+"""Real-Postgres tests for the rule-history write path (milestone 323 step 2).
+
+Every assertion here is about ORDERING or ABSENCE, which is why none of them
+can be made with mocks.
+
+What a version is for: it holds what the rule USED TO SAY. The edit destroys
+that, and nothing can recompute it — the rescoping of rule 79 had to be
+hand-copied into a task log to survive (#3237), which is not a process, it is
+a person remembering.
+
+Two of these pin a DELIBERATE ABSENCE. `note_versions` carries a 300-second
+interval gate and a 50-version cap, both defences against note autosave. Rules
+have no autosave, so here those guards would only ever discard a real edit.
+Nothing in the code says so on its own, and "make it consistent with the
+sibling" is a plausible-sounding change that would silently start dropping
+history — so the absence is a test, not a comment.
+"""
+import pytest
+import pytest_asyncio
+from sqlalchemy import select
+
+from scribe.models import async_session
+from scribe.models.rulebook import Rule, Rulebook
+from scribe.models.rule_version import RuleVersion
+from scribe.services import rule_versions as rv_svc
+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_history_owner"
+
+
+@pytest_asyncio.fixture
+async def constraint():
+    """One rule carrying a check, with no history yet.
+
+    Cleanup is in this fixture's own teardown, never in an autouse one:
+    `_dispose_engine` arrives through usefixtures, so it tears down BEFORE an
+    autouse fixture would, and a database call after that point orphans a
+    pooled connection and breaks the NEXT test to touch Postgres (#3240).
+    """
+    async with async_session() as s:
+        owner = await ensure_user(s, OWNER_USERNAME)
+        uid = owner.id
+        await s.commit()
+        # A previous failed run would leave a book whose topic title collides.
+        for book in (await s.execute(
+            select(Rulebook).where(Rulebook.owner_user_id == uid)
+        )).scalars().all():
+            await s.delete(book)
+        await s.commit()
+
+    book = await rulebooks_svc.create_rulebook(uid, "History fixtures")
+    topic = await rulebooks_svc.create_topic(book.id, uid, "ci")
+    rule = await rulebooks_svc.create_rule(
+        topic.id, uid, "The runner has no bash",
+        "Write every `run:` step in POSIX sh.",
+        why="the image ships no bash",
+        verify_with="read the workflow's shell setting",
+    )
+
+    yield {"uid": uid, "rule_id": rule.id}
+
+    async with async_session() as s:
+        row = await s.get(Rulebook, book.id)
+        if row is not None:
+            await s.delete(row)
+        await s.commit()
+
+
+async def _versions(rule_id: int) -> list[RuleVersion]:
+    async with async_session() as s:
+        return list((await s.execute(
+            select(RuleVersion).where(RuleVersion.rule_id == rule_id)
+            .order_by(RuleVersion.id)
+        )).scalars().all())
+
+
+async def test_an_edit_leaves_the_text_it_replaced(constraint):
+    """The headline. The version holds the OLD statement — the rule row
+    already holds the new one, so a snapshot of the new text would record
+    nothing that was not already there."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"],
+        statement="Write every `run:` step in POSIX sh, and set shell: sh.",
+    )
+    [version] = await _versions(constraint["rule_id"])
+    assert version.statement == "Write every `run:` step in POSIX sh."
+    assert version.why == "the image ships no bash"
+
+
+async def test_the_actor_is_recorded(constraint):
+    """`user_id` is who made the edit — the question an audit trail over a
+    binding instruction is actually asked."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"], title="The runner ships no bash",
+    )
+    [version] = await _versions(constraint["rule_id"])
+    assert version.user_id == constraint["uid"]
+
+
+async def test_an_edit_that_changes_no_text_writes_nothing(constraint):
+    """Git semantics. `update_rule` is reached by callers that resend every
+    field, so without the skip a form saved twice would file an identical
+    snapshot and the history would stop reading as a list of edits."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"],
+        statement="Write every `run:` step in POSIX sh.",
+        title="The runner has no bash",
+    )
+    assert await _versions(constraint["rule_id"]) == []
+
+
+async def test_reordering_is_not_an_edit(constraint):
+    """`order_index` goes through the same door but is not text. A run of
+    "moved rule 4 above rule 3" snapshots would bury the edits somebody is
+    looking for."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"], order_index=7,
+    )
+    assert await _versions(constraint["rule_id"]) == []
+
+
+async def test_the_snapshot_holds_the_CHECK_that_was_in_force(constraint):
+    """The ordering trap. `update_rule` drops `verified_at` when `verify_with`
+    changes, and rewriting the check is exactly the edit whose history matters
+    most — so the snapshot has to be taken before the field loop, or it
+    records the new check against the old wording."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"],
+        verify_with="run `sh -c 'echo $0'` in a job step",
+    )
+    [version] = await _versions(constraint["rule_id"])
+    assert version.verify_with == "read the workflow's shell setting"
+
+    async with async_session() as s:
+        rule = await s.get(Rule, constraint["rule_id"])
+        assert rule.verify_with == "run `sh -c 'echo $0'` in a job step"
+
+
+async def test_clearing_a_field_is_an_edit_worth_recording(constraint):
+    """A rule that stops being a constraint loses its check entirely, and the
+    version is then the only place the retired check exists. `clear` is a
+    separate argument from the field loop — a snapshot wired to only one of
+    the two would lose exactly this case."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"], clear=("verify_with",),
+    )
+    [version] = await _versions(constraint["rule_id"])
+    assert version.verify_with == "read the workflow's shell setting"
+
+    async with async_session() as s:
+        rule = await s.get(Rule, constraint["rule_id"])
+        assert rule.verify_with is None
+
+
+async def test_rapid_successive_edits_are_all_kept(constraint):
+    """DELIBERATE ABSENCE — no MIN_VERSION_INTERVAL_SECONDS.
+
+    `note_versions` skips a snapshot taken within 300 seconds of the last one,
+    because note autosave fires every 60 and would burn all 50 slots. Rules
+    have no autosave: every one of these is a deliberate update_rule, and
+    three edits in the same second are three edits. Porting the sibling's gate
+    here would silently keep only the first.
+    """
+    for n in (1, 2, 3):
+        await rulebooks_svc.update_rule(
+            constraint["rule_id"], constraint["uid"], statement=f"revision {n}",
+        )
+    versions = await _versions(constraint["rule_id"])
+    assert [v.statement for v in versions] == [
+        "Write every `run:` step in POSIX sh.", "revision 1", "revision 2",
+    ]
+
+
+async def test_the_service_carries_no_pruning_machinery(constraint):
+    """DELIBERATE ABSENCE — no MAX_VERSIONS and no pruning DELETE.
+
+    Asserted structurally because the alternative is 51 real edits to prove a
+    negative. The cap exists in `note_versions` to bound autosave volume; a
+    rule is edited a handful of times in its life, so a cap here could only
+    ever discard the one edit somebody went looking for.
+    """
+    assert not hasattr(rv_svc, "MAX_VERSIONS")
+    assert not hasattr(rv_svc, "MIN_VERSION_INTERVAL_SECONDS")
+
+
+async def test_history_reads_newest_first(constraint):
+    """The reader's question is "what did this just say?", so the most recent
+    supersession has to be the first row."""
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"], statement="first change",
+    )
+    await rulebooks_svc.update_rule(
+        constraint["rule_id"], constraint["uid"], statement="second change",
+    )
+    listed = await rv_svc.list_versions(constraint["rule_id"])
+    assert [v.statement for v in listed] == [
+        "first change", "Write every `run:` step in POSIX sh.",
+    ]