Skip to content

Adopt a *_SKIP_REASON declaration convention; tether the roster to a doc - #567

Merged
WilfordGrimley merged 1 commit into
masterfrom
feat/skip-reason-declaration-convention
Jul 29, 2026
Merged

Adopt a *_SKIP_REASON declaration convention; tether the roster to a doc#567
WilfordGrimley merged 1 commit into
masterfrom
feat/skip-reason-declaration-convention

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Why

docs/pipeline-fidelity-gate.md fired as an audit partly on the precondition that "every empirically-derived constant/threshold/override/skip-reason [is] mapped to its home in the new pipeline, or flagged missing".

That claim could never have been tested, because the skip reasons could not be enumerated. Roughly thirty distinct values existed; ~11 were declared as *_SKIP_REASON constants and the rest were bare inline literals, several of them reaching the database through a dynamic pass-through (skip_reason=outcome.ocr_skip_reason, skip_reason=verdict.skip_reason, skip_reason=skip_reason). No static analysis could produce a complete roster, so the audit checked itself against an incomplete list. Twelve values appeared nowhere in docs/ at all. question_feed.py already documented the problem in a comment.

Meanwhile docs/features/catalog-stats.md and cardpicker/catalog_stats.py both asserted "~11 distinct reasons observed in production". The column actually holds 23.

A set cannot be tethered until it can be enumerated, so this PR does the code half first and the doc half second.

What shipped

1. *_SKIP_REASON at every write site. Every value written to CardScanLog.skip_reason now originates from a module-level constant, including the source behind each dynamic pass-through. Where a string is emitted by several calculators under different anonymous_ids with different meanings (no-evidence, ambiguous, no-text, frame-mismatch), each calculator declares its own prefixed constant — same-value/different-constant is the correct shape, a single shared constant would falsely imply one shared concept.

Pure refactor: no string value changed, and SLOW_PATH_TO_REVIEW_REASONSLOW_PATH_TO_REVIEW_SKIP_REASON is the only rename (constant name only; "to-review" is untouched).

2. Statically enumerable. Scanning column-0 *_SKIP_REASON = "<literal>" declarations under MPCAutofill/cardpicker/ now yields the complete roster: 38 values from 55 constants. All 23 values present in production are covered.

