From cb74e153e11007aa6295a64dc304c52c120ba71a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 23 Aug 2026 18:48:50 -0700 Subject: [PATCH 1/2] fix(ci): stop the migration safety audit from passing on a branch it never read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 []`. --- .github/workflows/test-build.yml | 9 +++++- scripts/check-migrations-safety.test.ts | 43 +++++++++++++++++++++++++ scripts/check-migrations-safety.ts | 40 ++++++++++++++++++++--- 3 files changed, 86 insertions(+), 6 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 007df411aa5..94f5699dd23 100644 --- a/.github/workflows/test-build.yml +++ b/.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 diff --git a/scripts/check-migrations-safety.test.ts b/scripts/check-migrations-safety.test.ts new file mode 100644 index 00000000000..3a3560de926 --- /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 0c7fd3dd302..ecb35762f4c 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) { From 44115b0c4c7e367df833d1ad30ef86494afc4b6c Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 23 Aug 2026 18:54:08 -0700 Subject: [PATCH 2/2] fix(ci): fetch the base ref once, and stop swallowing the failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index 94f5699dd23..181f2f9b8df 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 @@ -126,14 +137,6 @@ 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