CI & Build / Python lint (push) Failing after 3s
CI & Build / Plugin hooks (push) Failing after 12s
CI & Build / integration (push) Failing after 27s
CI & Build / TypeScript typecheck (push) Failing after 35s
CI & Build / Python tests (push) Failing after 37s
CI & Build / Build & push image (push) Skipped
Milestone 394, steps 5-8. Operator: "remove the always on rule functionality as the goal was to not have it at all since it didn't seem to work as expected." Unconditional preload had three failures the retrieval arms do not. It could not be MEASURED — a resident rule is in the context whether or not it mattered, so nothing distinguished "this governed the act" from "this was scenery", and it was the one surface structurally exempt from the scoreboard judging every other. It was SUMMARISED AWAY by compaction while the session went on believing it held the rules. And it CROWDED OUT the few rules that applied with the thirty that did not. WHAT GOES Schema (0100): rules.tier + ck_rules_tier, rule_versions.tier, rulebooks.always_on, and project_rulebook_exclusions — a table recording a project's opt-out of something that no longer binds it unasked. Tools: list_always_on_rules, exclude_always_on_rulebook, include_always_on_rulebook. Service: the same three plus rules_etag_for, _valid_tier and the whole etag family. The SessionStart preload and the write-path staleness arm go with them: nothing is resident, so nothing can have drifted since a session loaded it. THREE CALLS WORTH REVIEWING enter_project got NARROWER, not wider. Its filter was `always_on OR area-tagged`; dropping the tier arm leaves the deterministic half, so a project with no canonical-tagged Systems gets no bulk rules and reaches them by retrieval instead. Dropping the whole clause would have made that payload bigger than the preload this milestone deletes. Backups import tolerantly. A pre-394 archive carries tier, always_on and the retired inception choice; none is read, and the exclusion key is DROPPED rather than remapped, because restoring it would write data that validate_inception now rejects as unknown. The migration is irreversible in the way that matters and says so: downgrade recreates the columns at their defaults and cannot restore which rules were always-on. A value invented to fill a hole is not a measurement. THE INSTRUCTION SURFACES SAY THE HARDER THING Deleting "call list_always_on_rules()" is easy; replacing it is not, because the new model asks a session to trust something it cannot see. All three surfaces now say a session holds nothing, that rules arrive when work matches them, and — the half that got dangerous — that "no rule arrived" means "nothing matched", never "there is no rule". Under residency an empty session was rare and suspicious; it is now the ordinary state of most turns, so reading it as permission is wrong on nearly every turn rather than occasionally. That is #3720's defect at session scale. test_instruction_surfaces_agree is repointed rather than retired: its two halves collapsed into one instruction, and it gains a guard that every surface states what absence means. _INSTRUCTIONS is back at 1999/2000 — the inception clause paid for the longer HOW line. UI (rule 27, and the opportunity step 8 named) The tier selector is gone, and what replaces it is the point: `when_to_apply` is now the field that decides whether a rule is ever seen, so the editor marks it required, warns while it is empty, and both rule lists badge a trigger-less rule "never surfaces". A rule without one is not quiet, it is unreachable. TESTS Two files deleted outright — test_rules_etag.py and test_inception_rules.py tested subsystems that no longer exist. Elsewhere obsolete cases were removed and the rest repointed. One deserves naming: the wiring test asserted the act arms pass no `tier`, which had become an assertion that could not fail. It is repointed onto `kind`, which does still exist and where the same claim is live — a preference must reach a write exactly as a rule does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011cPyzNnegXHr5iRMzzy5KJ
264 lines
11 KiB
Python
264 lines
11 KiB
Python
"""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.
|
|
|
|
SETUP ONLY, and that is not a stylistic choice. `_dispose_engine` is a
|
|
usefixtures entry, so it sets up AFTER this autouse one and therefore
|
|
tears down BEFORE it. Any database call here after a `yield` would open a
|
|
fresh pooled connection that the closing loop then orphans, and the next
|
|
test to touch Postgres dies on "Future attached to a different loop" —
|
|
including tests in other files. Cleanup belongs in the fixtures below,
|
|
whose teardowns run while the engine is still live.
|
|
"""
|
|
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",
|
|
),
|
|
# 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.",
|
|
),
|
|
])
|
|
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):
|
|
"""Runs the real restore, then hands back the new rows.
|
|
|
|
The restore mints a NEW user from the payload, so the restored corpus is
|
|
entirely separate from the source one — which is what makes the id
|
|
assertions below able to fail.
|
|
"""
|
|
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()
|
|
yield {"user": user, "rule": rule, "versions": versions, "source": source}
|
|
|
|
# Here rather than in the autouse fixture: this teardown still runs while
|
|
# the engine is live. See _no_leftovers.
|
|
await _purge_restored()
|
|
|
|
|
|
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"
|
|
)
|