Skip to content

Declare local_fallback's skip reasons at source (second authorised protected-core exception) - #584

Merged
WilfordGrimley merged 2 commits into
masterfrom
fix/protected-core-local-fallback-skip-reasons
Jul 29, 2026
Merged

Declare local_fallback's skip reasons at source (second authorised protected-core exception)#584
WilfordGrimley merged 2 commits into
masterfrom
fix/protected-core-local-fallback-skip-reasons

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Stacked on #574 (base branch fix/protected-core-skip-reason-constants), not on master. #574 creates license-provenance.md §2.1 — the exception log this PR adds its entry to — and the test_skip_reason_roster.py guard block this PR's guards sit beside. Basing on master would have meant inventing §2.1 twice and conflicting on every shared hunk. Merge #574 first; this PR's own diff is one commit.

The defect

local_fallback.run_fallback_for_card set FallbackOutcome.skip_reason from three bare inline literals:

return FallbackOutcome(skip_reason="no-evidence", ...)
return FallbackOutcome(skip_reason="eliminated", ...)
return FallbackOutcome(skip_reason="ambiguous", ...)

The roster tether from #567 derives the skip-reason roster by scanning for module-level *_SKIP_REASON = "<literal>" declarations. It cannot enumerate literals it cannot see. A fourth reason added beside these three would have joined three invisible siblings on the way to CardScanLog.skip_reason — ~2.7M production rows, no choices list, no foreign key.

local_fallback.py is PROTECTED CORE, so #567 could not close it. The owner granted a second narrow exception on 2026-07-29 — separate from #574's, whose own SCOPE section names this file explicitly as one it does not cover.

Latent, not live — and that is the argument for closing it now

Nothing persists this outcome today. The module's 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. The local-fallback-v1 rows in CardScanLog are historical.

The invisibility, though, is a property of the literals and not of the caller. It becomes a live hole the instant anything persists the outcome, and nothing would fail at that moment to mark it. Closing it while the change is provably inert is the smaller intervention in a protected file, not the larger one.

What changed

local_fallback.py Three LOCAL_FALLBACK_*_SKIP_REASON constants declared, exported, passed by name. Two docstrings + one field comment extended. Nothing else.
test_skip_reason_roster.py Three new names into EXPECTED_SKIP_REASONS; three guards for what the tether cannot see.
docs/reference/skip-reasons.md New "Local-fallback pilot engine" section (marked Latent); the "not declared at their origin" entry closed; a wrong function name corrected.
docs/upstreaming/license-provenance.md §2.1's second entry. §2's file list annotated.

Completeness — verified, not taken on trust

The request named three literals; the file was re-scanned rather than trusted. AST pass over every skip_reason= keyword argument, every .skip_reason attribute assignment, and every lowercase/hyphenated string constant in all 736 lines. Three is the complete set. The other lowercase literals the scan surfaced — black, white, silver, borderless, modern, old, trimmed, bleed, appropriate-bleed — are classifier outputs and tag names on paths that never touch skip_reason.

No mirror was removed, because none existed

Unlike the local_phash pair, these values were never re-declared for this engine anywhere: its one caller never reads them, so there was nothing to mirror them into.

The FALLBACK_*_SKIP_REASON family in local_calculate_verdicts.py is not a mirror of these and was deliberately left alone. It belongs to stage-d-fallback-v1 — a separate calculator with its own row population — and it renames this engine's no-evidence to no-sub-check-evidence precisely to avoid a collision. Collapsing the two families would erase a real distinction. Hence the LOCAL_FALLBACK_ prefix, which also matches this module's own FALLBACK_ANONYMOUS_ID = "local-fallback-v1".

The mirror guard is narrower than #574's, on purpose

no-evidence, eliminated and ambiguous are shared vocabulary — five calculators declare no-evidence under five prefixes today, by design. A flat "declared nowhere else" ban, which is what #574's phash guard can afford, would forbid the roster's own model here.

So test_local_fallback_skip_reasons_are_not_re_mirrored_elsewhere checks against the pinned co-tenants instead: the exact constant-name set EXPECTED_SKIP_REASONS already declares for each value, minus this module's own. Any declaration outside that set fails. It additionally rejects any LOCAL_FALLBACK_-prefixed declaration outside local_fallback.py regardless of value. Adding a mirror therefore cannot be done quietly — it has to be written into EXPECTED_SKIP_REASONS, which that file designates as the deliberate-decision gate, and survive the prefix check.

Verification

Byte-identical proof. The after-source, with the three constants inlined back to their literals and their declarations and __all__ entries removed, parses to an AST identical to the before-source once docstrings are normalised (docstrings are the only other difference, and are not executable). Independently, the sequence of strings run_fallback_for_card can put on skip_reason, resolved statically through the module's constant table, is ['no-evidence', 'eliminated', 'ambiguous'] before and after.

tests/test_local_fallback.py asserts eliminated and no-evidence against live output as raw string literals and was deliberately not edited — an independent behavioural pin that does not read the new constants at all. (ambiguous has no such behavioural test; stated rather than glossed. It is pinned statically instead.)

