From 25a24f5ce36982a1cdff36621efaf80e66f460df Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Wed, 29 Jul 2026 13:57:12 +0000 Subject: [PATCH 1/2] Declare local_phash's skip reasons at source (protected-core exception) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 = ""` 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) Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN --- .../local_identify_printing_tags.py | 28 ++++--- MPCAutofill/cardpicker/local_phash.py | 46 ++++++++-- .../tests/test_skip_reason_roster.py | 80 ++++++++++++++++++ docs/reference/skip-reasons.md | 53 +++++++----- docs/upstreaming/license-provenance.md | 83 ++++++++++++++++++- 5 files changed, 248 insertions(+), 42 deletions(-) diff --git a/MPCAutofill/cardpicker/local_identify_printing_tags.py b/MPCAutofill/cardpicker/local_identify_printing_tags.py index e15acc8d1..48418885e 100644 --- a/MPCAutofill/cardpicker/local_identify_printing_tags.py +++ b/MPCAutofill/cardpicker/local_identify_printing_tags.py @@ -82,6 +82,7 @@ local_phash, ) from cardpicker.local_fallback import FALLBACK_ANONYMOUS_ID +from cardpicker.local_phash import PHASH_NO_CLEAR_WINNER_SKIP_REASON from cardpicker.models import ( CanonicalCard, Card, @@ -167,15 +168,18 @@ # The phash engine's own outcomes (`run_phash_for_card`). PHASH_TOO_MANY_CANDIDATES_SKIP_REASON = "too-many-candidates" -# `local_phash.find_best_match` is PROTECTED CORE (docs/upstreaming/license-provenance.md §2) and -# returns these two strings as its own inline literals; they cannot be declared at their true -# source without editing a protected file. They are MIRRORED here - the one roster entry whose -# declaration is not co-located with its origin - and used for the equality test below so the -# coupling is at least named rather than anonymous. NO_CLEAR_WINNER is never written to -# `CardScanLog` by this module today (`_classify_no_clear_winner` always refines it into one of -# the two variants below); HISTORICAL rows predating that refinement still carry it. -PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON = "no-hashable-candidates" -PHASH_NO_CLEAR_WINNER_SKIP_REASON = "no-clear-winner" +# `no-hashable-candidates` and `no-clear-winner` are NOT declared here. They originate inside +# `local_phash.find_best_match` and, as of the 2026-07-29 protected-core exception +# (docs/upstreaming/license-provenance.md §2), are declared THERE as +# `PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON` / `PHASH_NO_CLEAR_WINNER_SKIP_REASON`. They were +# briefly mirrored here because that file is protected core and could not be edited; the mirror is +# gone, so there is ONE declaration per value and nothing that can drift. This module IMPORTS the +# one it needs (see the import block above) rather than re-declaring it. +# +# The two refinements below ARE this module's own: `_classify_no_clear_winner` splits phash's +# undifferentiated `no-clear-winner` into a threshold miss and a margin miss, so plain +# `no-clear-winner` is never written to `CardScanLog` by this module today - HISTORICAL rows +# predating that refinement still carry it. PHASH_NO_CLEAR_WINNER_DISTANCE_SKIP_REASON = "no-clear-winner-distance" PHASH_NO_CLEAR_WINNER_MARGIN_SKIP_REASON = "no-clear-winner-margin" @@ -1994,8 +1998,10 @@ def verify_zero_resolutions(card_ids: list[int], batch_size: int = 2000) -> list "OCR_UNKNOWN_SET_CODE_SKIP_REASON", "PARSED_BUT_NO_MATCH_SKIP_REASON", "PHASH_TOO_MANY_CANDIDATES_SKIP_REASON", - "PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON", - "PHASH_NO_CLEAR_WINNER_SKIP_REASON", + # `PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON` / `PHASH_NO_CLEAR_WINNER_SKIP_REASON` are + # deliberately absent: they are declared and exported by `cardpicker.local_phash`, their + # origin. Re-exporting them from here would recreate the two-names-one-value ambiguity that + # removing the mirror was meant to end. "PHASH_NO_CLEAR_WINNER_DISTANCE_SKIP_REASON", "PHASH_NO_CLEAR_WINNER_MARGIN_SKIP_REASON", "RESCANNABLE_SKIP_REASONS", diff --git a/MPCAutofill/cardpicker/local_phash.py b/MPCAutofill/cardpicker/local_phash.py index 3f0fc10b0..c10c8945a 100644 --- a/MPCAutofill/cardpicker/local_phash.py +++ b/MPCAutofill/cardpicker/local_phash.py @@ -72,6 +72,27 @@ # win either one already gets over full resolution. INGEST_HASH_FETCH_DPI = 40 +# THIS MODULE'S SKIP VOCABULARY (2026-07-29, see docs/reference/skip-reasons.md). `find_best_match` +# below returns these two strings; via `local_identify_printing_tags.run_phash_for_card` they reach +# `CardScanLog.skip_reason`, so they are roster members and must be statically enumerable - the +# docs_lint roster tether derives the roster from module-level `*_SKIP_REASON = ""` +# declarations and CANNOT see a bare inline literal, which is exactly the hole a new literal added +# inside `find_best_match` would have fallen through. +# +# THIS FILE IS PROTECTED CORE (docs/upstreaming/license-provenance.md section 2). These two +# declarations exist under a NARROW, EXPLICIT owner exception granted 2026-07-29, recorded in that +# section - it authorises declaring skip-reason constants HERE and nothing else. The file remains +# protected; any other change to it still needs its own ruling. +# +# Naming: the `PHASH_` prefix is kept even though this module is already the phash engine, because +# these names moved here VERBATIM from `local_identify_printing_tags`, where they were mirrored, +# and because the consuming module reads them alongside its own +# `PHASH_NO_CLEAR_WINNER_{DISTANCE,MARGIN}_SKIP_REASON` refinements as one family. Keeping the +# names byte-identical (not only the values) is what makes this change nothing but a move: the +# roster's pinning test and the doc's Constant column needed no edit at all. +PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON = "no-hashable-candidates" +PHASH_NO_CLEAR_WINNER_SKIP_REASON = "no-clear-winner" + def _hash_to_int(image_hash: "imagehash.ImageHash") -> int: return twos_complement(str(image_hash), _HASH_BITS) @@ -376,15 +397,20 @@ def find_best_match( margin: int = DEFAULT_MARGIN, ) -> tuple[Optional[PhashMatch], str]: """ - Returns (match, skip_reason). skip_reason is "no-hashable-candidates" (every candidate - failed to fetch/hash), "no-clear-winner" (best distance is over threshold, or the runner-up - is too close behind it), or "" (matched). Requires at least 2 hashed candidates to compute a - margin at all when there's more than one name-candidate in the first place; a genuinely - single-candidate name (already excluded by the orchestrator's selection - phash only runs on - multi-candidate names in practice) would just need the threshold. + Returns (match, skip_reason). skip_reason is PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON + ("no-hashable-candidates" - every candidate failed to fetch/hash), + PHASH_NO_CLEAR_WINNER_SKIP_REASON ("no-clear-winner" - best distance is over threshold, or the + runner-up is too close behind it), or "" (matched). Requires at least 2 hashed candidates to + compute a margin at all when there's more than one name-candidate in the first place; a + genuinely single-candidate name (already excluded by the orchestrator's selection - phash only + runs on multi-candidate names in practice) would just need the threshold. + + Any NEW skip reason added here must be declared as a module-level `*_SKIP_REASON` constant + above and documented in docs/reference/skip-reasons.md - a bare literal returned from here + reaches `CardScanLog` without the roster tether ever seeing it. """ if not candidates_with_hashes: - return None, "no-hashable-candidates" + return None, PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON # card_hash and each candidate hash are both plain ints (the DB storage representation) - # ImageHash's `-` operator (Hamming distance) needs two ImageHash objects, not raw ints. @@ -400,9 +426,9 @@ def find_best_match( runner_up_distance = scored[1][1] if len(scored) > 1 else None if best_distance > distance_threshold: - return None, "no-clear-winner" + return None, PHASH_NO_CLEAR_WINNER_SKIP_REASON if runner_up_distance is not None and (runner_up_distance - best_distance) <= margin: - return None, "no-clear-winner" + return None, PHASH_NO_CLEAR_WINNER_SKIP_REASON return PhashMatch(candidate=best_candidate, distance=best_distance, runner_up_distance=runner_up_distance), "" @@ -412,6 +438,8 @@ def find_best_match( "DEFAULT_MARGIN", "ART_CROP_BOX", "INGEST_HASH_FETCH_DPI", + "PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON", + "PHASH_NO_CLEAR_WINNER_SKIP_REASON", "DEFAULT_BACKFILL_BATCH_SIZE", "DEFAULT_BACKFILL_WORKERS", "DEFAULT_PIPELINE_QUEUE_DEPTH_BATCHES", diff --git a/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py b/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py index 0865e76ae..eeeb0746d 100644 --- a/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py +++ b/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py @@ -21,6 +21,7 @@ hand-written set, so a value reintroduced as a bare inline literal (which the derivation cannot see) fails here even if nothing else notices. """ +import ast import re from pathlib import Path @@ -204,3 +205,82 @@ def test_docs_roster_tether_is_clean(): spec.loader.exec_module(module) assert module.check_skip_reason_roster_tether() == [] + + +# --------------------------------------------------------------------------- +# The protected-core exception's own guard rails (2026-07-29, +# docs/upstreaming/license-provenance.md section 2.1). +# +# `local_phash.find_best_match` produces two roster values. Until the owner +# granted a narrow exception, that file could not be edited, so the constants +# were MIRRORED in `local_identify_printing_tags.py` — which meant a NEW bare +# literal returned from `find_best_match` reached `CardScanLog` with nothing to +# catch it, because the roster tether cannot enumerate literals it cannot see. +# +# The exception was granted to close exactly that hole, and these two tests are +# what keep it closed. They fail if the mirror comes back (two declarations that +# can drift) or if a bare literal is reintroduced at the origin (a roster member +# no derivation can find). Neither failure mode is visible to the tether itself, +# which is the whole reason they are pinned here. +# --------------------------------------------------------------------------- + +PHASH_ORIGIN_SKIP_REASONS = { + "PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON": "no-hashable-candidates", + "PHASH_NO_CLEAR_WINNER_SKIP_REASON": "no-clear-winner", +} + + +def _module_level_str_constants(path: Path) -> dict[str, str]: + tree = ast.parse(path.read_text()) + return { + node.targets[0].id: node.value.value + for node in tree.body + if isinstance(node, ast.Assign) + and len(node.targets) == 1 + and isinstance(node.targets[0], ast.Name) + and isinstance(node.value, ast.Constant) + and isinstance(node.value.value, str) + } + + +def test_phash_skip_reasons_are_declared_at_their_origin_and_nowhere_else(): + """One declaration per value, in the module that produces it.""" + declared_in_phash = _module_level_str_constants(CARDPICKER_DIR / "local_phash.py") + for name, value in PHASH_ORIGIN_SKIP_REASONS.items(): + assert declared_in_phash.get(name) == value, ( + f"{name} must be declared in local_phash.py, where find_best_match produces it, " + f"with the value {value!r} — see license-provenance.md section 2.1." + ) + + for py in sorted(CARDPICKER_DIR.glob("*.py")): + if py.name == "local_phash.py": + continue + for name, value in _module_level_str_constants(py).items(): + assert value not in set(PHASH_ORIGIN_SKIP_REASONS.values()), ( + f"{py.name} re-declares a local_phash skip reason as {name}={value!r}. " + f"That mirror was removed on purpose: two declarations of one value can drift. " + f"Import it from cardpicker.local_phash instead." + ) + + +def test_find_best_match_returns_no_bare_skip_reason_literal(): + """Every skip reason `find_best_match` returns must be a NAME bound to one of + its module's own constants. A bare literal here is invisible to the roster + derivation and would reach `CardScanLog` unnoticed — the exact defect the + protected-core exception was granted to fix.""" + path = CARDPICKER_DIR / "local_phash.py" + tree = ast.parse(path.read_text()) + constants = _module_level_str_constants(path) + fn = next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "find_best_match") + + returned = [n.value.elts[1] for n in ast.walk(fn) if isinstance(n, ast.Return) and isinstance(n.value, ast.Tuple)] + assert returned, "find_best_match no longer returns a (match, skip_reason) tuple" + for node in returned: + if isinstance(node, ast.Constant) and node.value == "": + continue # the "matched, not a skip" sentinel, not a roster member + assert isinstance(node, ast.Name), ( + f"line {node.lineno}: find_best_match returns a bare skip-reason literal. " + f"Declare it as a module-level *_SKIP_REASON constant and document it in " + f"docs/reference/skip-reasons.md." + ) + assert node.id in constants, f"line {node.lineno}: {node.id} is not a module-level string constant" diff --git a/docs/reference/skip-reasons.md b/docs/reference/skip-reasons.md index 2b2ddb543..30357a99e 100644 --- a/docs/reference/skip-reasons.md +++ b/docs/reference/skip-reasons.md @@ -140,11 +140,17 @@ longer exists in the code; it wrote `fetch_failed` like the rest. | `no-clear-winner-margin` | `PHASH_NO_CLEAR_WINNER_MARGIN_SKIP_REASON` | `local-phash-v1` | A candidate cleared the threshold, but the runner-up was too close behind it. | Live | `no-hashable-candidates` and `no-clear-winner` are the two values whose -strings physically originate inside `MPCAutofill/cardpicker/local_phash.py`, -which is PROTECTED CORE (`docs/upstreaming/license-provenance.md` section 2) and therefore cannot be edited to declare them at their true source. -They are **mirrored** in the consuming module, which is the one place in -the roster where the declaration is not co-located with the origin. See -"The values that are not declared at their origin" below. +strings physically originate inside `MPCAutofill/cardpicker/local_phash.py` +(`find_best_match`), and that is where their constants are **declared** — +not in this section's own module. That file is PROTECTED CORE +(`docs/upstreaming/license-provenance.md` section 2); the two declarations +sit there under the narrow owner exception granted 2026-07-29 and recorded +in that section. They were briefly mirrored in +`local_identify_printing_tags.py`, while that file was the only editable +one; the mirror is gone, so every roster value now has exactly one +declaration and it is co-located with its origin. +`local_identify_printing_tags` imports `PHASH_NO_CLEAR_WINNER_SKIP_REASON` +from `local_phash` for its `_classify_no_clear_winner` refinement test. ## Stage D join-key calculator — `MPCAutofill/cardpicker/local_calculate_verdicts.py` @@ -266,27 +272,32 @@ persisted values with no further work. Stated explicitly rather than forced, per the sweep's own brief. -**`local_phash.find_best_match`'s two return literals** -(`no-hashable-candidates`, `no-clear-winner`) are emitted from +**`local_phash.find_best_match`'s two return values — CLOSED 2026-07-29.** +`no-hashable-candidates` and `no-clear-winner` were the sweep's one +exception: they are produced inside `MPCAutofill/cardpicker/local_phash.py`, which is PROTECTED CORE -(`docs/upstreaming/license-provenance.md` section 2) and cannot be edited -to declare them at source without an owner exception. They are mirrored as -constants in the consuming module, -`MPCAutofill/cardpicker/local_identify_printing_tags.py`, and that module's -equality test reads the mirrored constant, so the coupling is named rather -than anonymous. The roster is still complete — but the guarantee is -"declared where it is consumed", not "declared where it is produced", for -these two alone. A NEW literal added to `find_best_match` would flow -through to `CardScanLog` without any lint failure. That residual gap closes -only by editing a protected file. +(`docs/upstreaming/license-provenance.md` section 2), so the sweep could +not declare them at source and mirrored them in the consuming module +instead. That left a real hole — a NEW literal added inside +`find_best_match` would have reached `CardScanLog` without the tether ever +seeing it, because the tether cannot enumerate literals it cannot see. +The owner granted a narrow exception on 2026-07-29 (recorded in +`license-provenance.md` section 2, which also states its limits): the two +constants are now declared in `local_phash.py` itself, the mirror in +`local_identify_printing_tags.py` is removed, and the tether reports +`local_phash.py` as the declaration site. **Every roster value is now +declared where it is produced.** The exception covers skip-reason constants +in that one file only; the file remains protected. **The lands module's phash-branch composition.** That module reports `f"{LANDS_PHASH_SKIP_REASON_PREFIX}{reason}"` — a `phash-` prefix concatenated onto whatever `find_best_match` returned. The prefix is a -constant and the routing test reads it, but the composed value is not a -declared string and cannot be made one without enumerating protected-core -returns. This is harmless today precisely because nothing in that module is -persisted; the constant's own comment records that a `CardScanLog` write +constant and the routing test reads it, but the composed value is still not +a declared string — the concatenation is what the tether cannot see, and +that is true regardless of the two `find_best_match` returns now being +named constants. This is harmless today precisely because nothing in that +module is persisted; the constant's own comment records that a +`CardScanLog` write must not be added there until the composition is replaced with explicit per-outcome constants. diff --git a/docs/upstreaming/license-provenance.md b/docs/upstreaming/license-provenance.md index a4f9275d0..101c1458f 100644 --- a/docs/upstreaming/license-provenance.md +++ b/docs/upstreaming/license-provenance.md @@ -156,7 +156,9 @@ nobody can point at doesn't function as one: - `MPCAutofill/cardpicker/printing_consensus.py` - `MPCAutofill/cardpicker/tag_consensus.py` - `MPCAutofill/cardpicker/artist_consensus.py` -- `MPCAutofill/cardpicker/local_phash.py` +- `MPCAutofill/cardpicker/local_phash.py` — **still protected**; carries + one authorised exception, 2026-07-29, logged in §2.1. The exception + covers that one change, not the file. - `MPCAutofill/cardpicker/local_fallback.py` - `federation-hash-tool/hash_my_cards.py` (+ its test) - `MPCAutofill/cardpicker/tests/test_federation_hash_tool_parity.py` (the @@ -230,6 +232,85 @@ is actually worried about: someone pasting AGPL-licensed _source code_ directly into a protected-core file, not a third-party package turning out to have an unexpected license three dependencies deep. +### 2.1 Authorised exceptions — the log + +Protection here means **deliberate review**, not immutability. A change to +a listed file is allowed when the owner rules on it specifically. Every +such ruling gets an entry below, so a reviewer who was not present can see +what was changed, why, who authorised it, and — the part that matters most +— **how far the authorisation reaches**. An entry that reads as a +precedent would be worse than no entry: the whole value of the policy is +that the next change has to be asked for again. + +The file stays on the list in §2 either way. An exception authorises **one +change**; it does not un-protect a file. + +**2026-07-29 — `MPCAutofill/cardpicker/local_phash.py`: declare the two +skip-reason constants at source.** + +- **What changed.** `find_best_match` returned two skip reasons as bare + inline string literals, `"no-hashable-candidates"` and + `"no-clear-winner"`. They are now declared as module-level + `PHASH_NO_HASHABLE_CANDIDATES_SKIP_REASON` and + `PHASH_NO_CLEAR_WINNER_SKIP_REASON`, exported in `__all__`, and returned + by name. The mirrored copies of both constants in + `local_identify_printing_tags.py` (added by PR #567 precisely because + this file could not be edited) were deleted; that module now imports the + one it uses. Nothing else in the file was touched. +- **Why.** The `*_SKIP_REASON` declaration convention and the roster tether + `check_skip_reason_roster_tether()` (PR #567, + `docs/reference/skip-reasons.md`) derive the skip-reason roster by + scanning for module-level `NAME = ""` declarations. **The tether + cannot enumerate literals it cannot see.** With the values declared only + in the consuming module, a NEW literal added inside `find_best_match` + would have reached `CardScanLog.skip_reason` — a column with ~2.7M + production rows and no `choices` list or foreign key protecting it — with + no lint failure anywhere. PR #567 documented that hole and could not + close it, because closing it required editing this file. Two declarations + in a protected file is the smaller risk; an undetectable roster gap in + the vote system's own scan log is the larger one. +- **Who authorised it.** The owner, ruling on 2026-07-29 on a request that + named this file and this change specifically. +- **Effect: none, and it is proved rather than asserted.** This is a + naming-only change. The string VALUES are untouched, so a `CardScanLog` + row written after it is byte-identical to one written before. Proof, in + the PR: (a) the after-source with the two constants inlined back to their + literals parses to an AST identical to the before-source; (b) the + sequence of strings `find_best_match` can return, resolved statically + through the module's constant table, is unchanged; (c) + `tests/test_skip_reason_roster.py`, which pins every roster value against + a hand-written expected set, needed **no edit** — the constant NAMES were + kept byte-identical too, so nothing about the roster moved except its + declaration site. There is no licensing effect of any kind: no import was + added to or removed from this file, no external code was introduced, and + the file's GPL-3.0 status and `PROVENANCE:`-header cleanliness are + unchanged. `check_protected_core_license.py` passes, and + `local_phash.py` remains in `PROTECTED_CORE_FILES` and in §2's list + above. + +- **What keeps the hole closed.** The tether alone cannot: it is blind to + exactly the two regressions that would undo this. Two guards in + `tests/test_skip_reason_roster.py` cover them — + `test_phash_skip_reasons_are_declared_at_their_origin_and_nowhere_else` + (fails if any other `cardpicker` module re-declares either value, i.e. if + the mirror comes back) and + `test_find_best_match_returns_no_bare_skip_reason_literal` (fails if a + skip reason is returned from `find_best_match` as a bare literal rather + than a named module-level constant). Both were mutation-checked against + the regression each claims to catch. + +- **SCOPE OF THIS EXCEPTION — read this before citing it.** It permits + **declaring skip-reason constants in `local_phash.py`**, and that is its + entire reach. It is specifically NOT: + - a general licence to edit `local_phash.py`; + - a licence to edit any other protected-core file, including the four + consensus modules, `local_fallback.py`, or the federation hash tool — + several of which emit skip-reason-shaped literals of their own (see + `docs/reference/skip-reasons.md`), and none of which are covered here; + - a standing rule that "lint-driven refactors are exempt". The next + change of any shape to any file on this list, including the next + lint-driven one, **needs its own ruling and its own entry below.** + ## 3. Absorption protocol For the day the "permitted zone" (everything outside protected core) ever From 90918fd53ee42d04db7d028bf8d4584812c21a54 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Wed, 29 Jul 2026 17:20:04 +0000 Subject: [PATCH 2/2] 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 = ""` 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) Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN --- MPCAutofill/cardpicker/local_fallback.py | 66 +++++++++- .../tests/test_skip_reason_roster.py | 120 +++++++++++++++++- docs/reference/skip-reasons.md | 97 ++++++++++++-- docs/upstreaming/license-provenance.md | 120 +++++++++++++++++- 4 files changed, 385 insertions(+), 18 deletions(-) diff --git a/MPCAutofill/cardpicker/local_fallback.py b/MPCAutofill/cardpicker/local_fallback.py index e572da754..4ed0c3768 100644 --- a/MPCAutofill/cardpicker/local_fallback.py +++ b/MPCAutofill/cardpicker/local_fallback.py @@ -79,6 +79,42 @@ # a ground-truth reading is than an estimate from the same OCR-weight source. GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE = 0.95 +# THIS MODULE'S SKIP VOCABULARY (2026-07-29, see docs/reference/skip-reasons.md). +# `run_fallback_for_card` below returns these three strings on `FallbackOutcome.skip_reason`. +# They are LATENT roster members, not live ones: the module's own `CardScanLog`/printing-vote +# write branch was retired on 2026-07-29 (see this module's docstring), 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 them TODAY. They go +# live the moment anything does, which is exactly why they must be enumerable BEFORE that +# happens: the docs_lint roster tether derives the roster from module-level +# `*_SKIP_REASON = ""` declarations and CANNOT see a bare inline literal, so a new +# literal added inside `run_fallback_for_card` plus one new write would reach +# `CardScanLog.skip_reason` (~2.7M rows, no `choices` list, no FK) with no lint failure anywhere. +# +# THIS FILE IS PROTECTED CORE (docs/upstreaming/license-provenance.md section 2). These three +# declarations exist under a NARROW, EXPLICIT owner exception granted 2026-07-29 and recorded in +# section 2.1 there - it authorises declaring skip-reason constants HERE and nothing else. The +# file remains protected; any other change to it still needs its own ruling. +# +# Naming: `LOCAL_FALLBACK_` and not `FALLBACK_`, because `local_calculate_verdicts` already +# declares a `FALLBACK_*_SKIP_REASON` family for the Stage D `stage-d-fallback-v1` calculator - +# a DIFFERENT calculator with its own rows and its own vocabulary, which deliberately shares +# `eliminated`/`ambiguous` verbatim with this module and deliberately renames this module's +# `no-evidence` to `no-sub-check-evidence`. Those are not mirrors of these and must not be +# collapsed into them; the prefix keeps the two families legible at a glance. `LOCAL_FALLBACK_` +# matches this module's own `FALLBACK_ANONYMOUS_ID = "local-fallback-v1"` identity. +# +# Each declaration must stay on ONE line starting at column 0 - that is the shape the tether's +# regex matches. Comments go ABOVE, never trailing: a trailing comment long enough to push the +# line past black's limit makes black wrap the value in parentheses, silently dropping the +# constant out of the derived roster. +# Not one sub-check (border, artist, symbol) produced a reading at all. +LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON = "no-evidence" +# Sub-checks ran and ruled out every candidate - zero survivors. +LOCAL_FALLBACK_ELIMINATED_SKIP_REASON = "eliminated" +# More than one candidate survived the sub-check intersection. +LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON = "ambiguous" + # --------------------------------------------------------------------------------------------- # 2a: artist OCR - full-width bottom band (not pass 1's narrower left-side collector-line crop): # old border prints "Illus. " centred at the very bottom; modern frames put the artist @@ -635,6 +671,9 @@ def cast_bleed_edge_vote(card: Card, bleed_class: Optional[str], run_id: Optiona class FallbackOutcome: printing_pk: Optional[int] = None evidence_types_used: list[str] = field(default_factory=list) + # One of this module's three `LOCAL_FALLBACK_*_SKIP_REASON` constants, or `""` for "matched, + # not a skip". Anything set here must be a named module-level constant, never a bare literal: + # the roster tether cannot enumerate what it cannot see. See the declarations at the top. skip_reason: str = "" # whether the "Illus." anchor was found at all (independent of whether the extracted name # went on to fuzzy-match a candidate) - the frame-style classifier's "old border" signal, @@ -654,7 +693,17 @@ def run_fallback_for_card( redundant tesseract call on cards where the artist line already happened to be visible in the narrower pass-1 crop. `bleed_class` (from classify_bleed_edge, run once per card ahead of everything else - see run_pilot) is threaded through to every sub-check's own - fixed-fraction crop box via normalize_crop_box.""" + fixed-fraction crop box via normalize_crop_box. + + `FallbackOutcome.skip_reason` is one of LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON + ("no-evidence" - not one sub-check produced a reading), LOCAL_FALLBACK_ELIMINATED_SKIP_REASON + ("eliminated" - zero survivors), LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON ("ambiguous" - more + than one survivor), or "" (matched). + + Any NEW skip reason added here must be declared as a module-level `*_SKIP_REASON` constant + above and documented in docs/reference/skip-reasons.md - a bare literal returned from here + is invisible to the roster tether, and would reach `CardScanLog` unnoticed the moment + anything persists this outcome.""" candidate_pks = {c.pk for c in selected.candidates} canonicals = { c.pk: c @@ -684,14 +733,20 @@ def run_fallback_for_card( evidence_types_used.append(name) if not evidence_types_used: - return FallbackOutcome(skip_reason="no-evidence", illus_anchor_fired=illus_anchor_fired) + return FallbackOutcome( + skip_reason=LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON, illus_anchor_fired=illus_anchor_fired + ) if len(survivors) == 0: return FallbackOutcome( - skip_reason="eliminated", evidence_types_used=evidence_types_used, illus_anchor_fired=illus_anchor_fired + skip_reason=LOCAL_FALLBACK_ELIMINATED_SKIP_REASON, + evidence_types_used=evidence_types_used, + illus_anchor_fired=illus_anchor_fired, ) if len(survivors) > 1: return FallbackOutcome( - skip_reason="ambiguous", evidence_types_used=evidence_types_used, illus_anchor_fired=illus_anchor_fired + skip_reason=LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON, + evidence_types_used=evidence_types_used, + illus_anchor_fired=illus_anchor_fired, ) return FallbackOutcome( @@ -707,6 +762,9 @@ def run_fallback_for_card( "FALLBACK_CONFIDENCE_SINGLE_EVIDENCE", "BORDER_ATTRIBUTE_VOTE_CONFIDENCE", "GROUND_TRUTH_ATTRIBUTE_VOTE_CONFIDENCE", + "LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON", + "LOCAL_FALLBACK_ELIMINATED_SKIP_REASON", + "LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON", "BORDER_COLOR_TO_TAG", "FRAME_VOTE_CONFIDENCE", "FRAME_STYLE_TO_TAG", diff --git a/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py b/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py index eeeb0746d..5a3572591 100644 --- a/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py +++ b/MPCAutofill/cardpicker/tests/test_skip_reason_roster.py @@ -58,7 +58,6 @@ "transferred-interim-guard": {"TRANSFERRED_INTERIM_GUARD_SKIP_REASON"}, # Stage D fallback calculator — local_calculate_verdicts.py "no-sub-check-evidence": {"FALLBACK_NO_SUB_CHECK_EVIDENCE_SKIP_REASON"}, - "eliminated": {"FALLBACK_ELIMINATED_SKIP_REASON"}, # Stage D slow path — local_calculate_verdicts.py "to-review": {"SLOW_PATH_TO_REVIEW_SKIP_REASON"}, # Illustration calculator — local_illustration.py @@ -87,18 +86,31 @@ "artist-no-match": {"LANDS_ARTIST_NO_MATCH_SKIP_REASON"}, "no-content-phash": {"LANDS_NO_CONTENT_PHASH_SKIP_REASON"}, "fetch-budget-exhausted": {"LANDS_FETCH_BUDGET_EXHAUSTED_SKIP_REASON"}, - # Values emitted by MORE THAN ONE calculator, each with its own constant + # Values emitted by MORE THAN ONE calculator, each with its own constant. + # The `LOCAL_FALLBACK_*` three belong to the `local-fallback-v1` pilot engine + # (`local_fallback.py`, PROTECTED CORE — declared there under the 2026-07-29 + # exception, license-provenance.md section 2.1). They are LATENT: nothing + # persists that engine's outcome today. The `FALLBACK_*` three next to them are + # the SEPARATE Stage D `stage-d-fallback-v1` calculator's own vocabulary in + # `local_calculate_verdicts.py`, which is live. Two calculators, two constants, + # one value — the same shape as "no-evidence" already had five times over. "no-evidence": { "JOIN_KEY_NO_EVIDENCE_SKIP_REASON", "FALLBACK_NO_EVIDENCE_SKIP_REASON", + "LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON", "AI_ART_NO_EVIDENCE_SKIP_REASON", "NO_EVIDENCE_SKIP_REASON", "LAYOUT_CLASS_NO_EVIDENCE_SKIP_REASON", }, + "eliminated": { + "FALLBACK_ELIMINATED_SKIP_REASON", + "LOCAL_FALLBACK_ELIMINATED_SKIP_REASON", + }, "ambiguous": { "EXTRACTOR_AMBIGUOUS_SKIP_REASON", "JOIN_KEY_AMBIGUOUS_SKIP_REASON", "FALLBACK_AMBIGUOUS_SKIP_REASON", + "LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON", "OCR_AMBIGUOUS_SKIP_REASON", "LAYOUT_CLASS_AMBIGUOUS_SKIP_REASON", }, @@ -284,3 +296,107 @@ def test_find_best_match_returns_no_bare_skip_reason_literal(): f"docs/reference/skip-reasons.md." ) assert node.id in constants, f"line {node.lineno}: {node.id} is not a module-level string constant" + + +# --------------------------------------------------------------------------- +# The SECOND protected-core exception's guard rails (2026-07-29, +# docs/upstreaming/license-provenance.md section 2.1, second entry). +# +# `local_fallback.run_fallback_for_card` returns three roster values on +# `FallbackOutcome.skip_reason`. They were bare inline literals in a PROTECTED +# CORE file, so the roster tether could not see them — and a NEW literal added +# beside them would have been just as invisible. The defect was LATENT rather +# than live (the module's own write branch was retired 2026-07-29, and its one +# non-test caller, `local_residual_classify. +# recover_frame_mismatch_printing_via_fallback_refetch`, reads `printing_pk` +# and discards `skip_reason`), which is precisely why it had to be closed +# BEFORE something persists the outcome rather than after. +# +# These three tests are what keep it closed. Two of them guard the regressions +# the tether is structurally blind to — the value being MIRRORED back into +# another module, and a bare literal returning at the origin — and the third +# pins the name/value pairs to this file. +# --------------------------------------------------------------------------- + +LOCAL_FALLBACK_ORIGIN_SKIP_REASONS = { + "LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON": "no-evidence", + "LOCAL_FALLBACK_ELIMINATED_SKIP_REASON": "eliminated", + "LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON": "ambiguous", +} + + +def test_local_fallback_skip_reasons_are_declared_at_their_origin(): + """Declared in the module that produces them, with the exact production values.""" + declared = _module_level_str_constants(CARDPICKER_DIR / "local_fallback.py") + for name, value in LOCAL_FALLBACK_ORIGIN_SKIP_REASONS.items(): + assert declared.get(name) == value, ( + f"{name} must be declared in local_fallback.py, where run_fallback_for_card " + f"produces it, with the value {value!r} — see license-provenance.md section 2.1." + ) + + +def test_local_fallback_skip_reasons_are_not_re_mirrored_elsewhere(): + """No NEW declaration of one of these three values may appear in another module. + + This cannot be the flat "declared nowhere else" check the `local_phash` pair + gets, and the difference is not a weakening. `no-evidence`, `eliminated` and + `ambiguous` are SHARED VOCABULARY: several calculators legitimately emit each + of them under different `anonymous_id`s with different meanings, and the + roster's whole design is one prefixed constant per calculator per value. A + value-identity ban would forbid the design. + + So the check is against the pinned co-tenants instead — the exact set of + constant names EXPECTED_SKIP_REASONS already declares for each of these three + values, minus this module's own. Any declaration outside that set fails here. + Adding a mirror therefore cannot be done quietly: it must be written into + EXPECTED_SKIP_REASONS above, which this file's own docstring designates as the + deliberate-decision gate for the roster, AND it must survive the prefix check + below, which rejects the `LOCAL_FALLBACK_` family being re-declared anywhere + else regardless of value. + """ + tracked = set(LOCAL_FALLBACK_ORIGIN_SKIP_REASONS.values()) + allowed_cotenants = { + value: EXPECTED_SKIP_REASONS[value] - set(LOCAL_FALLBACK_ORIGIN_SKIP_REASONS) for value in tracked + } + + for py in sorted(CARDPICKER_DIR.glob("*.py")): + for name, value in _module_level_str_constants(py).items(): + if py.name != "local_fallback.py" and name.startswith("LOCAL_FALLBACK_"): + raise AssertionError( + f"{py.name} declares {name}={value!r}. The `LOCAL_FALLBACK_` skip-reason " + f"family is declared in local_fallback.py, where run_fallback_for_card " + f"produces it. Import it from cardpicker.local_fallback instead — two " + f"declarations of one value can drift." + ) + if py.name == "local_fallback.py" or value not in tracked: + continue + assert name in allowed_cotenants[value], ( + f"{py.name} declares {name}={value!r}, which is not one of the calculators " + f"already pinned for that value ({sorted(allowed_cotenants[value])}). If this is " + f"a genuinely different calculator's own vocabulary, add it to " + f"EXPECTED_SKIP_REASONS above as a deliberate decision. If it is a mirror of " + f"local_fallback's constant, delete it and import the original." + ) + + +def test_run_fallback_for_card_sets_no_bare_skip_reason_literal(): + """Every `skip_reason=` this function passes must be a NAME bound to one of its + module's own constants. A bare literal here is invisible to the roster + derivation and would reach `CardScanLog` unnoticed the moment anything persists + the outcome — the exact defect the protected-core exception was granted to fix.""" + path = CARDPICKER_DIR / "local_fallback.py" + tree = ast.parse(path.read_text()) + constants = _module_level_str_constants(path) + fn = next(n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name == "run_fallback_for_card") + + passed = [kw.value for kw in ast.walk(fn) if isinstance(kw, ast.keyword) and kw.arg == "skip_reason"] + assert passed, "run_fallback_for_card no longer sets FallbackOutcome.skip_reason" + for node in passed: + if isinstance(node, ast.Constant) and node.value == "": + continue # the "matched, not a skip" sentinel, not a roster member + assert isinstance(node, ast.Name), ( + f"line {node.lineno}: run_fallback_for_card sets a bare skip-reason literal. " + f"Declare it as a module-level *_SKIP_REASON constant and document it in " + f"docs/reference/skip-reasons.md." + ) + assert node.id in constants, f"line {node.lineno}: {node.id} is not a module-level string constant" diff --git a/docs/reference/skip-reasons.md b/docs/reference/skip-reasons.md index 30357a99e..aed8d18f6 100644 --- a/docs/reference/skip-reasons.md +++ b/docs/reference/skip-reasons.md @@ -152,6 +152,52 @@ declaration and it is co-located with its origin. `local_identify_printing_tags` imports `PHASH_NO_CLEAR_WINNER_SKIP_REASON` from `local_phash` for its `_classify_no_clear_winner` refinement test. +## Local-fallback pilot engine — `MPCAutofill/cardpicker/local_fallback.py` + +`anonymous_id` is `local-fallback-v1` (`FALLBACK_ANONYMOUS_ID`). The pilot's +pass 2: border / artist / symbol sub-checks intersected against the card's +own name-candidates, run against a fresh per-invocation image fetch. +`run_fallback_for_card` puts one of these three on +`FallbackOutcome.skip_reason`. + +| Reason | Constant | Means | Status | +| ------------- | ---------------------------------------- | --------------------------------------------------------------------- | ---------------------- | +| `no-evidence` | `LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON` | Not one sub-check (border, artist, symbol) produced a reading at all. | **Latent — see below** | +| `eliminated` | `LOCAL_FALLBACK_ELIMINATED_SKIP_REASON` | Sub-checks ran and ruled out every candidate — zero survivors. | **Latent — see below** | +| `ambiguous` | `LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON` | More than one candidate survived the sub-check intersection. | **Latent — see below** | + +**Latent, not live, and that is the reason they are declared.** Nothing +persists these three today. This module's own printing-vote / `CardScanLog` +write branch was retired on 2026-07-29 (redundancy doctrine — see the +module's docstring), 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 existing +`local-fallback-v1` rows in `CardScanLog` are historical. They go live the +moment anything persists this outcome — which is why they had to become +enumerable **before** that happens rather than after: the tether cannot +enumerate a literal it cannot see, so a fourth reason added inside +`run_fallback_for_card` plus one new write would have reached the column +with no lint failure anywhere. + +`local_fallback.py` is PROTECTED CORE +(`docs/upstreaming/license-provenance.md` section 2). The three declarations +sit there under a second narrow owner exception, granted 2026-07-29 and +recorded in section 2.1 alongside the `local_phash.py` one. The exception +covers skip-reason constants in that one file; the file remains protected. + +**Not the same constants as the Stage D fallback calculator's, on purpose.** +The `FALLBACK_*_SKIP_REASON` family two sections below belongs to +`stage-d-fallback-v1` in `local_calculate_verdicts.py` — a different +calculator, a different population of rows, its own vocabulary. It shares +`eliminated` and `ambiguous` verbatim with this engine (same meaning, no +rename needed) and deliberately renames this engine's `no-evidence` to +`no-sub-check-evidence` to avoid colliding with Stage D's own established +`no-evidence`. Those are parallel declarations, not mirrors of these, and +collapsing them would erase a real distinction between two row populations — +exactly the "one prefixed constant per calculator per value" shape +`no-evidence` already has five times over. The `LOCAL_FALLBACK_` prefix here +keeps the two families legible. + ## Stage D join-key calculator — `MPCAutofill/cardpicker/local_calculate_verdicts.py` `anonymous_id` is `stage-d-join-key-v1` (`JOIN_KEY_ANONYMOUS_ID`). Several @@ -184,6 +230,10 @@ reselection. `anonymous_id` is `stage-d-fallback-v1` (`STAGE_D_FALLBACK_ANONYMOUS_ID`). Border / artist / symbol sub-checks intersected against the candidate pool. +A port of the pilot engine's decision model onto stored `ImageEvidence`; its +constants are its own, **not** mirrors of the pilot's `LOCAL_FALLBACK_*` +three — see the Local-fallback pilot engine section above for why the two +families are kept separate. | Reason | Constant | Means | Status | | ----------------------- | -------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ---------------- | @@ -285,9 +335,30 @@ The owner granted a narrow exception on 2026-07-29 (recorded in `license-provenance.md` section 2, which also states its limits): the two constants are now declared in `local_phash.py` itself, the mirror in `local_identify_printing_tags.py` is removed, and the tether reports -`local_phash.py` as the declaration site. **Every roster value is now -declared where it is produced.** The exception covers skip-reason constants -in that one file only; the file remains protected. +`local_phash.py` as the declaration site. The exception covers skip-reason +constants in that one file only; the file remains protected. + +**`local_fallback.run_fallback_for_card`'s three return values — CLOSED +2026-07-29.** `no-evidence`, `eliminated` and `ambiguous` were bare inline +literals inside `MPCAutofill/cardpicker/local_fallback.py`, PROTECTED CORE +under the same section 2, for the same reason. The defect was **latent**, +not live: this module's own write branch was retired the same day and its +one non-test caller discards `skip_reason` (see the Local-fallback pilot +engine section above), so nothing reached `CardScanLog`. That made it +cheaper to close, not less necessary to — the invisibility is a property of +the literals, and it would have become a live hole the instant anything +persisted the outcome, with no lint failure to mark the moment. The owner +granted a second narrow exception on 2026-07-29 (recorded in +`license-provenance.md` section 2.1, which also states its limits, and which +is a per-change log rather than a precedent): the three constants are now +declared in `local_fallback.py` itself as +`LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON` / +`LOCAL_FALLBACK_ELIMINATED_SKIP_REASON` / +`LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON`, and the tether reports +`local_fallback.py` as their declaration site. There was no mirror to +remove: unlike the phash pair, these values were never re-declared for this +engine anywhere else, because its one caller never reads them. **Every +roster value is now declared where it is produced.** **The lands module's phash-branch composition.** That module reports `f"{LANDS_PHASH_SKIP_REASON_PREFIX}{reason}"` — a `phash-` prefix @@ -301,14 +372,18 @@ module is persisted; the constant's own comment records that a must not be added there until the composition is replaced with explicit per-outcome constants. -**Not in scope, and why.** `local_ocr.validate_against_candidates` and -`local_fallback.compute_fallback_outcome` both return skip-reason-shaped -strings as inline literals. Neither reaches `CardScanLog`: -`validate_against_candidates`' three values are consumed as control flow -and re-expressed by the caller's own declared constants, and the fallback -outcome's write branch was retired on 2026-07-29 — the `local-fallback-v1` -rows in production are historical. `local_fallback.py` is PROTECTED CORE in -any case. +**Not in scope, and why.** `local_ocr.validate_against_candidates` returns +skip-reason-shaped strings as inline literals. They do not reach +`CardScanLog`: all three are consumed as control flow and re-expressed by +the caller's own declared constants, which is a different situation from an +undeclared value flowing through to the column. + +This paragraph previously also listed `local_fallback` here, under the +function name `compute_fallback_outcome` — a name that does not exist in the +code; the function is `run_fallback_for_card`. Both the wrong name and the +"not in scope" classification are corrected: those three values are now +declared at their origin (see the entry above and the Local-fallback pilot +engine section) and are ordinary roster members, latent rather than live. ## Related diff --git a/docs/upstreaming/license-provenance.md b/docs/upstreaming/license-provenance.md index 101c1458f..bfa7d8d28 100644 --- a/docs/upstreaming/license-provenance.md +++ b/docs/upstreaming/license-provenance.md @@ -159,7 +159,9 @@ nobody can point at doesn't function as one: - `MPCAutofill/cardpicker/local_phash.py` — **still protected**; carries one authorised exception, 2026-07-29, logged in §2.1. The exception covers that one change, not the file. -- `MPCAutofill/cardpicker/local_fallback.py` +- `MPCAutofill/cardpicker/local_fallback.py` — **still protected**; carries + one authorised exception, 2026-07-29, logged in §2.1. The exception + covers that one change, not the file. - `federation-hash-tool/hash_my_cards.py` (+ its test) - `MPCAutofill/cardpicker/tests/test_federation_hash_tool_parity.py` (the parity tether between the previous two) @@ -311,6 +313,122 @@ skip-reason constants at source.** change of any shape to any file on this list, including the next lint-driven one, **needs its own ruling and its own entry below.** +**2026-07-29 — `MPCAutofill/cardpicker/local_fallback.py`: declare the +three skip-reason constants at source.** + +The second entry in this log, and it is the second entry rather than a +continuation of the first: the previous entry's own SCOPE section names +`local_fallback.py` explicitly as a file it does **not** cover. This change +was asked for and ruled on separately. + +- **What changed.** `run_fallback_for_card` set + `FallbackOutcome.skip_reason` from three bare inline string literals, + `"no-evidence"`, `"eliminated"` and `"ambiguous"`. They are now declared + as module-level `LOCAL_FALLBACK_NO_EVIDENCE_SKIP_REASON`, + `LOCAL_FALLBACK_ELIMINATED_SKIP_REASON` and + `LOCAL_FALLBACK_AMBIGUOUS_SKIP_REASON`, exported in `__all__`, and passed + by name. Two docstrings and one field comment were extended to say so. + **Nothing else in the file was touched, and no mirror was deleted because + none existed** — unlike the `local_phash` case, these values were never + re-declared in a consuming module, because this engine's one non-test + caller never reads them (see "Effect" below). The `FALLBACK_*_SKIP_REASON` + family in `local_calculate_verdicts.py` is **not** a mirror of these: it + belongs to the separate `stage-d-fallback-v1` calculator, has its own + rows, and deliberately renames this engine's `no-evidence` to + `no-sub-check-evidence`. It was checked and left alone; collapsing the two + families would erase a real distinction. The `LOCAL_FALLBACK_` prefix + exists to keep them apart, and matches this module's own + `FALLBACK_ANONYMOUS_ID = "local-fallback-v1"`. +- **Completeness — verified, not taken on trust.** The request named three + literals. The file was re-scanned with an AST pass for every `skip_reason=` + keyword argument, every `.skip_reason` attribute assignment, and every + lowercase/hyphenated string constant it contains. 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`. +- **Why.** Identical to the previous entry's reasoning, and the reasoning is + the point rather than the precedent: the `*_SKIP_REASON` declaration + convention and the roster tether `check_skip_reason_roster_tether()` + (PR #567, `docs/reference/skip-reasons.md`) derive the roster by scanning + for module-level `NAME = ""` declarations. **The tether cannot + enumerate literals it cannot see.** A fourth reason added inside + `run_fallback_for_card` would have joined three invisible siblings, and + `CardScanLog.skip_reason` — ~2.7M production rows, no `choices` list, no + foreign key — is what it would eventually reach. +- **The defect was LATENT, not live, and that is an argument for closing it + rather than against.** Nothing persists this outcome today: the module's + own printing-vote/scan-log write branch was retired by PR #560 on + 2026-07-29, and its one non-test caller, + `local_residual_classify.recover_frame_mismatch_printing_via_fallback_refetch`, + reads `outcome.printing_pk` and discards `skip_reason` entirely. So no row + reaches the column through this path right now. The invisibility, however, + 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 it is cheap, and while the change + is provably inert, is the smaller intervention in a protected file — not + the larger one. +- **Who authorised it.** The owner, ruling on 2026-07-29 on a request that + named this file, this function and these literals specifically, and that + set the same condition as the first ruling: that it hold up to audit. +- **Effect: none, and it is proved rather than asserted.** This is a + naming-only change. The string VALUES are untouched, so a `CardScanLog` + row written after it is byte-identical to one written before. Proof, in + the PR: (a) 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 they are not + executable; (b) 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; (c) + `tests/test_local_fallback.py` asserts two of the three (`eliminated`, + `no-evidence`) against `run_fallback_for_card`'s live output as raw string + literals, and was deliberately **not** edited — it is an independent, + behavioural pin that does not read the new constants at all and would fail + if a value moved. (`ambiguous` has no such behavioural test; it is pinned + statically instead, by `EXPECTED_SKIP_REASONS` and by the origin guard in + `tests/test_skip_reason_roster.py`. Stated rather than glossed: the + behavioural coverage is 2/3, and adding a third case is a test-coverage + question outside this exception's scope, not a gap in the proof — (a) and + (b) already cover all three at the source level.) There is no licensing + effect of any kind: no import was added to or removed from this file, no + external code was introduced, and the file's GPL-3.0 status and + `PROVENANCE:`-header cleanliness are unchanged. + `check_protected_core_license.py` passes, and `local_fallback.py` remains + in `PROTECTED_CORE_FILES` and in §2's list above. +- **What keeps the hole closed.** The tether alone cannot; it is blind to + both ways this can be undone. Two guards in + `MPCAutofill/cardpicker/tests/test_skip_reason_roster.py` cover them — + `test_local_fallback_skip_reasons_are_not_re_mirrored_elsewhere` (fails if + any other `cardpicker` module declares one of these three values under a + constant name not already pinned for it, and fails outright on any + `LOCAL_FALLBACK_`-prefixed declaration outside this file) and + `test_run_fallback_for_card_sets_no_bare_skip_reason_literal` (fails if + `skip_reason` is set from a bare literal rather than a named module-level + constant). A third, + `test_local_fallback_skip_reasons_are_declared_at_their_origin`, pins the + three name/value pairs to this file. The mirror guard is deliberately + narrower than the `local_phash` one: `no-evidence`, `eliminated` and + `ambiguous` are shared vocabulary that several calculators legitimately + declare under their own prefixes, so a flat "declared nowhere else" ban + would forbid the roster's own design. All three were mutation-checked + against the regression each claims to catch. +- **SCOPE OF THIS EXCEPTION — read this before citing it.** It permits + **declaring skip-reason constants in `local_fallback.py`**, and that is + its entire reach. It is specifically NOT: + - a general licence to edit `local_fallback.py` — every other function in + it (the border/artist/symbol sub-checks, the three `cast_*_vote` + helpers, `classify_bleed_edge`, `normalize_crop_box`) is untouched and + stays untouchable without a fresh ruling; + - a licence to edit any other protected-core file, including + `local_phash.py` (whose own exception is likewise spent), the four + consensus modules, the federation hash tool, or the saved-deck decrypt + tool; + - a precedent that the _pattern_ is now pre-authorised. Two rulings on two + files with the same defect do not make a third automatic. **The next + change of any shape to any file on this list needs its own ruling and + its own entry in this log** — including the next skip-reason one. + ## 3. Absorption protocol For the day the "permitted zone" (everything outside protected core) ever