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