diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 587285f426..6704e263f0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -192,6 +192,12 @@ jobs: - name: Check migrations if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} run: npm run db:migrations:check + # Guards against a DIFFERENT drift class than db:migrations:check's own column-collision check (#2551, + # which only catches two migration FILES independently adding the same column): src/db/schema.ts's + # DECLARED shape silently drifting from what migrations/ ACTUALLY produces when replayed (#2565). + - name: Schema-vs-migrations drift check + if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' }} + run: npm run db:schema-drift:check # Guards against the same staleness-drift class as db:migrations:check/ui:openapi:check: two PRs that # each independently add a wrangler.jsonc binding can both pass CI in isolation, then merge sequentially # and leave a stale committed worker-configuration.d.ts with zero prior gate signal (#2557). diff --git a/package.json b/package.json index 1ea9cc4e44..8d5c49b206 100644 --- a/package.json +++ b/package.json @@ -32,6 +32,7 @@ "rees:metadata:check": "npm --prefix review-enrichment run metadata:check", "rees:validate-sourcemaps": "npm --prefix review-enrichment run validate:sourcemaps", "db:migrations:check": "tsx scripts/check-migrations.mjs", + "db:schema-drift:check": "tsx scripts/check-schema-drift.mjs", "actionlint": "node scripts/actionlint.mjs", "ui:dev": "npm run ui:preview", "extension:build": "node scripts/build-extension.mjs", @@ -67,7 +68,7 @@ "test:smoke:observability": "node scripts/smoke-observability-traces.mjs", "test:smoke:browser:install": "playwright install chromium", "test:smoke:browser": "node scripts/smoke-ui-browser.mjs", - "test:ci": "git diff --check && npm run actionlint && npm run db:migrations:check && npm run selfhost:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run typecheck && npm run test:coverage && npm run test:workers && npm run build:mcp && npm run test:mcp-pack && npm run build:miner && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", + "test:ci": "git diff --check && npm run actionlint && npm run db:migrations:check && npm run db:schema-drift:check && npm run selfhost:env-reference:check && npm run selfhost:validate-observability && npm run cf-typegen:check && npm run typecheck && npm run test:coverage && npm run test:workers && npm run build:mcp && npm run test:mcp-pack && npm run build:miner && npm run rees:test && npm run ui:openapi:check && npm run ui:openapi:settings-parity && npm run ui:version-audit && npm run ui:lint && npm run ui:typecheck && npm run ui:test && npm run ui:build", "test:release": "npm run test:ci && npm run changelog:check", "test:release:mcp": "npm run test:ci && npm run changelog:check:mcp", "test:watch": "vitest", diff --git a/scripts/check-schema-drift.d.mts b/scripts/check-schema-drift.d.mts new file mode 100644 index 0000000000..9ed4c098d1 --- /dev/null +++ b/scripts/check-schema-drift.d.mts @@ -0,0 +1,16 @@ +import type { DatabaseSync } from "node:sqlite"; +import type { SQLiteTable } from "drizzle-orm/sqlite-core"; + +export const RAW_SQL_ONLY_TABLES: Set; + +export function replayMigrations(dir: string): DatabaseSync; + +export function listActualTables(db: DatabaseSync): Set; + +export function actualColumnsFor(db: DatabaseSync, table: string): Set; + +export function collectSchemaTables(schemaModule: Record): Map; + +export function declaredColumnsFor(table: SQLiteTable): Set; + +export function diffSchemaAgainstMigrations(db: DatabaseSync, schemaModule: Record, rawSqlOnlyTables?: Set): string[]; diff --git a/scripts/check-schema-drift.mjs b/scripts/check-schema-drift.mjs new file mode 100755 index 0000000000..119524becf --- /dev/null +++ b/scripts/check-schema-drift.mjs @@ -0,0 +1,164 @@ +#!/usr/bin/env tsx +// #2565: src/db/schema.ts (Drizzle ORM sqliteTable declarations) is a single shared file where two +// independently-valid PRs can each add a new column to the SAME table. scripts/check-migrations.mjs's +// detectColumnCollisions (#2551) only catches a git-merge-race collision between two DIFFERENT migration +// files that both add the same (table, column) pair -- it never reads src/db/schema.ts at all, so it cannot +// see the DIFFERENT gap this check closes: schema.ts's DECLARED shape (what Drizzle thinks a table's columns +// are) drifting from what migrations/ ACTUALLY produces when replayed against a fresh DB -- e.g. schema.ts +// declares a column no migration ever created, or a migration created a column later dropped from schema.ts +// without cleaning up the migration. Nothing else in CI catches that. +// +// Mechanism: replay every migrations/*.sql file into a fresh in-memory node:sqlite DB (mirrors +// test/helpers/d1.ts's TestD1Database -- same concatenate-sorted-files-then-exec approach, so this check and +// the test suite's DB can never silently disagree about what migrations "actually" produce), introspect each +// table's REAL columns via `PRAGMA table_info`, then compare against src/db/schema.ts's DECLARED columns via +// drizzle-orm's getTableColumns (keyed by each column's .name -- the actual DB column name, not the JS +// property name). Diff the two column-name sets per table. +// +// Run via `tsx` (not plain `node`) for the same reason as check-migrations.mjs and +// check-openapi-settings-parity.mjs: this script imports src/db/schema.ts (a .ts module) directly, and a bare +// `node` invocation can't resolve a `.ts` import without an experimental flag CI's pinned Node isn't +// guaranteed to support. +import { readdirSync, readFileSync } from "node:fs"; +import { DatabaseSync } from "node:sqlite"; +import { fileURLToPath } from "node:url"; +import { getTableColumns, is } from "drizzle-orm"; +import { SQLiteTable } from "drizzle-orm/sqlite-core"; +import * as schema from "../src/db/schema.ts"; + +const MIGRATIONS_DIR = process.env.CHECK_SCHEMA_DRIFT_DIR || "migrations"; + +// Feature/aggregate tables that intentionally live ONLY in migrations/ and are accessed via raw SQL +// (env.DB.prepare(...)) rather than through a Drizzle sqliteTable declaration -- the house pattern documented +// in .claude/skills/contributing-to-gittensory/reference.md ("core tables use Drizzle; feature/aggregate +// tables use raw-SQL migrations"). Each of these is confirmed (by direct inspection at the time this check +// was added) to be actively read/written via raw SQL elsewhere in src/ -- this is not a dead-table allowlist, +// it is a declared exception to "every migrated table must have a matching schema.ts declaration". Adding a +// table here without also confirming it is genuinely raw-SQL-only is a reviewer-visible diff, not a silent +// gap this check would otherwise catch. +export const RAW_SQL_ONLY_TABLES = new Set([ + "global_agent_controls", + "global_contributor_blacklist", + "orb_enrollments", + "orb_export_cursor", + "orb_github_installations", + "orb_instances", + "orb_pr_outcomes", + "orb_relay_failures", + "orb_signals", + "orb_webhook_events", + "override_audit", + "repo_chunks", + "review_audit", + "review_targets", + "submission_drafts", + "submission_user_tokens", + "submitter_stats", + "system_flags", + "tunables_overrides", + "tunables_overrides_shadow", +]); + +/** Replay every migrations/*.sql file (concatenated, sorted -- the same order wrangler and + * test/helpers/d1.ts's TestD1Database use) into a fresh in-memory SQLite DB and return it. Deliberately not + * importing TestD1Database directly: that helper caches the concatenated SQL into module-scope state sized + * for the test suite's lifetime and always reads the repo's real `migrations/` dir, whereas this script's + * tests need an independently directed migrations dir (CHECK_SCHEMA_DRIFT_DIR) to exercise a deliberately + * drifted fixture without touching the real migrations/. */ +export function replayMigrations(dir) { + const sql = readdirSync(dir) + .filter((file) => file.endsWith(".sql")) + .sort() + .map((file) => readFileSync(`${dir}/${file}`, "utf8")) + .join("\n"); + const db = new DatabaseSync(":memory:"); + db.exec(sql); + return db; +} + +/** The set of real, non-sqlite-internal table names present in a replayed migrations DB. */ +export function listActualTables(db) { + const rows = db.prepare("SELECT name FROM sqlite_master WHERE type = 'table' AND name NOT LIKE 'sqlite_%'").all(); + return new Set(rows.map((row) => String(row.name))); +} + +/** The actual DB column names (via PRAGMA table_info's `name`, not cid/type/etc.) for one table. */ +export function actualColumnsFor(db, table) { + const rows = db.prepare(`PRAGMA table_info(${table})`).all(); + return new Set(rows.map((row) => String(row.name))); +} + +/** Every exported drizzle sqliteTable in the given schema module, keyed by its declared DB table name (not + * the JS export name -- schema.ts's export identifiers are camelCase, but getTableColumns / PRAGMA both key + * on the actual snake_case DB name). Guards against a non-table export (schema.ts today exports only + * tables, but this stays correct if a helper/type export is ever added alongside them). */ +export function collectSchemaTables(schemaModule) { + const tables = new Map(); + for (const value of Object.values(schemaModule)) { + if (!is(value, SQLiteTable)) continue; + tables.set(value[SQLiteTable.Symbol.Name], value); + } + return tables; +} + +/** The declared DB column names (getTableColumns(table)[key].name -- the actual DB column, not the JS + * property name) for one drizzle table object. */ +export function declaredColumnsFor(table) { + return new Set(Object.values(getTableColumns(table)).map((column) => column.name)); +} + +/** + * Diff migrations/'s actually-produced schema against src/db/schema.ts's declared shape. Pure given the two + * already-loaded inputs (an open replayed DB and the imported schema module) -- no filesystem/import side + * effects of its own, so it's directly unit-testable against a hand-built fixture DB + fake schema module. + * Returns one mismatch entry per (table, column) or per whole missing table; empty when they agree. + */ +export function diffSchemaAgainstMigrations(db, schemaModule, rawSqlOnlyTables = RAW_SQL_ONLY_TABLES) { + const actualTables = listActualTables(db); + const schemaTables = collectSchemaTables(schemaModule); + const mismatches = []; + + for (const [tableName, table] of [...schemaTables].sort(([a], [b]) => a.localeCompare(b))) { + if (!actualTables.has(tableName)) { + mismatches.push(`table "${tableName}" is declared in src/db/schema.ts but no migration creates it`); + continue; + } + const declared = declaredColumnsFor(table); + const actual = actualColumnsFor(db, tableName); + const missingFromMigrations = [...declared].filter((column) => !actual.has(column)).sort(); + const missingFromSchema = [...actual].filter((column) => !declared.has(column)).sort(); + for (const column of missingFromMigrations) { + mismatches.push(`${tableName}.${column} is declared in src/db/schema.ts but no migration creates that column`); + } + for (const column of missingFromSchema) { + mismatches.push(`${tableName}.${column} exists in migrations/ but is missing from src/db/schema.ts's ${tableName} declaration`); + } + } + + // A table that only exists in migrations/ (never in schema.ts at all) is either a legitimate raw-SQL-only + // feature table (rawSqlOnlyTables) or an undeclared drift -- flag anything not on the allowlist. + for (const tableName of [...actualTables].sort()) { + if (schemaTables.has(tableName) || rawSqlOnlyTables.has(tableName)) continue; + mismatches.push(`table "${tableName}" exists in migrations/ but has no src/db/schema.ts declaration and is not in RAW_SQL_ONLY_TABLES (scripts/check-schema-drift.mjs)`); + } + + return mismatches; +} + +function main() { + const db = replayMigrations(MIGRATIONS_DIR); + const mismatches = diffSchemaAgainstMigrations(db, schema); + + if (mismatches.length > 0) { + process.stderr.write(`check-schema-drift: src/db/schema.ts has drifted from migrations/ -- ${mismatches.length} mismatch(es):\n`); + for (const mismatch of mismatches) process.stderr.write(` - ${mismatch}\n`); + process.stderr.write("Fix by either updating src/db/schema.ts to match migrations/, adding a migration for the missing column/table, or (for an intentionally raw-SQL-only table) adding it to RAW_SQL_ONLY_TABLES in scripts/check-schema-drift.mjs.\n"); + process.exit(1); + } + + const tableCount = collectSchemaTables(schema).size; + process.stdout.write(`check-schema-drift: src/db/schema.ts matches migrations/ -- ${tableCount} Drizzle tables OK, ${RAW_SQL_ONLY_TABLES.size} raw-SQL-only tables allowlisted.\n`); +} + +// Guard so importing this module for its pure exports (tests) never triggers the file-read/exit side effects. +if (process.argv[1] === fileURLToPath(import.meta.url)) main(); diff --git a/test/unit/check-schema-drift-script.test.ts b/test/unit/check-schema-drift-script.test.ts new file mode 100644 index 0000000000..e60b3e2f65 --- /dev/null +++ b/test/unit/check-schema-drift-script.test.ts @@ -0,0 +1,191 @@ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { sqliteTable, text } from "drizzle-orm/sqlite-core"; +import { afterEach, describe, expect, it } from "vitest"; +import { + actualColumnsFor, + collectSchemaTables, + declaredColumnsFor, + diffSchemaAgainstMigrations, + listActualTables, + RAW_SQL_ONLY_TABLES, + replayMigrations, +} from "../../scripts/check-schema-drift.mjs"; +import * as realSchema from "../../src/db/schema"; + +// #2565: the script imports src/db/schema.ts (a .ts module), so -- like check-migrations.mjs and +// check-openapi-settings-parity.mjs -- it must run via `tsx`, the same binary package.json's +// db:schema-drift:check uses, rather than plain `node`. +const TSX_BIN = join(process.cwd(), "node_modules", ".bin", "tsx"); + +const tmpDirs: string[] = []; +afterEach(() => { + for (const dir of tmpDirs.splice(0)) rmSync(dir, { recursive: true, force: true }); +}); + +describe("check-schema-drift script (#2565)", () => { + // Most important regression test in this file: proves the REAL current src/db/schema.ts and the REAL + // migrations/ directory are not already drifted -- if they were, this check would fail on `main` from + // the moment it merges. + it("the real repo's schema.ts and migrations/ agree (regression guard)", () => { + const db = replayMigrations("migrations"); + const mismatches = diffSchemaAgainstMigrations(db, realSchema); + + expect(mismatches).toEqual([]); + }); + + it("prints a clean summary for the real repo state when run as a subprocess", () => { + const output = execFileSync(TSX_BIN, ["scripts/check-schema-drift.mjs"], { encoding: "utf8" }); + + expect(output).toMatch(/src\/db\/schema\.ts matches migrations\/ -- \d+ Drizzle tables OK/); + }); + + it("fails when schema.ts declares a column no migration creates", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_widgets.sql"), "CREATE TABLE widgets (id INTEGER PRIMARY KEY, name TEXT);\n"); + // Fake a schema module whose declared shape has an extra column vs. the fixture migration. + const fakeSchema = { + widgets: fakeSqliteTable("widgets", { id: "id", name: "name", color: "color" }), + }; + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, fakeSchema); + + expect(mismatches).toContain("widgets.color is declared in src/db/schema.ts but no migration creates that column"); + }); + + it("fails when a migration creates a column schema.ts no longer declares", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_widgets.sql"), "CREATE TABLE widgets (id INTEGER PRIMARY KEY, name TEXT, legacy_flag INTEGER);\n"); + const fakeSchema = { widgets: fakeSqliteTable("widgets", { id: "id", name: "name" }) }; + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, fakeSchema); + + expect(mismatches).toContain("widgets.legacy_flag exists in migrations/ but is missing from src/db/schema.ts's widgets declaration"); + }); + + it("fails when schema.ts declares a table no migration ever creates", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_widgets.sql"), "CREATE TABLE widgets (id INTEGER PRIMARY KEY);\n"); + const fakeSchema = { + widgets: fakeSqliteTable("widgets", { id: "id" }), + ghosts: fakeSqliteTable("ghosts", { id: "id" }), + }; + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, fakeSchema); + + expect(mismatches).toContain('table "ghosts" is declared in src/db/schema.ts but no migration creates it'); + }); + + it("fails when a migrated table has no schema.ts declaration and is not raw-SQL-allowlisted", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_undeclared.sql"), "CREATE TABLE undeclared_thing (id INTEGER PRIMARY KEY);\n"); + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, {}); + + expect(mismatches).toContain('table "undeclared_thing" exists in migrations/ but has no src/db/schema.ts declaration and is not in RAW_SQL_ONLY_TABLES (scripts/check-schema-drift.mjs)'); + }); + + it("does not flag a migrated table that is on the raw-SQL-only allowlist", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_raw.sql"), "CREATE TABLE feature_flags (id INTEGER PRIMARY KEY);\n"); + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, {}, new Set(["feature_flags"])); + + expect(mismatches).toEqual([]); + }); + + it("does not flag a non-table export (e.g. a helper/type) alongside real tables", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_widgets.sql"), "CREATE TABLE widgets (id INTEGER PRIMARY KEY);\n"); + const fakeSchema = { + widgets: fakeSqliteTable("widgets", { id: "id" }), + someHelperFn: () => "not a table", + someConstant: 42, + }; + const db = replayMigrations(dir); + const mismatches = diffSchemaAgainstMigrations(db, fakeSchema); + + expect(mismatches).toEqual([]); + }); + + it("passes cleanly when schema.ts and migrations/ agree exactly", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-")); + tmpDirs.push(dir); + writeFileSync(join(dir, "0001_widgets.sql"), "CREATE TABLE widgets (id INTEGER PRIMARY KEY, name TEXT);\n"); + const fakeSchema = { widgets: fakeSqliteTable("widgets", { id: "id", name: "name" }) }; + const db = replayMigrations(dir); + + expect(diffSchemaAgainstMigrations(db, fakeSchema)).toEqual([]); + }); + + it("CLI: reports every mismatch and exits non-zero for a deliberately drifted schema/migration pair", () => { + const dir = mkdtempSync(join(tmpdir(), "gtschema-drift-cli-")); + tmpDirs.push(dir); + // No src/db/schema.ts override exists for the CLI path (main() always imports the REAL schema.ts), so + // exercise the CLI's drift-reporting/exit-code behavior through a migrations fixture that omits a + // column the real schema.ts declares on an actual table, guaranteeing at least one reported mismatch + // without needing to fake-import a schema module through the CLI entrypoint. + writeFileSync(join(dir, "0001_stub.sql"), "CREATE TABLE stub_only (id INTEGER PRIMARY KEY);\n"); + try { + execFileSync(TSX_BIN, ["scripts/check-schema-drift.mjs"], { + encoding: "utf8", + env: { ...process.env, CHECK_SCHEMA_DRIFT_DIR: dir }, + }); + expect.unreachable("expected the CLI to exit non-zero for a drifted fixture"); + } catch (err) { + const e = err as { status?: number; stdout?: string; stderr?: string }; + expect(e.status).toBe(1); + const out = `${e.stdout ?? ""}${e.stderr ?? ""}`; + expect(out).toContain("check-schema-drift: src/db/schema.ts has drifted from migrations/"); + expect(out).toContain("Fix by either updating src/db/schema.ts"); + } + }); + + it("collectSchemaTables / declaredColumnsFor / actualColumnsFor / listActualTables agree on the real repo's webhook_events table", () => { + const db = replayMigrations("migrations"); + const tables = collectSchemaTables(realSchema); + const webhookEventsTable = tables.get("webhook_events"); + expect(webhookEventsTable).toBeDefined(); + + const declared = declaredColumnsFor(webhookEventsTable!); + const actual = actualColumnsFor(db, "webhook_events"); + expect(declared).toEqual(actual); + expect(listActualTables(db).has("webhook_events")).toBe(true); + }); + + it("the RAW_SQL_ONLY_TABLES allowlist covers every migrated table that has no schema.ts declaration", () => { + // A structural guard on the allowlist itself: every entry must correspond to a table that ACTUALLY + // exists in the real migrations/ output (otherwise it's dead weight that could mask a real removal), + // and no entry may also be schema.ts-declared (that would make the allowlist entry redundant/misleading). + const db = replayMigrations("migrations"); + const actualTables = listActualTables(db); + const schemaTableNames = new Set(collectSchemaTables(realSchema).keys()); + + for (const table of RAW_SQL_ONLY_TABLES) { + expect(actualTables.has(table)).toBe(true); + expect(schemaTableNames.has(table)).toBe(false); + } + }); +}); + +// --- test-local helpers ----------------------------------------------------------------------------------- + +/** Build a minimal fake drizzle-shaped table object sufficient for collectSchemaTables/declaredColumnsFor: + * real sqliteTable() objects key their DB table name under drizzle-orm's internal Symbol and expose their + * columns via getTableColumns(), which itself reads each column's own internal name Symbol -- too much + * drizzle-internal surface to fake directly. Instead, build a REAL sqliteTable() via drizzle-orm/sqlite-core + * so is(value, SQLiteTable) and getTableColumns() behave identically to the production schema.ts tables + * these functions are exercised against elsewhere in this file. */ +function fakeSqliteTable(tableName: string, columnDbNames: Record) { + const columns: Record> = {}; + for (const [jsKey, dbName] of Object.entries(columnDbNames)) columns[jsKey] = text(dbName); + return sqliteTable(tableName, columns); +}