From 6fa66f202b60abf8bc5582242a21f6f9c102cf06 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 29 Aug 2026 23:01:16 -0400 Subject: [PATCH] feat(rules): an edit leaves behind what it replaced (#3241, milestone 323 step 2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 --- src/scribe/services/rule_versions.py | 82 ++++++++++ src/scribe/services/rulebooks.py | 10 ++ tests/test_integration_rule_versions.py | 201 ++++++++++++++++++++++++ 3 files changed, 293 insertions(+) create mode 100644 src/scribe/services/rule_versions.py create mode 100644 tests/test_integration_rule_versions.py diff --git a/src/scribe/services/rule_versions.py b/src/scribe/services/rule_versions.py new file mode 100644 index 0000000..2d905a4 --- /dev/null +++ b/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()) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index bfc8488..e37a99a 100644 --- a/src/scribe/services/rulebooks.py +++ b/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) diff --git a/tests/test_integration_rule_versions.py b/tests/test_integration_rule_versions.py new file mode 100644 index 0000000..27475fd --- /dev/null +++ b/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.", + ]