fix(backup): a restore stops flattening the record vocabulary, and a column guard stops the next one (#3182)
CI & Build / Python lint (push) Successful in 6s
CI & Build / Plugin hooks (push) Successful in 18s
CI & Build / TypeScript typecheck (push) Successful in 39s
CI & Build / integration (push) Successful in 48s
CI & Build / Python tests (push) Failing after 56s
CI & Build / Build & push image (push) Skipped
CI & Build / Python lint (push) Successful in 6s
CI & Build / Plugin hooks (push) Successful in 18s
CI & Build / TypeScript typecheck (push) Successful in 39s
CI & Build / integration (push) Successful in 48s
CI & Build / Python tests (push) Failing after 56s
CI & Build / Build & push image (push) Skipped
`_note_rows` carried 16 of the `notes` table's 27 columns. A backup -> restore cycle reported success and handed back a corpus with every snippet and process flattened into a plain note, every issue and spike into `work`, every provenance edge gone, and recurring tasks no longer recurring. The record-type and kind vocabulary is what #3128 and milestone 312 were about, and a restore erased it. Two more found by auditing every row helper rather than only the one being edited: `_milestone_rows` dropped `body` — a milestone IS the plan (0066), so every plan restored as a title with no reasoning behind it — and `_repo_binding_rows` dropped `ref`, the branch a ledger follows (#2873), so a restored binding silently accounts for a different tree. `arose_from_id` is deferred to a second pass beside `parent_id`, never written in the constructor: it is an id in the SOURCE database, so copying it through lands the edge on whatever record happens to hold that number here. An edge whose target did not survive stays NULL rather than being guessed at. This is the trap that kept the fix out of milestone 317 step 1. THE STRUCTURAL HALF. The coverage guard from #2293 checks TABLES against Base.metadata; nothing checked COLUMNS, which is how nine went missing from a table that had been "covered" for years — added to the model and the migration, both of which fail loudly, and never to the serialiser, which fails silently. `_COLUMN_EXCLUSIONS` now declares, per table, every column deliberately not exported and why, and a parameterised guard walks all 23 helpers and asserts the two agree. Forgetting is no longer expressible. Reconciling all 23 turned up one more deliberate exclusion worth naming: the `code_shapes` proposal columns are the machine's standing suggestion, cleared by judgment and recomputed by every refresh, so carrying them would restore stale guesses over a tree the proposer has not seen. Tests: the round trip drives the REAL restore_full_backup against Postgres, not a reimplementation of its loop — a test that re-derives the remap it is checking would agree with whatever the product does, including nothing. Backup v12.
This commit is contained in:
@@ -0,0 +1,207 @@
|
||||
"""Real-Postgres round trip for the note fields #3182 restored.
|
||||
|
||||
The unit lane can prove a serialiser EMITS a key. It cannot prove a restore
|
||||
puts the value back on the right row, and the note->note edges are exactly
|
||||
where that distinction bites: `parent_id` and `arose_from_id` hold ids from
|
||||
the SOURCE database, so a restore that writes them straight through succeeds,
|
||||
reports success, and silently points every edge at whatever record happens to
|
||||
hold that number here.
|
||||
|
||||
That is why #3182 was not fixed in passing.
|
||||
|
||||
These drive the REAL `restore_full_backup`, not a reimplementation of its
|
||||
loop. A test that re-derives the remap it is checking 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.note import Note
|
||||
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")]
|
||||
|
||||
RESTORED_USERNAME = "backup_roundtrip_restored"
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def source():
|
||||
"""A snippet, a process, and an issue that arose from a work task — the
|
||||
shapes whose identity the backup used to drop — exported as backup rows.
|
||||
|
||||
Deleted children-first: `arose_from_id` is a real FK, so removing the
|
||||
origin while the issue still points at it is asking the database a
|
||||
question the test has no reason to ask.
|
||||
"""
|
||||
async with async_session() as s:
|
||||
owner = await ensure_user(s, "backup_roundtrip_owner")
|
||||
uid = owner.id
|
||||
await s.commit()
|
||||
|
||||
async with async_session() as s:
|
||||
origin = Note(
|
||||
user_id=uid, title="the work that broke", body="",
|
||||
status="done", task_kind="work",
|
||||
)
|
||||
snippet = Note(
|
||||
user_id=uid, title="debounce — rate-limit", body="```js\n1\n```",
|
||||
note_type="snippet", data={"name": "debounce", "language": "js"},
|
||||
)
|
||||
process = Note(
|
||||
user_id=uid, title="DRY pass", body="steps", note_type="process",
|
||||
)
|
||||
s.add_all([origin, snippet, process])
|
||||
await s.commit()
|
||||
origin_id = origin.id
|
||||
|
||||
issue = Note(
|
||||
user_id=uid, title="the fix", body="symptom -> cause -> fix",
|
||||
status="done", task_kind="issue", arose_from_id=origin_id,
|
||||
description="one-liner",
|
||||
)
|
||||
s.add(issue)
|
||||
await s.commit()
|
||||
order = [issue.id, origin_id, snippet.id, process.id]
|
||||
|
||||
async with async_session() as s:
|
||||
rows = (await s.execute(select(Note).where(Note.id.in_(order)))).scalars().all()
|
||||
note_rows = backup._note_rows(list(rows))
|
||||
user_rows = backup._user_rows(
|
||||
[(await s.execute(select(User).where(User.id == uid))).scalars().one()]
|
||||
)
|
||||
# Restore mints a NEW user from the payload, so the restored corpus is
|
||||
# entirely separate from the source — which is what makes the id
|
||||
# assertions meaningful. Renamed here rather than in a sibling fixture:
|
||||
# `restored` depends on this one, and a rename elsewhere might not have
|
||||
# run by the time the restore does.
|
||||
user_rows[0]["username"] = RESTORED_USERNAME
|
||||
|
||||
yield {
|
||||
"payload": {
|
||||
"version": backup.BACKUP_VERSION,
|
||||
"users": user_rows,
|
||||
"notes": note_rows,
|
||||
},
|
||||
"origin_id": origin_id,
|
||||
"owner_id": uid,
|
||||
}
|
||||
|
||||
async with async_session() as s:
|
||||
for nid in order:
|
||||
row = await s.get(Note, nid)
|
||||
if row is not None:
|
||||
await s.delete(row)
|
||||
await s.commit()
|
||||
|
||||
|
||||
@pytest_asyncio.fixture
|
||||
async def restored(source):
|
||||
"""Run the real restore, then hand back the new rows by title.
|
||||
|
||||
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 meaningful.
|
||||
"""
|
||||
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"
|
||||
rows = (await s.execute(
|
||||
select(Note).where(Note.user_id == user.id)
|
||||
)).scalars().all()
|
||||
by_title = {n.title: n for n in rows}
|
||||
new_user_id = user.id
|
||||
|
||||
yield by_title, source
|
||||
|
||||
async with async_session() as s:
|
||||
fresh = [await s.get(Note, r.id) for r in by_title.values()]
|
||||
for row in fresh:
|
||||
if row is not None:
|
||||
row.arose_from_id = None
|
||||
row.parent_id = None
|
||||
await s.flush()
|
||||
for row in fresh:
|
||||
if row is not None:
|
||||
await s.delete(row)
|
||||
await s.commit()
|
||||
async with async_session() as s:
|
||||
user = await s.get(User, new_user_id)
|
||||
if user is not None:
|
||||
await s.delete(user)
|
||||
await s.commit()
|
||||
|
||||
|
||||
@pytest_asyncio.fixture(autouse=True)
|
||||
async def _no_leftover_restored_user():
|
||||
"""The payload's username is fixed, so a previous failed run would leave a
|
||||
row that makes `restored` pick the wrong user. Clear it first."""
|
||||
async with async_session() as s:
|
||||
stale = (await s.execute(
|
||||
select(User).where(User.username == RESTORED_USERNAME)
|
||||
)).scalars().all()
|
||||
for user in stale:
|
||||
notes = (await s.execute(
|
||||
select(Note).where(Note.user_id == user.id)
|
||||
)).scalars().all()
|
||||
for n in notes:
|
||||
n.arose_from_id = None
|
||||
n.parent_id = None
|
||||
await s.flush()
|
||||
for n in notes:
|
||||
await s.delete(n)
|
||||
await s.delete(user)
|
||||
await s.commit()
|
||||
|
||||
|
||||
async def test_a_restored_record_keeps_what_it_IS(restored):
|
||||
"""#3182's headline. Without note_type and task_kind a restore reported
|
||||
success and handed back a corpus where every snippet and process was a
|
||||
plain note and every issue and spike was `work` — the whole vocabulary
|
||||
milestone 312 and #3128 were about, gone, with nothing to notice it by."""
|
||||
by_title, _ = restored
|
||||
|
||||
assert by_title["debounce — rate-limit"].note_type == "snippet"
|
||||
assert by_title["DRY pass"].note_type == "process"
|
||||
assert by_title["the fix"].task_kind == "issue"
|
||||
assert by_title["the work that broke"].task_kind == "work"
|
||||
|
||||
|
||||
async def test_a_restored_snippet_keeps_its_queryable_mirror(restored):
|
||||
"""The one field that would self-heal — backfill_snippet_data rebuilds it
|
||||
from the body at startup — but a restore should not hand back a corpus
|
||||
that needs a restart before it is findable by location."""
|
||||
by_title, _ = restored
|
||||
assert by_title["debounce — rate-limit"].data == {
|
||||
"name": "debounce", "language": "js",
|
||||
}
|
||||
|
||||
|
||||
async def test_the_provenance_edge_is_remapped_not_copied(restored):
|
||||
"""THE regression, and the reason this needed a real database.
|
||||
|
||||
The payload's `arose_from_id` is an id in the SOURCE database. Copying it
|
||||
through would leave the restored issue pointing at whatever record happens
|
||||
to hold that number — a restore that succeeds and silently rewires
|
||||
history. The edge must land on the RESTORED origin instead.
|
||||
"""
|
||||
by_title, src = restored
|
||||
issue = by_title["the fix"]
|
||||
origin = by_title["the work that broke"]
|
||||
|
||||
assert issue.arose_from_id == origin.id
|
||||
# ...and that is a different row from the one the payload named.
|
||||
assert issue.arose_from_id != src["origin_id"]
|
||||
|
||||
|
||||
async def test_description_and_status_survive(restored):
|
||||
by_title, _ = restored
|
||||
assert by_title["the fix"].description == "one-liner"
|
||||
assert by_title["the fix"].status == "done"
|
||||
+181
-29
@@ -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 == 11
|
||||
assert backup.BACKUP_VERSION == 12
|
||||
|
||||
|
||||
def _exportable_note(**over):
|
||||
@@ -31,10 +31,14 @@ def _exportable_note(**over):
|
||||
MagicMock: `_note_rows` calls .isoformat() on the timestamps, and a mock
|
||||
would happily return another mock instead of failing."""
|
||||
base = dict(
|
||||
id=1, user_id=7, title="t", body="b", tags=["x"], parent_id=None,
|
||||
id=1, user_id=7, title="t", body="b", description=None, tags=["x"],
|
||||
parent_id=None, arose_from_id=None,
|
||||
project_id=None, milestone_id=None, status=None, priority=None,
|
||||
due_date=None, created_at=datetime(2026, 1, 1, tzinfo=timezone.utc),
|
||||
updated_at=datetime(2026, 1, 2, tzinfo=timezone.utc),
|
||||
note_type="note", task_kind="work", data=None,
|
||||
started_at=None, completed_at=None,
|
||||
recurrence_rule=None, recurrence_next_spawn_at=None,
|
||||
verify_with=None, expires_when=None, verified_at=None,
|
||||
)
|
||||
base.update(over)
|
||||
@@ -66,40 +70,188 @@ def test_a_never_checked_note_exports_a_null_stamp_and_restores_as_one():
|
||||
assert backup._dt(row["verified_at"]) is not None
|
||||
|
||||
|
||||
def test_the_note_section_gap_is_pinned_rather_than_silent():
|
||||
"""#3182. `_note_rows` carries 16 of the `notes` table's 27 columns, and the
|
||||
absences are not harmless: without `note_type` every snippet and process
|
||||
restores as a plain note, and without `task_kind` every issue and spike
|
||||
restores as `work`.
|
||||
def test_the_record_type_and_kind_survive_the_export():
|
||||
"""#3182's headline. Without these two columns a restore reported success
|
||||
and handed back a corpus where all 90 snippets and 3 processes were plain
|
||||
notes and all 435 issues and the spike were `work` — the entire vocabulary
|
||||
milestone 312 and #3128 were about, gone, with nothing to notice it by."""
|
||||
[snippet] = backup._note_rows([_exportable_note(note_type="snippet")])
|
||||
[issue] = backup._note_rows([_exportable_note(task_kind="issue", status="done")])
|
||||
assert snippet["note_type"] == "snippet"
|
||||
assert issue["task_kind"] == "issue"
|
||||
|
||||
The table-coverage guard cannot see this — it asserts that every TABLE is
|
||||
backed up or declared excluded, and nothing checks COLUMNS, which is how
|
||||
these went missing quietly.
|
||||
|
||||
This test exists to make the gap loud and to make fixing it visible: when
|
||||
#3182 lands, this list shrinks, and a reviewer sees exactly which fields
|
||||
started travelling. It is not an endorsement of the omissions.
|
||||
"""
|
||||
def test_provenance_and_lifecycle_travel():
|
||||
started = datetime(2026, 3, 1, tzinfo=timezone.utc)
|
||||
[row] = backup._note_rows([_exportable_note(
|
||||
arose_from_id=42,
|
||||
description="one-liner",
|
||||
started_at=started,
|
||||
recurrence_rule={"freq": "weekly"},
|
||||
)])
|
||||
assert row["arose_from_id"] == 42
|
||||
assert row["description"] == "one-liner"
|
||||
assert row["started_at"] == started.isoformat()
|
||||
assert row["recurrence_rule"] == {"freq": "weekly"}
|
||||
# Absent lifecycle stamps stay absent — a note that never started must not
|
||||
# restore as one that started at restore time.
|
||||
assert row["completed_at"] is None
|
||||
|
||||
|
||||
def test_a_milestone_carries_its_plan():
|
||||
"""A milestone IS the plan (0066); `body` is its design and intent and
|
||||
`description` is only the one-line summary. Dropping it restored every
|
||||
plan as a title with no reasoning behind it (#3182)."""
|
||||
m = SimpleNamespace(
|
||||
id=1, user_id=7, project_id=2, title="t", description="d",
|
||||
body="## Goal\n\nthe actual plan", status="active", order_index=0,
|
||||
created_at=datetime(2026, 1, 1, tzinfo=timezone.utc),
|
||||
updated_at=datetime(2026, 1, 2, tzinfo=timezone.utc),
|
||||
)
|
||||
[row] = backup._milestone_rows([m])
|
||||
assert row["body"] == "## Goal\n\nthe actual plan"
|
||||
|
||||
|
||||
def test_a_repo_binding_carries_the_branch_its_ledger_follows():
|
||||
"""#2873. Without `ref` a restored binding silently falls back to the
|
||||
default branch and the shape ledger starts accounting for a different
|
||||
tree — a wrong answer that looks like a working one."""
|
||||
b = SimpleNamespace(user_id=7, project_id=2, repo_key="Scribe", ref="dev")
|
||||
[row] = backup._repo_binding_rows([b])
|
||||
assert row["ref"] == "dev"
|
||||
|
||||
|
||||
# The table -> (model, row helper) registry the column guard walks. Kept here
|
||||
# rather than in the service because it exists only to be introspected: the
|
||||
# product code already knows these pairings by calling them.
|
||||
def _column_guard_targets():
|
||||
from scribe.models.canonical_system import CanonicalSystem
|
||||
from scribe.models.code_shape import CodeShape, CodeShapeEvent, CodeShapeUse
|
||||
from scribe.models.design_system import DesignSystem, DesignToken
|
||||
from scribe.models.milestone import Milestone
|
||||
from scribe.models.note import Note
|
||||
from scribe.models.note_draft import NoteDraft
|
||||
from scribe.models.note_supersession import NoteSupersession
|
||||
from scribe.models.note_usage import NoteUsageEvent
|
||||
from scribe.models.note_version import NoteVersion
|
||||
from scribe.models.project import Project
|
||||
from scribe.models.repo_binding import RepoBinding
|
||||
from scribe.models.rulebook import Rule, Rulebook, RulebookTopic, RuleRelation
|
||||
from scribe.models.setting import Setting
|
||||
from scribe.models.system import RecordSystem, System
|
||||
from scribe.models.task_log import TaskLog
|
||||
from scribe.models.user import User
|
||||
|
||||
carried = set(backup._note_rows([_exportable_note()])[0])
|
||||
missing = {c.name for c in Note.__table__.columns} - carried
|
||||
|
||||
assert missing == {
|
||||
# Deliberate: trashed rows are not exported, so neither is the batch
|
||||
# id that groups them for restore().
|
||||
"deleted_at", "deleted_batch_id",
|
||||
# NOT deliberate — the #3182 gap, in the order they hurt.
|
||||
"note_type", # snippets and processes flatten into notes
|
||||
"task_kind", # issues and spikes flatten into work
|
||||
"arose_from_id", # every issue -> origin edge is dropped
|
||||
"recurrence_rule", "recurrence_next_spawn_at", # recurring tasks stop
|
||||
"started_at", "completed_at", # lifecycle history
|
||||
"description",
|
||||
"data", # self-heals: backfill_snippet_data rebuilds it
|
||||
return {
|
||||
"users": (User, backup._user_rows),
|
||||
"projects": (Project, backup._project_rows),
|
||||
"milestones": (Milestone, backup._milestone_rows),
|
||||
"notes": (Note, backup._note_rows),
|
||||
"task_logs": (TaskLog, backup._task_log_rows),
|
||||
"note_drafts": (NoteDraft, backup._note_draft_rows),
|
||||
"note_versions": (NoteVersion, backup._note_version_rows),
|
||||
"settings": (Setting, backup._setting_rows),
|
||||
"rulebooks": (Rulebook, backup._rulebook_rows),
|
||||
"rulebook_topics": (RulebookTopic, backup._topic_rows),
|
||||
"rules": (Rule, backup._rule_rows),
|
||||
"systems": (System, lambda rows: backup._system_rows(rows, {})),
|
||||
"canonical_systems": (CanonicalSystem, backup._canonical_system_rows),
|
||||
"record_systems": (RecordSystem, backup._record_system_rows),
|
||||
"note_supersessions": (NoteSupersession, backup._note_supersession_rows),
|
||||
"rule_relations": (RuleRelation, backup._rule_relation_rows),
|
||||
"note_usage_events": (NoteUsageEvent, backup._usage_event_rows),
|
||||
"design_systems": (DesignSystem, backup._design_system_rows),
|
||||
"design_tokens": (DesignToken, backup._design_token_rows),
|
||||
"repo_bindings": (RepoBinding, backup._repo_binding_rows),
|
||||
"code_shapes": (CodeShape, backup._code_shape_rows),
|
||||
"code_shape_events": (CodeShapeEvent, backup._code_shape_event_rows),
|
||||
"code_shape_uses": (CodeShapeUse, backup._code_shape_use_rows),
|
||||
}
|
||||
|
||||
|
||||
def _stand_in(model):
|
||||
"""A real instance of `model` with every column set to a value of roughly
|
||||
the right type, so the serialiser runs and we can read which KEYS it
|
||||
produced. Values are meaningless; only the shape of the output dict is
|
||||
under test.
|
||||
|
||||
A real instance rather than a MagicMock because several helpers delegate to
|
||||
the model's own `to_dict()`, and a mock would return another mock instead
|
||||
of a dict. Typed rather than a bare instance because the helpers call
|
||||
`.isoformat()` on the timestamps, which `None` does not have.
|
||||
"""
|
||||
import sqlalchemy as sa
|
||||
|
||||
row = model()
|
||||
for column in model.__table__.columns:
|
||||
t = column.type
|
||||
if isinstance(t, sa.DateTime):
|
||||
value = datetime(2026, 1, 1, tzinfo=timezone.utc)
|
||||
elif isinstance(t, sa.Date):
|
||||
value = datetime(2026, 1, 1).date()
|
||||
elif isinstance(t, sa.Boolean):
|
||||
value = False
|
||||
elif isinstance(t, sa.Integer):
|
||||
value = 1
|
||||
elif isinstance(t, sa.ARRAY) or isinstance(getattr(t, "impl", None), sa.ARRAY):
|
||||
value = []
|
||||
elif isinstance(t, (sa.Text, sa.String)):
|
||||
value = "x"
|
||||
else:
|
||||
# JSON/JSONB and anything exotic. None is what these actually hold
|
||||
# most of the time, and no serialiser calls a method on one.
|
||||
value = None
|
||||
setattr(row, column.name, value)
|
||||
return row
|
||||
|
||||
|
||||
@pytest.mark.parametrize("table", sorted(_column_guard_targets()))
|
||||
def test_every_column_is_exported_or_declared_excluded(table):
|
||||
"""THE COLUMN GUARD (#3182) — _NOT_INCLUDED's shape, one level down.
|
||||
|
||||
The table guard below catches a whole table going missing. It cannot catch
|
||||
a COLUMN going missing from a table it already considers covered, which is
|
||||
how nine of them vanished from `notes` alone: note_type and task_kind, so
|
||||
every snippet and process restored as a plain note and every issue and
|
||||
spike as `work`; arose_from_id, so every provenance edge went; the
|
||||
recurrence pair, so recurring tasks stopped recurring. Plus milestones.body
|
||||
— which IS the plan — and repo_bindings.ref.
|
||||
|
||||
Each arrived the same way: added to the model and the migration, both of
|
||||
which fail loudly, and never to the serialiser, which fails silently.
|
||||
|
||||
A new column must now be exported or named in _COLUMN_EXCLUSIONS with a
|
||||
reason. Forgetting is no longer expressible.
|
||||
"""
|
||||
model, helper = _column_guard_targets()[table]
|
||||
[row] = helper([_stand_in(model)])
|
||||
|
||||
columns = {c.name for c in model.__table__.columns}
|
||||
missing = columns - set(row)
|
||||
declared = backup._COLUMN_EXCLUSIONS[table]
|
||||
|
||||
assert missing == declared, (
|
||||
f"{table}: exported columns and _COLUMN_EXCLUSIONS disagree.\n"
|
||||
f" dropped but not declared: {sorted(missing - declared)}\n"
|
||||
f" declared but exported anyway: {sorted(declared - missing)}"
|
||||
)
|
||||
|
||||
|
||||
def test_the_column_guard_covers_every_table_with_a_row_helper():
|
||||
"""The guard is only as good as its registry — a table added to _BACKED_UP
|
||||
with a new helper, and not to the registry, would be unguarded and look
|
||||
guarded. Join tables have no model class and carry both their columns by
|
||||
construction, so they are the only permitted absences."""
|
||||
join_tables = {
|
||||
"rulebook_subscriptions", "rule_suppressions",
|
||||
"topic_suppressions", "rulebook_exclusions", "rule_systems",
|
||||
}
|
||||
covered = set(_column_guard_targets()) | join_tables
|
||||
assert set(backup._BACKED_UP) - covered == set()
|
||||
# And no stale entries: every declaration must name a real target.
|
||||
assert set(backup._COLUMN_EXCLUSIONS) == set(_column_guard_targets())
|
||||
|
||||
|
||||
def test_not_included_lists_the_known_gaps():
|
||||
# The deferred tables must be surfaced explicitly, not silently dropped.
|
||||
# forge_connections is excluded as CREDENTIALS (api_keys reasoning): a
|
||||
|
||||
Reference in New Issue
Block a user