Preferences — a rule kind that Scribe keeps up to date (#3849 steps 1–3) #150
@@ -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")
|
||||||
@@ -59,6 +59,12 @@ class RuleVersion(Base, CreatedAtMixin):
|
|||||||
how_to_apply: 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)
|
when_to_apply: Mapped[str | None] = mapped_column(Text, nullable=True)
|
||||||
tier: 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)
|
verify_with: Mapped[str | None] = mapped_column(Text, nullable=True)
|
||||||
expires_when: 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 "",
|
"how_to_apply": self.how_to_apply or "",
|
||||||
"when_to_apply": self.when_to_apply or "",
|
"when_to_apply": self.when_to_apply or "",
|
||||||
"tier": self.tier or "",
|
"tier": self.tier or "",
|
||||||
|
"kind": self.kind or "",
|
||||||
"verify_with": self.verify_with or "",
|
"verify_with": self.verify_with or "",
|
||||||
"expires_when": self.expires_when or "",
|
"expires_when": self.expires_when or "",
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -105,6 +105,24 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin):
|
|||||||
# default preserves existing behaviour exactly: nothing stops binding
|
# default preserves existing behaviour exactly: nothing stops binding
|
||||||
# because of an upgrade. CHECK ck_rules_tier (migration 0088, rule 36).
|
# 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")
|
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)
|
why: Mapped[str | None] = mapped_column(Text, nullable=True)
|
||||||
how_to_apply: 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
|
# The three fields that tell a CONSTRAINT apart from a NORM (milestone
|
||||||
@@ -144,6 +162,13 @@ class Rule(Base, TimestampMixin, SoftDeleteMixin):
|
|||||||
"statement": self.statement,
|
"statement": self.statement,
|
||||||
"when_to_apply": self.when_to_apply or "",
|
"when_to_apply": self.when_to_apply or "",
|
||||||
"tier": self.tier,
|
"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 "",
|
"why": self.why or "",
|
||||||
"how_to_apply": self.how_to_apply or "",
|
"how_to_apply": self.how_to_apply or "",
|
||||||
"verify_with": self.verify_with or "",
|
"verify_with": self.verify_with or "",
|
||||||
|
|||||||
@@ -513,7 +513,7 @@ def _rule_version_rows(rows) -> list[dict]:
|
|||||||
"id": rv.id, "rule_id": rv.rule_id, "user_id": rv.user_id,
|
"id": rv.id, "rule_id": rv.rule_id, "user_id": rv.user_id,
|
||||||
"title": rv.title, "statement": rv.statement, "why": rv.why,
|
"title": rv.title, "statement": rv.statement, "why": rv.why,
|
||||||
"how_to_apply": rv.how_to_apply, "when_to_apply": rv.when_to_apply,
|
"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,
|
"expires_when": rv.expires_when,
|
||||||
"created_at": rv.created_at.isoformat(),
|
"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,
|
"id": r.id, "topic_id": r.topic_id, "project_id": r.project_id,
|
||||||
"title": r.title, "statement": r.statement, "why": r.why,
|
"title": r.title, "statement": r.statement, "why": r.why,
|
||||||
"how_to_apply": r.how_to_apply, "order_index": r.order_index,
|
"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,
|
"verify_with": r.verify_with, "expires_when": r.expires_when,
|
||||||
"verified_at": r.verified_at.isoformat() if r.verified_at else None,
|
"verified_at": r.verified_at.isoformat() if r.verified_at else None,
|
||||||
"arose_from_id": r.arose_from_id,
|
"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
|
# is the pre-0088 behaviour, so an old backup restores rules
|
||||||
# that bind exactly as they did when it was taken.
|
# that bind exactly as they did when it was taken.
|
||||||
tier=r_data.get("tier") or "always_on",
|
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,
|
verify_with=r_data.get("verify_with") or None,
|
||||||
expires_when=r_data.get("expires_when") or None,
|
expires_when=r_data.get("expires_when") or None,
|
||||||
# Restored as-is, NOT reset to null. `verified_at` records
|
# 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"),
|
how_to_apply=rv.get("how_to_apply"),
|
||||||
when_to_apply=rv.get("when_to_apply"),
|
when_to_apply=rv.get("when_to_apply"),
|
||||||
tier=rv.get("tier"),
|
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"),
|
verify_with=rv.get("verify_with"),
|
||||||
expires_when=rv.get("expires_when"),
|
expires_when=rv.get("expires_when"),
|
||||||
created_at=_dt(rv.get("created_at")),
|
created_at=_dt(rv.get("created_at")),
|
||||||
|
|||||||
@@ -36,7 +36,7 @@ from scribe.models.rule_version import RuleVersion
|
|||||||
# snapshots would bury the edits somebody is actually looking for.
|
# snapshots would bury the edits somebody is actually looking for.
|
||||||
SNAPSHOT_FIELDS = (
|
SNAPSHOT_FIELDS = (
|
||||||
"title", "statement", "why", "how_to_apply", "when_to_apply",
|
"title", "statement", "why", "how_to_apply", "when_to_apply",
|
||||||
"tier", "verify_with", "expires_when",
|
"tier", "kind", "verify_with", "expires_when",
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -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).
|
# two in step; this keeps the error readable).
|
||||||
TIERS = ("always_on", "conditional")
|
TIERS = ("always_on", "conditional")
|
||||||
RELATION_KINDS = ("co_surfaces", "overrides", "elaborates")
|
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
|
# 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"
|
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
|
# 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
|
# 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
|
# — 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,
|
"statement": rule.statement,
|
||||||
"topic_id": rule.topic_id,
|
"topic_id": rule.topic_id,
|
||||||
"tier": rule.tier,
|
"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,
|
"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
|
# 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,
|
topic_id: int, user_id: int, title: str, statement: str,
|
||||||
why: str = "", how_to_apply: str = "", order_index: int = 0,
|
why: str = "", how_to_apply: str = "", order_index: int = 0,
|
||||||
when_to_apply: str = "", tier: str = "always_on", arose_from_id: 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:
|
) -> Rule:
|
||||||
async with async_session() as session:
|
async with async_session() as session:
|
||||||
await _assert_topic_owned(session, topic_id, user_id)
|
await _assert_topic_owned(session, topic_id, user_id)
|
||||||
@@ -488,6 +514,7 @@ async def create_rule(
|
|||||||
statement=statement,
|
statement=statement,
|
||||||
when_to_apply=when_to_apply or None,
|
when_to_apply=when_to_apply or None,
|
||||||
tier=_valid_tier(tier),
|
tier=_valid_tier(tier),
|
||||||
|
kind=_valid_kind(kind),
|
||||||
why=why or None,
|
why=why or None,
|
||||||
how_to_apply=how_to_apply or None,
|
how_to_apply=how_to_apply or None,
|
||||||
verify_with=verify_with 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,
|
project_id: int, user_id: int, title: str, statement: str,
|
||||||
why: str = "", how_to_apply: str = "", order_index: int = 0,
|
why: str = "", how_to_apply: str = "", order_index: int = 0,
|
||||||
when_to_apply: str = "", tier: str = "always_on", arose_from_id: 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:
|
) -> Rule:
|
||||||
"""Create a rule scoped to a single project (no rulebook ceremony).
|
"""Create a rule scoped to a single project (no rulebook ceremony).
|
||||||
|
|
||||||
@@ -522,6 +549,7 @@ async def create_project_rule(
|
|||||||
statement=statement,
|
statement=statement,
|
||||||
when_to_apply=when_to_apply or None,
|
when_to_apply=when_to_apply or None,
|
||||||
tier=_valid_tier(tier),
|
tier=_valid_tier(tier),
|
||||||
|
kind=_valid_kind(kind),
|
||||||
why=why or None,
|
why=why or None,
|
||||||
how_to_apply=how_to_apply or None,
|
how_to_apply=how_to_apply or None,
|
||||||
verify_with=verify_with or None,
|
verify_with=verify_with or None,
|
||||||
@@ -752,7 +780,7 @@ async def update_rule(
|
|||||||
return None
|
return None
|
||||||
allowed = {
|
allowed = {
|
||||||
"title", "statement", "why", "how_to_apply", "order_index",
|
"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",
|
"verify_with", "expires_when",
|
||||||
}
|
}
|
||||||
check_before = rule.verify_with
|
check_before = rule.verify_with
|
||||||
@@ -770,6 +798,8 @@ async def update_rule(
|
|||||||
continue
|
continue
|
||||||
if key == "tier":
|
if key == "tier":
|
||||||
value = _valid_tier(value)
|
value = _valid_tier(value)
|
||||||
|
elif key == "kind":
|
||||||
|
value = _valid_kind(value)
|
||||||
elif key in NULLABLE_RULE_TEXT:
|
elif key in NULLABLE_RULE_TEXT:
|
||||||
value = value or None
|
value = value or None
|
||||||
elif key == "arose_from_id":
|
elif key == "arose_from_id":
|
||||||
|
|||||||
@@ -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"
|
||||||
Reference in New Issue
Block a user