Skip to content

test(schema): parity checks schema.py against a fresh install in both directions (#3395) - #3973

Merged
vybe merged 3 commits into
devfrom
AndriiPasternak31/issue-3395-s1
Oct 27, 2026
Merged

vybe merged 3 commits into
devfrom
AndriiPasternak31/issue-3395-s1

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Part of #3395. This is slice A of three. Slice B (the merge-base → head SQLite upgrade-parity guard) carries the closing keyword for #3395; slice C (every _migrate* function is registered in MIGRATIONS) is also Part of #3395. No slice shares a file with another, so they can merge in any order.

What changed and why

tests/unit/test_schema_parity.py builds two in-memory SQLite snapshots, init_schema() on an empty DB and the fresh-install lifecycle from database.py::init_database (migrations, init_schema, migrations). Until now it asked only one question: does the lifecycle create something schema.py does not declare? The reverse went unchecked, so an object schema.py declares that a fresh install never ends up with could ship green. Two fresh-install shapes produce that:

  • an old creating migration's CREATE TABLE loses a column, and init_schema's CREATE TABLE IF NOT EXISTS never adds it back;
  • a rebuild migration that runs on the second pass (after init_schema) drops indexes or triggers. _atomic_rebuild(..., indexes=()) recreates no indexes by default, and DROP TABLE takes the triggers with it.

TestReverseSchemaParity mirrors the four forward assertions over the same two fixtures (schema − production: tables, columns, indexes, triggers). Each failure names the object and the fix. The module docstring now describes both directions, cites the lifecycle as database.py::init_database instead of line numbers that had gone stale, and states the limits. tests/registry.json gets the entry this file never had.

Other shape: fresh install. This label is nominal, and the test docstring says so: both fixtures already are the fresh-install shape (an empty database through the init_database order), and the drift caught here exists only on that shape.

Blind spot, stated: this slice cannot see a migration skipped on a fresh install. If a column's migration is never registered and its table is created only by init_schema (31 of 92 tables; 213 of 306 ADD COLUMN sites target them), the fresh first pass hits "no such table" and skips, then init_schema creates the table with the column. Both snapshots agree, and every existing install never gets the column. Slice B covers that existing-install shape. This file also compares names (and, forward, column types) only, not index or trigger SQL or column defaults / NOT NULL / primary keys.

Left alone on purpose: docs/memory/learnings.md still says not to count on test_schema_parity because it is one-directional. That is a dated ledger entry, and its instruction is still right for the existing-install case. No docs/ change: this is a test-only fix.

Journey Impact: none: test-only change to the schema-parity unit test and the test registry; no product code, route, or user-visible behaviour changes.

Validation: pytest tests/unit/test_schema_parity.py → 8 passed (4 forward + 4 reverse) on origin/dev @ 40b3295; test_schema_parity.py + test_migrations.py + test_alembic_parity_guard.py → 49 passed under pytest-randomly seeds 1, 2 and 3; test_2338_journey_catalog.py (registry consumer) green; tests/lint_sys_modules.py and tests/lint_root_test_placement.py OK; ruff check clean; gitleaks over the branch's 3 commits: no leaks.
Mutation: each applied to a clean tree, restored from a cp snapshot. Forward-only means the four pre-existing assertions (-k "not Reverse"). M3 (signal_query TEXT, removed from _migrate_canary_violations_table's CREATE TABLE): forward-only 4 passed, 4 deselected; full file FAILED tests/unit/test_schema_parity.py::TestReverseSchemaParity::test_no_schema_only_columns with reverse schema drift: columns declared in init_schema() are absent after init_database() on a fresh install: canary_violations.signal_query (TEXT), 1 failed, 7 passed. M7 (a registered chat_messages rebuild that skips on pass 1 and runs on pass 2 with indexes=()): forward-only 4 passed, 4 deselected; full file FAILED tests/unit/test_schema_parity.py::TestReverseSchemaParity::test_no_schema_only_indexes with indexes declared in init_schema() are absent after init_database() on a fresh install: ['idx_chat_messages_agent', 'idx_chat_messages_session', 'idx_chat_messages_subscription', 'idx_chat_messages_timestamp', 'idx_chat_messages_user'], 1 failed, 7 passed. The other two assertions have teeth too: a pass-2 DROP TRIGGER audit_log_no_update turns test_no_schema_only_triggers red, and a pass-2 DROP TABLE agent_tags turns test_no_schema_only_tables red. Forward-only stays green on both. test_migrations.py + test_alembic_parity_guard.py stay green (41 passed) under M3 and M7, so nothing else catches either one today.
Unverified: ran locally on Python 3.12 (venv) with macOS SQLite, not CI's Python 3.13 / Ubuntu SQLite. The assertions compare sqlite_master names and PRAGMA table_info, which should not vary across SQLite versions, but that was not checked. The full unit tier was not run on this slice alone; it runs once on the merged batch. The test-runner catalog in the private .claude submodule (agents/test-runner.md) still lists reverse-direction drift as out of scope. It was not edited, because the gitlink stays untouched.

Sibling PRs: #3972 (slice C, registry bijection), #3973 (slice A, reverse parity), #3974 (slice B, upgrade guard; carries the closing keyword for #3395). Follow-ups filed: F1=#3975 F2=#3976 F3=#3977 F4=#3978 F5=#3979 F6=#3980.

… directions (#3395)

test_schema_parity.py asked only `init_database() - init_schema()`: an
object a migration creates that schema.py lacks. The reverse went
unchecked, so an object schema.py declares that a fresh install never ends
up with shipped green. Two fresh-install shapes produce that: an old
creating migration's CREATE TABLE losing a column (init_schema's
IF NOT EXISTS never adds it back), and a rebuild migration on the second
pass dropping indexes or triggers (_atomic_rebuild defaults to indexes=()).

TestReverseSchemaParity mirrors the four forward assertions over the same
two fixtures (schema - production: tables, columns, indexes, triggers), each
naming the object and the fix. The module docstring now describes both
directions, cites the lifecycle as database.py::init_database instead of
stale line numbers, and states the limit: both fixtures are the fresh-install
shape, so a column whose migration was never registered on a table only
init_schema creates stays invisible here (the upgrade guard covers it).

Mutation (forward-only = the four pre-existing assertions):
- M3, `signal_query TEXT,` removed from _migrate_canary_violations_table's
  CREATE TABLE: forward-only 4 passed; full file 1 failed,
  test_no_schema_only_columns names canary_violations.signal_query (TEXT).
- M7, a registered chat_messages rebuild that skips on pass 1 and runs on
  pass 2 with indexes=(): forward-only 4 passed; full file 1 failed,
  test_no_schema_only_indexes names the 5 idx_chat_messages_* indexes.
- M8, a pass-2 DROP TRIGGER audit_log_no_update: test_no_schema_only_triggers
  red. M9, a pass-2 DROP TABLE agent_tags: test_no_schema_only_tables (and
  _indexes) red. Forward-only green on both.
…ns (#3395)

The schema-parity suite had no registry entry. Adds one describing the
forward (#713) and reverse (#3395) checks and their stated blind spot:
a column missing only on existing installs.
…hout citing an unmerged guard (#3395)

Review finding: the limit paragraph said the existing-install shape "is
covered by the merge-base upgrade guard", which does not exist on dev until
its own PR lands. It now states only what this file cannot see and why.

@dolho dolho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/validate-pr — Lane A (tests + registry; the only - lines are the docstring rewrite, no assertion removed)

Check Result
Scope tests/unit/test_schema_parity.py + tests/registry.json; the forward class is untouched ✅
Commits / issue conventional; Part of #3395, closing keyword on #3974 ✅
Secrets / host paths none ✅
Run on the PR head 8 passed ✅
Mutation, reproduced here appended a migration that hits "no such table" on the fresh first pass and drops idx_users_username on the second (after init_schema) → only TestReverseSchemaParity::test_no_schema_only_indexes goes red; the forward suite stays green, as it would on dev today ✅
CI all checks green ✅

Execution coverage (§5.4)

Both fixtures execute the real init_schema and the real run_all_migrations on in-memory SQLite, so the reverse assertions run the live lifecycle. Worth knowing for the next reader: a mutation that drops an object on the first pass is invisible here by design, because the runner records it as applied and skips it on the second. The drift this catches is specifically the second-pass shape, which is what the docstring says.

The blind spot the PR states (an unregistered migration on a table only init_schema creates) is real and is #3972's and #3974's to catch.

APPROVE.

🤖 Reviewed with Claude Code

@vybe
vybe merged commit 1ee5854 into dev Oct 27, 2026
22 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants