From 73dddf372a26dd1e5bff69d9084d6e893a8fa563 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 05:41:30 -0700 Subject: [PATCH 1/2] feat(gate): detect same-table/same-column collisions across differently-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 --- scripts/check-migrations.mjs | 16 ++ src/db/migration-column-extraction.ts | 234 ++++++++++++++++++ test/unit/check-migrations-script.test.ts | 35 +++ test/unit/migration-column-extraction.test.ts | 217 ++++++++++++++++ 4 files changed, 502 insertions(+) create mode 100644 src/db/migration-column-extraction.ts create mode 100644 test/unit/migration-column-extraction.test.ts diff --git a/scripts/check-migrations.mjs b/scripts/check-migrations.mjs index cb3c2be963..0930fb0824 100644 --- a/scripts/check-migrations.mjs +++ b/scripts/check-migrations.mjs @@ -28,6 +28,7 @@ // replay either ALTER under a new migration name. import { readdirSync, readFileSync } from "node:fs"; import { detectMigrationCollisions, extractMigrationNumber, KNOWN_MIGRATION_DUPLICATES, MIGRATION_FILENAME_PATTERN } from "../src/db/migration-collisions.ts"; +import { detectColumnCollisions } from "../src/db/migration-column-extraction.ts"; const DIR = process.env.CHECK_MIGRATIONS_DIR || "migrations"; const NAME = MIGRATION_FILENAME_PATTERN; @@ -187,6 +188,21 @@ if (sqlViolations.length > 0) { ); } +// #2551: two DIFFERENT, individually-valid migration numbers can each add the SAME column to the SAME +// table — different files, no git conflict, both pass the number-collision check above, and both show +// `mergeable_state: clean` — only failing at actual `wrangler d1 migrations apply` deploy time, after merge, +// with zero prior CI signal. `files` is already numerically sorted (4-digit zero-padded lexicographic sort), +// which detectColumnCollisions requires so a documented DROP TABLE + CREATE TABLE recreate (e.g. +// migrations/0060_orb_fleet_collector.sql's orb_signals) correctly clears the table it replaces instead of +// reading as a collision with it. +const columnCollisions = detectColumnCollisions(files.map((file) => [file, readFileSync(`${DIR}/${file}`, "utf8")])); +if (columnCollisions.length > 0) { + const { table, column, files: group } = columnCollisions[0]; + fail( + `duplicate column ${table}.${column} defined by more than one migration: ${group.map((f) => `"${f}"`).join(", ")}. Two migrations independently added the same column under different numbers — this passes CI and shows a clean merge state, but fails at "wrangler d1 migrations apply" deploy time. Rename or remove the newer migration's column (or confirm the table is DROPped and recreated before it).`, + ); +} + const first = String(numbers[0]).padStart(4, "0"); const last = String(numbers.at(-1)).padStart(4, "0"); const grandfatheredNumbers = [...KNOWN_DUPLICATES.keys()] diff --git a/src/db/migration-column-extraction.ts b/src/db/migration-column-extraction.ts new file mode 100644 index 0000000000..a7ccc1a804 --- /dev/null +++ b/src/db/migration-column-extraction.ts @@ -0,0 +1,234 @@ +// Pure, fs-free (table, column) collision detection across migration files (#2551), shared by +// scripts/check-migrations.mjs's cross-migration collision check. Sufficient for this repo's actual migration +// corpus -- verified by direct inspection: no CREATE TRIGGER statements (so no trigger-body-aware semicolon +// handling is needed, unlike src/selfhost/migrate.ts's statement splitter) and every identifier is a bare +// lowercase snake_case name (no quoted/bracketed identifiers anywhere) -- not a general-purpose SQL parser. + +/** Split SQL text into individual statements on top-level semicolons (outside string quotes/comments). */ +export function splitSqlStatements(sql: string): string[] { + const statements: string[] = []; + let start = 0; + let quote: "'" | '"' | "`" | null = null; + let lineComment = false; + let blockComment = false; + + for (let i = 0; i < sql.length; i += 1) { + const char = sql[i]; + const next = sql[i + 1]; + + if (lineComment) { + if (char === "\n") lineComment = false; + continue; + } + if (blockComment) { + if (char === "*" && next === "/") { + blockComment = false; + i += 1; + } + continue; + } + if (quote) { + if (char === quote) { + if (next === quote) i += 1; + else quote = null; + } + continue; + } + + if (char === "-" && next === "-") { + lineComment = true; + i += 1; + continue; + } + if (char === "/" && next === "*") { + blockComment = true; + i += 1; + continue; + } + if (char === "'" || char === '"' || char === "`") { + quote = char; + continue; + } + + if (char === ";") { + // The slice always ends with this `;`, so `.trim()` can never produce an empty string here (unlike + // the trailing tail below, which genuinely can be empty) -- push unconditionally. + statements.push(sql.slice(start, i + 1).trim()); + start = i + 1; + } + } + + const tail = sql.slice(start).trim(); + if (tail) statements.push(tail); + return statements; +} + +/** Split a CREATE TABLE column-list body on top-level commas -- respecting nested parens (CHECK(...), + * FOREIGN KEY(a) REFERENCES b(c)) so a comma inside one of those doesn't split a single column/constraint + * definition in two. */ +function splitTopLevelCommaList(body: string): string[] { + const parts: string[] = []; + let depth = 0; + let start = 0; + for (let i = 0; i < body.length; i += 1) { + const char = body[i]; + if (char === "(") depth += 1; + else if (char === ")") depth -= 1; + else if (char === "," && depth === 0) { + parts.push(body.slice(start, i)); + start = i + 1; + } + } + parts.push(body.slice(start)); + return parts.map((p) => p.trim()).filter((p) => p.length > 0); +} + +const TABLE_LEVEL_CONSTRAINT_KEYWORDS = /^(PRIMARY\s+KEY|FOREIGN\s+KEY|UNIQUE|CHECK|CONSTRAINT)\b/i; +const IDENTIFIER = /^(\w+)/; + +/** Strip `--` line comments and `/* *\/` block comments (outside string quotes) from a statement before + * matching against it. Required for two reasons: a statement split at a top-level semicolon can have + * LEADING full-line comments preceding the actual keyword (breaking a `^`-anchored match like DROP TABLE's), + * and a CREATE TABLE column list's per-column trailing `-- comment, with a comma in it` would otherwise + * split a single column definition into two at that comment's comma (verified against + * migrations/0060_orb_fleet_collector.sql's inline column comments, which contain commas). */ +function stripSqlComments(text: string): string { + let result = ""; + let quote: "'" | '"' | "`" | null = null; + let lineComment = false; + let blockComment = false; + for (let i = 0; i < text.length; i += 1) { + const char = text[i]; + const next = text[i + 1]; + if (lineComment) { + if (char === "\n") { + lineComment = false; + result += char; + } + continue; + } + if (blockComment) { + if (char === "*" && next === "/") { + blockComment = false; + i += 1; + } + continue; + } + if (quote) { + result += char; + if (char === quote) { + if (next === quote) { + result += next; + i += 1; + } else { + quote = null; + } + } + continue; + } + if (char === "-" && next === "-") { + lineComment = true; + i += 1; + continue; + } + if (char === "/" && next === "*") { + blockComment = true; + i += 1; + continue; + } + if (char === "'" || char === '"' || char === "`") { + quote = char; + result += char; + continue; + } + result += char; + } + return result; +} + +/** A single schema-affecting event a statement produces, in the order that lets a caller replay migration + * history statement-by-statement: `drop_table` clears every column previously tracked for that table (a + * DROP+CREATE recreate, e.g. migrations/0060_orb_fleet_collector.sql's documented SQLite-ALTER-limitation + * workaround for orb_signals, must not read as colliding with the table it replaces); `remove_column` + * (DROP/RENAME COLUMN) untracks a single column rather than flagging it as a fresh collision candidate. */ +export type SchemaEvent = + | { type: "define_column"; table: string; column: string } + | { type: "drop_table"; table: string } + | { type: "remove_column"; table: string; column: string }; + +/** Extract the schema-affecting events a single SQL statement produces. Statements that don't affect table + * shape (INSERT, CREATE INDEX, plain DROP INDEX, ...) yield no events. */ +export function extractSchemaEvents(rawStatement: string): SchemaEvent[] { + const statement = stripSqlComments(rawStatement); + const dropTableMatch = /^\s*DROP\s+TABLE\s+(?:IF\s+EXISTS\s+)?(\w+)/i.exec(statement); + if (dropTableMatch) return [{ type: "drop_table", table: dropTableMatch[1]!.toLowerCase() }]; + + const renameColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+RENAME\s+COLUMN\s+(\w+)\s+TO\s+(\w+)/i.exec(statement); + if (renameColumnMatch) { + const table = renameColumnMatch[1]!.toLowerCase(); + return [ + { type: "remove_column", table, column: renameColumnMatch[2]!.toLowerCase() }, + { type: "define_column", table, column: renameColumnMatch[3]!.toLowerCase() }, + ]; + } + + const dropColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+DROP\s+COLUMN\s+(\w+)/i.exec(statement); + if (dropColumnMatch) return [{ type: "remove_column", table: dropColumnMatch[1]!.toLowerCase(), column: dropColumnMatch[2]!.toLowerCase() }]; + + const addColumnMatch = /\bALTER\s+TABLE\s+(\w+)\s+ADD\s+COLUMN\s+(\w+)/i.exec(statement); + if (addColumnMatch) return [{ type: "define_column", table: addColumnMatch[1]!.toLowerCase(), column: addColumnMatch[2]!.toLowerCase() }]; + + const createTableMatch = /\bCREATE\s+TABLE\s+(?:IF\s+NOT\s+EXISTS\s+)?(\w+)\s*\(([\s\S]*)\)[^)]*$/i.exec(statement); + if (!createTableMatch) return []; + const table = createTableMatch[1]!.toLowerCase(); + const body = createTableMatch[2]!; + const events: SchemaEvent[] = []; + for (const clause of splitTopLevelCommaList(body)) { + if (TABLE_LEVEL_CONSTRAINT_KEYWORDS.test(clause)) continue; + const identifierMatch = IDENTIFIER.exec(clause); + if (!identifierMatch) continue; + events.push({ type: "define_column", table, column: identifierMatch[1]!.toLowerCase() }); + } + return events; +} + +export type ColumnCollision = { table: string; column: string; files: string[] }; + +/** + * Replay every migration file's schema events IN MIGRATION-NUMBER ORDER and return every (table, column) + * pair DEFINED by more than one file -- a same-table/same-column collision across differently-numbered, + * individually-valid migrations (#2551). `orderedFileContents` must already be sorted ascending by migration + * number (the same order `scripts/check-migrations.mjs` reads the directory in); a `drop_table` event clears + * every column tracked for that table so far, so a documented DROP+CREATE recreate never reads as a + * collision with the table it replaces. Pure, no I/O. + */ +export function detectColumnCollisions(orderedFileContents: ReadonlyArray): ColumnCollision[] { + const tracked = new Map }>(); + + for (const [filename, sql] of orderedFileContents) { + for (const statement of splitSqlStatements(sql)) { + for (const event of extractSchemaEvents(statement)) { + if (event.type === "drop_table") { + for (const [key, entry] of tracked) { + if (entry.table === event.table) tracked.delete(key); + } + continue; + } + const key = `${event.table}.${event.column}`; + if (event.type === "remove_column") { + tracked.delete(key); + continue; + } + const entry = tracked.get(key); + if (entry) entry.files.add(filename); + else tracked.set(key, { table: event.table, column: event.column, files: new Set([filename]) }); + } + } + } + + const collisions: ColumnCollision[] = []; + for (const { table, column, files } of tracked.values()) { + if (files.size > 1) collisions.push({ table, column, files: [...files].sort() }); + } + return collisions.sort((a, b) => (a.table === b.table ? a.column.localeCompare(b.column) : a.table.localeCompare(b.table))); +} diff --git a/test/unit/check-migrations-script.test.ts b/test/unit/check-migrations-script.test.ts index 0946b7f98c..46076733a9 100644 --- a/test/unit/check-migrations-script.test.ts +++ b/test/unit/check-migrations-script.test.ts @@ -133,4 +133,39 @@ describe("check-migrations script", () => { expect(r.out).toContain("1 migrations OK"); }, ); + + // #2551: two DIFFERENT, individually-valid migration numbers adding the SAME column to the SAME table. + it("rejects two migrations that independently add the same column to the same table", () => { + const r = runCheck({ + "0001_a.sql": "CREATE TABLE widgets (id INTEGER PRIMARY KEY);\n", + "0002_b.sql": "ALTER TABLE widgets ADD COLUMN color TEXT;\n", + "0003_c.sql": "ALTER TABLE widgets ADD COLUMN color TEXT;\n", + }); + + expect(r.status).toBe(1); + expect(r.out).toContain("duplicate column widgets.color"); + expect(r.out).toContain('"0002_b.sql"'); + expect(r.out).toContain('"0003_c.sql"'); + }); + + it("does not flag a DROP TABLE + CREATE TABLE recreate as colliding with the table it replaces", () => { + const r = runCheck({ + "0001_a.sql": "CREATE TABLE widgets (id INTEGER, old_col TEXT);\n", + "0002_b.sql": "DROP TABLE IF EXISTS widgets;\nCREATE TABLE widgets (id INTEGER, new_col TEXT);\n", + }); + + expect(r.status).toBe(0); + expect(r.out).toContain("2 migrations OK"); + }); + + it("passes cleanly when two migrations touch the same table with different columns", () => { + const r = runCheck({ + "0001_a.sql": "CREATE TABLE widgets (id INTEGER PRIMARY KEY);\n", + "0002_b.sql": "ALTER TABLE widgets ADD COLUMN color TEXT;\n", + "0003_c.sql": "ALTER TABLE widgets ADD COLUMN size TEXT;\n", + }); + + expect(r.status).toBe(0); + expect(r.out).toContain("3 migrations OK"); + }); }); diff --git a/test/unit/migration-column-extraction.test.ts b/test/unit/migration-column-extraction.test.ts new file mode 100644 index 0000000000..a0b8ddec83 --- /dev/null +++ b/test/unit/migration-column-extraction.test.ts @@ -0,0 +1,217 @@ +import { readdirSync, readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; +import { detectColumnCollisions, extractSchemaEvents, splitSqlStatements } from "../../src/db/migration-column-extraction"; + +describe("splitSqlStatements (#2551)", () => { + it("splits on top-level semicolons", () => { + expect(splitSqlStatements("SELECT 1; SELECT 2;")).toEqual(["SELECT 1;", "SELECT 2;"]); + }); + + it("ignores semicolons inside single/double/backtick-quoted strings", () => { + expect(splitSqlStatements("INSERT INTO t VALUES ('a;b', \"c;d\", `e;f`);")).toEqual(["INSERT INTO t VALUES ('a;b', \"c;d\", `e;f`);"]); + }); + + it("ignores semicolons inside line and block comments", () => { + const sql = "-- a comment; with a fake terminator\nCREATE TABLE t (a INT); /* another; one */ CREATE TABLE u (b INT);"; + const statements = splitSqlStatements(sql); + expect(statements).toHaveLength(2); + expect(statements[0]).toContain("CREATE TABLE t (a INT);"); + expect(statements[1]).toContain("CREATE TABLE u (b INT);"); + }); + + it("includes a trailing statement with no terminating semicolon", () => { + expect(splitSqlStatements("CREATE TABLE t (a INT)")).toEqual(["CREATE TABLE t (a INT)"]); + }); + + it("returns [] for empty/whitespace-only input", () => { + expect(splitSqlStatements("")).toEqual([]); + expect(splitSqlStatements(" \n ")).toEqual([]); + }); + + it("treats a doubled quote as an escaped quote, not the end of the string", () => { + expect(splitSqlStatements("INSERT INTO t VALUES ('it''s; still one statement');")).toEqual(["INSERT INTO t VALUES ('it''s; still one statement');"]); + }); + + it("treats a lone semicolon (whitespace-only content before it) as its own no-op statement", () => { + expect(splitSqlStatements("CREATE TABLE t (a INT); ;CREATE TABLE u (b INT);")).toEqual(["CREATE TABLE t (a INT);", ";", "CREATE TABLE u (b INT);"]); + }); +}); + +describe("extractSchemaEvents (#2551)", () => { + it("extracts every column from a CREATE TABLE column list", () => { + const events = extractSchemaEvents("CREATE TABLE widgets (id INTEGER PRIMARY KEY, name TEXT NOT NULL, created_at TEXT);"); + expect(events).toEqual([ + { type: "define_column", table: "widgets", column: "id" }, + { type: "define_column", table: "widgets", column: "name" }, + { type: "define_column", table: "widgets", column: "created_at" }, + ]); + }); + + it("lowercases table/column names so case differences never mask a real collision", () => { + expect(extractSchemaEvents("CREATE TABLE Widgets (Name TEXT);")).toEqual([{ type: "define_column", table: "widgets", column: "name" }]); + }); + + it("handles CREATE TABLE IF NOT EXISTS", () => { + expect(extractSchemaEvents("CREATE TABLE IF NOT EXISTS widgets (id INTEGER);")).toEqual([{ type: "define_column", table: "widgets", column: "id" }]); + }); + + it("excludes table-level PRIMARY KEY/FOREIGN KEY/UNIQUE/CHECK/CONSTRAINT clauses", () => { + const events = extractSchemaEvents( + "CREATE TABLE t (a INTEGER, b INTEGER, PRIMARY KEY (a, b), FOREIGN KEY (a) REFERENCES other(id), UNIQUE(a), CHECK (a > 0), CONSTRAINT named_check CHECK (b > 0));", + ); + expect(events).toEqual([ + { type: "define_column", table: "t", column: "a" }, + { type: "define_column", table: "t", column: "b" }, + ]); + }); + + it("does not split a column definition at a comma nested inside FOREIGN KEY(...) REFERENCES x(...)", () => { + const events = extractSchemaEvents("CREATE TABLE t (a INTEGER, FOREIGN KEY(a) REFERENCES other(id, name));"); + expect(events).toEqual([{ type: "define_column", table: "t", column: "a" }]); + }); + + it("does not split a column definition at a comma inside an inline trailing comment", () => { + // Regression: migrations/0060_orb_fleet_collector.sql has columns with trailing `-- ..., ...` comments + // that previously fooled the top-level comma splitter into treating comment text as a new column. + const sql = ["CREATE TABLE t (", " a TEXT NOT NULL, -- one, two, three", " b TEXT", ");"].join("\n"); + expect(extractSchemaEvents(sql)).toEqual([ + { type: "define_column", table: "t", column: "a" }, + { type: "define_column", table: "t", column: "b" }, + ]); + }); + + it("ignores a leading full-line comment before the actual statement (DROP TABLE)", () => { + // Regression: a `^`-anchored match against a statement with LEADING comment text (produced when a + // comment block precedes a statement with no semicolon of its own) previously failed to match. + const sql = "-- some explanatory comment\n-- spanning two lines\nDROP TABLE IF EXISTS widgets;"; + expect(extractSchemaEvents(sql)).toEqual([{ type: "drop_table", table: "widgets" }]); + }); + + it("extracts a single ADD COLUMN", () => { + expect(extractSchemaEvents("ALTER TABLE widgets ADD COLUMN color TEXT;")).toEqual([{ type: "define_column", table: "widgets", column: "color" }]); + }); + + it("extracts DROP TABLE (with or without IF EXISTS)", () => { + expect(extractSchemaEvents("DROP TABLE widgets;")).toEqual([{ type: "drop_table", table: "widgets" }]); + expect(extractSchemaEvents("DROP TABLE IF EXISTS widgets;")).toEqual([{ type: "drop_table", table: "widgets" }]); + }); + + it("extracts DROP COLUMN as a remove_column event", () => { + expect(extractSchemaEvents("ALTER TABLE widgets DROP COLUMN color;")).toEqual([{ type: "remove_column", table: "widgets", column: "color" }]); + }); + + it("extracts RENAME COLUMN as a remove_column + define_column pair", () => { + expect(extractSchemaEvents("ALTER TABLE widgets RENAME COLUMN color TO hue;")).toEqual([ + { type: "remove_column", table: "widgets", column: "color" }, + { type: "define_column", table: "widgets", column: "hue" }, + ]); + }); + + it("returns [] for a statement with no schema-shape effect (CREATE INDEX, INSERT)", () => { + expect(extractSchemaEvents("CREATE INDEX widgets_name_idx ON widgets (name);")).toEqual([]); + expect(extractSchemaEvents("INSERT INTO widgets (name) VALUES ('x');")).toEqual([]); + }); + + it("does not treat a comma inside a /* block comment */ column list entry as a clause separator", () => { + const events = extractSchemaEvents("CREATE TABLE t (a INTEGER, /* a note, with a comma */ b INTEGER);"); + expect(events).toEqual([ + { type: "define_column", table: "t", column: "a" }, + { type: "define_column", table: "t", column: "b" }, + ]); + }); + + it("preserves a doubled single-quote escape inside a DEFAULT string literal while stripping comments", () => { + const events = extractSchemaEvents("CREATE TABLE t (label TEXT NOT NULL DEFAULT 'it''s here' -- trailing, note\n);"); + expect(events).toEqual([{ type: "define_column", table: "t", column: "label" }]); + }); + + it("skips a top-level clause with no leading identifier instead of crashing", () => { + const events = extractSchemaEvents("CREATE TABLE t (a INTEGER, (1 = 1), b INTEGER);"); + expect(events).toEqual([ + { type: "define_column", table: "t", column: "a" }, + { type: "define_column", table: "t", column: "b" }, + ]); + }); +}); + +describe("detectColumnCollisions (#2551)", () => { + it("returns [] when no two files define the same (table, column)", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (a INTEGER);"], + ["0002_b.sql", "ALTER TABLE t ADD COLUMN b INTEGER;"], + ]; + expect(detectColumnCollisions(files)).toEqual([]); + }); + + it("flags a genuine collision: two migrations independently add the same column", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (id INTEGER);"], + ["0002_b.sql", "ALTER TABLE t ADD COLUMN color TEXT;"], + ["0003_c.sql", "ALTER TABLE t ADD COLUMN color TEXT;"], + ]; + expect(detectColumnCollisions(files)).toEqual([{ table: "t", column: "color", files: ["0002_b.sql", "0003_c.sql"] }]); + }); + + it("does NOT flag a DROP TABLE + CREATE TABLE recreate as colliding with the table it replaces", () => { + // Mirrors migrations/0060_orb_fleet_collector.sql's documented SQLite-ALTER-limitation workaround. + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (id INTEGER, old_col TEXT);"], + ["0002_b.sql", "DROP TABLE IF EXISTS t; CREATE TABLE t (id INTEGER, new_col TEXT);"], + ]; + expect(detectColumnCollisions(files)).toEqual([]); + }); + + it("STILL flags a collision after a recreate if a later migration repeats one of the recreated columns", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (id INTEGER);"], + ["0002_b.sql", "DROP TABLE IF EXISTS t; CREATE TABLE t (id INTEGER, fresh_col TEXT);"], + ["0003_c.sql", "ALTER TABLE t ADD COLUMN fresh_col TEXT;"], + ]; + expect(detectColumnCollisions(files)).toEqual([{ table: "t", column: "fresh_col", files: ["0002_b.sql", "0003_c.sql"] }]); + }); + + it("does not flag a genuine column rename as a collision with its old or new name", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (id INTEGER, old_name TEXT);"], + ["0002_b.sql", "ALTER TABLE t RENAME COLUMN old_name TO new_name;"], + ]; + expect(detectColumnCollisions(files)).toEqual([]); + }); + + it("does not flag a dropped-then-readded column as a collision", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (id INTEGER, temp_col TEXT);"], + ["0002_b.sql", "ALTER TABLE t DROP COLUMN temp_col;"], + ["0003_c.sql", "ALTER TABLE t ADD COLUMN temp_col TEXT;"], + ]; + expect(detectColumnCollisions(files)).toEqual([]); + }); + + it("returns [] for an empty file list", () => { + expect(detectColumnCollisions([])).toEqual([]); + }); + + it("sorts multiple simultaneous collisions by table then column", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE z (x INTEGER); CREATE TABLE a (y INTEGER);"], + ["0002_b.sql", "ALTER TABLE z ADD COLUMN x INTEGER; ALTER TABLE a ADD COLUMN y INTEGER;"], + ]; + expect(detectColumnCollisions(files).map((c) => `${c.table}.${c.column}`)).toEqual(["a.y", "z.x"]); + }); + + it("sorts two collisions on the SAME table by column name", () => { + const files: Array<[string, string]> = [ + ["0001_a.sql", "CREATE TABLE t (z_col INTEGER, a_col INTEGER);"], + ["0002_b.sql", "ALTER TABLE t ADD COLUMN z_col INTEGER; ALTER TABLE t ADD COLUMN a_col INTEGER;"], + ]; + expect(detectColumnCollisions(files).map((c) => `${c.table}.${c.column}`)).toEqual(["t.a_col", "t.z_col"]); + }); + + it("has zero false positives against the repo's real, already-consistent migrations/ directory", () => { + const files = readdirSync("migrations") + .filter((f) => f.endsWith(".sql")) + .sort(); + const contents = files.map((f) => [f, readFileSync(`migrations/${f}`, "utf8")] as const); + expect(detectColumnCollisions(contents)).toEqual([]); + }); +}); From 282077399225a47c71417438e6ef4fa0acc9f7e2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 2 Jul 2026 06:00:41 -0700 Subject: [PATCH 2/2] fix(gate): record a column collision before a later DROP TABLE can erase 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. --- src/db/migration-column-extraction.ts | 23 ++++++++++++------- test/unit/migration-column-extraction.test.ts | 8 +++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/src/db/migration-column-extraction.ts b/src/db/migration-column-extraction.ts index a7ccc1a804..6ab2b4e16d 100644 --- a/src/db/migration-column-extraction.ts +++ b/src/db/migration-column-extraction.ts @@ -200,10 +200,17 @@ export type ColumnCollision = { table: string; column: string; files: string[] } * individually-valid migrations (#2551). `orderedFileContents` must already be sorted ascending by migration * number (the same order `scripts/check-migrations.mjs` reads the directory in); a `drop_table` event clears * every column tracked for that table so far, so a documented DROP+CREATE recreate never reads as a - * collision with the table it replaces. Pure, no I/O. + * collision with the table it replaces. + * + * A collision is recorded PERMANENTLY the moment it's detected, before any later `drop_table` event can + * clear the tracking map -- real migration execution runs statements strictly in order, so + * `CREATE TABLE t (c INT); ALTER TABLE t ADD COLUMN c INT; DROP TABLE t;` already fails at the ADD COLUMN + * (duplicate column) and the DROP TABLE is never reached; a later DROP can never retroactively make an + * already-fatal duplicate definition safe. Pure, no I/O. */ export function detectColumnCollisions(orderedFileContents: ReadonlyArray): ColumnCollision[] { const tracked = new Map }>(); + const collisions = new Map(); for (const [filename, sql] of orderedFileContents) { for (const statement of splitSqlStatements(sql)) { @@ -220,15 +227,15 @@ export function detectColumnCollisions(orderedFileContents: ReadonlyArray 1) collisions.push({ table, column, files: [...files].sort() }); - } - return collisions.sort((a, b) => (a.table === b.table ? a.column.localeCompare(b.column) : a.table.localeCompare(b.table))); + return [...collisions.values()].sort((a, b) => (a.table === b.table ? a.column.localeCompare(b.column) : a.table.localeCompare(b.table))); } diff --git a/test/unit/migration-column-extraction.test.ts b/test/unit/migration-column-extraction.test.ts index a0b8ddec83..1ba9dc867e 100644 --- a/test/unit/migration-column-extraction.test.ts +++ b/test/unit/migration-column-extraction.test.ts @@ -170,6 +170,14 @@ describe("detectColumnCollisions (#2551)", () => { expect(detectColumnCollisions(files)).toEqual([{ table: "t", column: "fresh_col", files: ["0002_b.sql", "0003_c.sql"] }]); }); + it("STILL flags a collision even when a DROP TABLE for that table comes later in the SAME file (#2607 gate finding)", () => { + // Real migration execution runs statements strictly in order: `CREATE TABLE t (c INT); ALTER TABLE t + // ADD COLUMN c INT; DROP TABLE t;` already fails at the duplicate ADD COLUMN, so the DROP TABLE is + // never reached -- it must not retroactively erase the collision that already happened before it. + const files: Array<[string, string]> = [["0001_x.sql", "CREATE TABLE t (c INTEGER); ALTER TABLE t ADD COLUMN c INTEGER; DROP TABLE t;"]]; + expect(detectColumnCollisions(files)).toEqual([{ table: "t", column: "c", files: ["0001_x.sql"] }]); + }); + it("does not flag a genuine column rename as a collision with its old or new name", () => { const files: Array<[string, string]> = [ ["0001_a.sql", "CREATE TABLE t (id INTEGER, old_name TEXT);"],