The tether can still fail, and now names the new site. Removing eliminated's doc entries produces:

::error file=docs/reference/skip-reasons.md::skip-reason roster drift: reason `eliminated` is declared in code
(MPCAutofill/cardpicker/local_calculate_verdicts.py:1627 (FALLBACK_ELIMINATED_SKIP_REASON);
 MPCAutofill/cardpicker/local_fallback.py:114 (LOCAL_FALLBACK_ELIMINATED_SKIP_REASON))
but has no entry in docs/reference/skip-reasons.md.

Restored; docs_lint.py and docs_lint.py --strict clean. A brand-new undocumented constant added to local_fallback.py is likewise caught, naming local_fallback.py as the site.

Guards mutation-checked, each against the regression it claims to catch:

Mutation Result
Re-mirror LOCAL_FALLBACK_ELIMINATED_SKIP_REASON in local_residual_classify.py ..._are_not_re_mirrored_elsewhere fails (prefix branch)
Mirror it under a different name, RESIDUAL_ELIMINATED_SKIP_REASON ..._are_not_re_mirrored_elsewhere + ..._constant_name_is_accounted_for fail
Restore a bare skip_reason="eliminated" literal ..._sets_no_bare_skip_reason_literal fails
Rename a declaration ..._declared_at_their_origin + ..._constant_name_is_accounted_for fail

Other checks. check_protected_core_license.py clean, 9 files, local_fallback.py still in PROTECTED_CORE_FILES. black/isort/ruff/mypy/prettier clean via pre-commit; mypy finding count unchanged at the pre-existing 261/15 files baseline. Full cardpicker/tests/: 3153 passed, 11 skipped (run with #576's migration linearisation cherry-picked locally and dropped before pushing — master's two 0096 leaf nodes otherwise block test-DB creation; no migration is touched here).

Protected-core re-survey (report only — nothing fixed)

AST-verified after #567 and #574: vote_consensus.py, printing_consensus.py, tag_consensus.py, artist_consensus.py, federation-hash-tool/hash_my_cards.py and tests/test_federation_hash_tool_parity.py all show zero skip_reason= kwargs, zero .skip_reason assignments, zero *_SKIP_REASON declarations, and zero string constants matching any roster value. #574's finding still holds; local_fallback.py was the last one.

One unrelated finding, not fixed here — it is not mine to fix. decrypt-saved-deck-export/decrypt.mjs and its test are listed in §2's protected-core file list but are absent from PROTECTED_CORE_FILES in .github/scripts/check_protected_core_license.py (9 entries; these two are not among them). §2's own text says to add them "in the PR that merges #242 (or immediately after)"; #242 merged as 5ddf109c and the files exist. So two documented protected-core files carry no CI gate. Needs its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

`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
WilfordGrimley force-pushed the fix/protected-core-skip-reason-constants branch from 08da4ad to 25a24f5 Compare July 29, 2026 17:29
…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
WilfordGrimley force-pushed the fix/protected-core-local-fallback-skip-reasons branch from 3cb20ec to 90918fd Compare July 29, 2026 17:41
@WilfordGrimley
WilfordGrimley changed the base branch from fix/protected-core-skip-reason-constants to master July 29, 2026 18:47
@WilfordGrimley
WilfordGrimley merged commit d044223 into master Jul 29, 2026
9 checks passed
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
…607)

`test_no_declared_value_is_empty_or_whitespace` parametrised over
`EXPECTED_SKIP_REASONS` — the hand-written dict 100 lines above it — and
asserted those literals were non-empty and stripped. That is an assertion
about text typed into the test file, which no implementation change can
falsify.

Verified by mutation: setting `EXTRACTOR_FETCH_FAILED_SKIP_REASON = "   "` in
image_evidence.py — a whitespace-only reason, invisible to every consumer of
`CardScanLog.skip_reason`, which is exactly what this test names as the
failure mode — left it green. Three other tests caught it; this one could not.

  - parametrised over `_declared()` instead, the values actually read out of
    the production modules. The same mutation now produces a red
    `test_no_declared_value_is_empty_or_whitespace[   ]`.
  - added `test_declared_roster_is_not_empty`, because the parametrised shape
    has a second way of going vacuous: if `_declared()` ever returns `{}`
    (moved directory, regex that stops matching, glob that finds nothing)
    pytest generates zero cases and the suite stays green while checking
    nothing. Verified by mutation — pointing CARDPICKER_DIR at a nonexistent
    path silently skips the parametrised test and now trips this guard.
  - `test_every_declared_skip_reason_value_is_unchanged` is strictly subsumed
    by `test_every_declared_constant_name_is_accounted_for` (dict equality
    implies key-set equality). Kept, because its failure output names the
    changed production string directly where the dict comparison buries it in
    a value-to-name-set diff — but its docstring now says it is a diagnostic
    and adds no mutation coverage, so it stops being counted as a second check.

Coordination: PR #584 also touches this file. Its edits are in
EXPECTED_SKIP_REASONS (lines 58-100) and appended after line 296; these are at
lines 147-169. Disjoint, and this branch is based on origin/master so either
can merge first.

No production code changed.


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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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