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

* 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 []`.

* fix(ci): fetch the base ref once, and stop swallowing the failure

The same `git fetch --depth=1 … 2>/dev/null || true` appeared in both base-ref
audits. Fixing only the migration one would have left the identical defect a few
steps above it.

Neither audit can tell an absent base ref apart from a branch that changed
nothing. The block-registry check at least degrades to a visible
`⚠ Could not diff against base ref — skipping`; the migration audit printed
`✓ No new migrations to check` and exited 0.

Both now share one fetch step that fails the job when it fails.

Author: waleedlatif1

.github/workflows/test-build.yml

@@ -104,11 +104,22 @@ jobs:
 
           echo "✅ All env flags are properly configured"
 
+      # One fetch for both base-ref audits, and no `|| true`: a swallowed fetch leaves
+      # the base ref absent, which neither audit can tell apart from a branch that
+      # changed nothing. The block-registry check at least degrades to a visible
+      # `⚠ … skipping` line; the migration audit printed `✓ No new migrations to
+      # check` and exited 0, clearing the only guard on production DDL.
+      #
+      # Depth stays at 1 — without a merge-base the migration audit diffs the two
+      # tips, which under `--diff-filter=AM` is exactly the migrations new here.
+      - name: Fetch base ref for diff-based audits
+        if: github.event_name == 'pull_request'
+        run: git fetch --depth=1 origin "${{ github.base_ref }}"
+
       - name: Check block registry invariants
         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
@@ -130,7 +141,6 @@ jobs:
         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) {