diff --git a/.github/workflows/test-build.yml b/.github/workflows/test-build.yml index 007df411aa5..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 @@ -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 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) {