Skip to content

Commit f5d7855

Browse files
Constant-rename equivalence check: prove a rename changed no behaviour
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. The incident (2026-07-29): #567 renamed SLOW_PATH_TO_REVIEW_REASON to SLOW_PATH_TO_REVIEW_SKIP_REASON; #568 concurrently 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 ImportError at module-import time in a module reached by the catalog-stats view and the hourly warm_catalog_stats job. Thirteen reference sites, six modules. The generalisation: a textual merge cannot see a name graph. 1. .github/scripts/constant_rename_equivalence.py - two checks. --check-references (one revision, no judgment calls) resolves every matching constant reference: a `from x import NAME` where x declares no NAME is an ImportError, unconditionally. The equivalence check (two revisions) normalises each module at both - inlining matching constants with the map built across the WHOLE tree so cross-module imports resolve, deleting those declarations/__all__ entries/imports, deleting docstrings, constant-folding f-strings and string concatenation - then compares ast.dump() trees. 2. Generalised past skip reasons: --pattern is a regex searched against ALL-CAPS module-level names, defaulting to the families this repo actually refactors (SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT| _THRESHOLD|_PREFIX|_REASON). --pattern '.' inlines everything. 3. Revisions are arguments; default is HEAD vs its merge-base with origin/master, with fallbacks so it works in a worktree with no remote. --paths narrows, --all widens. 4. Failures name the module, the AST node path, and both sides unparsed back to source - not "these trees differ" on a 2,000-line file. 5. Scope includes modules the diff never touched. A changed-files-only scope would have missed catalog_stats.py, which is the whole point: in the merge that broke, the rename half touched only local_calculate_verdicts.py. CI wiring is deliberately two jobs with different trigger characters. `references` is an unconditional invariant that runs on every Python PR and 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. Tests follow test_docs_lint.py's conventions: fixture git repos, every rule with a passing AND a failing case, including genuine behaviour changes (a renamed constant whose value also moved; a comparison operand change alongside a rename; a frozenset membership change), plus a guard that a rename OUTSIDE the pattern is not normalised away. Two real-repo tests pin the incident. Two normaliser gaps were found and closed while running it against real history, both real: - matching had to widen to any name CONTAINING the pattern, because 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 #567 commit reported a false difference. Nothing was weakened to make the repo pass: the one remaining difference reported against #567 is docs_lint.py, which that same PR legitimately grew by 129 lines of new lint code. Documented in docs/reference/constant-rename-equivalence.md (indexed from docs/README.md and 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. Verified: 37 unit tests pass; docs-lint --strict clean; pre-commit clean; the tool reproduces the incident (6 ImportError findings plus 6 tree differences on a reconstructed auto-merge) and detects a rename-plus-behaviour-change (7 modules, 'to-review' -> 'to-review-v2'). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
1 parent 27d6cca commit f5d7855

9 files changed

Lines changed: 1773 additions & 44 deletions

File tree

.github/scripts/constant_rename_equivalence.py

Lines changed: 941 additions & 0 deletions
Large diffs are not rendered by default.

.github/scripts/tests/test_constant_rename_equivalence.py

