CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / Python lint (push) Successful in 2s
CI & Build / TypeScript typecheck (push) Successful in 54s
CI & Build / integration (push) Successful in 1m4s
CI & Build / Python tests (push) Failing after 1m11s
CI & Build / Build & push image (push) Skipped
Adds `kind` to rules — `rule` binds, `preference` is how the operator wants work done. One column, because the two differ in exactly one dimension and everything else a preference needs already lives on `rules`: a trigger column, a trigger-dominated embedding document, ownership-scoped search, three retrieval arms with telemetry, typed relations, and versioning. Defaults to `rule`, so nothing changes force on upgrade — 0088's argument for `tier`, unchanged. `rule_versions` gets the column too, and that half is not bookkeeping. `record_if_changed` decides whether an edit deserves a snapshot by comparing the fields a version carries, so a field absent from SNAPSHOT_FIELDS is a field whose change records no history at all. Without it, turning a rule into a preference — the moment something stops binding, and the single most consequential edit either kind can undergo — would leave the history silent. Backup carries it through all four seams. A missed one would have restored every preference as a rule, quietly. Guarded on real Postgres in three halves: a preference writes, a typo is refused (without which every other assertion would pass against a table whose CHECK had been dropped), and a row written with no kind reads back as `rule` — the migration's whole safety claim, asserted rather than assumed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
166 lines
6.7 KiB
Python
166 lines
6.7 KiB
Python
"""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"
|