From 9006affda82a2418e7d813db828551aba0cbe918 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Sat, 29 Aug 2026 18:38:16 -0400 Subject: [PATCH] =?UTF-8?q?feat(rules):=20a=20rule=20keeps=20what=20it=20u?= =?UTF-8?q?sed=20to=20say=20=E2=80=94=20rule=5Fversions=20(#3240,=20milest?= =?UTF-8?q?one=20323=20step=201)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sibling notes already had. `note_versions` snapshots a note's every meaningful edit; a RULE, which binds behaviour on every session that loads it, had nothing — an edit destroyed the previous wording with no record anywhere. Rescoping rule 79 meant hand-copying the superseded statement into a task log to keep it (#3237). The more consequential record had the weaker protection. Schema and transport only. Nothing writes a version yet — that is step 2. Three guards are deliberately NOT copied from note_versions, each defending against autosave, which rules do not have: no pruning or MAX_VERSIONS, no pin columns, no minimum interval. A rule is edited a handful of times in its life, and capping invites losing the one edit somebody needed. `user_id` is the ACTOR rather than the owner, and SET NULL rather than CASCADE: deleting a user must not erase the history of the rules they edited. The restore diverges from its NoteVersion sibling accordingly — an unmappable user leaves the row with a null actor instead of dropping it, which is the whole point of choosing SET NULL. The integration round trip pins that, because nothing in the code says which of the two shapes is intended and "make it match the sibling" would silently delete the record. Backup goes to v13. Both export paths carry the table; the per-user one scopes through the rule rather than the version's user_id, or it would carry the versions this user wrote on someone else's rule and drop the ones someone else wrote on theirs. The restore remaps rule_id through rule_id_map — #3182's arose_from_id trap on a new table. Co-Authored-By: Claude Opus 5 --- alembic/versions/0093_rule_versions.py | 83 ++++++ src/scribe/models/__init__.py | 1 + src/scribe/models/rule_version.py | 91 +++++++ src/scribe/services/backup.py | 69 ++++- ...tegration_backup_rule_version_roundtrip.py | 250 ++++++++++++++++++ tests/test_services_backup.py | 4 +- 6 files changed, 495 insertions(+), 3 deletions(-) create mode 100644 alembic/versions/0093_rule_versions.py create mode 100644 src/scribe/models/rule_version.py create mode 100644 tests/test_integration_backup_rule_version_roundtrip.py diff --git a/alembic/versions/0093_rule_versions.py b/alembic/versions/0093_rule_versions.py new file mode 100644 index 0000000..1040196 --- /dev/null +++ b/alembic/versions/0093_rule_versions.py @@ -0,0 +1,83 @@ +"""rules gain an edit history — rule_versions (milestone 323 step 1) + +Revision ID: 0093 +Revises: 0092 +Create Date: 2026-08-29 + +The sibling `note_versions` has had for a long time. A note's every meaningful +edit is snapshotted, and the design-system note calls that history "the +changelog". A RULE — which binds behaviour on every session that loads it — +had nothing: an edit destroyed what it used to say, with no record anywhere. + +Rescoping rule 79 on 2026-08-29 is what surfaced it. The superseded statement +had to be hand-copied into a task log to survive the edit (#3237), which is +not a process, it is a person remembering. The more consequential record had +the weaker protection. + +Three things are deliberately NOT copied from note_versions, and each is a +guard that exists there for a reason that does not hold here: + +- **No pruning, and no MAX_VERSIONS.** That cap defends against note autosave + filling every slot. Rules have no autosave; every edit is a deliberate + update_rule. A rule is edited a handful of times in its life, and capping + invites losing the one edit somebody needed. +- **No pin columns.** `pin_kind`/`pin_label` exist so a note's version can + survive that pruning. With nothing pruning, a pin protects a row that was + never at risk. +- **No minimum interval.** 300 seconds between snapshots is also an autosave + defence; here it would only ever discard a second deliberate edit. + +`user_id` is the ACTOR rather than the owner, and is SET NULL rather than +CASCADE: deleting a user must not erase the history of the rules they edited. +The edit still happened and the rule still binds because of it. + +No CHECK constraint, so rule 36 does not apply. Nothing is backfilled — a +migration cannot invent the text a rule used to have, and inventing "the +current text, as of now" would be worse than an empty history, because it +would look like a record of an edit that never occurred. +""" +import sqlalchemy as sa +from alembic import op + +revision = "0093" +down_revision = "0092" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + op.create_table( + "rule_versions", + sa.Column("id", sa.BigInteger(), primary_key=True), + sa.Column( + "rule_id", + sa.BigInteger(), + sa.ForeignKey("rules.id", ondelete="CASCADE"), + nullable=False, + ), + sa.Column( + "user_id", + sa.BigInteger(), + sa.ForeignKey("users.id", ondelete="SET NULL"), + nullable=True, + ), + sa.Column("title", sa.Text(), nullable=False, server_default=""), + sa.Column("statement", sa.Text(), nullable=False, server_default=""), + sa.Column("why", sa.Text(), nullable=True), + sa.Column("how_to_apply", sa.Text(), nullable=True), + sa.Column("when_to_apply", sa.Text(), nullable=True), + sa.Column("tier", sa.Text(), nullable=True), + sa.Column("verify_with", sa.Text(), nullable=True), + sa.Column("expires_when", sa.Text(), nullable=True), + sa.Column("created_at", sa.DateTime(timezone=True), nullable=False), + ) + # The only query this table serves is "the history of THIS rule, newest + # first" — unlike 0092's columns, which are read by an operator-initiated + # sweep over the whole set. Every read here is keyed on rule_id, so the + # index earns its write cost immediately rather than on a hunch. + op.create_index("ix_rule_versions_rule_id", "rule_versions", ["rule_id"]) + + +def downgrade() -> None: + op.drop_index("ix_rule_versions_rule_id", table_name="rule_versions") + op.drop_table("rule_versions") diff --git a/src/scribe/models/__init__.py b/src/scribe/models/__init__.py index d55a116..203d962 100644 --- a/src/scribe/models/__init__.py +++ b/src/scribe/models/__init__.py @@ -33,6 +33,7 @@ from scribe.models.milestone import Milestone # noqa: E402, F401 from scribe.models.task_log import TaskLog # noqa: E402, F401 from scribe.models.note_draft import NoteDraft # noqa: E402, F401 from scribe.models.note_version import NoteVersion # noqa: E402, F401 +from scribe.models.rule_version import RuleVersion # noqa: E402, F401 from scribe.models.note_supersession import NoteSupersession # noqa: E402, F401 from scribe.models.group import Group, GroupMembership # noqa: E402, F401 from scribe.models.share import NoteShare, ProjectShare # noqa: E402, F401 diff --git a/src/scribe/models/rule_version.py b/src/scribe/models/rule_version.py new file mode 100644 index 0000000..a8f8fac --- /dev/null +++ b/src/scribe/models/rule_version.py @@ -0,0 +1,91 @@ +from datetime import datetime + +from sqlalchemy import BigInteger, ForeignKey, Text +from sqlalchemy.orm import Mapped, mapped_column + +from scribe.models import Base +from scribe.models.base import CreatedAtMixin, iso + + +class RuleVersion(Base, CreatedAtMixin): + """One snapshot of a rule's text, taken before an edit overwrote it. + + THE SIBLING NOTES ALREADY HAD. `note_versions` has existed for a long + time, and the design-system note calls its history "the changelog". Rules + — which BIND BEHAVIOUR on every session that loads them — had nothing, so + an edit destroyed what the rule used to say. Rescoping rule 79 on + 2026-08-29 meant hand-copying the superseded statement into a task log to + keep it (#3237). The more consequential record had the weaker protection. + + `CreatedAtMixin`, not `TimestampMixin`: a version is an EVENT. It is + written once and never updated, so an `updated_at` on it would be a column + that can only ever lie. + + WHAT IS DELIBERATELY DIFFERENT FROM NoteVersion (milestone 323): + + - `user_id` is the ACTOR — who made the edit — where NoteVersion's is the + owner, because `update_note` passes an owner-scoped id. For an audit + trail over a binding instruction, "who changed this" is the question + being asked, and a rule is editable by anyone with rulebook access. + - No `pin_kind` / `pin_label`. Those exist so a note's version can survive + autosave pruning. Nothing prunes here, so a pin would protect a row that + was never at risk. + - TEXT ONLY. A rule's Systems and its typed relations are edges with their + own lifecycle; folding them in would make one word, "version", mean two + different things — the rule's wording, and the rule's place in the + graph. `verified_at` is likewise absent: it is a stamp about a check, + not a property of the text, and step 2's snapshot is taken before + update_rule clears it precisely so the history shows the check that was + in force when this wording was written. + """ + + __tablename__ = "rule_versions" + + id: Mapped[int] = mapped_column(BigInteger, primary_key=True) + rule_id: Mapped[int] = mapped_column( + BigInteger, ForeignKey("rules.id", ondelete="CASCADE"), index=True + ) + # The actor. SET NULL rather than CASCADE: deleting a user must not erase + # the history of the rules they edited — the edit still happened, and the + # rule is still binding because of it. NoteVersion cascades because a + # note's versions belong to its owner; a rule's belong to the rule. + user_id: Mapped[int | None] = mapped_column( + BigInteger, ForeignKey("users.id", ondelete="SET NULL"), nullable=True + ) + + title: Mapped[str] = mapped_column(Text, default="") + statement: Mapped[str] = mapped_column(Text, default="") + why: Mapped[str | None] = mapped_column(Text, nullable=True) + how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) + when_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) + tier: Mapped[str | None] = mapped_column(Text, nullable=True) + verify_with: Mapped[str | None] = mapped_column(Text, nullable=True) + expires_when: Mapped[str | None] = mapped_column(Text, nullable=True) + + def to_dict(self, include_text: bool = True) -> dict: + """The row. `include_text=False` gives the listing form. + + A rule's `statement` and `why` run to thousands of characters — rule + 149's `why` alone is longer than most notes — so a history LIST that + carried every field would be unreadable and expensive. The listing + answers "when, and by whom"; opening one answers "and what did it + say". Same split NoteVersion makes with `include_body`. + """ + out: dict = { + "id": self.id, + "rule_id": self.rule_id, + "user_id": self.user_id, + "title": self.title, + "created_at": iso(self.created_at), + } + if include_text: + out.update({ + "statement": self.statement, + "why": self.why or "", + "how_to_apply": self.how_to_apply or "", + "when_to_apply": self.when_to_apply or "", + "tier": self.tier or "", + "verify_with": self.verify_with or "", + "expires_when": self.expires_when or "", + }) + return out diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 232afc2..a791824 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -9,6 +9,7 @@ from scribe.models.note import Note from scribe.models.note_draft import NoteDraft from scribe.models.note_supersession import NoteSupersession from scribe.models.note_version import NoteVersion +from scribe.models.rule_version import RuleVersion from scribe.models.design_system import DesignSystem, DesignToken from scribe.models.note_usage import NoteUsageEvent from scribe.models.canonical_system import CanonicalSystem @@ -60,8 +61,9 @@ logger = logging.getLogger(__name__) # note, every issue and spike into `work`, and every plan reduced to a title. # _COLUMN_EXCLUSIONS and its guard landed with it, so the next such column # fails the build instead. +# v13 (2026-08) added rule_versions — a rule's edit history (milestone 323). # Bump when the serialized schema changes. -BACKUP_VERSION = 12 +BACKUP_VERSION = 13 # Every table this backup carries, by its REAL name. Paired with _NOT_INCLUDED # below, these two lists must together account for the entire schema — which is @@ -87,6 +89,9 @@ _BACKED_UP = [ "canonical_systems", # v10 (2026-08): a rule's area tag and its typed edges (milestone 307). "rule_systems", "rule_relations", + # v13 (2026-08): a rule's edit history (milestone 323). note_versions has + # always travelled; its sibling has no excuse not to. + "rule_versions", ] # Tables intentionally NOT in the backup, surfaced in the payload so the gap is @@ -176,6 +181,10 @@ _COLUMN_EXCLUSIONS: dict[str, set[str]] = { "design_systems": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "design_tokens": {"deleted_at", "deleted_batch_id", "created_at", "updated_at"}, "repo_bindings": {"id", "created_at", "updated_at"}, + # Everything travels. A version row IS the audit trail, so a column left + # behind is a fact about a binding instruction that no longer exists + # anywhere. + "rule_versions": set(), # Serialised via the model's own to_dict(), so a column reaches the backup # the moment it reaches that method — and the guard still catches one that # reaches neither. @@ -472,6 +481,23 @@ def _note_version_rows(rows) -> list[dict]: ] +def _rule_version_rows(rows) -> list[dict]: + """A rule's edit history. Sibling of _note_version_rows, and it travels for + the same reason: a version is the only record of what a binding + instruction used to say, and nothing can recompute it.""" + return [ + { + "id": rv.id, "rule_id": rv.rule_id, "user_id": rv.user_id, + "title": rv.title, "statement": rv.statement, "why": rv.why, + "how_to_apply": rv.how_to_apply, "when_to_apply": rv.when_to_apply, + "tier": rv.tier, "verify_with": rv.verify_with, + "expires_when": rv.expires_when, + "created_at": rv.created_at.isoformat(), + } + for rv in rows + ] + + def _setting_rows(rows) -> list[dict]: return [{"user_id": s.user_id, "key": s.key, "value": s.value} for s in rows] @@ -553,6 +579,9 @@ async def export_full_backup() -> dict: note_versions = (await session.execute( select(NoteVersion).order_by(NoteVersion.note_id, NoteVersion.id) )).scalars().all() + rule_versions = (await session.execute( + select(RuleVersion).order_by(RuleVersion.rule_id, RuleVersion.id) + )).scalars().all() settings = (await session.execute(select(Setting))).scalars().all() systems = (await session.execute(select(System))).scalars().all() canonical_systems = (await session.execute( @@ -617,6 +646,7 @@ async def export_full_backup() -> dict: "task_logs": _task_log_rows(task_logs), "note_drafts": _note_draft_rows(note_drafts), "note_versions": _note_version_rows(note_versions), + "rule_versions": _rule_version_rows(rule_versions), "settings": _setting_rows(settings), "rulebooks": _rulebook_rows(rulebooks), "rulebook_topics": _topic_rows(topics), @@ -753,6 +783,14 @@ async def export_user_backup(user_id: int) -> dict: .join(CanonicalSystem, CanonicalSystem.id == rule_systems_t.c.canonical_id) .where(rule_systems_t.c.rule_id.in_(_rule_ids)) )).all() if _rule_ids else [] + # Scoped through the RULE, not the version's user_id. That column is + # the ACTOR (milestone 323), so filtering on it would carry the + # versions this user wrote on someone ELSE's rule and drop the ones + # someone else wrote on theirs — the opposite of a per-user export. + rule_versions = (await session.execute( + select(RuleVersion).where(RuleVersion.rule_id.in_(_rule_ids)) + .order_by(RuleVersion.rule_id, RuleVersion.id) + )).scalars().all() if _rule_ids else [] rule_relations = (await session.execute( select(RuleRelation).where( RuleRelation.from_rule_id.in_(_rule_ids), @@ -801,6 +839,7 @@ async def export_user_backup(user_id: int) -> dict: "task_logs": _task_log_rows(task_logs), "note_drafts": _note_draft_rows(note_drafts), "note_versions": _note_version_rows(note_versions), + "rule_versions": _rule_version_rows(rule_versions), "settings": _setting_rows(settings), "rulebooks": _rulebook_rows(rulebooks), "rulebook_topics": _topic_rows(topics), @@ -958,7 +997,7 @@ async def _restore_v2(data: dict) -> dict: "design_tokens": 0, "note_usage_events": 0, "repo_bindings": 0, "note_supersessions": 0, "code_shapes": 0, "code_shape_events": 0, "code_shape_uses": 0, "canonical_systems": 0, - "rule_systems": 0, "rule_relations": 0, + "rule_systems": 0, "rule_relations": 0, "rule_versions": 0, } async with async_session() as session: @@ -1327,6 +1366,32 @@ async def _restore_v2(data: dict) -> dict: )) stats["rule_relations"] += 1 + # A rule's edit history (milestone 323). Must come after the rules + # themselves — rule_id_map is only populated above — and both ids are + # ids in the SOURCE database, which is #3182's arose_from_id trap. + for rv in data.get("rule_versions", []): + mapped_rid = rule_id_map.get(rv.get("rule_id", 0)) + if mapped_rid is None: + continue + # Unlike NoteVersion, an unmappable user does NOT drop the row. + # user_id is the ACTOR and is nullable by design: the column is + # SET NULL precisely so history outlives the account that wrote + # it. Skipping here would delete the record the FK preserves. + session.add(RuleVersion( + rule_id=mapped_rid, + user_id=user_id_map.get(rv.get("user_id") or 0), + title=rv.get("title", ""), + statement=rv.get("statement", ""), + why=rv.get("why"), + how_to_apply=rv.get("how_to_apply"), + when_to_apply=rv.get("when_to_apply"), + tier=rv.get("tier"), + verify_with=rv.get("verify_with"), + expires_when=rv.get("expires_when"), + created_at=_dt(rv.get("created_at")), + )) + stats["rule_versions"] += 1 + # 15. Systems for sy_data in data.get("systems", []): mapped_uid = user_id_map.get(sy_data.get("user_id", 0)) diff --git a/tests/test_integration_backup_rule_version_roundtrip.py b/tests/test_integration_backup_rule_version_roundtrip.py new file mode 100644 index 0000000..7e5d3c1 --- /dev/null +++ b/tests/test_integration_backup_rule_version_roundtrip.py @@ -0,0 +1,250 @@ +"""Real-Postgres round trip for rule_versions (milestone 323 step 1). + +Two things here cannot be shown with mocks, and both are the kind that fail +QUIETLY — a restore reports success and hands back history that is wrong. + +1. **`rule_id` is remapped, not copied.** It is an id in the SOURCE database. + A restore that writes it straight through succeeds and attaches every + snapshot to whatever rule happens to hold that number here — an edit + history filed against the wrong binding instruction, which is worse than + no history at all. This is #3182's `arose_from_id` trap on a new table. + +2. **A null actor does not drop the row.** `user_id` is SET NULL precisely so + history outlives the account that wrote it, so the restore deliberately + diverges from its `NoteVersion` sibling, which skips a version it cannot + map to a user. Nothing about the code says which of the two shapes is + intended; without this test, "make it match the sibling" reads as a tidy-up + and silently deletes the record the FK was chosen to preserve. + +These drive the REAL `restore_full_backup`. A test that re-derived the remap +would agree with whatever the product does, including nothing. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select + +from scribe.models import async_session +from scribe.models.rule_version import RuleVersion +from scribe.models.rulebook import Rule, Rulebook, RulebookTopic +from scribe.models.user import User +from scribe.services import backup +from tests.helpers import ensure_user + +pytestmark = [pytest.mark.integration, pytest.mark.usefixtures("_dispose_engine")] + +OWNER_USERNAME = "rule_version_roundtrip_owner" +RESTORED_USERNAME = "rule_version_roundtrip_restored" + + +async def _purge_books(username: str) -> None: + """user -> rulebook -> topic -> rule -> rule_version is ON DELETE CASCADE + the whole way down, and no ORM relationships are configured, so dropping + the books clears every row this file made under them.""" + async with async_session() as s: + users = (await s.execute( + select(User).where(User.username == username) + )).scalars().all() + for user in users: + books = (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == user.id) + )).scalars().all() + for book in books: + await s.delete(book) + await s.commit() + + +async def _purge_restored() -> None: + """The restored user is minted by the restore itself, so it goes too.""" + await _purge_books(RESTORED_USERNAME) + 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() + + +@pytest_asyncio.fixture(autouse=True) +async def _no_leftovers(): + """The usernames are fixed, so a previous failed run would leave rows that + make the fixtures below pick the wrong user — or hit `.one()` with two.""" + await _purge_restored() + await _purge_books(OWNER_USERNAME) + yield + await _purge_restored() + await _purge_books(OWNER_USERNAME) + + +@pytest_asyncio.fixture +async def source(): + """One rule with two snapshots: one written by a user who still exists, + one whose actor is already gone. + + Both are needed. With only the attributed version, dropping unmappable + rows would pass; with only the orphaned one, so would dropping the actor + from every row. + """ + async with async_session() as s: + owner = await ensure_user(s, OWNER_USERNAME) + uid = owner.id + await s.commit() + + async with async_session() as s: + book = Rulebook(owner_user_id=uid, title="Environment facts") + s.add(book) + await s.flush() + topic = RulebookTopic(rulebook_id=book.id, title="ci") + s.add(topic) + await s.flush() + rule = Rule( + topic_id=topic.id, + title="The runner has no bash", + statement="Write every `run:` step in POSIX sh.", + verify_with="read the workflow's shell setting", + ) + s.add(rule) + await s.flush() + s.add_all([ + RuleVersion( + rule_id=rule.id, user_id=uid, + title="The runner has no bash", + statement="Use sh.", + why="the image ships no bash", + verify_with="read the workflow's shell setting", + tier="always_on", + ), + # The actor is already gone — what SET NULL leaves behind. + RuleVersion( + rule_id=rule.id, user_id=None, + title="The runner has no bash", + statement="Use POSIX sh in run steps.", + tier="always_on", + ), + ]) + await s.commit() + book_id, rule_id = book.id, rule.id + + async with async_session() as s: + user_rows = backup._user_rows( + [(await s.execute(select(User).where(User.id == uid))).scalars().one()] + ) + book_rows = backup._rulebook_rows( + [(await s.execute(select(Rulebook).where(Rulebook.id == book_id))) + .scalars().one()] + ) + topic_rows = backup._topic_rows( + (await s.execute( + select(RulebookTopic).where(RulebookTopic.rulebook_id == book_id) + )).scalars().all() + ) + rule_rows = backup._rule_rows( + [(await s.execute(select(Rule).where(Rule.id == rule_id))).scalars().one()] + ) + version_rows = backup._rule_version_rows( + (await s.execute( + select(RuleVersion).where(RuleVersion.rule_id == rule_id) + .order_by(RuleVersion.id) + )).scalars().all() + ) + # The restore mints a NEW user from the payload, so the restored corpus is + # separate from the source one — which is what makes the id assertion + # below able to fail. + user_rows[0]["username"] = RESTORED_USERNAME + + yield { + "payload": { + "version": backup.BACKUP_VERSION, + "users": user_rows, + "rulebooks": book_rows, + "rulebook_topics": topic_rows, + "rules": rule_rows, + "rule_versions": version_rows, + }, + "source_rule_id": rule_id, + "source_user_id": uid, + } + + async with async_session() as s: + book = await s.get(Rulebook, book_id) + if book is not None: + await s.delete(book) + await s.commit() + + +@pytest_asyncio.fixture +async def restored(source): + await backup.restore_full_backup(source["payload"]) + async with async_session() as s: + user = (await s.execute( + select(User).where(User.username == RESTORED_USERNAME) + )).scalars().first() + assert user is not None, "the payload's user was not restored" + book = (await s.execute( + select(Rulebook).where(Rulebook.owner_user_id == user.id) + )).scalars().one() + topic = (await s.execute( + select(RulebookTopic).where(RulebookTopic.rulebook_id == book.id) + )).scalars().one() + rule = (await s.execute( + select(Rule).where(Rule.topic_id == topic.id) + )).scalars().one() + versions = (await s.execute( + select(RuleVersion).where(RuleVersion.rule_id == rule.id) + .order_by(RuleVersion.id) + )).scalars().all() + return {"user": user, "rule": rule, "versions": versions, "source": source} + + +async def test_both_snapshots_come_back(restored): + """The count first: everything below reads the same on an empty list, so + without this a restore that silently dropped both would look like a pass + on the shape assertions.""" + assert len(restored["versions"]) == 2 + + +async def test_the_history_attaches_to_the_RESTORED_rule(restored): + """#3182's trap. The source rule still exists and holds a different id, so + a straight-through copy would file this history against it — or against + whatever unrelated rule owns that number.""" + new_rule_id = restored["rule"].id + source_rule_id = restored["source"]["source_rule_id"] + assert new_rule_id != source_rule_id, ( + "the restore reused the source id, so this test cannot tell a remap " + "from a copy — the fixture is not proving what it claims" + ) + assert {v.rule_id for v in restored["versions"]} == {new_rule_id} + + +async def test_the_actor_is_remapped_to_the_restored_user(restored): + """`user_id` is an id in the source database too — the same trap, on the + column that answers "who changed this".""" + attributed = [v for v in restored["versions"] if v.user_id is not None] + assert len(attributed) == 1 + assert attributed[0].user_id == restored["user"].id + assert attributed[0].user_id != restored["source"]["source_user_id"] + + +async def test_a_snapshot_with_no_actor_survives(restored): + """The deliberate divergence from NoteVersion. `user_id` is SET NULL so + that history outlives the account that wrote it; skipping the row on an + unmappable user would throw away exactly what the FK preserves.""" + orphaned = [v for v in restored["versions"] if v.user_id is None] + assert len(orphaned) == 1, ( + "the version whose actor was already gone did not come back. A rule's " + "history is the only record of what it used to say — losing it " + "because nobody can be credited is the wrong trade." + ) + assert orphaned[0].statement == "Use POSIX sh in run steps." + + +async def test_the_text_survives(restored): + """The whole point of the table: what the rule USED to say. A restore that + kept the rows and lost their wording would preserve a changelog of empty + entries.""" + by_statement = {v.statement: v for v in restored["versions"]} + assert set(by_statement) == {"Use sh.", "Use POSIX sh in run steps."} + assert by_statement["Use sh."].why == "the image ships no bash" + assert by_statement["Use sh."].verify_with == ( + "read the workflow's shell setting" + ) + assert by_statement["Use sh."].tier == "always_on" diff --git a/tests/test_services_backup.py b/tests/test_services_backup.py index b58afee..274fde3 100644 --- a/tests/test_services_backup.py +++ b/tests/test_services_backup.py @@ -23,7 +23,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 == 12 + assert backup.BACKUP_VERSION == 13 def _exportable_note(**over): @@ -134,6 +134,7 @@ def _column_guard_targets(): from scribe.models.note_supersession import NoteSupersession from scribe.models.note_usage import NoteUsageEvent from scribe.models.note_version import NoteVersion + from scribe.models.rule_version import RuleVersion from scribe.models.project import Project from scribe.models.repo_binding import RepoBinding from scribe.models.rulebook import Rule, Rulebook, RulebookTopic, RuleRelation @@ -154,6 +155,7 @@ def _column_guard_targets(): "rulebooks": (Rulebook, backup._rulebook_rows), "rulebook_topics": (RulebookTopic, backup._topic_rows), "rules": (Rule, backup._rule_rows), + "rule_versions": (RuleVersion, backup._rule_version_rows), "systems": (System, lambda rows: backup._system_rows(rows, {})), "canonical_systems": (CanonicalSystem, backup._canonical_system_rows), "record_systems": (RecordSystem, backup._record_system_rows),