refactor: file-based migrations — eliminate silent version-collision bugs

__tests__/migrations-registry.test.ts

@@ -0,0 +1,95 @@
+/**
+ * Migration registry: structural invariants.
+ *
+ * Guards against the silent-no-op bug class that motivated this
+ * refactor. If a future PR introduces a duplicate version,
+ * out-of-order versions, or fails to register a new migration
+ * file, one of these tests fails loudly.
+ */
+import { describe, it, expect } from 'vitest';
+import * as fs from 'fs';
+import * as path from 'path';
+import {
+  ALL_MIGRATIONS,
+  CURRENT_SCHEMA_VERSION,
+} from '../src/db/migrations';
+
+describe('migration registry — structural invariants', () => {
+  it('registry is non-empty', () => {
+    expect(ALL_MIGRATIONS.length).toBeGreaterThan(0);
+  });
+
+  it('versions are unique', () => {
+    const seen = new Set<number>();
+    for (const m of ALL_MIGRATIONS) {
+      expect(seen.has(m.version)).toBe(false);
+      seen.add(m.version);
+    }
+  });
+
+  it('versions are strictly ascending', () => {
+    for (let i = 1; i < ALL_MIGRATIONS.length; i++) {
+      expect(ALL_MIGRATIONS[i]!.version).toBeGreaterThan(
+        ALL_MIGRATIONS[i - 1]!.version
+      );
+    }
+  });
+
+  it('each migration has a non-empty description and a function up()', () => {
+    for (const m of ALL_MIGRATIONS) {
+      expect(m.description.length).toBeGreaterThan(0);
+      expect(typeof m.up).toBe('function');
+    }
+  });
+
+  it('CURRENT_SCHEMA_VERSION matches the highest registered version', () => {
+    const max = ALL_MIGRATIONS[ALL_MIGRATIONS.length - 1]!.version;
+    expect(CURRENT_SCHEMA_VERSION).toBe(max);
+  });
+});
+
+describe('migration files — filename ↔ version coupling', () => {
+  // Read the actual filenames on disk and assert each matches an
+  // entry in the registry. Catches the case where someone drops a
+  // new file in src/db/migrations/ but forgets to register it.
+  const migrationsDir = path.resolve(__dirname, '../src/db/migrations');
+  const SUPPORT_FILES = new Set(['index.ts', 'types.ts']);
+  const STRICT_NNN_PATTERN = /^\d{3}-[a-z0-9]+(?:-[a-z0-9]+)*\.ts$/;
+
+  function listMigrationFiles(): string[] {
+    return fs.readdirSync(migrationsDir).filter((f) => f.endsWith('.ts') && !SUPPORT_FILES.has(f));
+  }
+
+  it('every migration file matches the strict `NNN-kebab-name.ts` pattern', () => {
+    const offenders: string[] = [];
+    for (const f of listMigrationFiles()) {
+      if (!STRICT_NNN_PATTERN.test(f)) {
+        offenders.push(f);
+      }
+    }
+    expect(offenders).toEqual([]);
+  });
+
+  it('every src/db/migrations/NNN-*.ts file is registered (no orphan files)', () => {
+    const files = listMigrationFiles().filter((f) => STRICT_NNN_PATTERN.test(f));
+    expect(files.length).toBeGreaterThan(0);
+    const registeredVersions = new Set(ALL_MIGRATIONS.map((m) => m.version));
+    for (const f of files) {
+      const version = parseInt(f.slice(0, 3), 10);
+      if (!registeredVersions.has(version)) {
+        throw new Error(
+          `Migration file ${f} exists on disk but is not registered in src/db/migrations/index.ts. ` +
+            `Add an import + array entry for it.`
+        );
+      }
+    }
+  });
+
+  it('every registered version has a matching NNN-*.ts file (no phantom registrations)', () => {
+    const files = listMigrationFiles().filter((f) => STRICT_NNN_PATTERN.test(f));
+    const filenameVersions = new Set(files.map((f) => parseInt(f.slice(0, 3), 10)));
+    for (const m of ALL_MIGRATIONS) {
+      expect(filenameVersions.has(m.version)).toBe(true);
+    }
+  });
+});

