Skip to content

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

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix-migration-gate
Aug 24, 2026
Merged

fix(ci): stop the migration safety audit from passing on a branch it never read#7022
waleedlatif1 merged 2 commits into
stagingfrom
fix-migration-gate

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

The check reports success when it cannot run

check-migrations-safety.ts returns the same empty file list for "this branch adds no migrations" and "I could not diff against the base". The second prints as a pass:

$ 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 call to the caller — but resolveFiles only ever recognised a missing git binary (git rev-parse HEAD === null), never an unusable ref.

CI supplies exactly that input

test-build.yml:

git fetch --depth=1 origin "${{ github.base_ref }}" 2>/dev/null || true

The || true swallows a failed fetch, which leaves origin/<base> absent — the precise condition above. A PR adding a destructive DROP COLUMN then clears the only guard on production DDL with a green check.

This is the one audit in the repo whose failure mode is silent and whose subject is irreversible.

The fix, both halves

The audit distinguishes the two cases. Absent git remains the one legitimate skip and is now checked before the diff; a diff that fails with git present raises BaseRefUnusableError and exits 1 with a message naming the ref.

The fetch is its own step with no || true, so a failed fetch fails the job instead of quietly degrading the next one.

Depth stays at 1 deliberately. Without a merge-base the audit falls back to diffing the two tips, and under --diff-filter=AM that is exactly the set of migrations new on the branch — so a shallow fetch is correct here and a full-history fetch would slow every PR for nothing.

Testing

The defect lived in the exit code, not in any function's return value, so the test runs the script end to end and asserts on the code and stderr.

Verified it can fail: reverting the throw to return [] turns the first case red with expected +0 to be 1, and restoring it goes green.

✓ fails loudly when the base ref cannot be diffed
✓ passes against a real base ref with no new migrations

Also confirmed the real-base path still exits 0, so this does not turn every PR red.

Provenance

Found while auditing the repo's custom gates for blind spots — the same sweep that produced #7020 (where \buseQuery\s*\( never matched useQuery<T>(, leaving 10 strict-zone queries unscanned). A passing gate that cannot see part of its own domain is the most expensive kind of green.