Lines changed: 490 additions & 0 deletions
Large diffs are not rendered by default.
Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
name: Constant-rename equivalence
2+
3+
# Two checks over module-level constants, both from
4+
# .github/scripts/constant_rename_equivalence.py. Motivating incident and
5+
# full rationale: docs/reference/constant-rename-equivalence.md.
6+
#
7+
# PR #567 renamed SLOW_PATH_TO_REVIEW_REASON -> SLOW_PATH_TO_REVIEW_SKIP_REASON
8+
# while PR #568 concurrently added NEW code using the OLD name. Git
9+
# auto-merged with no conflict (#568 only added lines; #567 only touched a
10+
# nearby docstring), and the merged result would have raised ImportError at
11+
# module-import time in a module reached by the catalog-stats view and the
12+
# hourly warm_catalog_stats job. Neither branch was broken on its own — only
13+
# their merge was, which is precisely the revision a `pull_request` checkout
14+
# hands you.
15+
#
16+
# DELIBERATELY TWO JOBS, with different trigger characters:
17+
#
18+
# `references` is an unconditional invariant. A matching constant imported
19+
# from, or read in, a module that declares it nowhere is an ImportError or
20+
# NameError, always, with no judgment call — so it runs on every Python PR
21+
# and can genuinely fail. Same posture as protected-core-license: it passes
22+
# with zero findings today, correctly, and exists to catch the day that
23+
# stops being true.
24+
#
25+
# `equivalence` is the expensive proof, and its natural trigger is a PR
26+
# that RENAMES a constant, which is not most PRs. Rather than run always
27+
# and pass always (which teaches people to ignore it), the script gates
28+
# itself: with no renamed or removed constant in the diff it prints
29+
# "nothing to prove" and exits 0. When it does fire, its output names the
30+
# renamed constants and the exact differing AST node.
31+
#
32+
# Stdlib only — reads source text via `git cat-file`, never imports or
33+
# executes it, so no Django/third-party deps are needed in these jobs.
34+
35+
on:
36+
pull_request:
37+
paths:
38+
- "**/*.py"
39+
- ".github/workflows/constant-rename-equivalence.yml"
40+
push:
41+
branches: ["master"]
42+
paths:
43+
- "**/*.py"
44+
workflow_dispatch:
45+
46+
permissions:
47+
contents: read
48+
49+
jobs:
50+
references:
51+
name: Every matching constant reference resolves
52+
runs-on: ubuntu-latest
53+
steps:
54+
- uses: actions/checkout@v6
55+
- name: Resolve every *_SKIP_REASON / *_ANONYMOUS_ID / threshold reference
56+
run: python3 .github/scripts/constant_rename_equivalence.py --check-references
57+
58+
equivalence:
59+
name: Renamed constants are behaviour-preserving
60+
runs-on: ubuntu-latest
61+
steps:
62+
- uses: actions/checkout@v6
63+
with:
64+
# Needs both sides of the comparison, and the merge-base.
65+
fetch-depth: 0
66+
- name: Prove the rename changed no behaviour
67+
run: |
68+
BASE="${{ github.event.pull_request.base.sha }}"
69+
if [ -z "$BASE" ]; then BASE="$(git rev-parse HEAD~1)"; fi
70+
python3 .github/scripts/constant_rename_equivalence.py --base "$BASE" --head HEAD
71+
72+
unit-tests:
73+
name: constant_rename_equivalence.py unit tests
74+
# Fixture git repos exercising each normalisation rule's passing AND
75+
# failing case, plus real-repo regression pins for the #567 rename.
76+
runs-on: ubuntu-latest
77+
steps:
78+
- uses: actions/checkout@v6
79+
with:
80+
fetch-depth: 0
81+
- name: Run constant-rename equivalence unit tests
82+
run: python3 .github/scripts/tests/test_constant_rename_equivalence.py

CLAUDE.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,15 @@ not described here so it doesn't get re-derived.)
7070
by that doc's own mechanical tether in `docs_lint.py`, but only after the
7171
fact; get the judgment right at the row's creation instead of relying on
7272
CI to catch a wrong one later.
73+
- **Constant renames**: did this branch rename, extract, move or retire a
74+
module-level constant (`*_SKIP_REASON`, `*_ANONYMOUS_ID`, a weight, a
75+
threshold)? Run
76+
`python3 .github/scripts/constant_rename_equivalence.py` before pushing.
77+
It proves the touched modules normalise to identical ASTs once constants
78+
are inlined, and separately resolves every matching reference — the check
79+
that a clean textual auto-merge cannot do for you. Motivating incident and
80+
limits:
81+
[`docs/reference/constant-rename-equivalence.md`](docs/reference/constant-rename-equivalence.md).
7382
- **Push policy**: commit and push straight to `master` for solo work on
7483
this repo, no PR needed. PRs (+ user approval before merge) are reserved
7584
for upstreaming to `chilli-axe/mpc-autofill`. Never `git push --force` as

docs/MANIFEST.md

Lines changed: 45 additions & 44 deletions
Large diffs are not rendered by default.

