feat(gate): detect same-table/same-column collisions across differently-numbered migrations - #2607
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 13:51:25 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 6 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2607 +/- ##
==========================================
+ Coverage 96.02% 96.04% +0.02%
==========================================
Files 233 234 +1
Lines 26077 26209 +132
Branches 9474 9510 +36
==========================================
+ Hits 25041 25173 +132
Misses 425 425
Partials 611 611
🚀 New features to boost your workflow:
|
…ly-numbered migrations scripts/check-migrations.mjs's collision logic only grouped migration files BY FILENAME NUMBER -- it never parsed the SQL body to detect two DIFFERENT, individually-valid numbers adding the SAME column to the SAME table. repository_settings alone has taken 5+ independent ALTER TABLE ... ADD COLUMN migrations under unique filenames, confirming this is the hottest actual collision surface in the schema. Two concurrent PRs each independently picking the same column/table combination under different numbers would both pass CI and show mergeable_state: clean (different files, no git conflict), only failing at actual wrangler d1 migrations apply deploy time -- after merge, with zero CI signal. Added src/db/migration-column-extraction.ts: a pure, fs-free module that replays every migration file's schema-affecting statements (CREATE TABLE column lists, ALTER TABLE ADD/DROP/RENAME COLUMN) IN MIGRATION-NUMBER ORDER and flags any (table, column) pair defined by more than one file. A DROP TABLE event clears every column tracked for that table so far, so migrations/0060_orb_fleet_collector.sql's documented DROP+CREATE recreate (SQLite can't ALTER away a table-level UNIQUE constraint) correctly does not read as colliding with the table it replaces -- this was the first real false positive found while building the check, along with a second bug where an inline trailing column comment containing its own comma fooled the top-level clause splitter into extracting comment words as fake columns. Both are fixed by stripping comments before matching. Verified zero false positives against the full 95-file migrations/ directory. Wired into scripts/check-migrations.mjs (no new CI job -- already runs via the existing db:migrations:check step in test:ci). 26 new unit tests for the extraction module (100% line/branch coverage) plus 3 new CLI-level tests in check-migrations-script.test.ts. Closes #2551
…ase it Gate review (gittensory-orb) flagged a real defect: the collision detector only checked its tracking map for files.size > 1 at the very end, after replaying every event -- so CREATE TABLE t (c INT); ALTER TABLE t ADD COLUMN c INT; DROP TABLE t; was silently accepted, even though the ADD COLUMN would already fail at real migration execution time (SQLite runs statements strictly in order) well before the DROP TABLE is ever reached. A later drop_table event was clearing evidence of a collision that had already happened. Fixed by recording a collision into a separate, permanent map the moment a redefinition is detected against the live tracking state -- before drop_table's clearing logic runs on any subsequent statement. The existing DROP+CREATE-recreate false-positive fix is unaffected (verified against the full 95-file migrations/ directory and all existing tests): a table dropped with no prior collision still clears cleanly, since nothing was ever recorded for it. Added a regression test for the exact scenario the gate flagged. 100% branch coverage maintained.
d036cf9 to
2820773
Compare
Summary
scripts/check-migrations.mjs's collision logic only grouped migration files BY FILENAME NUMBER — it never parsed the SQL body to detect two DIFFERENT, individually-valid numbers adding the SAME column to the SAME table.repository_settingsalone has taken 5+ independentALTER TABLE ... ADD COLUMNmigrations under unique filenames, confirming this is the hottest actual collision surface in the schema.mergeable_state: clean(different files, no git conflict), only failing at actualwrangler d1 migrations applydeploy time — after merge, with zero CI signal.Scope
src/db/migration-column-extraction.ts— new pure, fs-free module. Replays every migration file's schema-affecting statements (CREATE TABLEcolumn lists,ALTER TABLE ADD/DROP/RENAME COLUMN) IN MIGRATION-NUMBER ORDER and flags any(table, column)pair defined by more than one file. ADROP TABLEevent clears every column tracked for that table so far, somigrations/0060_orb_fleet_collector.sql's documented DROP+CREATE recreate (SQLite can'tALTERaway a table-levelUNIQUEconstraint) correctly does not read as colliding with the table it replaces — the first real false positive found while building this, along with a second bug where an inline trailing column comment's own comma fooled the top-level clause splitter into extracting comment words as fake columns. Both fixed by stripping comments before matching.scripts/check-migrations.mjs— wired in; no new CI job needed, it already runs via the existingdb:migrations:checkstep intest:ci.test/unit/check-migrations-script.test.ts.Validation
migrations/directory (this is the acceptance criterion the issue calls out explicitly).npx vitest run test/unit/migration-column-extraction.test.ts test/unit/check-migrations-script.test.ts— 58/58 passing, 100% coverage on the new modulenpm run typecheck— cleannpm run test:ci— full local gate greennpm audit --audit-level=moderate— 0 vulnerabilitiesSafety
migrations/**), not a general-purpose SQL parser — scoped to what this repo's migrations actually contain, per the issue's own guidance.Closes #2551