fix(ci): stop the migration safety audit from passing on a branch it never read

The zero-downtime audit reports the same empty file list for 'this branch adds
no migrations' and 'I could not diff against the base', and the second prints
as `✓ No new migrations to check` with exit 0. Reproduced on this checkout:

    $ bun run scripts/check-migrations-safety.ts origin/does-not-exist-branch
    ✓ No new migrations to check.     exit=0

`changedMigrationFiles` returned `[]` whenever `git diff` failed, with a comment
deferring the decision to the caller — but the caller only recognised a missing
git binary (`git rev-parse HEAD === null`), never an unusable ref.

CI supplied exactly that input. `git fetch --depth=1 … 2>/dev/null || true` hid a
failed fetch, leaving `origin/<base>` absent, so a PR adding a destructive
`DROP COLUMN` would clear the only guard on production DDL with a green check.

Two halves:

- The audit now distinguishes the cases. Absent git is still the one legitimate
  skip and is checked before the diff; a diff that fails with git present raises
  `BaseRefUnusableError` and exits 1.
- The fetch is its own step with no `|| true`, so a failure fails the job. Depth
  stays 1: without a merge-base the audit diffs the two tips, which under
  `--diff-filter=AM` is exactly the migrations new on the branch.

Covered by a test that runs the script end to end, since the defect was in the
exit code rather than in any function's return value. Verified it fails when the
throw is reverted to `return []`.

Author: waleedlatif1

.github/workflows/test-build.yml

@@ -126,11 +126,18 @@ jobs:
       - name: Verify docs manifest is in sync
         run: bun run docs-manifest:check
 
+      # Its own step, and no `|| true`: a swallowed fetch leaves the base ref absent,
+      # which the audit cannot distinguish from a branch that changed no migrations.
+      # The depth stays at 1 — without a merge-base the audit diffs the two tips,
+      # which under `--diff-filter=AM` is exactly the migrations new on this branch.
+      - name: Fetch base ref for migration diff
+        if: github.event_name == 'pull_request'
+        run: git fetch --depth=1 origin "${{ github.base_ref }}"
+
       - name: Migration safety (zero-downtime) audit
         run: |
           if [ "${{ github.event_name }}" = "pull_request" ]; then
             BASE_REF="origin/${{ github.base_ref }}"
-            git fetch --depth=1 origin "${{ github.base_ref }}" 2>/dev/null || true
           else
             BASE_REF="HEAD~1"
           fi

scripts/check-migrations-safety.test.ts

@@ -0,0 +1,43 @@
+import { execFile } from 'node:child_process'
+import path from 'node:path'
+import { promisify } from 'node:util'
+import { describe, expect, it } from 'vitest'
+
+const execFileAsync = promisify(execFile)
+const ROOT = path.resolve(import.meta.dirname, '..')
+const SCRIPT = path.join(ROOT, 'scripts/check-migrations-safety.ts')
+
+async function runAudit(
+  baseRef: string
+): Promise<{ code: number; stdout: string; stderr: string }> {
+  try {
+    const { stdout, stderr } = await execFileAsync('bun', ['run', SCRIPT, baseRef], { cwd: ROOT })
+    return { code: 0, stdout, stderr }
+  } catch (error) {
+    const failure = error as { code?: number; stdout?: string; stderr?: string }
+    return { code: failure.code ?? 1, stdout: failure.stdout ?? '', stderr: failure.stderr ?? '' }
+  }
+}
+
+describe('migration safety audit', () => {
+  /**
+   * The regression this guards: an unresolvable base ref made `git diff` fail, the
+   * failure was read as an empty file list, and the audit printed
+   * `✓ No new migrations to check` and exited 0 — green on a branch it never read.
+   * CI reached that state whenever its `git fetch ... || true` swallowed a failure.
+   */
+  it('fails loudly when the base ref cannot be diffed', async () => {
+    const { code, stderr } = await runAudit('origin/branch-that-does-not-exist')
+
+    expect(code).toBe(1)
+    expect(stderr).toContain('could not run')
+    expect(stderr).not.toContain('No new migrations to check')
+  }, 30_000)
+
+  it('passes against a real base ref with no new migrations', async () => {
+    const { code, stdout } = await runAudit('HEAD')
+
+    expect(code).toBe(0)
+    expect(stdout).toContain('No new migrations to check')
+  }, 30_000)
+})

scripts/check-migrations-safety.ts

@@ -390,6 +390,24 @@ function git(args: string[]): string | null {
   }
 }
 
+/**
+ * Raised when the base ref cannot be compared against `HEAD`.
+ *
+ * Distinct from "no migrations changed", which is the same empty list. Conflating
+ * the two is how this check came to pass on a branch it had never read: an
+ * unresolvable base made `git diff` fail, the failure became `[]`, and `[]`
+ * printed as `✓ No new migrations to check`.
+ */
+class BaseRefUnusableError extends Error {
+  constructor(readonly baseRef: string) {
+    super(
+      `Cannot diff against '${baseRef}'. The ref is missing, or was fetched without enough ` +
+        `history for a merge-base. Fetch it with full history before running this check.`
+    )
+    this.name = 'BaseRefUnusableError'
+  }
+}
+
 /** New migration files on this branch vs base, plus uncommitted ones locally. */
 function changedMigrationFiles(baseRef: string): string[] {
   const files = new Set<string>()
@@ -405,7 +423,9 @@ function changedMigrationFiles(baseRef: string): string[] {
     '--',
     MIGRATIONS_DIR,
   ])
-  if (committed === null) return [] // git unavailable → fail open (handled by caller)
+  /* Only a missing git binary is a legitimate skip, and `resolveFiles` detects that
+     separately. A diff that fails with git present means the ref is unusable. */
+  if (committed === null) throw new BaseRefUnusableError(baseRef)
   for (const f of committed.split('\n')) if (inDir(f)) files.add(f)
 
   const status = git(['status', '--porcelain', '--', MIGRATIONS_DIR])
@@ -442,16 +462,26 @@ async function resolveFiles(argv: string[]): Promise<string[] | null> {
     return (await listSqlFiles(path.resolve(dir))).map((f) => path.relative(ROOT, f))
   }
   const baseRef = argv.find((a) => !a.startsWith('--')) ?? 'origin/staging'
-  const files = changedMigrationFiles(baseRef)
-  if (files.length === 0 && git(['rev-parse', 'HEAD']) === null) {
+  /* Checked before the diff: without git there is nothing to compare, and that is the
+     one case where skipping is right. Every other failure must be loud. */
+  if (git(['rev-parse', 'HEAD']) === null) {
     console.warn('⚠ git unavailable — skipping migration safety check.')
     return null
   }
-  return files
+  return changedMigrationFiles(baseRef)
 }
 
 async function main() {
-  const files = await resolveFiles(process.argv.slice(2))
+  let files: string[] | null
+  try {
+    files = await resolveFiles(process.argv.slice(2))
+  } catch (error) {
+    if (error instanceof BaseRefUnusableError) {
+      console.error(`✗ Migration safety check could not run.\n  ${error.message}`)
+      process.exit(1)
+    }
+    throw error
+  }
   if (files === null) process.exit(0)
 
   if (files.length === 0) {