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

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",

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 = "<literal>"`
+# 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",

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"

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.