[AndriiPasternak31]

Closes #3395. This is one of three independent slices and the one that closes the issue. The other two are the fresh-install reverse-parity assertions in tests/unit/test_schema_parity.py (slice A) and the MIGRATIONS registry bijection in tests/unit/test_migrations.py (slice C). The three share no file and merge in any order.

The finding: the issue's own check is blind on 70% of ADD COLUMN sites

The issue proposes adding the reverse direction (schema − production) to test_schema_parity.py. That test models a fresh install only: an empty database through run_all_migrations → init_schema → run_all_migrations.

On that path, a migration whose target table does not exist yet raises no such table. The runner skips that as a fresh install (migrations.py:170-174). init_schema then creates the table with every column schema.py lists, including the one whose migration was never registered. So the fresh database is complete and the reverse diff stays clean, while every existing install never gets the column.

That holds for every table only init_schema creates and no migration does. There are 31 such tables of 92, among them schedule_executions, users, chat_messages and mcp_api_keys. 213 of the 306 ALTER TABLE <t> ADD COLUMN / _safe_add_column(cursor, "<t>", ...) sites in db/migrations.py target one of them (counted on dev at 40b3295). The issue's headline case is on one of them: schedule_executions.a2a_caller_identity, from #3913.

What changed

scripts/ci/check_sqlite_upgrade_parity.py (new, pure stdlib) models the existing install instead. Steps:

1. Resolve the merge-base of the PR's base and head.
2. git archive the merge-base's src/backend. In its own interpreter, run that tree's lifecycle on a temporary file database. This builds the tables, indexes and schema_migrations ledger the merge-base would have left.
3. In a second interpreter, run the checkout's lifecycle on the same database. This is the upgrade.
4. Require that every table, column, index and trigger the checkout's init_schema declares exists in the upgraded database, and that migration_health is (n, n, None).

Each missing object is named with what to do. Exit codes:

• 0: pass.
• 1: gap. A head lifecycle that raises on the upgraded install also counts here, because every existing install would crash-loop the same way.
• 2: the check could not run. Causes: no base/head, no merge-base, the merge-base tree failing to build its install, or the checkout's db modules not importing. Exit 2 never reads as "no drift".

Using the merge-base as the baseline means nothing is frozen and nothing goes stale. A table added since any release is covered as soon as it exists on dev. The pattern is the one check_alembic_parity.py already uses: compare the PR's base and head.

tests/unit/test_sqlite_upgrade_parity_guard.py (new, 22 tests) drives the comparison core over synthetic two-file backends: db/schema.py plus db/migrations.py in tmp_path, with no git and no backend dependencies. It runs in the unit tier on every PR. The five cases the guard exists for:

• a registered migration passes;
• a migration written but not registered is a gap naming t.b;
• a schema.py column with no migration at all is a gap naming t.b;
• a new init_schema-only table is not a false red;
• an unbuildable base exits 2.

The other tests cover:

• objects a migration drops after init_schema (an index, a trigger, a table);
• a renamed migration, a duplicate registration, and a registered migration that is never recorded;
• the restated lifecycle order (migrations before init_schema; the second pass);
• rows read by name, because the connection uses sqlite3.Row as get_db_connection does;
• the head-does-not-import, usage and git fail-closed paths;
• main() end to end over a throwaway git repository.

.github/workflows/schema-parity.yml gains one step, "SQLite upgrade parity (merge-base → head)".

• It runs on pull_request only, behind the job's existing self-skip, after the dependency install, with the same dummy env as the parity pytest. The checkout already uses fetch-depth: 0.
• The script and its self-test join the change-detection regex and push.paths.
• The self-test joins the parity pytest line.
• The header comment gains one paragraph.
• The job id, and so the required check name schema-parity, is unchanged.

tests/registry.json gains one entry for the self-test.

No docs: this is a CI and test change on the bug-fix tier. docs/memory/learnings.md:1238 ("do not count on test_schema_parity … one-directional") is a dated ledger entry and is left as it is. Its instruction still holds.

