Constant-rename equivalence check: prove a rename changed no behaviour

[WilfordGrimley]

Why

A throwaway script written for PR #567 caught a real production-breaking bug that git, the 3,036-test backend suite, and the committed lint chain all passed over. It lived only in a session scratchpad. This commits it somewhere it will be used rather than rediscovered.

The incident (2026-07-29). PR #567 renamed SLOW_PATH_TO_REVIEW_REASON to SLOW_PATH_TO_REVIEW_SKIP_REASON. Concurrently, PR #568 added brand new code in catalog_stats.py importing and using the old name. Git auto-merged with no conflict — #568 only ADDED lines, #567 only touched a nearby docstring, so the rename and the new references never textually collided. The merged result would have raised, at module-import time:

ImportError: cannot import name 'SLOW_PATH_TO_REVIEW_REASON'

in a module reached by the catalog-stats view and the hourly warm_catalog_stats job. Thirteen reference sites across six modules.

The generalisation: a textual merge cannot see a name graph.

What ships

.github/scripts/constant_rename_equivalence.py — stdlib only, reads source via git cat-file, never imports or executes it. Two checks:

1. --check-references (one revision, no judgment calls): every from x import NAME matching the pattern must actually be declared in x; every matching ALL-CAPS name read must resolve somewhere. A violation is an ImportError/NameError, unconditionally.
2. Equivalence (two revisions): normalise each module at both — inline matching constants (map built across the whole tree, so cross-module imports resolve), delete those declarations / __all__ entries / imports, delete docstrings, constant-fold f-strings and string + — then compare ast.dump() trees.

Generalised past skip reasons: --pattern is a regex searched against ALL-CAPS module-level names, defaulting to SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT|_THRESHOLD|_PREFIX|_REASON. Revisions are arguments (default HEAD vs merge-base with origin/master). --paths narrows, --all widens. Failures name the module, the AST node path, and both sides unparsed back to source.

Scope deliberately includes modules the diff never touched — a changed-files-only scope would have missed catalog_stats.py, which is the whole point.

CI wiring — two jobs, different trigger characters

references is an unconditional invariant on every Python PR that can genuinely fail (same posture as protected-core-license). equivalence gates itself: with no renamed or removed constant in the diff it prints "nothing to prove" and exits 0, rather than being a job that runs always and passes always.

On a pull_request event actions/checkout hands you the merge result — exactly the revision the #567/#568 bug existed in and neither contributing branch did.

Can it fail?

Reconstructed the real auto-merge (master @ 9952865b + only #567's rename half):

• --check-references: 6 findings, e.g. catalog_stats.py::from cardpicker.local_calculate_verdicts import SLOW_PATH_TO_REVIEW_REASON — ... declares no SLOW_PATH_TO_REVIEW_REASON. This is an ImportError at module-import time.
• equivalence: 12 findings, each naming the node where 'to-review' became an unresolvable SLOW_PATH_TO_REVIEW_REASON.

Rename plus a genuine behaviour change ("to-review" → "to-review-v2" on top of the real #567 commit): 7 modules flagged, each pinpointing the value change. Nothing else in the repo catches that one — the rename is valid and the tests use the constant.

Two real normaliser gaps found and closed

Both while running against real history, neither by weakening anything:

• matching had to widen to any name containing the pattern — LANDS_PHASH_SKIP_REASON_PREFIX ends in _PREFIX, not _REASON;
• the f-string folder has to fold a single interpolated constant back into the surrounding literal, not just whole f-strings, because f"phash-{r}" and f"{PREFIX}{r}" are the same value in different shapes. Without this the real Adopt a *_SKIP_REASON declaration convention; tether the roster to a doc #567 commit reported a false difference.

The one remaining difference reported against #567 is docs_lint.py, which that same PR legitimately grew by 129 lines of new lint code. Correctly reported, not excluded.

Tests

.github/scripts/tests/test_constant_rename_equivalence.py — 37 tests following test_docs_lint.py's conventions. Fixture git repos; every rule has a passing and a failing case, including genuine behaviour changes and a guard that a rename outside the pattern is not normalised away. Two real-repo tests pin the incident.

Docs

docs/reference/constant-rename-equivalence.md (where it lives, when to run it, the incident, and its honest limits), indexed from docs/README.md + docs/MANIFEST.md, cross-referenced from docs/reference/skip-reasons.md, promoted into docs/lessons.md per that file's own triage ritual, and added to CLAUDE.md's task-end checks.

Verification

37 unit tests pass · docs_lint.py --strict clean · test_docs_lint.py OK · pre-commit run --all-files clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

---

Update — owner rulings applied (2026-07-29)

Q1: references as a required branch-protection check — yes in principle, but it could not be required as configured. GitHub leaves a required status check that never runs in pending forever rather than treating it as passed, so the workflow's paths: "**/*.py" filter would have made every docs-only or frontend-only PR permanently unmergeable the moment it was marked required.

• Dropped paths: from on: pull_request for the whole workflow, not just that job — leaving it on the siblings would only relocate the trap for whoever marks the next one required. The reasoning is in the workflow header, in a block that says not to add one back.
• Added --changed-since REV: when no *.py changed it prints nothing to check — no *.py file changed between X and Y and exits before any parsing. Measured 63ms, versus 4.7s for the full scan.
• The gate cannot wedge the check it protects: an unresolvable REV (shallow clone, fork without base history) degrades to running the full check with a printed note. The gate is an optimisation, never a correctness input.
• A clean run now reports what it did — CI log from this PR: 123 reference(s) to constants matching /.../ all resolve at HEAD (05d92ed2); 373 modules scanned. "clean" alone is indistinguishable from a check that silently stopped finding anything.

The code side is ready. Branch protection was not touched — that is governed by docs/infrastructure.md and is an owner action in the GitHub UI (Settings → Branches → master → Require status checks → add Every matching constant reference resolves). The other two jobs are equally safe to require now, but only that one was ruled on.

Q2: default pattern breadth — keep it broad. No code change. Recorded in the doc as a deliberate ruling, with the one-line narrowing path (SKIP_REASON|ANONYMOUS_ID) spelled out so a future reader does not "tidy" the generic families out of it.

Self-audit against the concurrent docs_lint.py finding. That audit found both roster tethers using a non-recursive src_dir.glob("*.py"), hiding scryfall-tagger-v1 in management/commands/. This script does not share the defect — it scans via git ls-tree -r over the whole tree with no directory filtering. Made that deliberate rather than incidental: read_python_tree() now documents why recursion and tests/ inclusion are both load-bearing, and new TestScanCoverage pins a nested management/commands/ module and a tests/ module are both scanned, plus a derivation guard against the real tree.

tests/ is exactly where the two tools diverge: the tethers ask "is this production value documented?", so fixture declarations are noise; this tool asks "does the reference still resolve?" — and four of the six modules broken by the #567/#568 merge were test modules. Excluding them would have hidden two thirds of the incident.

Tests: 45 (was 37). All 11 checks green.