docs/README.md

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@ The methodology and the systems it governs.
3434
declarations in code by `check_skip_reason_roster_tether()` in
3535
`.github/scripts/docs_lint.py`, on the same terms as the calculator
3636
roster: code is the source of truth, the doc is the thing checked.
37+
- [`reference/constant-rename-equivalence.md`](reference/constant-rename-equivalence.md)
38+
— the AST-level check that proves a constant rename or extraction changed
39+
no behaviour, and catches the concurrently-merged PR that silently breaks
40+
one. Run it on any branch that renames a `*_SKIP_REASON`,
41+
`*_ANONYMOUS_ID`, weight or threshold. Written up around the 2026-07-29
42+
#567/#568 incident, where a clean textual auto-merge produced an
43+
`ImportError` at module-import time that git, the 3,036-test backend
44+
suite and the lint chain all passed over.
3745
- [`reference/vote-weight-matrix.md`](reference/vote-weight-matrix.md)
3846
the owner-ratified 2026-07-22 vote-weight scenario matrix (raw decision
3947
record, implemented in PR #325) that `theory.md`'s §4/§7a and

docs/lessons.md

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1023,3 +1023,28 @@ cards the broken gate already logged - they stay excluded forever. The fix has t
10231023
`-vN` bump (family-keyed behaviour follows automatically via `models.calculator_family`), which
10241024
makes "is this repair reachable by the population it is for?" a standing question for any Stage D
10251025
predicate change, not an afterthought.
1026+
1027+
## A clean textual auto-merge can still produce an unresolvable name — git, the test suite and mypy all agree, and all three are wrong
1028+
1029+
A branch renamed `SLOW_PATH_TO_REVIEW_REASON` to `SLOW_PATH_TO_REVIEW_SKIP_REASON`. Concurrently,
1030+
another branch added BRAND NEW code importing and using the OLD name. Git auto-merged with no
1031+
conflict, and could not have done otherwise: one side only ADDED lines, the other only touched a
1032+
nearby docstring, so the rename and the new references never textually collided. The merged
1033+
result would have raised `ImportError: cannot import name 'SLOW_PATH_TO_REVIEW_REASON'` at
1034+
module-**import** time, in a module reached by both a live view and an hourly scheduled job. The
1035+
full backend suite passed on each branch, because **neither branch was broken** — only their
1036+
merge was, and no CI job ran against that merge result with a checker that resolves names.
1037+
1038+
The generalisation is the useful part: **a textual merge cannot see a name graph.** Two changes
1039+
that never touch the same lines can still produce a tree in which a name no longer resolves, or
1040+
resolves to a different value. Rename/extract/move refactors are the standing generator of this
1041+
because they are exactly the changes whose diff is spread thinly over many files while carrying
1042+
one indivisible semantic edit. Green CI on both branches is not evidence about the merge.
1043+
1044+
Promoted to a gate, per this file's own triage ritual:
1045+
[`reference/constant-rename-equivalence.md`](reference/constant-rename-equivalence.md)
1046+
`.github/scripts/constant_rename_equivalence.py` resolves every matching constant reference at
1047+
the merge revision (which is what a `pull_request` checkout hands you), and, when a PR actually
1048+
renames something, proves the touched modules normalise to identical ASTs once constants are
1049+
inlined. Run it on any branch that renames a constant; do not weaken its normaliser to make a
1050+
real repo pass — the one time a reported difference looked spurious, it was this bug.
Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
# Constant-rename equivalence check
2+
3+
**What it is.** `.github/scripts/constant_rename_equivalence.py` — an
4+
AST-level checker that proves a constant-renaming or constant-extraction
5+
refactor changed no behaviour, and that catches the concurrent merge that
6+
silently breaks one.
7+
8+
**When to run it.** Any time a branch renames, extracts, moves or retires a
9+
module-level constant — `*_SKIP_REASON`, `*_ANONYMOUS_ID`,
10+
`*_EXTRACTOR_VERSION`, a weight, a threshold. It runs itself in CI
11+
(`.github/workflows/constant-rename-equivalence.yml`), but the useful moment
12+
is before you push, on the branch, where the output is a two-line diff
13+
instead of a red check.
14+
15+
```bash
16+
# HEAD vs its merge-base with origin/master
17+
python3 .github/scripts/constant_rename_equivalence.py
18+
19+
# the name-resolution half alone: one revision, no diff needed
20+
python3 .github/scripts/constant_rename_equivalence.py --check-references
21+
```
22+
23+
Exit code is the number of findings (0 = clean), matching
24+
`docs_lint.py`'s and `check_protected_core_license.py`'s convention.
25+
Stdlib only; it reads source text via `git cat-file` and never imports or
26+
executes it, so it needs no Django or third-party dependencies.
27+
28+
## Why it exists — the #567/#568 incident, 2026-07-29
29+
30+
PR #567 renamed `SLOW_PATH_TO_REVIEW_REASON` to
31+
`SLOW_PATH_TO_REVIEW_SKIP_REASON` in
32+
`MPCAutofill/cardpicker/local_calculate_verdicts.py`, as part of adopting
33+
the `*_SKIP_REASON` declaration convention that
34+
[`skip-reasons.md`](skip-reasons.md)'s roster tether depends on.
35+
36+
Concurrently, PR #568 added **brand new** code in
37+
`MPCAutofill/cardpicker/catalog_stats.py` that imported and used the **old**
38+
name.
39+
40+
Git auto-merged the two with **no conflict**. It could not have done
41+
otherwise: #568 only ADDED lines, and #567 only touched a nearby docstring,
42+
so the rename and the new references never textually collided. Nothing else
43+
caught it either — not the full backend suite (3,036 tests), and not the
44+
committed lint chain as it stood.
45+
46+
The merged result would have raised, at module-**import** time:
47+
48+
```
49+
ImportError: cannot import name 'SLOW_PATH_TO_REVIEW_REASON'
50+
```
51+
52+
in a module imported by the catalog-stats view **and** by the hourly
53+
`warm_catalog_stats` job. Thirteen reference sites needed fixing across six
54+
modules. The throwaway script that found it was written for one PR and
55+
lived in a session scratchpad; this file is the version that does not get
56+
lost.
57+
58+
The general shape of the defect: **a textual merge cannot see a name graph.**
59+
Two changes that never touch the same lines can still produce a tree in
60+
which a name no longer resolves, or resolves to a different value. Only
61+
something that resolves names across the whole tree at both revisions can
62+
tell.
63+
64+
## What it checks
65+
66+
Two independent checks, in increasing order of strength.
67+
68+
### 1. References — one revision, no false positives
69+
70+
Every `from x import NAME` whose NAME matches the pattern must actually be
71+
declared in `x`, and every matching ALL-CAPS name a module reads must
72+
resolve to a declaration somewhere in the tree. A violation is an
73+
`ImportError`/`NameError` waiting to happen, unconditionally — there is no
74+
judgment call and nothing to tune.
75+
76+
This is the half that runs on **every** Python PR. Note that on a
77+
`pull_request` event, `actions/checkout` gives you the **merge result**, not
78+
the branch tip — which is exactly the revision the #567/#568 bug existed in
79+
and neither contributing branch did.
80+
81+
### 2. Equivalence — two revisions
82+
83+
For each module in scope, parse it at both revisions, then **normalise both
84+
sides** by:
85+
86+
- inlining every module-level constant whose name matches the pattern, with
87+
the constant map built across the **whole tree** at each revision, so a
88+
cross-module `from x import Y_SKIP_REASON` resolves;
89+
- deleting those declarations, their `__all__` entries, and the imports that
90+
only pulled them in;
91+
- deleting every docstring (prose is expected to change — in a rename PR it
92+
is usually most of the diff);
93+
- constant-folding f-strings and string `+` concatenation;
94+
95+
and comparing `ast.dump()` trees. **Identical trees mean every expression
96+
that used to evaluate to a given value still evaluates to the same value,
97+
under a different name.** A difference is reported with the module, the node
98+
path, and both sides rendered back to source:
99+
100+
```
101+
::error file=MPCAutofill/cardpicker/catalog_stats.py::normalised trees differ at Module.body[21].body[7].value.keywords[1].value
102+
9952865b: 'to-review'
103+
90c34e3c: 'to-review-v2'
104+
```
105+
106+
Scope is deliberately narrow: the modules that touch a constant whose
107+
declared **name** changed between the two revisions — which includes modules
108+
the diff never touched, and that is the point. `--all` widens it to every
109+
module mentioning the pattern; `--paths` narrows it to an explicit list.
110+
111+
## The pattern is a parameter
112+
113+
`--pattern` is a regex `re.search`-ed against ALL-CAPS module-level constant
114+
names. The default covers the families this repo actually refactors:
115+
116+
```
117+
SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT|_THRESHOLD|_PREFIX|_REASON
118+
```
119+
120+
It is a `search`, not a suffix test, for a concrete reason:
121+
`LANDS_PHASH_SKIP_REASON_PREFIX` ends in `_PREFIX`, not `_REASON`, and its
122+
value is composed at runtime as `f"{PREFIX}{reason}"` — which is also why
123+
the f-string folder has to fold a _single interpolated constant_ back into
124+
the surrounding literal, not just whole f-strings. `--pattern '.'` inlines
125+
every ALL-CAPS module-level constant, the strongest and slowest setting.
126+
127+
## Honest limits
128+
129+
Stated plainly rather than discovered later:
130+
131+
- The equivalence check is a proof about **pure** refactors. A PR that
132+
renames a constant _and_ changes behaviour in the same modules will be
133+
reported. That is correct, not a false positive: the tool cannot know the
134+
behaviour change was intended, only that the refactor was not
135+
behaviour-preserving. Split the PR, or read the reported node and land it
136+
knowingly.
137+
- A matching constant that is declared but never referenced is deleted from
138+
both sides by normalisation, so a change to _its_ value alone is invisible
139+
to the tree comparison. Those are surfaced separately as informational
140+
`note:` lines — not failures, because a legitimate constant-_extraction_
141+
refactor moves the same multiset.
142+
- Declarations whose right-hand side is not a static expression (a call
143+
outside the `frozenset`/`set`/`tuple`/`list`/`dict` allowlist, a
144+
comprehension) are not inlined. They are listed as `note:` so you know
145+
what the proof did **not** cover.
146+
- It reasons about names, not types or control flow. It is not a
147+
replacement for mypy or the test suite; it covers a gap both of them have
148+
demonstrably passed over.
149+
150+
**Do not weaken the normaliser to make a real repo pass.** The one time a
151+
reported difference looked spurious, it was the production-breaking bug
152+
above. Investigate before excluding.
153+
154+
## Tests
155+
156+
`.github/scripts/tests/test_constant_rename_equivalence.py`, run by the same
157+
workflow and by `python3 .github/scripts/tests/test_constant_rename_equivalence.py`.
158+
Every rule has a passing and a failing case, including a genuine
159+
behaviour change (a renamed constant whose _value_ also moved) — a checker
160+
that only ever reports "identical" proves nothing. Two real-repo tests pin
161+
the incident: the committed tree resolves every matching reference, and the
162+
actual #567 commit still normalises identically against its parent across
163+
the twelve pipeline modules it touched.

docs/reference/skip-reasons.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,16 @@ and `MPCAutofill/cardpicker/tests/test_skip_reason_roster.py` pins every
4242
declared value against an explicit expected set so a future rename cannot
4343
silently alter production data.
4444

45+
That sweep is also where
46+
[`constant-rename-equivalence.md`](constant-rename-equivalence.md) came
47+
from: the rename half of it collided with a concurrently-merged PR that had
48+
added new code using an old constant name, and git auto-merged the two with
49+
no conflict into a tree that raised `ImportError` at module-import time.
50+
Run `.github/scripts/constant_rename_equivalence.py` on any branch that
51+
renames, extracts or retires a `*_SKIP_REASON` — the roster tether above
52+
checks that a value is DOCUMENTED, which is a different question from
53+
whether the code still resolves and still evaluates to the same string.
54+
4555
## How this doc is tethered
4656

4757
`check_skip_reason_roster_tether()` in `.github/scripts/docs_lint.py`

0 commit comments

Comments
 (0)