diff --git a/.github/scripts/check_extractor_ownership_totality.py b/.github/scripts/check_extractor_ownership_totality.py new file mode 100644 index 000000000..2e4ef80b6 --- /dev/null +++ b/.github/scripts/check_extractor_ownership_totality.py @@ -0,0 +1,348 @@ +#!/usr/bin/env python3 +""" +CODE-TO-CODE tether: Stage C extractor *ownership* totality (issue #509's own +follow-up — "make every evidence-affecting code path version-visible"). + +`check_extractor_manifest_sync.py` proves the eleven `*_EXTRACTOR_VERSION` +constants and the cohort's resume-filter map agree. It says nothing about +the code that FEEDS those eleven extractors: `image_evidence.py`'s own +module-private helpers (`_extract_legal_line`, `_parse_artist_is_contradicted`, +`_crop_box_to_pixels`, ...) and the callables it imports from +`collector_line_artist.py`/`local_ocr.py`/`local_image_quality.py` +(`recover_artist_from_card_text`, `preprocess_variants`, ...) all help decide +what `compute_card_evidence` stores, and none of them carry a version of +their own. A change to any of them can silently change a stored field while +`MANIFEST_EXTRACTOR_CURRENT_VERSIONS` stays put, so the resume filter marks +the row "already done" and it is never re-extracted — the exact failure mode +`check_extractor_manifest_sync.py` exists to catch, one level lower, where +that script's own AST derivation cannot see it (it only reads +`*_EXTRACTOR_VERSION` constants and `extractor_versions[...]` assignments, +neither of which this code touches). + +This script is the tether for THAT layer. `EXTRACTOR_OWNERSHIP` below is the +SOURCE OF TRUTH — a hand-declared, per-entry-justified map from a +contributor's name to the `MANIFEST_EXTRACTOR_KEYS` member(s) whose stored +fields it helps determine, exactly the judgement call +docs/features/catalog-completion-plan.md's "Stage C extractor ownership" +section describes: it either gets its own `*_v1` extractor (not something +this script can decide) or it is declared here as a component of an +existing one. What THIS script derives and checks is TOTALITY: every +contributor `image_evidence.py`'s own source actually reaches from +`compute_card_evidence` must have an entry here, and every entry here must +still point at a real `MANIFEST_EXTRACTOR_KEYS` member. Neither direction +is a hand-listed set trusted on its own — the reachable-contributor set is +derived by AST from the real module, matching `check_extractor_manifest_sync.py`'s +own "never regex/hand-list, always derive" discipline. + +WHAT COUNTS AS A "CONTRIBUTOR" (the reachable set) +--------------------------------------------------- +Two kinds, both read via `ast`, never by importing/executing: + + 1. Every module-level `def _name(...)` in `image_evidence.py` itself — + its own private helpers — EXCEPT `EXCLUDED_HELPERS` (see below). + 2. Every name imported at module level from one of `SCOPED_EXTERNAL_MODULES` + that is actually CALLED somewhere in `image_evidence.py` (an `ast.Call` + whose `func` is that bare `ast.Name` — an import that is only ever used + as a type hint or a bare constant, e.g. `ArtistLexicon`/`DEFAULT_CROP_BOX`, + is not a callable code path and is correctly not required to have an + entry; see the module's own docstring for why constants are out of this + script's scope). + +DELIBERATE EXCLUSIONS, and why they are not silent +---------------------------------------------------- +`EXCLUDED_HELPERS = {"_collector_line_ocr_attempts"}` — the OCR +attempt-tier ladder (issue #259). Named out of scope by this PR's own brief +("being worked in parallel by other branches"); touching its ownership here +would either fight that branch's own edits or require this script to freeze +a design that branch is actively changing. Its own body is excluded from +the call-scan too (not just the function name itself), so a name ONLY +called from inside it (`preprocess_fallback_variants`, at this writing) is +correctly not required to have an entry either — it is reachable exclusively +through the excluded ladder, and re-included in full the day that ladder's +own PR lands and this exclusion is lifted. + +`local_fallback.py`'s own exported helpers (`classify_bleed_edge`, +`classify_border_color`, `classify_frame_style`, `compute_bleed_diff_mm`, +`normalize_crop_box`, `extract_artist_name`) are out of `SCOPED_EXTERNAL_MODULES` +entirely, same reason and same brief citation — that whole file is being +worked on in parallel. They are also PROTECTED CORE +(`docs/upstreaming/license-provenance.md` §2), each already the direct, +named mechanism of an existing versioned extractor +(`geometry_bleed`/`layout_class`/`artbox_phash`), so their omission here is +not a coverage gap for THIS PR's own inventory — it is a decision to let the +parallel branch own that declaration when it lands. + +WHY NOT A REGEX/HAND-LIST OVER CALL SITES +------------------------------------------- +Same reasoning `check_extractor_manifest_sync.py`'s own module docstring +gives: a hand-listed reachable set is the exact invisible-drift problem one +level up from what this script exists to close. AST derivation means a +newly-added module-private helper, or a newly-called import, is caught the +moment it is written — not the next time someone remembers to update a list. + +Exit code is the number of findings (0 = clean), matching +`check_extractor_manifest_sync.py`'s own convention. +""" + +import ast +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] + +SOURCE_REL = "MPCAutofill/cardpicker/image_evidence.py" + +# The OCR attempt-tier ladder (issue #259) — out of scope for this PR, being +# worked on in parallel. See module docstring's "DELIBERATE EXCLUSIONS". +EXCLUDED_HELPERS = frozenset({"_collector_line_ocr_attempts"}) + +# Modules whose imported-and-called names are in this script's scope. +# `cardpicker.local_fallback` and `cardpicker.local_phash` are deliberately +# NOT here — see module docstring. +SCOPED_EXTERNAL_MODULES = frozenset( + { + "cardpicker.collector_line_artist", + "cardpicker.local_ocr", + "cardpicker.local_image_quality", + } +) + +# THE OWNERSHIP MAP — hand-declared, per-entry judgement call (see module +# docstring). {contributor name: frozenset of MANIFEST_EXTRACTOR_KEYS +# members whose stored field(s) it helps determine}. A contributor that +# feeds more than one key (e.g. the artist-contradiction gate governs +# whether `collector_line_ocr`'s own escalation loop keeps running, which +# also changes what `collector_line_tsv`'s word boxes and `artist_ocr`'s +# raw-text reuse see) is declared under every key it actually reaches — the +# convention this enforces is "bump every listed key together", not "pick +# one owner and hope the others notice". +EXTRACTOR_OWNERSHIP: dict = { + # --- image_evidence.py's own private helpers --- + # Shared crop-box-to-pixel remap (normalize_crop_box + scale) behind + # every `*_crop_px` field except collector/artist (those two are + # computed inline in the crop_coordinates block itself, not through + # this helper). + "_crop_box_to_pixels": frozenset({"crop_coordinates", "artbox_phash", "symbol_region", "legal_line"}), + # Shared crop-then-phash helper behind both region-hash extractors. + "_compute_region_phash": frozenset({"artbox_phash", "symbol_region"}), + # STAGE_C_NO_SHORTCIRCUIT env resolution — controls whether the + # pre-classification short-circuit fires, which governs how many OCR + # attempts run and therefore what collector_line_ocr/collector_line_tsv + # store and what raw texts are available for artist_ocr's reuse pass. + "_short_circuit_enabled_by_env": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # The short-circuit gate's own digit-scan primitive — same reach as + # `_confidently_digit_free`, which is built entirely on top of it. + "_contains_digit": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # SET-CODE LEXICON GATE (issue #370): gates the escalation loop's + # acceptance criterion for collector_line_ocr's own selected parse, + # which is what collector_line_tsv's word boxes are selected against + # and what artist_ocr's collector-raw-text reuse pass sees. + "_parse_is_lexicon_valid": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # legal_line's own compute, hoisted ahead of the OCR group but still + # exclusively legal_line's own fields (legal_line_raw_text/ + # legal_line_copyright_year/legal_line_proxy_marker_detected/ + # legal_line_crop_px). Its OUTPUT is consumed elsewhere (the artist + # gate, the artist_ocr_name recovery fallback) as an already-computed + # value, not by this function itself changing behavior for those keys. + "_extract_legal_line": frozenset({"legal_line"}), + # COLLECTOR-LINE ARTIST GATE (2026-07-29): same reach as the lexicon + # gate above, for the same reason (governs escalation continuation). + "_parse_artist_is_contradicted": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # Pre-classification short-circuit's own acceptance predicate — same + # reach as `_short_circuit_enabled_by_env`/`_contains_digit` above. + "_confidently_digit_free": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # --- cardpicker.collector_line_artist --- + # Called from both `_parse_artist_is_contradicted` (gates + # collector_line_ocr/collector_line_tsv/artist_ocr's raw-text-reuse + # reach, as above) AND directly as the `artist_ocr_name` storage + # fallback when the "Illus." anchor found nothing — the second call + # site is artist_ocr's own field, already covered by the first site's + # broader set. + "recover_artist_from_card_text": frozenset({"collector_line_ocr", "collector_line_tsv", "artist_ocr"}), + # --- cardpicker.local_image_quality --- (each wholly owned by + # quality_signals; no other extractor calls any of these) + "compute_blur_variance": frozenset({"quality_signals"}), + "compute_entropy": frozenset({"quality_signals"}), + "is_image_truncated": frozenset({"quality_signals"}), + # --- cardpicker.local_ocr --- + # Determines collector_line_set_code/collector_line_collector_number + # (collector_line_ocr) and which winning variant's word boxes get + # stored (collector_line_tsv) - called both inside the OCR loop and in + # the no-attempts-parsed fallback. + "parse_collector_line": frozenset({"collector_line_ocr", "collector_line_tsv"}), + # legal_line's own tolerant parse - called only from `_extract_legal_line`. + "parse_legal_line": frozenset({"legal_line"}), + # Called directly (outside the excluded tier ladder) from + # `_extract_legal_line` and the artist_ocr crop+OCR fallback loop - + # NOT from collector_line_ocr's own tier-1 attempts, which live inside + # the excluded `_collector_line_ocr_attempts` generator (see module + # docstring's "DELIBERATE EXCLUSIONS" - that reach is this contributor's + # too, but is out of scope for this PR and left to the parallel branch). + "preprocess_variants": frozenset({"legal_line", "artist_ocr"}), + # Same two in-scope call sites as `preprocess_variants` above (legal_line's + # OCR pass, artist_ocr's crop+OCR fallback) - not the excluded ladder. + "run_tesseract": frozenset({"legal_line", "artist_ocr"}), + # Called directly inside compute_card_evidence's own OCR loop (the + # `_collector_line_ocr_attempts` generator only yields preprocessed + # variants + config + tier; the actual tesseract call, and therefore + # this function's own reach, is in the loop body, outside the excluded + # generator's subtree). + "run_tesseract_text_and_words": frozenset({"collector_line_ocr", "collector_line_tsv"}), +} + + +def _parse(rel: str): + path = REPO_ROOT / rel + if not path.is_file(): + return None + return ast.parse(path.read_text(), filename=str(path)) + + +def _module_private_helper_names(tree: ast.Module) -> set: + """Every top-level `def _name(...)` in the module, minus EXCLUDED_HELPERS.""" + return { + node.name + for node in tree.body + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) + and node.name.startswith("_") + and node.name not in EXCLUDED_HELPERS + } + + +def _excluded_node_ids(tree: ast.Module) -> set: + """Object ids of every AST node inside an EXCLUDED_HELPERS function body. + + Same node OBJECTS as the whole-module walk (this is one parse, not two), + so `id()` equality correctly identifies "found while walking the excluded + subtree" versus "found elsewhere in the module". + """ + excluded = set() + for node in tree.body: + if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)) and node.name in EXCLUDED_HELPERS: + excluded.update(id(n) for n in ast.walk(node)) + return excluded + + +def _scoped_external_imports(tree: ast.Module) -> set: + """Names imported at module level from SCOPED_EXTERNAL_MODULES.""" + names = set() + for node in tree.body: + if isinstance(node, ast.ImportFrom) and node.module in SCOPED_EXTERNAL_MODULES: + names.update(alias.asname or alias.name for alias in node.names) + return names + + +def _called_names(tree: ast.Module, candidates: set, skip_node_ids: set) -> set: + """Subset of `candidates` that appear as a bare-name `ast.Call` func, + outside of `skip_node_ids` (the excluded helper's own subtree).""" + called = set() + for node in ast.walk(tree): + if id(node) in skip_node_ids: + continue + if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id in candidates: + called.add(node.func.id) + return called + + +def derive_reachable_contributors() -> tuple: + """ + DERIVE the set of contributor names this script requires an + EXTRACTOR_OWNERSHIP entry for, from `image_evidence.py` itself. + + Returns (contributors, findings). Findings are non-empty only when the + derivation itself fails - same anti-vacuous-pass discipline + `check_extractor_manifest_sync.py`'s own `derive_expected_manifest` + uses: a derivation that silently finds nothing would compare an empty + set to an empty allowance and pass forever. + """ + tree = _parse(SOURCE_REL) + if tree is None: + return set(), [f"::error::check_extractor_ownership_totality.py: {SOURCE_REL} not found"] + + helpers = _module_private_helper_names(tree) + skip_ids = _excluded_node_ids(tree) + external_imports = _scoped_external_imports(tree) + called_externals = _called_names(tree, external_imports, skip_ids) + + contributors = helpers | called_externals + + findings = [] + if not contributors: + findings.append( + f"::error file={SOURCE_REL}::derived zero reachable contributors (module-private " + f"helpers + called scoped-external imports). This tether derives its expectation " + f"from the real module; finding none means it would compare an empty set forever, " + f"so the empty derivation is itself the finding." + ) + return contributors, findings + + +def check() -> list: + contributors, findings = derive_reachable_contributors() + if findings: + return findings + + declared = set(EXTRACTOR_OWNERSHIP) + + for name in sorted(contributors - declared): + findings.append( + f"::error file={SOURCE_REL}::`{name}` is reachable from `compute_card_evidence`'s " + f"own call graph (a module-private helper, or a scoped-external import it calls) " + f"but has no entry in EXTRACTOR_OWNERSHIP " + f"(.github/scripts/check_extractor_ownership_totality.py). A code path that helps " + f"determine a stored ImageEvidence field must be either its own versioned extractor " + f"or declared here as a component of an existing one - see this script's own module " + f"docstring." + ) + + for name in sorted(declared - contributors): + findings.append( + f"::error file={SOURCE_REL}::EXTRACTOR_OWNERSHIP declares `{name}` " + f"(.github/scripts/check_extractor_ownership_totality.py) but it is not reachable " + f"from `compute_card_evidence`'s own call graph in {SOURCE_REL} - a stale entry, " + f"most likely a rename or removal that was not reflected here." + ) + + # Cross-check: every declared owning key must be a real manifest key - + # imported rather than re-derived, so the two scripts can never + # disagree about what a "real" MANIFEST_EXTRACTOR_KEYS member is. + import sys as _sys + + _sys.path.insert(0, str(Path(__file__).resolve().parent)) + import check_extractor_manifest_sync as manifest_sync + + manifest_sync.REPO_ROOT = REPO_ROOT + expected_manifest, manifest_findings = manifest_sync.derive_expected_manifest() + if manifest_findings: + findings.extend(manifest_findings) + return findings + real_keys = set(expected_manifest) + + for name, owning_keys in sorted(EXTRACTOR_OWNERSHIP.items()): + for key in sorted(owning_keys - real_keys): + findings.append( + f"::error file=.github/scripts/check_extractor_ownership_totality.py::" + f"EXTRACTOR_OWNERSHIP[{name!r}] names `{key}`, which is not a real manifest key " + f"derived from {SOURCE_REL} ({sorted(real_keys)}). A stale or mistyped owning " + f"key silently exempts this contributor from ever being tied to a real extractor " + f"version." + ) + + return findings + + +def main() -> int: + findings = check() + for finding in findings: + print(finding) + + if findings: + print(f"\n{len(findings)} extractor-ownership-totality finding(s).") + else: + contributors, _ = derive_reachable_contributors() + print(f"extractor-ownership-totality: clean ({len(contributors)} declared contributors).") + + return len(findings) + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/scripts/tests/test_check_extractor_ownership_totality.py b/.github/scripts/tests/test_check_extractor_ownership_totality.py new file mode 100644 index 000000000..c74bb0f4e --- /dev/null +++ b/.github/scripts/tests/test_check_extractor_ownership_totality.py @@ -0,0 +1,259 @@ +""" +Unit + real-repo tests for check_extractor_ownership_totality.py — the +code-to-code tether between `image_evidence.py`'s reachable-from- +`compute_card_evidence` call graph and the hand-declared `EXTRACTOR_OWNERSHIP` +map. + +Conventions follow test_check_extractor_manifest_sync.py: fixture tests +build a miniature `image_evidence.py` under a redirected REPO_ROOT and +exercise each rule's passing AND failing case; real-repo tests assert the +committed tree is clean and, separately, that the derivation still sees the +real contributors — the guard against a tether that passes because it +compares nothing to nothing. + +Run: python3 .github/scripts/tests/test_check_extractor_ownership_totality.py +""" + +import contextlib +import sys +import tempfile +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +REPO_ROOT = SCRIPTS_DIR.parents[1] +sys.path.insert(0, str(SCRIPTS_DIR)) + +import check_extractor_manifest_sync as manifest_sync # noqa: E402 +import check_extractor_ownership_totality as lint # noqa: E402 + +# A minimal fixture `image_evidence.py`: one module-private helper +# (`_helper_one`) called from `compute_card_evidence`, one scoped-external +# import (`external_thing`, standing in for e.g. `recover_artist_from_card_text`) +# also called from there, one excluded-ladder helper (`_collector_line_ocr_attempts`) +# whose own internal call (`preprocess_fallback_variants`-analogue, +# `ladder_only_thing`) must NOT be required, and the manifest wiring +# `check_extractor_manifest_sync`'s own derivation needs to find real keys. +SOURCE_OK = ''' +"""fixture image_evidence""" +from cardpicker.collector_line_artist import external_thing +from cardpicker.local_ocr import ladder_only_thing + +FETCH_HEALTH_EXTRACTOR_VERSION = "fetch-health-v2" +LEGAL_LINE_EXTRACTOR_VERSION = "legal-line-v1" + + +def _helper_one(x): + return x + + +def _collector_line_ocr_attempts(cropped): + yield ladder_only_thing(cropped) + + +def compute_card_evidence(card): + extractor_versions: dict[str, str] = {} + extractor_versions["fetch_health"] = FETCH_HEALTH_EXTRACTOR_VERSION + extractor_versions["legal_line"] = LEGAL_LINE_EXTRACTOR_VERSION + _helper_one(card) + external_thing(card) + for _ in _collector_line_ocr_attempts(card): + pass + return extractor_versions +''' + +COHORT_OK = """ +MANIFEST_EXTRACTOR_KEYS = frozenset({"fetch_health", "legal_line"}) +MANIFEST_EXTRACTOR_CURRENT_VERSIONS: dict[str, str] = { + "fetch_health": "fetch-health-v2", + "legal_line": "legal-line-v1", +} +""" + +OWNERSHIP_OK = { + "_helper_one": frozenset({"fetch_health"}), + "external_thing": frozenset({"legal_line"}), +} + + +@contextlib.contextmanager +def fixture_repo(source: str = SOURCE_OK, cohort: str = COHORT_OK, ownership: dict = None): + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + for rel, text in ((lint.SOURCE_REL, source), (manifest_sync.COHORT_REL, cohort)): + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + saved_lint_root = lint.REPO_ROOT + saved_manifest_root = manifest_sync.REPO_ROOT + saved_ownership = lint.EXTRACTOR_OWNERSHIP + lint.REPO_ROOT = root + manifest_sync.REPO_ROOT = root + lint.EXTRACTOR_OWNERSHIP = OWNERSHIP_OK if ownership is None else ownership + try: + yield root + finally: + lint.REPO_ROOT = saved_lint_root + manifest_sync.REPO_ROOT = saved_manifest_root + lint.EXTRACTOR_OWNERSHIP = saved_ownership + + +def joined(findings) -> str: + return " || ".join(findings) + + +class TestDerivation(unittest.TestCase): + def test_derives_private_helper_and_called_external_import(self): + with fixture_repo(): + contributors, findings = lint.derive_reachable_contributors() + self.assertEqual(findings, []) + self.assertEqual(contributors, {"_helper_one", "external_thing"}) + + def test_excluded_ladder_function_itself_is_not_a_contributor(self): + # _collector_line_ocr_attempts is in EXCLUDED_HELPERS by name - it + # must never itself require an entry. + with fixture_repo(): + contributors, _ = lint.derive_reachable_contributors() + self.assertNotIn("_collector_line_ocr_attempts", contributors) + + def test_name_called_only_inside_the_excluded_ladder_is_not_a_contributor(self): + # ladder_only_thing is a scoped-external import, but its one call + # site is inside _collector_line_ocr_attempts's own body - excluded + # per this PR's own brief (that ladder is being worked on in + # parallel by another branch). + with fixture_repo(): + contributors, _ = lint.derive_reachable_contributors() + self.assertNotIn("ladder_only_thing", contributors) + + def test_import_used_only_as_a_type_hint_is_not_a_contributor(self): + source = SOURCE_OK.replace( + "from cardpicker.collector_line_artist import external_thing", + "from cardpicker.collector_line_artist import ArtistLexicon, external_thing", + ).replace( + "def compute_card_evidence(card):", + 'def compute_card_evidence(card, lexicon: "ArtistLexicon" = None):', + ) + with fixture_repo(source=source): + contributors, _ = lint.derive_reachable_contributors() + self.assertNotIn("ArtistLexicon", contributors) + + def test_empty_derivation_is_a_finding_not_a_pass(self): + with fixture_repo(source='"""no helpers, no calls"""\n'): + contributors, findings = lint.derive_reachable_contributors() + self.assertEqual(contributors, set()) + self.assertEqual(len(findings), 1) + self.assertIn("empty derivation is itself the finding", joined(findings)) + + def test_missing_source_file_is_a_finding(self): + with fixture_repo() as root: + (root / lint.SOURCE_REL).unlink() + _, findings = lint.derive_reachable_contributors() + self.assertIn("not found", joined(findings)) + + +class TestTotality(unittest.TestCase): + def test_fully_declared_is_clean(self): + with fixture_repo(): + self.assertEqual(lint.check(), []) + + def test_undeclared_new_contributor_fails(self): + # THE case the brief requires: a newly-added, newly-called + # module-private helper with no EXTRACTOR_OWNERSHIP entry must fail + # CI, not merge silently invisible the way the eleven + # *_EXTRACTOR_VERSION constants used to be able to. + source = SOURCE_OK.replace( + "def _helper_one(x):\n return x\n", + "def _helper_one(x):\n return x\n\n\ndef _helper_two_undeclared(x):\n return x\n", + ).replace( + " _helper_one(card)\n", + " _helper_one(card)\n _helper_two_undeclared(card)\n", + ) + with fixture_repo(source=source): + out = joined(lint.check()) + self.assertIn("_helper_two_undeclared", out) + self.assertIn("no entry in EXTRACTOR_OWNERSHIP", out) + + def test_undeclared_new_external_call_fails(self): + source = SOURCE_OK.replace( + "from cardpicker.local_ocr import ladder_only_thing", + "from cardpicker.local_ocr import ladder_only_thing, undeclared_external", + ).replace( + " external_thing(card)\n", + " external_thing(card)\n undeclared_external(card)\n", + ) + with fixture_repo(source=source): + out = joined(lint.check()) + self.assertIn("undeclared_external", out) + self.assertIn("no entry in EXTRACTOR_OWNERSHIP", out) + + def test_stale_ownership_entry_for_a_removed_contributor_fails(self): + ownership = dict(OWNERSHIP_OK) + ownership["_removed_long_ago"] = frozenset({"fetch_health"}) + with fixture_repo(ownership=ownership): + out = joined(lint.check()) + self.assertIn("_removed_long_ago", out) + self.assertIn("not reachable", out) + + def test_ownership_entry_naming_a_fake_manifest_key_fails(self): + ownership = dict(OWNERSHIP_OK) + ownership["_helper_one"] = frozenset({"not_a_real_key"}) + with fixture_repo(ownership=ownership): + out = joined(lint.check()) + self.assertIn("not_a_real_key", out) + self.assertIn("not a real manifest key", out) + + def test_manifest_derivation_failure_propagates(self): + # If the sibling manifest-sync script's own derivation fails (e.g. + # no `extractor_versions[...]` assignments in image_evidence.py), + # that failure must surface here too rather than being swallowed - + # the owning-key cross-check below it must not run against an + # empty/unreliable manifest. + source = SOURCE_OK.replace( + ' extractor_versions["fetch_health"] = FETCH_HEALTH_EXTRACTOR_VERSION\n' + ' extractor_versions["legal_line"] = LEGAL_LINE_EXTRACTOR_VERSION\n', + "", + ) + with fixture_repo(source=source): + out = joined(lint.check()) + self.assertIn("no `extractor_versions", out) + + +class TestAgainstRealRepo(unittest.TestCase): + def test_real_repo_is_in_sync(self): + self.assertEqual(lint.check(), [], "extractor ownership map is out of sync") + + def test_derivation_sees_the_real_contributors(self): + contributors, findings = lint.derive_reachable_contributors() + self.assertEqual(findings, []) + for name in ( + "_crop_box_to_pixels", + "_compute_region_phash", + "_extract_legal_line", + "_parse_artist_is_contradicted", + "_parse_is_lexicon_valid", + "_confidently_digit_free", + "recover_artist_from_card_text", + "compute_blur_variance", + "compute_entropy", + "is_image_truncated", + "parse_collector_line", + "parse_legal_line", + "run_tesseract_text_and_words", + ): + self.assertIn(name, contributors) + + def test_excluded_ladder_is_really_excluded_in_the_real_module(self): + contributors, _ = lint.derive_reachable_contributors() + self.assertNotIn("_collector_line_ocr_attempts", contributors) + self.assertNotIn("preprocess_fallback_variants", contributors) + + def test_every_declared_owning_key_set_is_non_empty(self): + for name, keys in lint.EXTRACTOR_OWNERSHIP.items(): + self.assertTrue(keys, f"{name} has an empty owning-key set") + + def test_main_exits_zero(self): + self.assertEqual(lint.main(), 0) + + +if __name__ == "__main__": + unittest.main() diff --git a/.github/workflows/docs-lint.yml b/.github/workflows/docs-lint.yml index 446349a17..1de77bc9a 100644 --- a/.github/workflows/docs-lint.yml +++ b/.github/workflows/docs-lint.yml @@ -187,3 +187,25 @@ jobs: run: python3 .github/scripts/tests/test_check_extractor_manifest_sync.py - name: Run extractor-manifest tether against the real repo run: python3 .github/scripts/check_extractor_manifest_sync.py + + extractor-ownership-totality: + name: Extractor ownership totality (image_evidence.py's own call graph -> EXTRACTOR_OWNERSHIP) + # CODE-TO-CODE tether, one layer below extractor-manifest-sync above: the + # eleven `*_EXTRACTOR_VERSION` constants only cover code that bumps a + # KEY's own version. Module-private helpers in image_evidence.py, and + # callables it imports from collector_line_artist.py/local_ocr.py/ + # local_image_quality.py, help decide what compute_card_evidence stores + # but carry no version of their own - a change to any of them can + # silently change a stored field while MANIFEST_EXTRACTOR_CURRENT_VERSIONS + # stays put. AST-based, stdlib only - reads source text, never imports or + # executes it, so no Django/third-party deps in this job. Imports + # check_extractor_manifest_sync.py directly for its owning-key cross-check + # (see that script's own module docstring), so this job also needs that + # sibling script present. + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v6 + - name: Run extractor-ownership-totality tether unit tests + run: python3 .github/scripts/tests/test_check_extractor_ownership_totality.py + - name: Run extractor-ownership-totality tether against the real repo + run: python3 .github/scripts/check_extractor_ownership_totality.py diff --git a/MPCAutofill/cardpicker/image_evidence.py b/MPCAutofill/cardpicker/image_evidence.py index ec3d223dc..b76704d73 100644 --- a/MPCAutofill/cardpicker/image_evidence.py +++ b/MPCAutofill/cardpicker/image_evidence.py @@ -231,6 +231,16 @@ groups may only narrow candidates (small-distance), never auto-vote, except at `d=0` for verified- identical uploads. This extractor emits the raw signal only - no consumer of any kind is built or wired in this PR. + +EXTRACTOR OWNERSHIP (2026-08-04, docs/features/catalog-completion-plan.md's "Stage C extractor +ownership" section has the full rule and rationale - this is a pointer, not a restatement): every +module-private helper in this file and every externally-imported callable it actually calls from +`collector_line_artist.py`/`local_ocr.py`/`local_image_quality.py` is either its own versioned +extractor or declared as a component of an existing one in `EXTRACTOR_OWNERSHIP` +(`.github/scripts/check_extractor_ownership_totality.py`), which is checked for totality against +this module's own call graph in CI, the same way `check_extractor_manifest_sync.py` already checks +the eleven `*_EXTRACTOR_VERSION` constants themselves. A new module-private helper, or a newly- +called import, needs an `EXTRACTOR_OWNERSHIP` entry the moment it is written. """ import logging diff --git a/docs/features/catalog-completion-plan.md b/docs/features/catalog-completion-plan.md index 3de58df92..2726060c0 100644 --- a/docs/features/catalog-completion-plan.md +++ b/docs/features/catalog-completion-plan.md @@ -1841,6 +1841,74 @@ as `ARTBOX_OLD_CROP_BOX`'s own crop-fraction confidence. **Border-color `CardTagVote` cast inline during Stage C — `extraction-soundness-batch` (2026-07-27)**: `extract_card_evidence` in `image_evidence.py` now casts a `CardTagVote` for border color immediately after `compute_card_evidence` returns, using the `layout_class` field already extracted by the `layout_class` extractor block. `cast_border_attribute_vote(card, layout_class, confidence=0.5)` (existing function in `local_fallback.py`) is called with machine weight 0.5 (vs the 0.75 default used in the write loop's own post-import re-vote path). The empty-string ambiguous sentinel (`layout_class == ""`) is coerced to `None` before the call so no vote fires for ambiguous classifications. `compute_card_evidence` itself is unchanged and remains DB-free (required for `ProcessPoolExecutor` workers); the vote cast is the only DB write in `extract_card_evidence`. Test: `TestExtractCardEvidenceBorderColorVote` in `test_image_evidence.py` asserts that the extraction count equals the vote count for a non-ambiguous border color, that no vote is cast for an ambiguous classification, and that no vote is cast when `layout_class` is absent from the result. +**Stage C extractor ownership — visibility + CI totality (issue #509's own follow-up, 2026-08-04)**: +the eleven `*_EXTRACTOR_VERSION` constants only cover code that changed a KEY's version when it +changed the KEY's own logic. A meaningful amount of what `compute_card_evidence` actually stores +lives in module-private helpers (`_extract_legal_line`, `_parse_artist_is_contradicted`, +`_parse_is_lexicon_valid`, `_crop_box_to_pixels`, ...) and in callables imported from +`collector_line_artist.py`/`local_ocr.py`/`local_image_quality.py` +(`recover_artist_from_card_text`, `preprocess_variants`, `run_tesseract_text_and_words`, ...) — +none of which carry a version of their own. The collector-line artist gate +(`_parse_artist_is_contradicted`/`recover_artist_from_card_text`, 2026-07-29) is the sharpest +example already on record: its own docstring states plainly that landing it bumped no version, +"deliberately," because a bump there would have forced a ~220k-card re-extraction that wasn't +being scheduled at the time. That is a reasoned decision about WHEN to re-extract, not a decision +that the code path is invisible — but nothing before this entry made the two distinguishable to a +future reader. + +THE RULE: every code path that helps determine a stored `ImageEvidence` field takes one of two +shapes, and the choice is a per-entry judgement call made explicitly, never left implicit. + +1. It is genuinely its own extractor — a new `*_EXTRACTOR_VERSION` constant, a new + `MANIFEST_EXTRACTOR_KEYS` member, `MANIFEST_EXTRACTOR_CURRENT_VERSIONS` entry, and (per + `golden_set.py`'s own task #145 gate) `GOLDEN_EXPECTATIONS` coverage for the value it produces. +2. It is a component of an existing extractor — declared in + `EXTRACTOR_OWNERSHIP` in `.github/scripts/check_extractor_ownership_totality.py`, naming every + `MANIFEST_EXTRACTOR_KEYS` member whose stored field it actually helps determine (more than one, + when it does — the artist gate above governs `collector_line_ocr`'s own escalation loop, which + is also what `collector_line_tsv`'s word boxes are selected against and what `artist_ocr`'s + raw-text reuse pass sees, so it is declared under all three). Declaring a component here does + NOT bump any version by itself; it is a visibility act. Whether/when to actually bump the + version(s) it names, once its own behaviour changes, is still the separate per-extractor + decision task #145's "one PR per extractor, golden-set-tested before merge" gate has always + required — this just makes the decision impossible to skip past unnoticed. + +WHAT KEEPS THE TWO ROSTERS IN SYNC: both are CI-enforced by AST derivation, never by a hand-list +trusted to stay current, matching this file's own repeated "the pipeline's own existing strings +verbatim, not a separately-invented vocabulary" discipline applied one level up, to CODE rather +than STRINGS. `.github/scripts/check_extractor_manifest_sync.py` (pre-existing, issue #509) ties +the eleven version constants to the cohort driver's resume-filter map. Its new sibling, +`.github/scripts/check_extractor_ownership_totality.py`, derives the set of module-private helpers +`image_evidence.py` defines plus every externally-imported callable it actually calls, and fails CI +if that derived set and `EXTRACTOR_OWNERSHIP`'s declared keys disagree in either direction — an +undeclared new contributor, or a stale entry for one that was renamed/removed. It also cross-checks +that every key an ownership entry names is a real, live `MANIFEST_EXTRACTOR_KEYS` member (imported +from `check_extractor_manifest_sync.py` directly, so the two scripts can never independently +disagree about what counts as "real"). + +TWO EXPLICIT EXCLUSIONS from this sweep, both stated in the totality script's own module docstring +rather than left to be discovered by a confused future reader: `local_fallback.py`'s own exported +helpers (`classify_bleed_edge`, `classify_border_color`, `classify_frame_style`, +`compute_bleed_diff_mm`, `normalize_crop_box`, `extract_artist_name`) and the OCR attempt-tier +ladder (`_collector_line_ocr_attempts`, issue #259) — both being worked on in parallel by other +branches at the time this sweep landed. Each is already the direct, named mechanism of an existing +versioned extractor (`geometry_bleed`/`layout_class`/`artbox_phash`/`collector_line_ocr`), so their +omission is a sequencing decision, not a coverage gap this sweep itself leaves open — lifting each +exclusion is one entry once its own parallel branch lands. + +CORRECTION TO THE ORIGINATING BRIEF: `local_art_edge.classify_art_edge_continuity` (issue #617's +own "Extended" art-edge continuity classifier) was named as a candidate for this sweep but is NOT +actually reachable from `compute_card_evidence` today — `local_art_edge.py` is a declared-but- +not-yet-live Stage D CALCULATOR (it reads `ImageEvidence.art_crop_px`, already-persisted, rather +than being called from within Stage C's own extraction pass), confirmed by grep against the whole +`cardpicker` package finding zero call sites outside its own tests. It is out of this sweep's scope +for the same reason `local_fallback.py`/the OCR ladder are — nothing currently calls it from +`compute_card_evidence` for the ownership map to declare. + +No stored `ImageEvidence` value changes as a result of this entry: all seventeen sweep-covered +contributors were already feeding their currently-versioned extractor(s) before this landed; the +work is declaration and CI enforcement only, not a version bump or a re-extraction trigger. + **Stage C bulk driver: compute profile + concurrency/OCR-cost fix (2026-07-20)** — `docs/reports/2026-07-20-pipeline-compute-profile.md` measured the bulk cohort driver (`run_image_evidence_cohort.py`, previously landed on an unmerged worktree branch only, ported to