Stated limits (also in the script's docstring)

• Names only. Column types, defaults, NOT NULL, keys and index/trigger SQL are not compared (F2).
• Missing objects only. An object the checkout stops declaring survives on existing installs and is not reported.
• No rows. The install it builds is empty, because init_database's seeding steps are not run. So a backfill, a rebuild's row copy, or a constraint an existing row would violate is never exercised (F5).
• Incremental. It checks merge-base → checkout, not an upgrade from every release (F3).
• The lifecycle is restated, not imported (F4).
• The ledger check is deliberately stricter than /health. A renamed or removed registered migration is red: a rename re-runs it on every existing install, and a removal keeps being counted by /health.

Follow-ups

• F1 (schema parity: a PostgreSQL upgrade-parity guard (the SQLite guard's sibling) #3975): PostgreSQL upgrade parity. A new init_schema-only table needs no SQLite migration, so check_alembic_parity.py never forces its Alembic revision. That script's docstring claim ("caught … by the existing schema-parity pytest") becomes true for a column on an existing table and stays false for a new table.
• F2 (schema parity: compare definitions, not names (index and trigger SQL, column attributes) #3976): definition parity: index and trigger SQL, plus column attributes.
• F3 (schema parity: frozen install fixtures per release #3977): release-level frozen-install fixtures, if an "every vX.Y install upgrades" statement is wanted.
• F4 (schema: pin the order of the init_database lifecycle #3978): pin the init_database lifecycle order, for example by running the real init_database() in a subprocess.
• F5 (schema parity: a data-bearing upgrade (seed the base install) #3979): a data-bearing upgrade: seed the base install so row copies, backfills and constraints on existing rows are exercised.
• F6 (ci: the migration self-skip regex misses the services migrations import #3980): the self-skip regex does not cover the modules the SQLite lifecycle loads (services/credential_encryption.py, services/secret_settings.py, services/platform_alerts.py, config.py, and what services/__init__.py imports). A PR changing only those skips both the parity pytest and this guard. Today the unit tier's conftest still catches a fresh-install crash there, because it runs init_database() at load.

Review and security

An independent code review (of the code, not this description) and a diff-scoped security review ran on the slice.

Findings fixed in 3900b7016, a2d972094 and f1b533aac:

• Self-test gaps. Seven of ten guard mutations survived the original 11 tests. All twelve mutations now turn a test red.
• Self-test not run by the job. It was in the trigger list but not the pytest line.
• Connection without row_factory. Production uses sqlite3.Row.
• Head import failure mislabelled. It was reported as a schema gap; it is now exit 2.
• Unstated limits. The no-rows and missing-only limits, and the ledger policy, are now in the docstring.
• Base-failure message. It now says what to do next.
• Robustness. Relative paths, tar errors, and one-argument argv.

Declined, with reasons:

• An allowlist for retiring a migration. No migration has ever been retired, and the policy is now stated.
• .gitattributes export-ignore under src/backend. None exists today.
• The 213/306 count "not reproducible" with grep -c. grep -c counts lines; the docstring now names the counting method.

Filed as #3979 (the no-rows install) and #3980 (the self-skip regex scope).

Security review of the diff: no findings.

• The SHAs reach the script through env: only; git is called with argv lists, never a shell string.
• The trigger is pull_request, not pull_request_target, under contents: read, with dummy secrets only.
• The merge-base is an ancestor of the base, so it is trusted dev code.
• Tar extraction uses filter="data".
• gitleaks over the branch commits found no leaks.
• zizmor --offline reports the same 3 findings on this workflow before and after. All are pre-existing lines (actions/checkout@v7 and actions/setup-python@v7 not SHA-pinned; persist-credentials default).

Journey Impact: none: CI and test infrastructure only. No runtime code, endpoint, UI, migration or boot behaviour changes, so no user or operator journey is affected.

Validation: guard on this branch vs origin/dev (merge-base 40b3295) → exit 0, 92 tables / 1111 columns / 198 indexes / 2 triggers, migration_health = (170, 170, None). The same holds with the branch merged into the current dev tip f754dfb (the CI shape). The same holds under python:3.13-slim (Python 3.13.17, SQLite 3.46.1, deps from tests/requirements-test.txt, worktree and .git mounted read-only); in that container, R0, R1, R1′ and N1 give the results in the Mutation line, and the self-test gives 22 passed (measured before the review fixes). N1 (a fresh install of a table the PR adds: n1_probe plus an index, init_schema only, no migration, on dev) → exit 0, 93 tables / 199 indexes, no false red. A real table-adding PR (5034304, room_gate_holds, migration-created) → exit 0. The reviewer independently ran older dev bases (1 week, 3 weeks, 6 weeks, 3 months; up to 71 migrations newly recorded) → all exit 0. Broken dev (review finding 1): commit A registers a migration that raises; commit B reverts it as the fix PR. Before the fix, guard A B → exit 2, CANNOT RUN. After it → exit 0, with a WARNING naming A as skipped and the install built by A's first-parent ancestor. pytest test_sqlite_upgrade_parity_guard.py test_schema_parity.py test_migrations.py test_alembic_parity_guard.py test_2338_journey_catalog.py → 122 passed (-p no:randomly, Python 3.12). Before the review fixes, the same set gave 118 passed under pytest-randomly seeds 11, 222 and 3333. tests/lint_sys_modules.py and tests/lint_root_test_placement.py pass. ruff (EXE001 gone, the script is now executable) and mypy --strict are clean on the script. actionlint is clean.

Mutation: replay PR #3913 (17adf5a against 17adf5a^, merge-base 0742886) with ("a2a_caller_identity", _migrate_a2a_caller_identity), deleted from MIGRATIONS while the function and the schema.py column stay (R1) → exit 1, "column schedule_executions.a2a_caller_identity: schema.py declares it, but an install that already has schedule_executions never gets it on upgrade", (169, 169, None). The same replay with the function deleted too (R1′) → exit 1, same column; slice C cannot see this one, because no function exists. As shipped (R0) → exit 0, 1 migration newly recorded, (170, 170, None). R0 and R1 were re-run after the review fixes, with the same results. Today's suite and slice A stay green on R1 (measured at planning). Reviewer: deleting ("held_gate_turns", …) against a 1-week-old base → exit 1 naming enterprise_portal_messages.gate_hold and public_chat_messages.gate_hold. Self-test: 12 guard mutations, each red in at least one test. They cover each missing-object loop, both ledger checks, first_pending, the second pass, the lifecycle order, base failure → pass, merge-base → head, head import → gap, and no row_factory. Walk-back: with the four new self-tests against the pre-fix guard, 3 failed / 23 passed. walks_back_to_the_newest_ancestor_that_builds and still_finds_a_gap_over_the_longer_range got exit 2. is_bounded_and_fails_closed_when_exhausted hit a missing MAX_BASE_WALKBACK. Against the fixed guard, 26 passed.

Unverified:

• CI has run the guard on the real pull_request merge ref. Run 40407332949, at checkout 339c1194d Merge f1b533aac into 40b329593, gave PASS with the same 92 / 1111 / 198 / 2 counts and (170, 170, None), and the self-tests ran in the parity pytest step. That was the pre-review head. The walk-back added in review is shown locally only (scratch clone and self-tests); no real broken dev was observed on GitHub Actions.
• The walk-back bound (20 first-parent ancestors) is a judgement. A base broken for longer needs the fix PR to raise it.
• The full tests/unit tier was not run on this branch alone; it runs once on the integration branch for the batch of slices.
• .claude/agents/test-runner.md (the private submodule's test catalog) was not updated. This PR leaves gitlinks untouched.
• The base install has no rows (F5), so data-dependent upgrade failures are out of this guard's reach.

[AndriiPasternak31]

Review pass (fresh reviewer, 2026-10-27), each finding resolved in 9e83235:

• A broken dev no longer blocks the PR that repairs it. When the merge-base cannot build its install, the guard walks back first-parent ancestors (bounded, MAX_BASE_WALKBACK = 20) to the newest one that builds, upgrades from there, and prints a warning naming every skipped commit. Exit 2 stays the answer when the walk is exhausted. Not fail-open: the head has to upgrade over the longer range, pinned by test_walk_back_still_finds_a_gap_over_the_longer_range. The circular "land the bump first" advice in the exhausted-walk message is corrected too.
• Stated limit added to the script and the workflow header: only db/migrations.py and db/schema.py are exercised; tables only the enterprise runner creates are out of scope (three enterprise_* tables live in the OSS schema.py and are covered).
• database.md names the guard and why the fresh-install parity test cannot see it; the Invariant Feature/vector log retention #3 line lists the guard among the schema-parity steps (core file stays at 160 lines).
• Executable bit set; ruff EXE001 gone, mypy --strict clean.
• Unverified line updated: CI already ran the guard on the real merge ref (run 40407332949, PASS, same counts).

Broken-dev scenario: pre-fix guard exit 2, fixed guard exit 0 with the warning naming the skipped commit. Self-tests 26 passed (3 red against the pre-fix guard). Guard against current dev: exit 0, same counts. R1 mutation still exit 1 with the same message.

[dolho]

/validate-pr — Lane C (.github/workflows/schema-parity.yml)

Check	Result
Commits / issue	conventional; Closes #3395 (this slice closes it; #3972 and #3973 say Part of) ✅
Workflow change	one PR-only step behind the existing self-skip; job id (required context schema-parity) unchanged; SHAs via env: only; pull_request, not pull_request_target; dummy secrets only ✅
Script safety	subprocess.run with argv lists, no shell; tar.extractall(..., filter="data"); merge-base is an ancestor of dev, so the old tree it executes is trusted ✅
Docs	architecture.md Invariant #3 gains one clause; architecture/database.md a paragraph ✅
Secrets / host paths / file modes	none (the script is now +x, intended) ✅
Self-test on the PR head	test_sqlite_upgrade_parity_guard.py: 26 passed ✅
Guard on the PR head vs dev f754dfb	exit 0; merge-base 40b3295; 92 tables / 1111 columns / 198 indexes / 2 triggers; (170, 170, None), the numbers the PR states ✅
Mutation, reproduced here	added zz_probe TEXT to schema.py's users with no migration → exit 1: "column users.zz_probe: schema.py declares it, but an install that already has users never gets it on upgrade. Write the migration…" ✅
CI	18 pass / 6 skipped; schema-parity green in 4m06s with the new step ✅

Execution coverage (§5.4)

The guard executes two real lifecycles (the merge-base tree's, then the checkout's) on a file database and compares live PRAGMA reads, and CI runs it on every schema-relevant PR. Its self-test drives the comparison core over synthetic backends. Both execute the path rather than read source.

Non-blocking

• MAX_BASE_WALKBACK = 20 is a judgement call, as you note. If dev is ever broken longer than that, the exit-2 message already says what to do.
• F5 (a data-bearing base install) and F6 (the self-skip regex missing the modules migrations import) are the right follow-ups; F6 is the one I'd pick up first, since it silently skips both this guard and the parity pytest.

APPROVE.

🤖 Reviewed with Claude Code

[AndriiPasternak31]

@dolho thanks. This merged with the train before I got to your notes, so here is where each one landed:

• MAX_BASE_WALKBACK = 20: keeping it. schema-parity has about 200 push runs on dev since mid-July and none failed, so a merge-base that can't build its install hasn't happened yet. When it does, it's visible straight away: the push run and every schema PR's parity pytest go red. 20 first-parent commits is about a day and a half of dev at the current rate.

  Checking "the exit-2 message already says what to do" turned up one gap, now in ci(schema-parity): an exhausted walk-back tells a PR branched while dev was broken to merge dev in (#3395) #4013. The message told every PR to raise the bound in the fix PR, but that only helps the fix PR. A PR branched while dev was broken and checked after the fix landed just needs dev merged in, which moves its merge-base past the break. ci(schema-parity): an exhausted walk-back tells a PR branched while dev was broken to merge dev in (#3395) #4013 makes the message say so, with a self-test showing such a PR exits 2 and then passes once dev is merged in. So only the fix PR can ever need a bigger bound.

• F5 is schema parity: a data-bearing upgrade (seed the base install) #3979 and F6 is ci: the migration self-skip regex misses the services migrations import #3980 (the description now cites both). On F6: I renamed plan_migration in services/secret_settings.py and nothing else. The job does self-skip. The unit tier still catches it, because its conftest runs init_database() on a fresh database and every test errors at collection. The upgrade guard would have passed it even if it ran, since existing installs have already recorded that migration. So the cost today is a failure in the wrong job, and the coverage goes away if conftest ever stops initialising the database at import. Agreed it's the one to do first. The lifecycle actually loads 20 backend files (including config.py), not three, so ci: the migration self-skip regex misses the services migrations import #3980 should cover that set and stop it drifting.