…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 []`.
@vercel

vercel Bot commented Aug 24, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
docs Ready Ready Preview Aug 24, 2026 1:59am

Request Review

@cursor

cursor Bot commented Aug 24, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Tightens a CI gate over production DDL rather than changing migrations themselves. A failed base-ref fetch now fails the job instead of silently skipping the audit.

Overview
The migration safety check no longer treats an unusable git base the same as “no new migrations.” A failed git diff now raises BaseRefUnusableError and exits 1; only a missing git binary still skips.

CI fetch of the PR base is its own step with no || true, so a swallowed fetch cannot leave origin/<base> absent. Shallow --depth=1 is kept: without a merge-base the audit diffs the two tips, which under --diff-filter=AM is the set of new migration files.

Adds an end-to-end test that a nonexistent base fails loudly and a real HEAD still reports no new migrations.

Reviewed by Cursor Bugbot for commit cb74e15. Configure here.

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.
@greptile-apps

greptile-apps Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR makes the migration safety audit fail closed when its base reference cannot be diffed.

  • Fetches the pull request base ref in a required, dedicated CI step.
  • Distinguishes an unavailable Git executable from an unusable base ref.
  • Adds end-to-end regression coverage for failing and successful audit invocations.

Confidence Score: 5/5

The PR appears safe to merge, with the migration audit now failing explicitly when it cannot inspect the requested base.

The workflow no longer suppresses base-fetch failures, and the audit preserves its intentional Git-unavailable skip while converting failed base comparisons into a nonzero exit.

Important Files Changed

Filename Overview
.github/workflows/test-build.yml Moves the base-ref fetch into a required pull-request-only step so fetch failures stop the CI job.
scripts/check-migrations-safety.ts Separates an unavailable Git executable from a failed base comparison and reports the latter as an audit error.
scripts/check-migrations-safety.test.ts Adds process-level regression tests covering an unusable base ref and a valid no-change comparison.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  PR[Pull request job] --> Fetch[Fetch base ref]
  Fetch -->|failure| JobFail[Fail CI job]
  Fetch -->|success| Audit[Run migration safety audit]
  Audit --> GitCheck{Git available?}
  GitCheck -->|no| Skip[Warn and skip]
  GitCheck -->|yes| Diff{Base ref can be diffed?}
  Diff -->|no| AuditFail[Exit 1 with unusable-ref error]
  Diff -->|yes| Scan[Scan changed migration files]
  Scan --> Result[Report audit result]
Loading

Reviews (1): Last reviewed commit: "fix(ci): stop the migration safety audit..." | Re-trigger Greptile

@waleedlatif1
waleedlatif1 merged commit 95d08d2 into staging Aug 24, 2026
28 checks passed
@waleedlatif1
waleedlatif1 deleted the fix-migration-gate branch August 24, 2026 01:59
waleedlatif1 added a commit that referenced this pull request Aug 24, 2026
Push builds fail the migration audit:

    ✗ Migration safety check could not run.
      Cannot diff against 'HEAD~1'.

`actions/checkout` sets no `fetch-depth`, so it defaults to 1 — a single-commit
clone in which `HEAD~1` does not resolve. Both diff-based audits named `HEAD~1`
as their push base, so neither has ever had a base to read. The migration audit
answered that with `✓ No new migrations to check` and exit 0, so it had never
run on a push build at all; #7022 made it say it could not run instead, which is
what surfaced this. The block-registry check reports `⚠ … skipping` on the same
input — visible, and equally never run.

`HEAD~1` was the wrong base regardless. It names the last commit, so a push
carrying several commits audits the tip and lets every earlier commit through:

    3-commit push, HEAD~1 base:   mig3.sql
    3-commit push, before base:   mig1.sql mig2.sql mig3.sql

The base is now `github.event.before` — the tip the branch had before the push,
which is what GitHub provides for exactly this. It is fetched by SHA at depth 1;
the audits diff two tips and need no common ancestry between them. Resolved once
in a step both audits read, so the two cannot drift apart.

`HEAD~1` survives only as the fallback for an all-zero `before` (a new branch,
with no predecessor to diff), which is what `fetch-depth: 2` now covers.

Verified: both audits accept a raw SHA base and pass; the multi-commit case above
is a real reproduction, not a description.
waleedlatif1 added a commit that referenced this pull request Aug 24, 2026
Push builds fail the migration audit:

    ✗ Migration safety check could not run.
      Cannot diff against 'HEAD~1'.

`actions/checkout` sets no `fetch-depth`, so it defaults to 1 — a single-commit
clone in which `HEAD~1` does not resolve. Both diff-based audits named `HEAD~1`
as their push base, so neither has ever had a base to read. The migration audit
answered that with `✓ No new migrations to check` and exit 0, so it had never
run on a push build at all; #7022 made it say it could not run instead, which is
what surfaced this. The block-registry check reports `⚠ … skipping` on the same
input — visible, and equally never run.

`HEAD~1` was the wrong base regardless. It names the last commit, so a push
carrying several commits audits the tip and lets every earlier commit through:

    3-commit push, HEAD~1 base:   mig3.sql
    3-commit push, before base:   mig1.sql mig2.sql mig3.sql

The base is now `github.event.before` — the tip the branch had before the push,
which is what GitHub provides for exactly this. It is fetched by SHA at depth 1;
the audits diff two tips and need no common ancestry between them. Resolved once
in a step both audits read, so the two cannot drift apart.

`HEAD~1` survives only as the fallback for an all-zero `before` (a new branch,
with no predecessor to diff), which is what `fetch-depth: 2` now covers.

Verified: both audits accept a raw SHA base and pass; the multi-commit case above
is a real reproduction, not a description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant