fix(ci): guard the Alembic (Postgres) track in schema-parity (#1342)#1345
Open
dolho wants to merge 2 commits into
Open
fix(ci): guard the Alembic (Postgres) track in schema-parity (#1342)#1345dolho wants to merge 2 commits into
dolho wants to merge 2 commits into
Conversation
The schema-parity required check validated only the SQLite track
(migrations.py ↔ schema.py). A schema change that ships the SQLite
migration but omits the Alembic revision under
src/backend/migrations/versions/ passed every required check green yet
broke PostgreSQL — init_database() runs alembic_runner.upgrade_to_head(),
which applies revision files only and does not autogenerate from
tables.py. Two PRs reached "green CI but PG-broken" and had to be held by
hand.
Add a cross-track guard, folded into the existing required schema-parity
job (no new required-check to manage):
- scripts/ci/check_alembic_parity.py — fails a PR that ADDS schema DDL to
db/{migrations,schema,tables}.py without a net-new revision file under
src/backend/migrations/versions/. Pure stdlib, PR-only (diffs base...head).
- Heuristic / false-positive guard: the signal is a DDL keyword on an
*added, non-comment* line (SQL: CREATE/ALTER/ADD COLUMN/…; SQLAlchemy:
Column(/Table(/Index(/…). Comment edits, data-only and down migrations
carry no DDL keyword, so they don't trip it. Documented in the script
docstring and the workflow header.
- tests/unit/test_alembic_parity_guard.py — 20 tests incl. the acceptance
fixtures (SQLite-only change fails; dual-tracked passes; comment/data-only
pass). Wired into the parity pytest run.
Also notes the enforcement in architecture.md Invariant #3.
Verified locally: 24 tests pass; end-to-end smoke across clean / SQLite-only
(exit 1) / paired-revision (exit 0) scenarios behaves correctly.
Related to #1342
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Verifying against real repo history surfaced a false positive: the migration-runner refactor #1263 (_atomic_rebuild table rebuilds) re-emits CREATE TABLE / CREATE INDEX for *existing* tables in a rename-swap but adds no actual schema and no new MIGRATIONS entry — yet the original "any added DDL keyword" heuristic flagged it, violating AC #4 (non-schema edits must not trip). Tighten the signal to two conjuncts: a schema change must (1) register a net-new ("name", _migrate_fn) entry in the MIGRATIONS list AND (2) carry a DDL keyword. Runner refactors / table rebuilds add no entry → exempt; data-only new migrations carry no DDL → exempt; real column/table adds do both → caught. Validated against real commits: • #740 agent_loops, #526 agent_ownership column → FAIL (correctly blocked) • #668 compat, voice_name (both shipped an Alembic revision) → PASS • #1263 runner refactor → PASS (false positive fixed) A full post-Alembic history scan finds 0 outstanding missing revisions, so no backfill is owed; the pre-Alembic columns are already in 0001_baseline. 28 tests pass (added MIGRATIONS-entry detection + the #1263 rebuild case). Related to #1342 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Resolve by running |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to #1342
Problem
The
schema-parityrequired check validated only the SQLite track (db/migrations.py↔db/schema.py). A schema change that ships the SQLite migration + updatesschema.py/tables.pybut omits the Alembic revision undersrc/backend/migrations/versions/passed every required check green — yet broke PostgreSQL, whereinit_database()runsalembic_runner.upgrade_to_head()(applies revision files only; does not autogenerate fromtables.py). Two PRs reached "green CI but PG-broken" in one review pass.Fix
A cross-track guard folded into the existing required
schema-parityjob (no new required-check to manage — AC #2):scripts/ci/check_alembic_parity.py— fails a PR that ADDS schema DDL todb/{migrations,schema,tables}.pywithout a net-new file undersrc/backend/migrations/versions/. Pure stdlib, PR-only (diffsbase...head).CREATE TABLE/INDEX/TRIGGER,ALTER TABLE,ADD/DROP COLUMN,RENAME …Column(/Table(/Index(/UniqueConstraint(/ForeignKey(tests/unit/test_alembic_parity_guard.py— 20 unit tests incl. the acceptance fixtures (SQLite-only change fails; dual-tracked passes; comment/data-only pass). Wired into the parity pytest run.architecture.mdInvariant Feature/vector log retention #3.Acceptance criteria
schema-parityjobVerification
End-to-end smoke against real git refs:
ADD COLUMN, no revision → FAIL (exit 1, prints offending line)🤖 Generated with Claude Code