From 95d08d2a49e1ebc8429cee896570eff0052538bc Mon Sep 17 00:00:00 2001 From: Waleed Date: Sun, 23 Aug 2026 18:59:54 -0700 Subject: [PATCH] fix(ci): stop the migration safety audit from passing on a branch it never read (#7022) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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/` 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. --- .github/workflows/test-build.yml | 14 ++++++-- scripts/check-migrations-safety.test.ts | 43 +++++++++++++++++++++++++ scripts/check-migrations-safety.ts | 40 ++++++++++++++++++++--- 3 files changed, 90 insertions(+), 7 deletions(-) create mode 100644 scripts/check-migrations-safety.test.ts diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index 007df411aa..181f2f9b8d 100644 --- a/.github/workflows/test-build.yml +++ b/.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 diff --git a/scripts/check-migrations-safety.test.ts b/scripts/check-migrations-safety.test.ts new file mode 100644 index 0000000000..3a3560de92 --- /dev/null +++ b/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) +}) diff --git a/scripts/check-migrations-safety.ts b/scripts/check-migrations-safety.ts index 0c7fd3dd30..ecb35762f4 100644 --- a/scripts/check-migrations-safety.ts +++ b/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() @@ -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 { 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) {