diff --git a/alembic/versions/0098_rule_kind.py b/alembic/versions/0098_rule_kind.py new file mode 100644 index 0000000..35bb0aa --- /dev/null +++ b/alembic/versions/0098_rule_kind.py @@ -0,0 +1,90 @@ +"""rules gain a `kind` — a preference is a rule that does not bind (#3849) + +Revision ID: 0098 +Revises: 0097 +Create Date: 2026-09-10 + +A rule says what must be followed. There was no way to record the other +thing the operator kept writing rules for: **how they want work done**. +Several rules in a mature rulebook are not really rules — pace this kind of +debugging, hand off an action with its reason, end a finding with an offer. +Ignoring one of those does not break anything or cross a boundary; it costs +consistency. They were written as rules because a rule was the only record +that is global, keyed to a situation, and delivered when that situation +arrives. + +So: `kind`. `rule` binds. `preference` describes how this person wants it +done, and — the part that makes it its own kind rather than a softer label — +**it is expected to change as the work teaches it.** The agent updates a +preference in the ordinary course of working, where a rule waits for its +author. + +WHY A COLUMN AND NOT A TABLE. Everything a preference needs already exists on +`rules` and nowhere else: `when_to_apply` as a real column, a +trigger-dominated embedding document, ownership-scoped search that is +deliberately not filtered to one project, three retrieval arms with per-arm +telemetry, typed relations, and versioning. The two differ in exactly one +dimension — force — and one dimension is a field. + +The drift machinery is the decisive part. `rule_versions` already snapshots +every write, and `rule_relations.overrides` already models "this supersedes +that for its scope". A separate table would have rebuilt both, and moving +existing rows into it would have changed their ids — silently invalidating +every record in the corpus that cites a rule by number. + +DEFAULTS TO `rule`, so this migration changes NOTHING about what binds. An +install upgrades and every existing row keeps the force it had. That is the +same reasoning 0088 used for `tier`, and it is the reason both are safe to +apply without reading the data first. + +The CHECK is created with the column (rule 36: there is no prior constraint, +so the pair is created together — a value added to it LATER does DROP + ADD +in one migration). + +`rule_versions` GETS THE COLUMN TOO, and that half is not bookkeeping. +`record_if_changed` decides whether an edit is worth a snapshot by comparing +the fields a version carries; a field a version does not carry is a field +whose change records NO HISTORY AT ALL. Without this, turning a rule into a +preference — the single most consequential edit either kind can undergo, +because it is the moment something stops binding — would leave the history +silent. Nullable and no CHECK there, matching `tier`: a version is a record +of what was, and a constraint on it would refuse to store a kind later +dropped from the live whitelist. + +Downgrade drops all three. Nothing reads `kind` for correctness; a rule that +was a preference simply becomes a rule again, which is the safe direction. +""" +import sqlalchemy as sa +from alembic import op + + +revision = "0098" +down_revision = "0097" +branch_labels = None +depends_on = None + +# Kept in one place so upgrade and the CHECK agree by construction — 0088's +# idiom, for the same reason. +_KINDS = ("rule", "preference") + + +def _in_list(column: str, values: tuple[str, ...]) -> str: + return f"{column} IN (" + ", ".join(f"'{v}'" for v in values) + ")" + + +def upgrade() -> None: + op.add_column( + "rules", + sa.Column("kind", sa.Text(), nullable=False, server_default="rule"), + ) + op.create_check_constraint("ck_rules_kind", "rules", _in_list("kind", _KINDS)) + # Nullable, no CHECK — see the module docstring. A version written before + # this migration genuinely does not know, and NULL there means "not + # recorded", never "was a rule". + op.add_column("rule_versions", sa.Column("kind", sa.Text(), nullable=True)) + + +def downgrade() -> None: + op.drop_column("rule_versions", "kind") + op.drop_constraint("ck_rules_kind", "rules", type_="check") + op.drop_column("rules", "kind") diff --git a/src/scribe/models/rule_version.py b/src/scribe/models/rule_version.py index a8f8fac..477a186 100644 --- a/src/scribe/models/rule_version.py +++ b/src/scribe/models/rule_version.py @@ -59,6 +59,12 @@ class RuleVersion(Base, CreatedAtMixin): 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) + # Carried so that a change of FORCE leaves a trace. `record_if_changed` + # snapshots only the fields a version holds, so a kind omitted here would + # make "this stopped binding" the one edit with no history behind it. + # NULL means a version older than migration 0098 — not recorded, never + # "was a rule". + kind: 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) @@ -85,6 +91,7 @@ class RuleVersion(Base, CreatedAtMixin): "how_to_apply": self.how_to_apply or "", "when_to_apply": self.when_to_apply or "", "tier": self.tier or "", + "kind": self.kind or "", "verify_with": self.verify_with or "", "expires_when": self.expires_when or "", }) diff --git a/src/scribe/models/rulebook.py b/src/scribe/models/rulebook.py index ac581cb..74dfc8e 100644 --- a/src/scribe/models/rulebook.py +++ b/src/scribe/models/rulebook.py @@ -105,6 +105,24 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): # default preserves existing behaviour exactly: nothing stops binding # because of an upgrade. CHECK ck_rules_tier (migration 0088, rule 36). tier: Mapped[str] = mapped_column(Text, default="always_on", server_default="always_on") + # WHAT KIND of instruction this is — force, where `tier` is delivery. + # `rule` must be FOLLOWED: ignoring it breaks something or crosses a + # boundary. `preference` is how this person wants work DONE: ignoring it + # costs consistency, not correctness. + # + # The second half is what makes it a kind rather than a softer label — a + # preference is expected to CHANGE as the work teaches it, and the agent + # updates it in the ordinary course of working, where a rule waits for its + # author. So one column decides two behaviours: whether create's approval + # gate fires, and which voice the injected line speaks in. + # + # Lives here and not in its own table because a preference needs exactly + # what a rule has and a note does not — a trigger column, a + # trigger-dominated document, ownership-scoped search, the retrieval arms, + # relations, and `rule_versions`, which is where its drift is recorded. + # Defaults to `rule` so nothing changes force on upgrade. + # CHECK ck_rules_kind (migration 0098, rule 36). + kind: Mapped[str] = mapped_column(Text, default="rule", server_default="rule") why: Mapped[str | None] = mapped_column(Text, nullable=True) how_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True) # The three fields that tell a CONSTRAINT apart from a NORM (milestone @@ -144,6 +162,13 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin): "statement": self.statement, "when_to_apply": self.when_to_apply or "", "tier": self.tier, + # Unconditional, unlike the `if present` keys below. A reader + # deciding how much force a record carries must never infer it + # from an ABSENT key: "no kind field" and "kind is rule" would be + # the same payload, and that equivalence is the defect shape this + # codebase keeps re-encountering. Twenty bytes buys an answer that + # cannot be misread. + "kind": self.kind or "rule", "why": self.why or "", "how_to_apply": self.how_to_apply or "", "verify_with": self.verify_with or "", diff --git a/src/scribe/services/backup.py b/src/scribe/services/backup.py index 50e9f82..1a8f8d8 100644 --- a/src/scribe/services/backup.py +++ b/src/scribe/services/backup.py @@ -513,7 +513,7 @@ def _rule_version_rows(rows) -> list[dict]: "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, + "tier": rv.tier, "kind": rv.kind, "verify_with": rv.verify_with, "expires_when": rv.expires_when, "created_at": rv.created_at.isoformat(), } @@ -575,7 +575,7 @@ def _rule_rows(rows) -> list[dict]: "id": r.id, "topic_id": r.topic_id, "project_id": r.project_id, "title": r.title, "statement": r.statement, "why": r.why, "how_to_apply": r.how_to_apply, "order_index": r.order_index, - "when_to_apply": r.when_to_apply, "tier": r.tier, + "when_to_apply": r.when_to_apply, "tier": r.tier, "kind": r.kind, "verify_with": r.verify_with, "expires_when": r.expires_when, "verified_at": r.verified_at.isoformat() if r.verified_at else None, "arose_from_id": r.arose_from_id, @@ -1283,6 +1283,11 @@ async def _restore_v2(data: dict) -> dict: # is the pre-0088 behaviour, so an old backup restores rules # that bind exactly as they did when it was taken. tier=r_data.get("tier") or "always_on", + # Same shape, same reason: a file written before 0098 has no + # kind, and every rule in it was a rule. Defaulting the other + # way would restore an old backup with things that had always + # bound quietly no longer binding. + kind=r_data.get("kind") or "rule", verify_with=r_data.get("verify_with") or None, expires_when=r_data.get("expires_when") or None, # Restored as-is, NOT reset to null. `verified_at` records @@ -1423,6 +1428,10 @@ async def _restore_v2(data: dict) -> dict: how_to_apply=rv.get("how_to_apply"), when_to_apply=rv.get("when_to_apply"), tier=rv.get("tier"), + # NOT defaulted, unlike the rule above. A version records what + # was; absent means nobody wrote it down, and inventing "rule" + # here would put an artifact where a measurement belongs. + kind=rv.get("kind"), verify_with=rv.get("verify_with"), expires_when=rv.get("expires_when"), created_at=_dt(rv.get("created_at")), diff --git a/src/scribe/services/rule_versions.py b/src/scribe/services/rule_versions.py index 2d905a4..004d481 100644 --- a/src/scribe/services/rule_versions.py +++ b/src/scribe/services/rule_versions.py @@ -36,7 +36,7 @@ from scribe.models.rule_version import RuleVersion # 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", + "tier", "kind", "verify_with", "expires_when", ) diff --git a/src/scribe/services/rulebooks.py b/src/scribe/services/rulebooks.py index c8e380e..8b1a40e 100644 --- a/src/scribe/services/rulebooks.py +++ b/src/scribe/services/rulebooks.py @@ -295,6 +295,9 @@ async def _assert_rulebook_rule_owned(session, rule_id: int, user_id: int) -> No # two in step; this keeps the error readable). TIERS = ("always_on", "conditional") RELATION_KINDS = ("co_surfaces", "overrides", "elaborates") +# Migration 0098's CHECK. `rule` binds; `preference` is how the operator +# wants work done — see the model comment for why both live on one table. +KINDS = ("rule", "preference") # The rule columns that are nullable, and therefore the ones where EMPTY has @@ -319,6 +322,22 @@ def _valid_tier(tier: str) -> str: return tier if tier in TIERS else "always_on" +def _valid_kind(kind: str) -> str: + """An unrecognised kind falls back to `rule` — the SAFE direction. + + Same shape as _valid_tier and the same argument, pointed at force instead + of delivery. A preference wrongly treated as binding costs a little + friction: the reader is told something is required that was only + preferred. A rule wrongly treated as a preference costs the thing the rule + was written to prevent, and costs it silently, because nothing downstream + can tell a softened rule from a preference that was always one. + + Between a reader who is too careful and a reader who is not careful + enough, the typo should produce the first. + """ + return kind if kind in KINDS else "rule" + + # Re-exported, not redefined. Notes gained the same trio in milestone 317 and # this reading of it is genuinely common, so it moved to services/verification # — the DRY win note 3163 names, as against sharing the QUERY, which the two @@ -351,6 +370,13 @@ def rule_brief(rule: Rule, **extra) -> dict: "statement": rule.statement, "topic_id": rule.topic_id, "tier": rule.tier, + # Unconditional, and the payload cost is accepted deliberately. Every + # other optional key below is attached only when present, because an + # absent key should never read as a capability the record lacks. Force + # is the opposite case: a reader seeing no `kind` would have to assume + # one, and the assumption it would reach for — "this binds" — is the + # expensive one to get wrong in the other direction. Say it outright. + "kind": rule.kind or "rule", "updated_at": rule.updated_at.date().isoformat() if rule.updated_at else None, } # Attached only when present (#2483: never a null key that reads as a @@ -478,7 +504,7 @@ async def create_rule( topic_id: int, user_id: int, title: str, statement: str, why: str = "", how_to_apply: str = "", order_index: int = 0, when_to_apply: str = "", tier: str = "always_on", arose_from_id: int = 0, - verify_with: str = "", expires_when: str = "", + verify_with: str = "", expires_when: str = "", kind: str = "rule", ) -> Rule: async with async_session() as session: await _assert_topic_owned(session, topic_id, user_id) @@ -488,6 +514,7 @@ async def create_rule( statement=statement, when_to_apply=when_to_apply or None, tier=_valid_tier(tier), + kind=_valid_kind(kind), why=why or None, how_to_apply=how_to_apply or None, verify_with=verify_with or None, @@ -506,7 +533,7 @@ async def create_project_rule( project_id: int, user_id: int, title: str, statement: str, why: str = "", how_to_apply: str = "", order_index: int = 0, when_to_apply: str = "", tier: str = "always_on", arose_from_id: int = 0, - verify_with: str = "", expires_when: str = "", + verify_with: str = "", expires_when: str = "", kind: str = "rule", ) -> Rule: """Create a rule scoped to a single project (no rulebook ceremony). @@ -522,6 +549,7 @@ async def create_project_rule( statement=statement, when_to_apply=when_to_apply or None, tier=_valid_tier(tier), + kind=_valid_kind(kind), why=why or None, how_to_apply=how_to_apply or None, verify_with=verify_with or None, @@ -752,7 +780,7 @@ async def update_rule( return None allowed = { "title", "statement", "why", "how_to_apply", "order_index", - "when_to_apply", "tier", "arose_from_id", + "when_to_apply", "tier", "kind", "arose_from_id", "verify_with", "expires_when", } check_before = rule.verify_with @@ -770,6 +798,8 @@ async def update_rule( continue if key == "tier": value = _valid_tier(value) + elif key == "kind": + value = _valid_kind(value) elif key in NULLABLE_RULE_TEXT: value = value or None elif key == "arose_from_id": diff --git a/tests/test_integration_rule_kind.py b/tests/test_integration_rule_kind.py new file mode 100644 index 0000000..613df6f --- /dev/null +++ b/tests/test_integration_rule_kind.py @@ -0,0 +1,165 @@ +"""Real-Postgres tests for `rules.kind` and its CHECK (0098, #3849 step 1). + +Rule 36 exists because a value and its constraint drift apart: the code +starts writing a new kind while the database still refuses it, and nothing +catches it until a write fails in front of someone. A mock cannot show that — +it has no CHECK — so the constraint gets a real-DB test, exactly as 0091's +`spike` did. + +THREE HALVES, not one. The positive (a preference writes), the negative (a +typo is refused — without it every other assertion here would pass just as +happily against a table whose CHECK was dropped and never re-added), and the +DEFAULT. + +The default is the one worth spelling out, because it is the half that has no +obvious failure. `kind` is NOT NULL with a server default, and the entire +safety argument for this migration is that every existing row keeps the force +it had. A row written without a kind must read back as `rule` — if the server +default did not apply, an upgraded install gets a NOT NULL violation on its +next rule write, or worse, a column that reads as something other than what +every rule in it has always been. +""" +import pytest +import pytest_asyncio +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError + +from scribe.models import async_session +from scribe.models.rulebook import Rule, Rulebook +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_kind_owner" + + +@pytest_asyncio.fixture +async def topic_id(): + """A topic to hang rules on. + + CLEANED UP AT SETUP, NOT TEARDOWN, for the reason spelled out in + test_integration_rule_versions: the rule write path fires a detached + embedding task that opens its own connection and UPDATEs the row, and a + teardown deleting the rulebook deadlocks against it. Purging at setup runs + on a fresh loop, after the previous test's loop cancelled whatever it left + in flight. + """ + async with async_session() as s: + owner = await ensure_user(s, OWNER_USERNAME) + uid = owner.id + await s.commit() + 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, "Kind fixtures") + topic = await rulebooks_svc.create_topic(book.id, uid, "conventions") + return uid, topic.id + + +async def _write_raw(topic: int, **kw) -> int: + """Straight to the model, bypassing `_valid_kind`. + + The service coerces an unrecognised kind to `rule`, which is right for + callers and useless for testing the CHECK — it means no service call can + ever reach the database with a bad value. These tests are about the + constraint, so they go around the coercion. + """ + async with async_session() as s: + rule = Rule(topic_id=topic, title="t", statement="s", **kw) + s.add(rule) + await s.commit() + return rule.id + + +async def test_a_preference_can_be_written(topic_id): + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "preference" + + +async def test_a_rule_still_writes(topic_id): + """0098 widens nothing — it adds a column — but it must not narrow either.""" + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="rule") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_an_unknown_kind_is_refused(topic_id): + """The half that proves the constraint is there at all. + + Without this, every other assertion in this file would pass against a + table whose CHECK had been dropped — the exact failure rule 36 is written + against. + """ + _uid, topic = topic_id + with pytest.raises(IntegrityError): + await _write_raw(topic, kind="suggestion") + + +async def test_a_rule_written_without_a_kind_defaults_to_rule(topic_id): + """The migration's whole safety claim, asserted rather than assumed. + + Every row that existed before 0098 was a rule and must stay one. This is + the closest a test can get to that: write the way code predating the + column would, and read back the force it should have. + """ + _uid, topic = topic_id + rule_id = await _write_raw(topic) + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_a_rule_can_become_a_preference_after_the_fact(topic_id): + """The read-back is the whole test. + + A version asserting only that the update call succeeded would pass against + code that accepted `kind` and dropped it — which is how the same bug + survived on `task_kind` long enough to be found by hand (#3129). + + This is also the migration path the always-on triage needs: the rules that + turn out to be preferences change one column and keep their id, history + and relations. + """ + uid, topic = topic_id + rule_id = await _write_raw(topic) + await rulebooks_svc.update_rule(rule_id, uid, kind="preference") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "preference" + + +async def test_an_unrecognised_kind_falls_back_rather_than_raising(topic_id): + """`_valid_kind` coerces, matching `_valid_tier`, and the direction matters. + + Deliberately NOT the `task_kind` behaviour, which raises a readable + ValueError. A rule's kind falls back to the binding value, because between + a reader who is too careful and one who is not careful enough, a typo + should produce the first. Asserted here so a later "make it consistent + with task_kind" change has to argue with a test rather than a comment. + """ + uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + await rulebooks_svc.update_rule(rule_id, uid, kind="prefrence") + async with async_session() as s: + assert (await s.get(Rule, rule_id)).kind == "rule" + + +async def test_kind_reaches_both_read_shapes(topic_id): + """`to_dict` and `rule_brief` both carry it, unconditionally. + + Force must never be inferred from an ABSENT key: "no kind field" and + "kind is rule" would be the same payload, and a reader would have to + assume one. Both shapes say it outright, so pin both — the two dicts are + built in different modules and have diverged before. + """ + _uid, topic = topic_id + rule_id = await _write_raw(topic, kind="preference") + async with async_session() as s: + rule = await s.get(Rule, rule_id) + assert rule.to_dict()["kind"] == "preference" + assert rulebooks_svc.rule_brief(rule)["kind"] == "preference"