3. Tethered. check_skip_reason_roster_tether() + _declared_skip_reasons() in .github/scripts/docs_lint.py, following check_calculator_roster_tether() (#562) in structure, error style and allowlist convention. The doc it checks is the new docs/reference/skip-reasons.md.

Why docs/reference/: docs/features/ documents user-visible product surfaces (catalog-stats, moderation, grid-selector); this is an internal vocabulary enumeration with no UI of its own, which is what reference/vote-weight-matrix.md and reference/funnel-spec.md already are. Indexed in docs/README.md and docs/MANIFEST.md.

Matching is on the literal value, not the constant name — the opposite choice from the calculator tether's full-identity-over-family rule, for a different reason rather than an inconsistent one: here the string IS the production datum (millions of rows key on it) while the constant name is an internal handle a refactor may legitimately change. Each tether matches the thing whose change would break production. docs/documentation-process.md's "Roster tethers" section now states that generalisation.

SKIP_REASON_ROSTER_ALLOWLIST is empty: every declared value has a real entry, including retired and report-only ones, because "nothing writes this any more" is exactly what an enumeration loses first.

4. Both false counts fixed — and replaced with no count rather than a corrected one. A hardcoded number nothing derives is what rotted last time; compute_skip_breakdown() is a plain GROUP BY that never needed the figure.

Verification

  • cardpicker/tests/: 3036 passed, 9 skipped. docs-lint clean, mypy clean (230 files), black/ruff/isort/prettier clean.
  • Byte-identical proof. An AST-level equivalence check parses each touched module at origin/master and at HEAD, inlines every *_SKIP_REASON binding (cross-module imports included), constant-folds the affected f-string, strips the declarations/__all__/docstrings, and compares the trees. All 13 touched modules: IDENTICAL. A row written after this change carries byte-identical text to one written before.
  • New test cardpicker/tests/test_skip_reason_roster.py (42 cases) pins every declared value against a hand-written expected set — deliberately hand-written, since deriving it from the constants it guards would assert nothing — and re-runs the linter's own derivation so the two can't disagree.
  • Fail-then-pass demo. Deleted the unmapped-layout-class, transfer-sha256-mismatch and fetch-budget-exhausted entries from the doc: linter exits 3, naming each reason and its declaration site. Restored: docs-lint: clean, exit 0.

Stated honestly rather than forced

local_phash.find_best_match is PROTECTED CORE and returns no-hashable-candidates / no-clear-winner as inline literals; they cannot be declared at source without editing a protected file. They are mirrored in the consuming module — the one place where declaration is not co-located with origin. A new literal added to find_best_match would reach CardScanLog without failing lint. That gap is recorded in the doc and closes only with an owner exception.

local_lands_identify is report-only (it never writes a CardScanLog row) and composes f"{PREFIX}{reason}" from that same protected-core return. Its five values are declared and documented anyway — they were the largest cluster of skip-reason strings with no named home, i.e. the exact defect this closes — and the prefix constant's comment records that a scan-log write must not be added there until the composition is replaced.

local_ocr and local_fallback return skip-reason-shaped strings that never reach CardScanLog (the fallback write branch was retired 2026-07-29); local_fallback is protected core in any case.

Notes for review

Diff is mechanical by design — two other branches are in flight touching image_evidence.py. Rebased onto aa56e4c0.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

docs/pipeline-fidelity-gate.md fired partly on the precondition that
"every empirically-derived constant/threshold/override/skip-reason [is]
mapped to its home in the new pipeline, or flagged missing". That claim
could never have been tested: the skip reasons could not be ENUMERATED.
~30 distinct values existed, ~11 were declared constants, the rest were
bare inline literals, and several write sites passed the value through
dynamically. Twelve values appeared nowhere in docs/ at all, and two
places asserted "~11 distinct reasons observed in production" against a
real figure of 23.

1. Every value written to CardScanLog.skip_reason now originates from a
   module-level *_SKIP_REASON constant, including the sources behind each
   dynamic pass-through. Pure refactor: no string value changed.
2. The roster is statically enumerable by scanning module-level
   declarations, same as *_ANONYMOUS_ID. 38 values, 55 constants.
3. check_skip_reason_roster_tether() in .github/scripts/docs_lint.py
   tethers the new docs/reference/skip-reasons.md to those declarations,
   following check_calculator_roster_tether() (PR #562).
4. Both "~11 distinct reasons" claims replaced with a pointer to the
   roster; neither code nor prose asserts a hardcoded count any more.

Verified: cardpicker/tests/ 3036 passed / 9 skipped; docs-lint clean;
mypy clean; an AST-level equivalence check proves all 13 touched modules
are behaviourally identical to origin/master once constants are inlined.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
@WilfordGrimley
WilfordGrimley force-pushed the feat/skip-reason-declaration-convention branch from 724c513 to 3df1322 Compare July 29, 2026 15:26
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
`local_phash.find_best_match` returned "no-hashable-candidates" and
"no-clear-winner" as bare inline literals. The roster tether added in
#567 derives the skip-reason roster from module-level
`*_SKIP_REASON = "<literal>"` declarations, and it cannot enumerate
literals it cannot see - so a NEW literal added inside `find_best_match`
would have reached `CardScanLog.skip_reason` (~2.7M rows, no `choices`
list, no FK) with nothing to catch it.

#567 could not close that: `local_phash.py` is PROTECTED CORE
(docs/upstreaming/license-provenance.md §2). It mirrored the two
constants in the consuming module instead and documented the residual
gap. The owner granted a narrow exception on 2026-07-29 to close it.

- declare PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON and
  PHASH_NO_CLEAR_WINNER_SKIP_REASON in `local_phash.py`, export them,
  and return them by name
- delete the mirror in `local_identify_printing_tags.py`, which now
  imports the one constant it uses; one declaration per value
- record the exception, its reasoning and - the part that matters - its
  LIMITS in license-provenance.md §2.1, a new exception log. It permits
  declaring skip-reason constants in this one file. It is not a licence
  to edit protected core; the next such change needs its own ruling.
  `local_phash.py` stays on the protected list.
- two guards in test_skip_reason_roster.py for the regressions the
  tether is structurally blind to: the mirror coming back, and a bare
  literal returning at the origin

Naming-only. The string VALUES are untouched - and so are the constant
NAMES, so the roster's pinning test and the doc's Constant column needed
no edit at all. A `CardScanLog` row written after this is byte-identical
to one written before.

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

Copy link
Copy Markdown
Author

Rebased onto master (post-#569), conflicts resolved

Two textual conflicts, plus one semantic conflict git could not see.

1. image_evidence.py — the expected one

#569 hoisted the legal-line compute into _extract_legal_line and left the old block storage-only. Resolved by keeping master's hoisted structure and applying this PR's intent to it — the two inline literals became EXTRACTOR_FETCH_FAILED_SKIP_REASON / EXTRACTOR_NO_TEXT_SKIP_REASON. _extract_legal_line itself writes no skip reasons, so nothing else moved.

2. docs/features/catalog-stats.md — panel table

Both sides edited the same rows. Took this PR's skipBreakdown cell (the de-counted one) and master's participation cell (#572's card-denominated pointer). Neither intent dropped.

3. catalog_stats.py — silent breakage the AST proof caught

#568 landed new code (distinctCardsRoutedToReview) that imports and uses SLOW_PATH_TO_REVIEW_REASON — the one constant this PR renames. Git auto-merged the file cleanly because master only added lines while this branch only touched the docstring, so the rename and the new references never textually collided.

Result would have been ImportError: cannot import name 'SLOW_PATH_TO_REVIEW_REASON' at import time — the module is imported by the catalog-stats view and the hourly warm_catalog_stats job. Fixed by carrying the rename into all 13 new reference sites (catalog_stats.py, test_catalog_stats.py, docs/features/catalog-stats.md).

This is exactly what the byte-identical proof is for, and it is the argument for keeping it in the review loop rather than treating it as a one-off.

4. multi-faced-v1 removed from the pinned roster

#565 deleted the border-colour "multi-faced" gate and deliberately did not replace SINGLE_FACED_ONLY_SKIP_REASON (its own comment says so). EXPECTED_SKIP_REASONS pins declarations, so the entry had to go — but ~3,409 production CardScanLog rows still carry the value, so it keeps its row in docs/reference/skip-reasons.md, now marked Retired. It is the one value in the roster with no live declaration, and the doc says that explicitly. Dropping it from the doc instead would have lost exactly what this PR exists to preserve.

Re-verification after rebase

  • Byte-identical proof re-run and re-implemented (the original script was not committed). Parses each touched module at origin/master and at HEAD, inlines every *_SKIP_REASON binding — cross-module imports and the LANDS_PHASH_SKIP_REASON_PREFIX composition included — strips declarations/__all__/docstrings, constant-folds f-strings and + concatenation, compares parse trees.
    All 14 touched modules: IDENTICAL. A CardScanLog row written after this change carries byte-identical text to one written before.
  • cardpicker/tests/: 3148 passed, 11 skipped, 0 failed.
  • test_skip_reason_roster.py: 41 passed (was 42 — one case less, being the retired multi-faced-v1).
  • docs-lint clean; ruff, isort, black, mypy, prettier all pass.

The suite cannot run on unmodified master at all right now — the two-0096 migration conflict makes pytest-django fail to build the test database. These runs layer #576 (the hotfix) on top, which is the state this PR will actually merge into.

@WilfordGrimley
WilfordGrimley merged commit bc7ad41 into master Jul 29, 2026
8 of 9 checks passed
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
`local_phash.find_best_match` returned "no-hashable-candidates" and
"no-clear-winner" as bare inline literals. The roster tether added in
#567 derives the skip-reason roster from module-level
`*_SKIP_REASON = "<literal>"` declarations, and it cannot enumerate
literals it cannot see - so a NEW literal added inside `find_best_match`
would have reached `CardScanLog.skip_reason` (~2.7M rows, no `choices`
list, no FK) with nothing to catch it.

#567 could not close that: `local_phash.py` is PROTECTED CORE
(docs/upstreaming/license-provenance.md §2). It mirrored the two
constants in the consuming module instead and documented the residual
gap. The owner granted a narrow exception on 2026-07-29 to close it.

- declare PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON and
  PHASH_NO_CLEAR_WINNER_SKIP_REASON in `local_phash.py`, export them,
  and return them by name
- delete the mirror in `local_identify_printing_tags.py`, which now
  imports the one constant it uses; one declaration per value
- record the exception, its reasoning and - the part that matters - its
  LIMITS in license-provenance.md §2.1, a new exception log. It permits
  declaring skip-reason constants in this one file. It is not a licence
  to edit protected core; the next such change needs its own ruling.
  `local_phash.py` stays on the protected list.
- two guards in test_skip_reason_roster.py for the regressions the
  tether is structurally blind to: the mirror coming back, and a bare
  literal returning at the origin

Naming-only. The string VALUES are untouched - and so are the constant
NAMES, so the roster's pinning test and the doc's Constant column needed
no edit at all. A `CardScanLog` row written after this is byte-identical
to one written before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
Semantic (not textual) conflict surfaced by rebasing onto master after
PR #567 landed. #567 renamed `SLOW_PATH_TO_REVIEW_REASON` ->
`SLOW_PATH_TO_REVIEW_SKIP_REASON` in
`cardpicker/local_calculate_verdicts.py`; this branch had concurrently
ADDED a new test
(`test_slow_path_scoped_and_unscoped_eligible_sets_agree`) referencing
the OLD name. Git merged both cleanly - #567 touched only the
declaration, this branch only added lines - so the breakage was
invisible in the diff and would have shown up as a `NameError:
SLOW_PATH_TO_REVIEW_REASON` at test run time (the import block at the
top of the file already imports the NEW name, so collection succeeds
and only this one test explodes).

No behaviour change: the value behind both names is the same string
"to-review".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…e exception)

`local_fallback.run_fallback_for_card` set `FallbackOutcome.skip_reason`
from three bare inline literals - "no-evidence", "eliminated", "ambiguous".
The roster tether added in #567 derives the skip-reason roster from
module-level `*_SKIP_REASON = "<literal>"` declarations, and it cannot
enumerate literals it cannot see, so a FOURTH literal added beside them
would have joined three invisible siblings on the way to
`CardScanLog.skip_reason` (~2.7M rows, no `choices` list, no FK).

Latent, not live: this module's own write branch was retired by #560 and its
one non-test caller
(`local_residual_classify.recover_frame_mismatch_printing_via_fallback_refetch`)
reads `outcome.printing_pk` and discards `skip_reason`. Nothing persists
these today. That is an argument for closing the hole now, while the change
is provably inert, not for leaving it: the invisibility is a property of the
literals, and it goes live the moment anything persists the outcome, with no
lint failure to mark the moment.

`local_fallback.py` is PROTECTED CORE
(docs/upstreaming/license-provenance.md section 2). The owner granted a
SECOND narrow exception on 2026-07-29 - separate from #574's, which names
this file explicitly as one it does NOT cover.

- declare LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON /
  LOCAL_FALLBACK_ELIMINATED_SKIP_REASON / LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON
  in `local_fallback.py`, export them, and pass them by name. The
  `LOCAL_FALLBACK_` prefix (not `FALLBACK_`) keeps them distinct from the
  SEPARATE `stage-d-fallback-v1` calculator's `FALLBACK_*` family in
  `local_calculate_verdicts.py`
- no mirror was deleted, because none existed: unlike the phash pair these
  values were never re-declared for this engine anywhere, since its one
  caller never reads them. The Stage D family was checked and left alone -
  a parallel calculator's own vocabulary, not a mirror of these
- completeness verified by AST scan of the whole file (every `skip_reason=`
  kwarg, every `.skip_reason` assignment, every lowercase/hyphenated string
  constant): three is the complete set, not just the three I was handed
- record the exception, its reasoning, its proof of nil effect and - the
  part that matters - its LIMITS as the second entry in
  license-provenance.md section 2.1. `local_fallback.py` stays on the
  protected list in section 2, now annotated
- document the three in docs/reference/skip-reasons.md under a new
  Local-fallback pilot engine section, marked Latent; correct that doc's
  reference to a function named `compute_fallback_outcome`, which does not
  exist (it is `run_fallback_for_card`)
- three guards in test_skip_reason_roster.py, all mutation-checked. The
  mirror guard is deliberately narrower than #574's: `no-evidence`,
  `eliminated` and `ambiguous` are shared vocabulary several calculators
  legitimately declare under their own prefixes, so a flat value ban would
  forbid the roster's own design

Naming-only. The string VALUES are untouched: the after-source with the
constants inlined back to literals parses to an AST identical to the
before-source (docstrings normalised - they are the only other difference),
and the sequence run_fallback_for_card can produce is
['no-evidence', 'eliminated', 'ambiguous'] before and after. A `CardScanLog`
row written after this is byte-identical to one written before.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
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
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…sion

Owner rulings on PR #585's two open questions, 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 and frontend-only PR permanently unmergeable the moment an owner
marked it required. This repo merges plenty of both.

1. Dropped the `paths:` filter from `on: pull_request` for the WHOLE
   workflow, not just the `references` job - leaving it on the siblings
   would only relocate the same trap for whoever marks the next one
   required. The reasoning is written into the workflow header in a block
   that says not to add one back.

2. Added `--changed-since REV` so the no-op path is cheap and explicit:
   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.

3. Made the gate incapable of wedging the check it protects: an
   unresolvable REV (shallow clone, fork without base history) degrades to
   running the full check with a printed note, never to an error. The gate
   is an optimisation, never a correctness input.

4. A clean run now reports what it DID - "123 reference(s) to constants
   matching /.../ all resolve at HEAD; 373 modules scanned" - and a tree
   with no matching references says "nothing to check" explicitly. "clean"
   alone is indistinguishable from a check that silently stopped finding
   anything.

Q2 - default pattern breadth: KEEP IT BROAD (owner: "i want the CI to do as
much troubleshooting for us as possible. we can cull it, and slice it up to
specific scopes later"). No code change. Recorded in the doc as a deliberate
ruling with the one-line narrowing path spelled out, so a future reader does
not "tidy" the generic families out of it.

Also, prompted by the concurrent audit that found BOTH roster tethers in
docs_lint.py using a non-recursive `src_dir.glob("*.py")` - which hid
`scryfall-tagger-v1` in management/commands/ from them - audited this
script for the same mistake. It does not have it: the scan is
`git ls-tree -r` over the whole tree at the revision, with no directory
filtering. Made that deliberate rather than incidental:

- read_python_tree() now documents why recursion AND tests/ inclusion are
  both load-bearing here. tests/ is exactly where these 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.
- New TestScanCoverage pins a nested management/commands/ module and a
  tests/ module are both scanned, plus a derivation guard against the real
  tree - without it every real-repo assertion could pass vacuously if the
  listing ever stopped recursing.

Branch protection itself is NOT touched - that is governed by
docs/infrastructure.md and is an owner action in the GitHub UI.

Verified: 45 unit tests pass (was 37); docs-lint --strict clean;
pre-commit --all-files clean; no-op path 63ms, full scan 4.7s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…579)

* perf: push card_ids into every Stage D dependency subquery (#533)

The last surviving instance of the defect class PR #541 was written to
eliminate - and the one that mattered most, because #541 fixed five
calculators OUTSIDE the dispatch loop and explicitly did not touch these,
so it survived in three of the four calculators already in Stage E's hot
path.

`_fallback_eligible_cards_queryset` forwarded `card_ids` to
`_eligible_cards_queryset` (the outer `Card` query and its own-exclusion)
but built its two join-key no-hit DEPENDENCY SUBQUERIES unscoped.
`_slow_path_eligible_cards_queryset` built four unscoped. And
`local_illustration.run_illustration_calculator` built the same join-key
pair unscoped at its call site, then handed it to
`_eligible_illustration_cards_queryset`, which cannot scope what it
receives as an argument.

Django compiles `.filter(pk__in=<values_list qs>)` as an UNCORRELATED
`IN (SELECT ...)`. The outer `.filter(pk__in=card_ids)` therefore bounds
the ROWS RETURNED, not the WORK DONE: every micro-batch paid a full pass
over `CardPrintingTag` (167,229 rows live) or `CardScanLog` (2,617,333
rows live, append-only, still growing) regardless of batch size. Keeping
the subqueries LAZY - which `local_illustration` already did - does not
help; laziness only decides whether the rows land in Python memory, not
whether the database scans the table.

`_join_key_no_hit_subqueries` is now the single place that pair is built,
for all three calculators, so a fourth cannot repeat the omission. The
slow-path calculator's own two exclusions (already-routed `CardScanLog`,
fallback-voted `CardPrintingTag`) are scoped inline alongside them.

Measured against the live catalogue, eligibility query alone, no
calculator work, median of 12 reps, read-only:

                            batch 25            batch 250
    stage-d-fallback-v1     1113.3 -> 2.6 ms    1212.5 -> 8.1 ms
    stage-d-slow-path-v1     959.5 -> 2.2 ms     991.0 -> 7.5 ms
    stage-d-illustration-v2 1117.2 -> 2.9 ms    2101.8 -> 10.7 ms

~3.2 s of fixed per-invocation cost removed at the production batch size
of 25 - which at that batch size outweighed all nine calculators' actual
compute, and is why Stage D cost 174-196 ms/card at batch 25 against
28-32 ms/card at batch 250.

BULK mode (`card_ids=None`, every management-command caller) is
byte-identical: the compiled SQL of all three eligibility queries was
diffed against `origin/master`'s under a fixed `PYTHONHASHSEED` and is
unchanged to the byte.

Tests assert on the COMPILED SQL, not the result set, per #541's
established rationale: the outer filter produces the same rows whether or
not the push-down happened, so a result-set test is green either way and
proves nothing. The new SQL assertions were mutation-proved - reverting
the push-down turns exactly those three red while every result-set test
in the same class stays green, which is the vacuous-green defect
demonstrated rather than argued.

Refs #533, #541, #469, #526, #458, #460.

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

* Rebase fix: use the post-#567 name SLOW_PATH_TO_REVIEW_SKIP_REASON

Semantic (not textual) conflict surfaced by rebasing onto master after
PR #567 landed. #567 renamed `SLOW_PATH_TO_REVIEW_REASON` ->
`SLOW_PATH_TO_REVIEW_SKIP_REASON` in
`cardpicker/local_calculate_verdicts.py`; this branch had concurrently
ADDED a new test
(`test_slow_path_scoped_and_unscoped_eligible_sets_agree`) referencing
the OLD name. Git merged both cleanly - #567 touched only the
declaration, this branch only added lines - so the breakage was
invisible in the diff and would have shown up as a `NameError:
SLOW_PATH_TO_REVIEW_REASON` at test run time (the import block at the
top of the file already imports the NEW name, so collection succeeds
and only this one test explodes).

No behaviour change: the value behind both names is the same string
"to-review".

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…te roster (#586)

Read-only audit, no code changes. Redoes the 2026-07-22 knowledge-inventory
sweep's mapping against what code declares today (51 *_SKIP_REASON / 20
*_ANONYMOUS_ID constants, PR #567), and re-tests whether the gate's FIRED
verdict survives.

Headline findings:
- The skip-reason clause the sweep could not test is now testable and PASSES.
- Artifact 1's 83.2% "OCR-channel agreement" is a self-comparison, not a
  cross-method one: the two identities agree 33,622/33,622 with zero conflicts,
  and 1,072 of the 1,261 artist-contradiction cards sit INSIDE that agreement.
- stage-d-fallback-v1 (30,311 votes, 18% of the printing pool) first ran during
  the fire itself and was never compared to anything; it loses 362 of 402
  adjudicable disagreements against local-ocr-v1.
- scryfall-tagger-v1 falls through the calculator tether's non-recursive glob.
- The resolution-level soundness property holds, re-verified against the worst
  population available.

Recommendation: keep FIRED, QUALIFIED to the resolution layer.


Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WilfordGrimley added a commit that referenced this pull request Jul 29, 2026
…otected-core exception) (#584)

* Declare local_phash's skip reasons at source (protected-core exception)

`local_phash.find_best_match` returned "no-hashable-candidates" and
"no-clear-winner" as bare inline literals. The roster tether added in
#567 derives the skip-reason roster from module-level
`*_SKIP_REASON = "<literal>"` declarations, and it cannot enumerate
literals it cannot see - so a NEW literal added inside `find_best_match`
would have reached `CardScanLog.skip_reason` (~2.7M rows, no `choices`
list, no FK) with nothing to catch it.

#567 could not close that: `local_phash.py` is PROTECTED CORE
(docs/upstreaming/license-provenance.md §2). It mirrored the two
constants in the consuming module instead and documented the residual
gap. The owner granted a narrow exception on 2026-07-29 to close it.

- declare PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON and
  PHASH_NO_CLEAR_WINNER_SKIP_REASON in `local_phash.py`, export them,
  and return them by name
- delete the mirror in `local_identify_printing_tags.py`, which now
  imports the one constant it uses; one declaration per value
- record the exception, its reasoning and - the part that matters - its
  LIMITS in license-provenance.md §2.1, a new exception log. It permits
  declaring skip-reason constants in this one file. It is not a licence
  to edit protected core; the next such change needs its own ruling.
  `local_phash.py` stays on the protected list.
- two guards in test_skip_reason_roster.py for the regressions the
  tether is structurally blind to: the mirror coming back, and a bare
  literal returning at the origin

Naming-only. The string VALUES are untouched - and so are the constant
NAMES, so the roster's pinning test and the doc's Constant column needed
no edit at all. A `CardScanLog` row written after this is byte-identical
to one written before.

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

* Declare local_fallback's skip reasons at source (second protected-core exception)

`local_fallback.run_fallback_for_card` set `FallbackOutcome.skip_reason`
from three bare inline literals - "no-evidence", "eliminated", "ambiguous".
The roster tether added in #567 derives the skip-reason roster from
module-level `*_SKIP_REASON = "<literal>"` declarations, and it cannot
enumerate literals it cannot see, so a FOURTH literal added beside them
would have joined three invisible siblings on the way to
`CardScanLog.skip_reason` (~2.7M rows, no `choices` list, no FK).

Latent, not live: this module's own write branch was retired by #560 and its
one non-test caller
(`local_residual_classify.recover_frame_mismatch_printing_via_fallback_refetch`)
reads `outcome.printing_pk` and discards `skip_reason`. Nothing persists
these today. That is an argument for closing the hole now, while the change
is provably inert, not for leaving it: the invisibility is a property of the
literals, and it goes live the moment anything persists the outcome, with no
lint failure to mark the moment.

`local_fallback.py` is PROTECTED CORE
(docs/upstreaming/license-provenance.md section 2). The owner granted a
SECOND narrow exception on 2026-07-29 - separate from #574's, which names
this file explicitly as one it does NOT cover.

- declare LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON /
  LOCAL_FALLBACK_ELIMINATED_SKIP_REASON / LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON
  in `local_fallback.py`, export them, and pass them by name. The
  `LOCAL_FALLBACK_` prefix (not `FALLBACK_`) keeps them distinct from the
  SEPARATE `stage-d-fallback-v1` calculator's `FALLBACK_*` family in
  `local_calculate_verdicts.py`
- no mirror was deleted, because none existed: unlike the phash pair these
  values were never re-declared for this engine anywhere, since its one
  caller never reads them. The Stage D family was checked and left alone -
  a parallel calculator's own vocabulary, not a mirror of these
- completeness verified by AST scan of the whole file (every `skip_reason=`
  kwarg, every `.skip_reason` assignment, every lowercase/hyphenated string
  constant): three is the complete set, not just the three I was handed
- record the exception, its reasoning, its proof of nil effect and - the
  part that matters - its LIMITS as the second entry in
  license-provenance.md section 2.1. `local_fallback.py` stays on the
  protected list in section 2, now annotated
- document the three in docs/reference/skip-reasons.md under a new
  Local-fallback pilot engine section, marked Latent; correct that doc's
  reference to a function named `compute_fallback_outcome`, which does not
  exist (it is `run_fallback_for_card`)
- three guards in test_skip_reason_roster.py, all mutation-checked. The
  mirror guard is deliberately narrower than #574's: `no-evidence`,
  `eliminated` and `ambiguous` are shared vocabulary several calculators
  legitimately declare under their own prefixes, so a flat value ban would
  forbid the roster's own design

Naming-only. The string VALUES are untouched: the after-source with the
constants inlined back to literals parses to an AST identical to the
before-source (docstrings normalised - they are the only other difference),
and the sequence run_fallback_for_card can produce is
['no-evidence', 'eliminated', 'ambiguous'] before and after. A `CardScanLog`
row written after this is byte-identical to one written before.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
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
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
…sion

Owner rulings on PR #585's two open questions, 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 and frontend-only PR permanently unmergeable the moment an owner
marked it required. This repo merges plenty of both.

1. Dropped the `paths:` filter from `on: pull_request` for the WHOLE
   workflow, not just the `references` job - leaving it on the siblings
   would only relocate the same trap for whoever marks the next one
   required. The reasoning is written into the workflow header in a block
   that says not to add one back.

2. Added `--changed-since REV` so the no-op path is cheap and explicit:
   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.

3. Made the gate incapable of wedging the check it protects: an
   unresolvable REV (shallow clone, fork without base history) degrades to
   running the full check with a printed note, never to an error. The gate
   is an optimisation, never a correctness input.

4. A clean run now reports what it DID - "123 reference(s) to constants
   matching /.../ all resolve at HEAD; 373 modules scanned" - and a tree
   with no matching references says "nothing to check" explicitly. "clean"
   alone is indistinguishable from a check that silently stopped finding
   anything.

Q2 - default pattern breadth: KEEP IT BROAD (owner: "i want the CI to do as
much troubleshooting for us as possible. we can cull it, and slice it up to
specific scopes later"). No code change. Recorded in the doc as a deliberate
ruling with the one-line narrowing path spelled out, so a future reader does
not "tidy" the generic families out of it.

Also, prompted by the concurrent audit that found BOTH roster tethers in
docs_lint.py using a non-recursive `src_dir.glob("*.py")` - which hid
`scryfall-tagger-v1` in management/commands/ from them - audited this
script for the same mistake. It does not have it: the scan is
`git ls-tree -r` over the whole tree at the revision, with no directory
filtering. Made that deliberate rather than incidental:

- read_python_tree() now documents why recursion AND tests/ inclusion are
  both load-bearing here. tests/ is exactly where these 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.
- New TestScanCoverage pins a nested management/commands/ module and a
  tests/ module are both scanned, plus a derivation guard against the real
  tree - without it every real-repo assertion could pass vacuously if the
  listing ever stopped recursing.

Branch protection itself is NOT touched - that is governed by
docs/infrastructure.md and is an owner action in the GitHub UI.

Verified: 45 unit tests pass (was 37); docs-lint --strict clean;
pre-commit --all-files clean; no-op path 63ms, full scan 4.7s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
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
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
…sion

Owner rulings on PR #585's two open questions, 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 and frontend-only PR permanently unmergeable the moment an owner
marked it required. This repo merges plenty of both.

1. Dropped the `paths:` filter from `on: pull_request` for the WHOLE
   workflow, not just the `references` job - leaving it on the siblings
   would only relocate the same trap for whoever marks the next one
   required. The reasoning is written into the workflow header in a block
   that says not to add one back.

2. Added `--changed-since REV` so the no-op path is cheap and explicit:
   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.

3. Made the gate incapable of wedging the check it protects: an
   unresolvable REV (shallow clone, fork without base history) degrades to
   running the full check with a printed note, never to an error. The gate
   is an optimisation, never a correctness input.

4. A clean run now reports what it DID - "123 reference(s) to constants
   matching /.../ all resolve at HEAD; 373 modules scanned" - and a tree
   with no matching references says "nothing to check" explicitly. "clean"
   alone is indistinguishable from a check that silently stopped finding
   anything.

Q2 - default pattern breadth: KEEP IT BROAD (owner: "i want the CI to do as
much troubleshooting for us as possible. we can cull it, and slice it up to
specific scopes later"). No code change. Recorded in the doc as a deliberate
ruling with the one-line narrowing path spelled out, so a future reader does
not "tidy" the generic families out of it.

Also, prompted by the concurrent audit that found BOTH roster tethers in
docs_lint.py using a non-recursive `src_dir.glob("*.py")` - which hid
`scryfall-tagger-v1` in management/commands/ from them - audited this
script for the same mistake. It does not have it: the scan is
`git ls-tree -r` over the whole tree at the revision, with no directory
filtering. Made that deliberate rather than incidental:

- read_python_tree() now documents why recursion AND tests/ inclusion are
  both load-bearing here. tests/ is exactly where these 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.
- New TestScanCoverage pins a nested management/commands/ module and a
  tests/ module are both scanned, plus a derivation guard against the real
  tree - without it every real-repo assertion could pass vacuously if the
  listing ever stopped recursing.

Branch protection itself is NOT touched - that is governed by
docs/infrastructure.md and is an owner action in the GitHub UI.

Verified: 45 unit tests pass (was 37); docs-lint --strict clean;
pre-commit --all-files clean; no-op path 63ms, full scan 4.7s.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
#585)

* 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

* Make the references check safe to mark required; pin the scan's recursion

Owner rulings on PR #585's two open questions, 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 and frontend-only PR permanently unmergeable the moment an owner
marked it required. This repo merges plenty of both.

1. Dropped the `paths:` filter from `on: pull_request` for the WHOLE
   workflow, not just the `references` job - leaving it on the siblings
   would only relocate the same trap for whoever marks the next one
   required. The reasoning is written into the workflow header in a block
   that says not to add one back.

2. Added `--changed-since REV` so the no-op path is cheap and explicit:
   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.

3. Made the gate incapable of wedging the check it protects: an
   unresolvable REV (shallow clone, fork without base history) degrades to
   running the full check with a printed note, never to an error. The gate
   is an optimisation, never a correctness input.

4. A clean run now reports what it DID - "123 reference(s) to constants
   matching /.../ all resolve at HEAD; 373 modules scanned" - and a tree
   with no matching references says "nothing to check" explicitly. "clean"
   alone is indistinguishable from a check that silently stopped finding
   anything.

Q2 - default pattern breadth: KEEP IT BROAD (owner: "i want the CI to do as
much troubleshooting for us as possible. we can cull it, and slice it up to
specific scopes later"). No code change. Recorded in the doc as a deliberate
ruling with the one-line narrowing path spelled out, so a future reader does
not "tidy" the generic families out of it.

Also, prompted by the concurrent audit that found BOTH roster tethers in
docs_lint.py using a non-recursive `src_dir.glob("*.py")` - which hid
`scryfall-tagger-v1` in management/commands/ from them - audited this
script for the same mistake. It does not have it: the scan is
`git ls-tree -r` over the whole tree at the revision, with no directory
filtering. Made that deliberate rather than incidental:

- read_python_tree() now documents why recursion AND tests/ inclusion are
  both load-bearing here. tests/ is exactly where these 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.
- New TestScanCoverage pins a nested management/commands/ module and a
  tests/ module are both scanned, plus a derivation guard against the real
  tree - without it every real-repo assertion could pass vacuously if the
  listing ever stopped recursing.

Branch protection itself is NOT touched - that is governed by
docs/infrastructure.md and is an owner action in the GitHub UI.

Verified: 45 unit tests pass (was 37); docs-lint --strict clean;
pre-commit --all-files clean; no-op path 63ms, full scan 4.7s.

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

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
WilfordGrimley added a commit that referenced this pull request Aug 4, 2026
… diff

scope_modules() computed the affected-name set as a symmetric difference
between base and head declarations, so a constant ADDED at head with
nothing removed anywhere also pulled its module into the whole-module
equivalence comparison. Since base never declared the new name, that
module can never normalise identically to base - any PR merely adding a
constant matching the pattern (SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT|
_THRESHOLD|_PREFIX|_REASON) failed this gate permanently. PR #686's own
two new reason_tags.py constants tripped exactly this.

Fix: use base - head (names that disappeared from base) instead of the
symmetric difference, matching the function's own docstring ('modules
that touch a constant whose DECLARED NAME changed'). Renames and
removals are unaffected - the old name is still base-only either way,
so the same module still gets pulled in and still gets the same
whole-module comparison it got before.

The removed/head-only reporting split at the print site is dead in its
'head only' branch under the new scope (moved is now always base-only),
so it's replaced with a plain removed-name list plus a separate,
purely-informational head-only listing an operator can use to see both
halves of a rename without it affecting scope.

One real tradeoff, surfaced and pinned rather than hidden: an ISOLATED
literal-to-constant extraction (no companion rename/removal in the same
module) is no longer auto-checked for a wrong extracted value, because
it is provably indistinguishable, by name-diffing alone, from a
harmless new constant backing brand-new logic - the exact shape that
caused PR #686's false positive. Verified against real history
(PR #567's LANDS_PHASH_SKIP_REASON_PREFIX) that this capability was
only ever exercised when a companion rename existed in the same module,
which remains fully covered; the isolated case is now pinned as a
documented limitation in
test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot
rather than silently regressing.

Adds three tests distinguishing the fix (pure addition -> nothing to
prove, rename -> still scoped and still catches a real diff, removal ->
still scoped) plus updates to the extraction-folding tests above.

Verified: gate script exits 0 against this PR's actual base/head (was
4); test_constant_rename_equivalence.py alone (49 passed) and the full
.github/scripts/tests/ suite together (214 passed, no order-dependence
per issue #679); py_compile and pre-commit both clean.
WilfordGrimley added a commit that referenced this pull request Aug 4, 2026
…ist/tag exclusions (#686)

* fix(question_feed): phase-C not-official-art routing + md5-expand artist/tag exclusions

Two independent routing gaps in the question feed (MPCAutofill/cardpicker/question_feed.py),
diagnosed against reason_tags.py's WTC phase B partition and issue #473's md5 identity groups:

1. reason_tags.py: promotes the WTC phase B "not-official-printing" vs. "not-official-art"
   partition from prose in the module docstring into two real, checked frozensets
   (NOT_OFFICIAL_PRINTING_REASON_TAGS / NOT_OFFICIAL_ART_REASON_TAGS), mirroring the frontend's
   NoMatchReasonStrip.tsx NO_MATCH_REASON_TAG_GROUPS. test_reason_tags.py asserts they are
   exhaustive and disjoint over NO_MATCH_REASON_TAGS.

2. question_feed.py: phase C routing. A card carrying a positive (VotePolarity.APPLY)
   CardTagVote for a not-official-art tag has had its artwork question declared unanswerable by
   a human, so the feed now excludes it from artist-shaped questions in both _tier_2_contested
   and _tier_4_fresh (_not_official_art_card_ids, md5-group-widened, computed once per feed
   request in get_next_question_feed_item). The printing question is unaffected.

   Judgment call: the exclusion requires a HUMAN-BACKED source (vote_consensus.
   is_human_backed_source), not merely "any positive vote". Reason tags are cast by a human
   through NoMatchReasonStrip today, but nothing in the schema stops a future machine caster
   from writing one, and this routing signal is meant to represent an actual human declaration
   that the artwork question is meaningless for the card - a machine-cast source earning the
   same trust would need its own explicit decision, not a silent inclusion via this change.

3. question_feed.py: _tier_2_contested's artist and tag own-vote exclusions are now md5-group-
   widened (_voter_answered_artist_card_ids / _voter_answered_tag_card_ids_by_tag), so a voter
   who answered one member of a byte-identical group is not re-asked the identical artist/tag
   question under a sibling's identifier - the same convention _voter_answered_printing_card_ids
   already established for the printing tiers. The tag widening is scoped to the CARD axis only
   (never the tag axis), preserving _tier_2_contested's existing per-tag granularity, and costs
   one query total (not one per review pair) plus one md5 expansion per distinct tag the voter
   has touched. Scoped to _tier_2_contested only, per the engineering brief - _tier_4_fresh's
   own artist/tag own-vote exclusions keep their pre-existing, unwidened form.

Observation (out of scope, no implementation): local_illustration.
_eligible_illustration_cards_queryset (the machine illustration calculator) has no awareness of
NOT_OFFICIAL_ART_REASON_TAGS either, and plausibly warrants the same exclusion for the same
reason phase C does - left as a follow-up.

New tests: test_reason_tags.py (partition exhaustiveness/disjointness), test_question_feed.py
(TestPhaseCNotOfficialArtRouting - positive not-official-art excludes, not-official-printing
doesn't, negative vote doesn't, machine-cast vote doesn't), test_md5_group_pooling.py
(TestPhaseCAndTierTwoMd5Expansion - artist/tag md5 widening, sibling non-re-serving, the
per-tag-axis regression the existing _tier_2_contested comment warns against, and the
not-official-art group-wide exclusion; plus a computed-once-per-feed-request pin for all three
new exclusions, extending the existing 2026-07-25 PR #482 condition f1 test).

* fix(ci): scope constant-rename gate to base-minus-head, not symmetric diff

scope_modules() computed the affected-name set as a symmetric difference
between base and head declarations, so a constant ADDED at head with
nothing removed anywhere also pulled its module into the whole-module
equivalence comparison. Since base never declared the new name, that
module can never normalise identically to base - any PR merely adding a
constant matching the pattern (SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT|
_THRESHOLD|_PREFIX|_REASON) failed this gate permanently. PR #686's own
two new reason_tags.py constants tripped exactly this.

Fix: use base - head (names that disappeared from base) instead of the
symmetric difference, matching the function's own docstring ('modules
that touch a constant whose DECLARED NAME changed'). Renames and
removals are unaffected - the old name is still base-only either way,
so the same module still gets pulled in and still gets the same
whole-module comparison it got before.

The removed/head-only reporting split at the print site is dead in its
'head only' branch under the new scope (moved is now always base-only),
so it's replaced with a plain removed-name list plus a separate,
purely-informational head-only listing an operator can use to see both
halves of a rename without it affecting scope.

One real tradeoff, surfaced and pinned rather than hidden: an ISOLATED
literal-to-constant extraction (no companion rename/removal in the same
module) is no longer auto-checked for a wrong extracted value, because
it is provably indistinguishable, by name-diffing alone, from a
harmless new constant backing brand-new logic - the exact shape that
caused PR #686's false positive. Verified against real history
(PR #567's LANDS_PHASH_SKIP_REASON_PREFIX) that this capability was
only ever exercised when a companion rename existed in the same module,
which remains fully covered; the isolated case is now pinned as a
documented limitation in
test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot
rather than silently regressing.

Adds three tests distinguishing the fix (pure addition -> nothing to
prove, rename -> still scoped and still catches a real diff, removal ->
still scoped) plus updates to the extraction-folding tests above.

Verified: gate script exits 0 against this PR's actual base/head (was
4); test_constant_rename_equivalence.py alone (49 passed) and the full
.github/scripts/tests/ suite together (214 passed, no order-dependence
per issue #679); py_compile and pre-commit both clean.
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