src/db/migrations.ts

@@ -1,60 +1,26 @@
 /**
- * Database Migrations
+ * Database Migrations — runner + backward-compat surface.
  *
- * Schema versioning and migration support.
+ * The migration definitions themselves live in
+ * `./migrations/<NNN>-<name>.ts`, one file per migration, with
+ * version derived from the filename prefix. This file is the
+ * runner (read schema_versions, apply pending in order) and the
+ * stable API surface that the rest of the codebase imports.
+ *
+ * Adding a migration: see `./migrations/index.ts`.
  */
 
 import { SqliteDatabase } from './sqlite-adapter';
+import { ALL_MIGRATIONS, CURRENT_SCHEMA_VERSION as REGISTRY_CURRENT } from './migrations/index';
+import type { Migration } from './migrations/types';
 
 /**
- * Current schema version
+ * Highest registered migration version. Derived from the
+ * registry; re-exported here unchanged so existing consumers
+ * (`import { CURRENT_SCHEMA_VERSION } from './migrations'`) keep
+ * working.
  */
-export const CURRENT_SCHEMA_VERSION = 3;
-
-/**
- * Migration definition
- */
-interface Migration {
-  version: number;
-  description: string;
-  up: (db: SqliteDatabase) => void;
-}
-
-/**
- * All migrations in order
- *
- * Note: Version 1 is the initial schema, handled by schema.sql
- * Future migrations go here.
- */
-const migrations: Migration[] = [
-  {
-    version: 2,
-    description: 'Add project metadata, provenance tracking, and unresolved ref context',
-    up: (db) => {
-      db.exec(`
-        CREATE TABLE IF NOT EXISTS project_metadata (
-          key TEXT PRIMARY KEY,
-          value TEXT NOT NULL,
-          updated_at INTEGER NOT NULL
-        );
-        ALTER TABLE unresolved_refs ADD COLUMN file_path TEXT NOT NULL DEFAULT '';
-        ALTER TABLE unresolved_refs ADD COLUMN language TEXT NOT NULL DEFAULT 'unknown';
-        ALTER TABLE edges ADD COLUMN provenance TEXT DEFAULT NULL;
-        CREATE INDEX IF NOT EXISTS idx_unresolved_file_path ON unresolved_refs(file_path);
-        CREATE INDEX IF NOT EXISTS idx_edges_provenance ON edges(provenance);
-      `);
-    },
-  },
-  {
-    version: 3,
-    description: 'Add lower(name) expression index for memory-efficient case-insensitive lookups',
-    up: (db) => {
-      db.exec(`
-        CREATE INDEX IF NOT EXISTS idx_nodes_lower_name ON nodes(lower(name));
-      `);
-    },
-  },
-];
+export const CURRENT_SCHEMA_VERSION: number = REGISTRY_CURRENT;
 
 /**
  * Get the current schema version from the database
@@ -84,17 +50,14 @@ function recordMigration(db: SqliteDatabase, version: number, description: strin
  * Run all pending migrations
  */
 export function runMigrations(db: SqliteDatabase, fromVersion: number): void {
-  const pending = migrations.filter((m) => m.version > fromVersion);
-
-  if (pending.length === 0) {
-    return;
-  }
+  const pending = ALL_MIGRATIONS.filter((m) => m.version > fromVersion);
+  if (pending.length === 0) return;
 
-  // Sort by version
-  pending.sort((a, b) => a.version - b.version);
+  // ALL_MIGRATIONS is already sorted by version, but filtering can
+  // be cheap to re-confirm.
+  const ordered = [...pending].sort((a, b) => a.version - b.version);
 
-  // Run each migration in a transaction
-  for (const migration of pending) {
+  for (const migration of ordered) {
     db.transaction(() => {
       migration.up(db);
       recordMigration(db, migration.version, migration.description);
@@ -111,13 +74,15 @@ export function needsMigration(db: SqliteDatabase): boolean {
 }
 
 /**
- * Get list of pending migrations
+ * Get list of pending migrations.
+ *
+ * Returned as a fresh mutable array (not the underlying readonly
+ * registry) so callers that previously assigned the result to a
+ * `Migration[]`-typed variable keep working unchanged.
  */
 export function getPendingMigrations(db: SqliteDatabase): Migration[] {
   const current = getCurrentVersion(db);
-  return migrations
-    .filter((m) => m.version > current)
-    .sort((a, b) => a.version - b.version);
+  return ALL_MIGRATIONS.filter((m) => m.version > current).slice();
 }
 
 /**
@@ -136,3 +101,7 @@ export function getMigrationHistory(
     description: row.description,
   }));
 }
+
+// Re-export the registry surface for callers that want it.
+export { ALL_MIGRATIONS } from './migrations/index';
+export type { Migration, MigrationModule } from './migrations/types';

src/db/migrations/002-project-metadata.ts

@@ -0,0 +1,19 @@
+import type { MigrationModule } from './types';
+
+export const MIGRATION: MigrationModule = {
+  description: 'Add project metadata, provenance tracking, and unresolved ref context',
+  up: (db) => {
+    db.exec(`
+      CREATE TABLE IF NOT EXISTS project_metadata (
+        key TEXT PRIMARY KEY,
+        value TEXT NOT NULL,
+        updated_at INTEGER NOT NULL
+      );
+      ALTER TABLE unresolved_refs ADD COLUMN file_path TEXT NOT NULL DEFAULT '';
+      ALTER TABLE unresolved_refs ADD COLUMN language TEXT NOT NULL DEFAULT 'unknown';
+      ALTER TABLE edges ADD COLUMN provenance TEXT DEFAULT NULL;
+      CREATE INDEX IF NOT EXISTS idx_unresolved_file_path ON unresolved_refs(file_path);
+      CREATE INDEX IF NOT EXISTS idx_edges_provenance ON edges(provenance);
+    `);
+  },
+};

src/db/migrations/003-lower-name-index.ts

@@ -0,0 +1,10 @@
+import type { MigrationModule } from './types';
+
+export const MIGRATION: MigrationModule = {
+  description: 'Add lower(name) expression index for memory-efficient case-insensitive lookups',
+  up: (db) => {
+    db.exec(`
+      CREATE INDEX IF NOT EXISTS idx_nodes_lower_name ON nodes(lower(name));
+    `);
+  },
+};

src/db/migrations/index.ts

@@ -0,0 +1,106 @@
+/**
+ * Migration registry.
+ *
+ * Adding a new schema migration is:
+ *
+ *   1. Pick the next free 3-digit prefix (`NNN`) — `git ls-files
+ *      'src/db/migrations/[0-9]*.ts'` shows what's taken.
+ *   2. Create `src/db/migrations/<NNN>-<short-description>.ts`
+ *      exporting a `MIGRATION: MigrationModule` (just `description`
+ *      and `up(db)`).
+ *   3. Add **one** import line and **one** array entry to this file.
+ *
+ * **Why filename-derived versions instead of a field?** Two PRs
+ * adding migrations independently used to collide on the
+ * `migrations[]` array AND the `CURRENT_SCHEMA_VERSION` const.
+ * With monolithic migrations.ts, "I claimed v4 / you claimed v4"
+ * resolved as "second PR's v4 silently no-ops" — a real bug class
+ * (PR #113's reviewer caught one). With filename-derived versions,
+ * two PRs both creating `004-foo.ts` produce a filesystem-level
+ * conflict the maintainer sees instantly.
+ *
+ * `CURRENT_SCHEMA_VERSION` is the max of all registered versions.
+ */
+
+import type { Migration, MigrationModule } from './types';
+
+import { MIGRATION as MIG_002 } from './002-project-metadata';
+import { MIGRATION as MIG_003 } from './003-lower-name-index';
+
+interface ModuleRef {
+  /**
+   * Source filename. The 3-digit prefix is the source of truth for
+   * the version number — `validateRegistered` parses it. Keep this
+   * field in sync with the actual file on disk; the
+   * filesystem-cross-check test catches drift.
+   */
+  filename: string;
+  module: MigrationModule;
+}
+
+/**
+ * Static-import list of every migration. Two PRs adding
+ * migrations both add a single entry here; alphabetical ordering
+ * puts adjacent additions on different lines unless the version
+ * numbers themselves collide, in which case the filesystem
+ * collision on `NNN-*.ts` surfaces the conflict instantly.
+ */
+const REGISTERED_MODULES: readonly ModuleRef[] = [
+  { filename: '002-project-metadata.ts', module: MIG_002 },
+  { filename: '003-lower-name-index.ts', module: MIG_003 },
+];
+
+/** Strict 3-digit prefix on each migration filename. */
+const FILENAME_PATTERN = /^(\d{3})-[a-z0-9]+(?:-[a-z0-9]+)*\.ts$/;
+
+/**
+ * Validate the registered set: filenames match the strict
+ * `NNN-name.ts` shape, version is parsed from the prefix (no
+ * hand-typed version field that can drift), versions are unique,
+ * and the result is sorted ascending. Throws loudly at module
+ * load if any invariant is violated rather than silently dropping
+ * a migration during `runMigrations()`.
+ */
+function validateRegistered(refs: readonly ModuleRef[]): readonly Migration[] {
+  if (refs.length === 0) {
+    throw new Error('[CodeGraph] migrations registry is empty');
+  }
+  const parsed = refs.map((r) => {
+    const m = FILENAME_PATTERN.exec(r.filename);
+    if (!m) {
+      throw new Error(
+        `[CodeGraph] migration filename "${r.filename}" does not match ` +
+          `expected pattern NNN-kebab-name.ts (3-digit prefix, lowercase kebab-case body)`
+      );
+    }
+    const version = parseInt(m[1]!, 10);
+    return {
+      version,
+      filename: r.filename,
+      description: r.module.description,
+      up: r.module.up,
+    };
+  });
+  const sorted = [...parsed].sort((a, b) => a.version - b.version);
+  for (let i = 1; i < sorted.length; i++) {
+    if (sorted[i]!.version === sorted[i - 1]!.version) {
+      throw new Error(
+        `[CodeGraph] duplicate migration version ${sorted[i]!.version}: ` +
+          `${sorted[i - 1]!.filename} vs ${sorted[i]!.filename}`
+      );
+    }
+  }
+  return sorted.map((r) => ({
+    version: r.version,
+    description: r.description,
+    up: r.up,
+  }));
+}
+
+export const ALL_MIGRATIONS: readonly Migration[] = validateRegistered(REGISTERED_MODULES);
+
+/**
+ * Highest registered migration version. Derived from the registry
+ * (no hand-maintained constant to keep in sync).
+ */
+export const CURRENT_SCHEMA_VERSION: number = ALL_MIGRATIONS[ALL_MIGRATIONS.length - 1]!.version;

src/db/migrations/types.ts

@@ -0,0 +1,25 @@
+/**
+ * Migration registry types.
+ *
+ * Each migration ships its own self-contained file
+ * (`./NNN-description.ts`) exporting a `MIGRATION:
+ * MigrationModule`. The version number is derived from the
+ * leading 3-digit prefix on the filename, NOT from a field in the
+ * module — this guarantees no two PRs can claim the same version
+ * silently (filenames collide on the filesystem; SQL migrations
+ * never silently no-op).
+ */
+
+import type { SqliteDatabase } from '../sqlite-adapter';
+
+export interface MigrationModule {
+  /** One-line description for `schema_versions` table + diagnostics. */
+  readonly description: string;
+  /** The actual schema-mutation function. Wrapped in a transaction. */
+  readonly up: (db: SqliteDatabase) => void;
+}
+
+export interface Migration extends MigrationModule {
+  /** Version derived from filename's leading NNN prefix. */
+  readonly version: number;
+}