Skip to content

Fail CI on a migration graph that forks WHEN MERGED, not just on the branch - #611

Merged
WilfordGrimley merged 1 commit into
masterfrom
fix/migration-leaf-guard
Jul 29, 2026
Merged

Fail CI on a migration graph that forks WHEN MERGED, not just on the branch#611
WilfordGrimley merged 1 commit into
masterfrom
fix/migration-leaf-guard

Conversation

@WilfordGrimley

@WilfordGrimley WilfordGrimley commented Jul 29, 2026

Copy link
Copy Markdown

Two branches can each add 0098_<something>.py depending on 0097. The filenames differ, so there is no textual conflict, GitHub reports the second PR MERGEABLE/CLEAN, and both branches are individually valid. The moment the second merges, cardpicker has two leaf nodes — and pytest-django builds its test database by running migrate, so the fork fails at test-database SETUP on every branch in the repo, not just the one that introduced it.

This has happened twice: at 0096 (#568 vs #570) and at 0098 (#573 vs #601). #576 repaired the first fork but prevented nothing, which is why the second arrived within days. Nothing in CI failed either time.

Why checking the branch is not enough

#601's checks were 10/10 green with the collision already live on master — they had run against master before #573 landed, and GitHub does not re-run a PR's checks when its base moves. A check that reads only the PR branch's files sees one leaf and passes; the fork exists only in the merge.

So the job resolves the base branch's current tip at run time and checks the union:

git fetch --no-tags origin "+refs/heads/${BASE_REF}:refs/remotes/origin/${BASE_REF}"
python3 .github/scripts/check_migration_leaves.py --base "origin/${BASE_REF}" --annotate

--no-renames on the deletion diff is load-bearing, not stylistic: renumbering a migration is a delete+add of near-identical content, which git otherwise reports as R. Without it the old number is resurrected from the base and the PR fails for a collision it had already fixed. A unit test pins this.

Demonstrated red-then-green against the real collision

Run against #601's pre-rebase commit 6c0af23d with master at d0442239:

# scenario exit
A #601 pre-rebase, branch in isolation 0 — green (this is the blindness)
B #601 pre-rebase, merged with origin/master 2 — RED
C #601 after the renumber to 0099, merged with origin/master 0 — green
D normal single-migration PR (adds 0099_ on top of master's 0098_) 0 — green
E PR touching no migrations at all 0 — green

Case B verbatim:

check_migration_leaves: 99 migration(s), scope = merge of the worktree with origin/master (+1 migration(s) from the base)

check_migration_leaves: 2 finding(s)

  app cardpicker: migration graph has 2 leaf nodes; Django requires exactly one.
    - 0098_card_illustration_consensus_fields  (MPCAutofill/cardpicker/migrations/0098_card_illustration_consensus_fields.py)
    - 0098_rename_printings_count_catalogued  (MPCAutofill/cardpicker/migrations/0098_rename_printings_count_catalogued.py)
  `migrate` - and therefore pytest-django's test-database setup on EVERY
  branch in this repo - fails with 'Conflicting migrations detected;
  multiple leaf nodes in the migration graph'.
  Fix: renumber the leaf your PR adds to 0099_... and repoint
  its `dependencies` at the other leaf. Update the number wherever the
  migration's own docstring or comments state it.
  app cardpicker: two migrations share the number prefix 0098: 0098_card_illustration_consensus_fields, 0098_rename_printings_count_catalogued. Renumber the one your PR adds and repoint its `dependencies`.

Cross-checked against Django's own MigrationLoader, which reports the same two leaves on the same tree — the static reader and the loader agree.

That reconstruction is kept as permanent regression coverage in .github/scripts/tests/test_check_migration_leaves.py (15 tests), not as a one-off local run: a scratch git repo where master has 0098_card_illustration_consensus_fields and a feature branch has 0098_rename_printings_count_catalogued, both on 0097, asserting clean on the branch alone, two leaves against the merge, and clean again once renumbered.

Proven end to end in real CI, not just locally

Throwaway draft #612 (now closed, branch deleted) added a second 0098 depending on 0097 on top of this branch. Run 30483031963:

check_migration_leaves: 99 migration(s), scope = merge of the worktree with origin/master (+0 migration(s) from the base)
check_migration_leaves: 2 finding(s)
  app cardpicker: migration graph has 2 leaf nodes; Django requires exactly one.
##[error][cardpicker] migration graph has 2 leaf nodes; Django requires exactly one.
  app cardpicker: two migrations share the number prefix 0098: 0098_card_illustration_consensus_fields, 0098_deliberate_collision_proof. ...
##[error]Process completed with exit code 2.

That proves both halves of the flagged trap: the **/migrations/** path filter actually fires on a PR whose only relevant change is under migrations/, and the job goes red with inline annotations. On this PR itself both jobs pass.

How it decides

Static ast read of every migrations/ package. Filenames are nodes; each file's dependencies gives same-app edges (mirroring MigrationLoader.graph.leaf_nodes(app)); run_before gives reversed ones; a squash's replaces removes the nodes it stands in for. Migration modules are never imported or executed — no settings module, no installed apps, no postgres, no requirements.txt. It runs on a bare actions/setup-python in about a second. Non-literal dependency entries (migrations.swappable_dependency(settings.AUTH_USER_MODEL), in seven of this repo's migrations) are cross-app by construction and are skipped, not guessed at.

Findings: more than one leaf per app (the failure); a duplicate NNNN prefix within an app (the same defect one step earlier, and the actionable instruction); a dependency naming a migration that does not exist. Exit code is the finding count, matching docs_lint.py's and check_protected_core_license.py's convention.

What it cannot do, stated plainly

A check run that passed before the base moved stays green in GitHub's UI. No CI job can fix that from the inside. Branch protection's "Require branches to be up to date before merging" is the setting that closes it; this makes the forced re-run meaningful. merge_group is wired up so a merge queue would close it too.

Because the workflow is path-filtered it does not run on a PR touching no migrations — correct (such a PR cannot fork the graph), but it means marking it required would leave those PRs waiting on a check that never reports. Either leave it unrequired, or require it and drop the paths filter; the script is cheap enough to always run, which is the route coverage-delta.yml already takes.

Coordination notes

Verification

  • python3 .github/scripts/tests/test_check_migration_leaves.py15 tests, OK
  • check_migration_leaves.py --base origin/master on this branch — clean
  • pre-commit (ruff, isort, black, mypy, check-yaml, prettier) — pass

…branch

Two branches can each add `0098_<something>.py` depending on `0097`. The
filenames differ, so there is no textual conflict, GitHub reports the
second PR MERGEABLE/CLEAN, and both branches are individually valid. The
moment the second merges, `cardpicker` has two leaf nodes - and
pytest-django builds its test database by running `migrate`, so the fork
fails at test-database SETUP on EVERY branch in the repo, not just the
one that introduced it.

This has now happened twice: at 0096 (#568 vs #570) and at 0098 (#573 vs
#601). #576 repaired the first fork but prevented nothing, which is why
the second arrived within days. Nothing in CI failed either time.

WHY THE MERGE RESULT IS THE WHOLE POINT

#601's checks were 10/10 green with the collision already live on master
- they had run against master BEFORE #573 landed, and GitHub does not
re-run a PR's checks when its base moves. A check reading only the PR
branch's files sees one leaf and passes; the fork exists only in the
merge. So `check_migration_leaves.py --base origin/<base_ref>` unions the
worktree's migrations with the base branch's CURRENT tip, resolved at run
time, honouring anything the PR deletes (`--no-renames` is load-bearing:
a renumber is a delete+add of near-identical content and git otherwise
reports it as a rename, which would resurrect the old number and fail a
PR that had already fixed itself).

HOW IT DECIDES

Static `ast` read of every `migrations/` package: filenames are nodes,
each file's `dependencies` gives same-app edges, `run_before` gives
reversed ones, and a squash's `replaces` removes the nodes it stands in
for. Migration modules are never imported or executed, so this needs no
settings module, no installed apps, no postgres and no
`requirements.txt` - it runs on a bare `actions/setup-python` in about a
second. Non-literal dependency entries
(`migrations.swappable_dependency(settings.AUTH_USER_MODEL)`, in seven of
this repo's migrations) are cross-app by construction and are skipped,
not guessed at. Exit code is the finding count, matching docs_lint.py's
and check_protected_core_license.py's convention.

Findings: more than one leaf per app (the failure), a duplicate NNNN
number prefix within an app (the same defect one step earlier, and the
actionable instruction), and a dependency naming a migration that does
not exist.

WHAT IT CANNOT DO, STATED PLAINLY

A check run that PASSED before the base moved stays green in GitHub's UI.
No CI job can fix that from the inside; branch protection's "Require
branches to be up to date before merging" is the setting that closes it,
and this makes the forced re-run meaningful. `merge_group` is wired up so
a merge queue would close it too.

The workflow is its own file rather than another entry in docs-lint.yml,
which four open PRs are already editing. Every path glob uses `**`: a
single `*` does not match a slash, so `MPCAutofill/cardpicker/*.py` would
miss `migrations/` entirely (#588 hit exactly that).

Demonstrated red-then-green against the real collision, and kept as
permanent regression coverage in
`.github/scripts/tests/test_check_migration_leaves.py` (15 tests) rather
than as a one-off local run: a scratch repo where master has
`0098_card_illustration_consensus_fields` and a feature branch has
`0098_rename_printings_count_catalogued`, both on 0097, asserts clean on
the branch alone, two leaves against the merge, and clean again once
renumbered to 0099 - plus no-finding cases for a normal single-migration
PR, a PR touching no migrations, cross-app dependencies, swappable
dependencies, squashes and this repo's own tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
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