Merge pull request #1163 from mikewheeleer/codex/issue-1148-callora-migrations

[#1148] Make migration drift protection blocking and explicit

Author: greatest0fallt1me

.github/workflows/ci.yml

@@ -7,6 +7,23 @@ on:
     branches: ["main", "develop"]
 
 jobs:
+  migration-policy:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v4
+      - name: Setup Node.js
+        uses: actions/setup-node@v4
+        with:
+          node-version: 20
+          cache: npm
+      - name: Install dependencies
+        run: npm ci
+      - name: Verify migration layout and schema drift
+        run: npx tsx scripts/check-migrations.ts
+        env:
+          CHECKSUM_CI_SKIP_MISSING: "1"
+
   build:
     runs-on: ubuntu-latest
     continue-on-error: true
@@ -65,7 +82,6 @@ jobs:
           echo "✅ Build artifacts verified"
 
       - name: Run Schema Versioning Check
-        continue-on-error: true
         run: npx tsx scripts/check-migrations.ts
         env:
           CHECKSUM_CI_SKIP_MISSING: "1"

migrations/README.md

@@ -76,3 +76,18 @@ Roll back in **reverse** order (highest prefix first).
 4. Run `npm test -- src/migrate.runner.test.ts` to verify the runner still passes.
 5. Run `npm run db:check-migrations` to verify the checksum gate passes.
 6. Commit both migration files.
+
+### CI policy for new migrations
+
+Historical migrations through `0021` are frozen because their names and
+versions are already recorded in deployed databases. New migrations must start
+at `0022`, use a unique four-digit prefix, and continue without gaps. A new
+forward migration containing `DROP`, `TRUNCATE`, or `DELETE FROM` must include
+an explicit approval marker on its own line:
+
+```sql
+-- destructive-approved: #1148
+```
+
+The schema-versioning CI step runs this layout check even when no local
+database exists, and its result is blocking.

scripts/check-migrations.ts

@@ -1,6 +1,6 @@
 #!/usr/bin/env tsx
 /**
- * check-migrations.ts  —  Schema Versioning CI Gate
+ * check-migrations.ts    Schema Versioning CI Gate
  *
  * Verifies that every migration file on disk matches its recorded checksum in the
  * schema_versions table.  Any mismatch means a migration was modified *after* being
@@ -20,6 +20,7 @@ import Database from 'better-sqlite3';
 import { readFileSync, readdirSync, existsSync } from 'fs';
 import path from 'path';
 import { createHash } from 'node:crypto';
+import { validateMigrationLayout } from './migrationPolicy.js';
 
 const rootDir = process.cwd();
 const dbPath = path.join(rootDir, 'database.db');
@@ -41,6 +42,12 @@ function main() {
   console.log('Schema Versioning Drift Check');
   console.log('================================');
   console.log('');
+  const layoutErrors = validateMigrationLayout(migrationDir);
+  if (layoutErrors.length > 0) {
+    console.error('Migration layout policy failed:');
+    layoutErrors.forEach(function(error) { console.error('  ' + error); });
+    process.exit(1);
+  }
   if (!existsSync(dbPath)) {
     console.log('No database file found. Skipping checksum verification.');
     console.log('(Expected on fresh checkout before running migrations.)');
@@ -112,4 +119,4 @@ function main() {
     process.exit(0);
   } finally { db.close(); }
 }
-main();
+main();

scripts/migrationPolicy.test.ts

@@ -0,0 +1,46 @@
+import { mkdtempSync, rmSync, writeFileSync } from 'node:fs';
+import os from 'node:os';
+import path from 'node:path';
+import { describe, expect, it } from '@jest/globals';
+import { validateMigrationLayout } from './migrationPolicy.js';
+
+function withMigrations(files: Record<string, string>, test: (dir: string) => void): void {
+  const dir = mkdtempSync(path.join(os.tmpdir(), 'callora-migrations-'));
+  try {
+    Object.entries(files).forEach(([name, body]) => writeFileSync(path.join(dir, name), body));
+    test(dir);
+  } finally {
+    rmSync(dir, { recursive: true, force: true });
+  }
+}
+
+describe('migration layout policy', () => {
+  it('accepts a new contiguous migration after the frozen history', () => {
+    withMigrations({ '0022_add_limits.sql': 'CREATE TABLE limits (id INTEGER);' }, (dir) => {
+      expect(validateMigrationLayout(dir)).toEqual([]);
+    });
+  });
+
+  it('rejects duplicate, gapped, and unnumbered new migrations', () => {
+    withMigrations({
+      '0022_first.sql': 'SELECT 1;',
+      '0022_second.sql': 'SELECT 1;',
+      '0024_gap.sql': 'SELECT 1;',
+      'new_feature.sql': 'SELECT 1;',
+    }, (dir) => {
+      const errors = validateMigrationLayout(dir).join('\n');
+      expect(errors).toContain('Duplicate new migration prefix 22');
+      expect(errors).toContain('new_feature.sql');
+      expect(errors).toContain('sequence must continue');
+    });
+  });
+
+  it('requires an issue marker for destructive SQL', () => {
+    withMigrations({ '0022_remove_legacy.sql': 'DROP TABLE legacy;' }, (dir) => {
+      expect(validateMigrationLayout(dir).join('\n')).toContain('destructive-approved');
+    });
+    withMigrations({ '0022_remove_legacy.sql': '-- destructive-approved: #1148\nDROP TABLE legacy;' }, (dir) => {
+      expect(validateMigrationLayout(dir)).toEqual([]);
+    });
+  });
+});

scripts/migrationPolicy.ts

@@ -0,0 +1,56 @@
+import { readdirSync, readFileSync } from 'node:fs';
+import path from 'node:path';
+
+// Historical migrations use two numbering schemes and contain deployed
+// duplicate prefixes. They are frozen; new migrations begin at 0022.
+const LEGACY_MAX_PREFIX = 21;
+const LEGACY_UNNUMBERED = new Set([
+  'add_refresh_token_family.sql', 'add_refresh_tokens.down.sql', 'add_refresh_tokens.sql',
+  'auth_index.down.sql', 'auth_index.sql', 'billing_index.down.sql', 'billing_index.sql',
+  'credits_index.down.sql', 'credits_index.sql',
+]);
+
+function prefix(filename: string): number | null {
+  const match = filename.match(/^(\d{4})_/);
+  return match ? Number(match[1]) : null;
+}
+
+function isUpMigration(filename: string): boolean {
+  return (filename.endsWith('.sql') || filename.endsWith('.up.sql')) && !filename.endsWith('.down.sql');
+}
+
+/** Return policy violations without mutating the migration directory. */
+export function validateMigrationLayout(migrationDir: string): string[] {
+  const files = readdirSync(migrationDir).filter(isUpMigration);
+  const violations: string[] = [];
+  const future = files.filter((file) => {
+    const number = prefix(file);
+    return number !== null ? number > LEGACY_MAX_PREFIX : !LEGACY_UNNUMBERED.has(file);
+  });
+
+  for (const file of future) {
+    const number = prefix(file);
+    if (number === null) {
+      violations.push(`Migration file "${file}" must use NNNN_description.sql naming.`);
+      continue;
+    }
+    if (!/^\d{4}_[a-z0-9][a-z0-9_-]*\.sql$/.test(file) && !/^\d{4}_[a-z0-9][a-z0-9_-]*\.up\.sql$/.test(file)) {
+      violations.push(`Migration file "${file}" must use four digits and a lowercase description.`);
+    }
+    const content = readFileSync(path.join(migrationDir, file), 'utf8');
+    if (/\b(?:DROP|TRUNCATE)\b|\bDELETE\s+FROM\b/i.test(content) && !/^\s*--\s*destructive-approved:\s*#[0-9]+\s*$/im.test(content)) {
+      violations.push(`Destructive migration "${file}" requires -- destructive-approved: #<issue>.`);
+    }
+  }
+
+  const numbers = future.map(prefix).filter((number): number is number => number !== null).sort((a, b) => a - b);
+  for (let index = 0; index < numbers.length; index += 1) {
+    const expected = LEGACY_MAX_PREFIX + 1 + index;
+    if (numbers[index] !== expected) {
+      violations.push(`Migration sequence must continue at ${String(expected).padStart(4, '0')}; found ${String(numbers[index]).padStart(4, '0')}.`);
+      break;
+    }
+    if (index > 0 && numbers[index] === numbers[index - 1]) violations.push(`Duplicate new migration prefix ${numbers[index]}.`);
+  }
+  return violations;
+}