Skip to content

Stage D: gate the frame agreement veto on artist_ocr having actually run - #656

Merged
WilfordGrimley merged 1 commit into
masterfrom
fix/stage-d-required-extractor-keys
Jul 30, 2026
Merged

Stage D: gate the frame agreement veto on artist_ocr having actually run#656
WilfordGrimley merged 1 commit into
masterfrom
fix/stage-d-required-extractor-keys

Conversation

@WilfordGrimley

Copy link
Copy Markdown

Item 4 of the "essential major blockers" brief, narrowed per its own instruction: "If the full sweep is too large, fix the artist_ocr → frame-veto path alone — that is the one producing permanent wrong answers."

The defect

run_join_key_calculator filters eligibility on extractor_versions__has_key="collector_line_ocr" and then reads six extractors' fields ungated: layout_class, illus_anchor_fired, legal_line_*, artist_ocr_name, symbol_phash, image_is_truncated.

Five degrade permissively — a blank legal line reads as "nothing to compare", a null image_is_truncated reads as "not truncated". A permissive degradation is recoverable, because the human-backed consensus gate still stands between it and any resolution.

The frame agreement veto degrades STRICT, and that is why it is the one fixed. ImageEvidence.illus_anchor_fired is nullable, so bool(None) is False — indistinguishable from "artist_ocr ran and found no anchor". On a card with no collector number either, classify_frame_style returns "modern" for a card it has no anchor evidence about at all, and a genuine old-frame printing is vetoed frame-mismatch.

That reason is deliberately not in JOIN_KEY_RESCANNABLE_SKIP_REASONS, so the wrong conclusion is permanent for that content hash: the card never becomes eligible again, and no later Stage C pass can undo it. A wrong answer nothing can revisit is strictly worse than a missing one.

The fix

FRAME_CHECK_REQUIRED_EXTRACTOR_KEYS = frozenset({"collector_line_ocr", "artist_ocr"}) — both extractors classify_frame_style actually reads, since its two arguments come from two different ones and the eligibility query only ever guaranteed the first.

An absent key skips the check, not the card. The match stands at its already-computed confidence, and once Stage C fills artist_ocr in, the check runs for real. This is not a new rule — it is the "missing data is not evidence" rule the function's own docstring already states, and which the copyright-year check and the metadata is None case already follow.

Declared per-CHECK, not per-calculator. local_detect_ai_art, local_lands_identify and local_layout_class_cast each declare one module-level REQUIRED_EXTRACTOR_KEYS because each runs one check. This calculator runs several with genuinely different needs — the join-key deduction itself needs only the collector line — so a calculator-wide gate would drop cards that have everything their own decision requires.

artist_ocr is at 220,579/220,579 coverage in production, so this changes nothing about the current catalogue. It removes a trap; it does not loosen a live gate.

Not in scope

The other five ungated reads are left as-is and named in the code comment. They degrade permissively, which is recoverable, and closing them is a coverage/behaviour tradeoff worth measuring on its own rather than folding into a permanent-wrong-answer fix.

Verification

mutation expected red result
remove the gate (restore the ungated read) old-frame card wrongly vetoed 1 failed, 179 passed
shrink the gate to collector_line_ocr alone the defect returns + the constant tripwire 2 failed, 178 passed
over-apply — skip the frame check unconditionally pre-existing test_frame_mismatch_withholds_the_match + the new control 2 failed, 178 passed
restored 180 passed

The third mutation is the important one: it proves the fix is not a licence to drop the veto. The control test shows the same card, same evidence, one extra manifest key is still vetoed.

  • pytest cardpicker/tests/ — 3464 passed, 11 skipped (full suite)
  • docs_lint.py --strict — clean
  • pre-commit (ruff/isort/black/mypy/prettier) — clean
  • No migration.

The shared _evidence test fixture now carries both OCR extractors by default, matching production. Without that, every existing frame-veto test would have become a silent no-op under the new gate — worth flagging explicitly, since a fixture that quietly disarms the tests around it is the same class of problem as the ungated read itself.

Docs

docs/identification-pipeline.md g4 — new "A check whose extractor never ran does not run" paragraph, stating the permissive-vs-strict distinction and why only one of the six needed gating (living wiki doc; no dated report).

🤖 Generated with Claude Code

https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN

`run_join_key_calculator` filters eligibility on
`extractor_versions__has_key="collector_line_ocr"` and then reads SIX
extractors' fields ungated (2026-07-29 composition audit, section 5). Five of
those degrade PERMISSIVELY when their extractor never ran - a blank legal line
reads as "nothing to compare", a null `image_is_truncated` reads as "not
truncated" - and a permissive degradation is recoverable, because the
human-backed consensus gate still stands between it and any resolution.

The frame agreement veto is the exception, and it is the one this closes.
`ImageEvidence.illus_anchor_fired` is NULLABLE, so `bool(None)` is False -
indistinguishable from "artist_ocr ran and found no anchor". On a card with no
collector number either, `classify_frame_style` then returns "modern" for a
card it has no anchor evidence about at all, and a genuine OLD-frame printing
is vetoed `frame-mismatch`. That reason is deliberately NOT in
`JOIN_KEY_RESCANNABLE_SKIP_REASONS`, so the wrong conclusion is PERMANENT for
that content hash: the card never becomes eligible again and no later Stage C
pass can undo it. A wrong answer nothing can revisit is strictly worse than a
missing one.

`FRAME_CHECK_REQUIRED_EXTRACTOR_KEYS` names both extractors the classifier
actually reads (`collector_line_ocr` for the collector number, `artist_ocr` for
the anchor); an absent one skips the CHECK, not the CARD, leaving the match at
its already-computed confidence. That is the "missing data is not evidence"
rule this function's docstring already states and which the copyright-year
check and the `metadata is None` case already follow.

Declared PER-CHECK rather than per-calculator, unlike `local_detect_ai_art`/
`local_lands_identify`/`local_layout_class_cast`'s single module-level
constant: this calculator runs several checks with genuinely different needs -
the join-key deduction itself needs only the collector line - so a
calculator-wide gate would drop cards that have everything their own decision
requires.

`artist_ocr` is at 220,579/220,579 coverage in production, so this changes
nothing about the current catalogue. It removes a trap; it does not loosen a
live gate - proven by the control test, which shows the same card still vetoed
once `artist_ocr` is present.

The shared `_evidence` test fixture now carries both OCR extractors by default,
matching production. Without that every existing frame-veto test would have
become a silent no-op under the new gate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013NhYmT1PxCcyemA16dFDxN
@WilfordGrimley
WilfordGrimley force-pushed the fix/stage-d-required-extractor-keys branch from a098d51 to 1b8eb11 Compare July 30, 2026 12:24
@WilfordGrimley
WilfordGrimley merged commit 1d433dd into master Jul 30, 2026
13 checks passed
WilfordGrimley added a commit that referenced this pull request Jul 30, 2026
…ess + census leak, mid-pass envelope re-sampling (#665)

FIX 1 - THE MD5 GROUP BEHAVES AS ONE UNIT THROUGH THE MONOLITH

Owner: "the md5 dedupe should only fetch each identical image once across sources and then
apply votes to the entire group as the fetched card passes through the monolith."

The fetch half already existed (`evidence_transfer`, keyed on `Card.md5_checksum`). The vote
half existed only on the phash distance-0 key, so the set that got a fetch saved and the set
that got a vote propagated were DIFFERENT SETS - byte-identical files always share a phash,
but files sharing a phash are not necessarily byte-identical.

CHECKED BEFORE BUILDING, as instructed: propagation is NOT redundant. `evidence_transfer`
gives every md5 sibling its own ImageEvidence row with byte-identical extractor values, so it
is reasonable to ask whether each member already reaches the same conclusion independently.
It does not, structurally: a Stage D printing deduction is not a function of the evidence row
alone. `_resolve_candidates_for_card` keys the candidate list on `Card.name`, and md5-identical
uploads from different sources routinely carry different names. Members also differ on per-card
eligibility. `test_the_unfetched_twin_has_no_verdict_of_its_own_without_propagation` falsifies
the "N independent deductions already agree" hypothesis on the fixture rather than arguing it.

Stage C+ now runs TWO tiers through ONE propagation engine (`_propagate_over_groups`), which
takes the grouping as a parameter and knows nothing about how it was keyed:

  md5       exact identity -> shares a PRINTING vote. New. Runs first.
  phash d0  unchanged from PR #660. Runs second, filling only what md5 did not.

Two defects in the PR #660 propagation are fixed on the way:

  - SOURCE VOTES WERE READ FROM REPRESENTATIVES ONLY. Stage D has no reason to reach a group's
    lowest pk first, so whenever it reached any other member, nothing propagated. Source votes
    are now read across every member, and one source per (group, identity) is chosen
    deterministically so two vote-holders in one group cannot generate duplicate rows inside a
    single write batch.
  - REPRESENTATIVES WERE NEVER PROPAGATION TARGETS. Same root cause, opposite direction.

PROPAGATION NEVER OVERRIDES A MEMBER'S OWN INELIGIBILITY (owner constraint). A member already
resolved, already confirmed to a `canonical_card`, not a CARD, or carrying a resolved
custom-art/non-english tag is skipped. custom-art is the catalogue DECLARING the image is not a
faithful depiction of a printing; a checksum must not overturn that.

THE PHASH TIER IS LEFT IN PLACE, flagged rather than accreted. Issue #661 holds what phash
grouping is FOR - the owner's direction is that it should eventually share an ILLUSTRATION
(same artwork, possibly a different printing), not a printing verdict. Removing it now would
itself be a behaviour change, and it is currently the only propagation reaching cards with no
md5 at all (md5 is NULL for every LOCAL_FILE source by design). The `groups` parameter is the
seam that tier plugs into later.

FIX 2a - `run_name_frequency_elimination` NEVER LOOKED AT THE IMAGE

Owner: "just because a card was printed exactly once doesn't mean that the image in our
catalogue is an accurate depiction of that card, it may have a different border or another
issue." Owner leaned toward adding the conjunct rather than dropping the tier; the conjunct is
what shipped, and the reasoning for keeping the tier is in its own docstring.

Everything the 1:1 gate checked was a COUNT. Counting establishes that IF the card depicts one
of the name's printings THEN it is the uncovered one; nothing established the antecedent, and
the only filters that spoke to it were the DECLARED custom-art/non-english tags - so an
untagged altered border sailed through. "It is only a vote" is weaker than it sounds: #593
established a machine vote is what the question feed renders as the suggestion to confirm, and
the human's click returns as a full-weight USER vote.

The missing conjunct now requires the card's ALREADY-STORED evidence to be consistent with the
candidate printing. NOT a new implementation: `_apply_agreement_checks`' border/frame check was
lifted to `local_identify_printing_tags.printing_attribute_disagreement` and both callers now
share it. That direction is forced - `local_calculate_verdicts` imports
`local_identify_printing_tags`, never the reverse. Sharing also inherits PR #656's `artist_ocr`
gate for free, which is the half a second copy would most likely have got wrong.

NO STORED EVIDENCE MEANS ABSTAIN. This module's "missing data is not evidence" rule protects a
match from being VETOED by silence; here silence is being asked to ESTABLISH something, so it
points the other way. Counted separately from mismatches so the cost is legible.

FIX 2b - THE CENSUS LEAK (a fresh wrong positive, not a stale vote)

`_eligible_base_queryset(NAME_FREQUENCY_ANONYMOUS_ID)` was called with no `run_id`, making its
"exclude cards already carrying this calculator's vote" LIFETIME. The gate is a COUNT over
exactly that population, so the calculator was taking a census over a pool it permanently
shrinks itself: run 1 votes on a card, a second upload of that name arrives, and run 2 sees one
unresolved card where there are really two - and votes. Nothing about the second card changed;
only the size of the population the gate counts.

`compute_covered_printing_pks()` stays catalogue-wide and unscoped, deliberately: "covered" is
a fact about the world, not about this calculator's progress. `run_pilot`'s `select_candidates`
and `count_below_resolution_floor` are LEFT UNSCOPED - neither gates on a count over the
returned population, so neither has this defect. Stated in `_eligible_base_queryset`'s docstring
so the asymmetry is visible from the function rather than only from its callers.

FIX 3 - THE MONOLITH RE-SAMPLES THE ENVELOPE MID-PASS

Owner: "host resampling is likely required (for steps that aren't fetch) as the same monolith
will run for small datasets and large ones so needs to fit the available compute appropriately."

PR #660 checked the envelope ONCE, before Stage C. `_EnvelopeSentry` now re-samples at every
stage seam: after Stage C, between each of Stage D's calculators/casters (via a new OPTIONAL
`envelope_check` callback on `stage_e_dispatch._run_stage_d`, defaulting to None so the
conveyor is byte-identical), and before each Stage C+ tier. Sample counts land on the ledger.

HALT SEMANTICS PRESERVED. A breach still persists an EnvelopeTrip, still exits 3, still needs
`resolve_envelope_trip` - no self-resume, and NOT converted to a throttle (that is rate
pressure's channel, beneath Stage C, PR #644). A mid-pass halt message differs from the
preflight's: rows already written STAY written, and it names the `--run-id` to resume with.

Interval-gated at 60s so the check cannot become its own load. The number is derived, not
tuned: the host-load bar reads the ONE-MINUTE load average, so sampling faster re-reads a
number that has not finished moving.

RESIDUAL, reported not hidden: the seams are BETWEEN calculators, not inside them. Closing that
gap means threading a callback into each of seven calculators' own batch loops - a real
refactor of shared code, deliberately not done here.

DELIBERATE DUPLICATION, WITH A TRIPWIRE. `_members_eligible_for_a_propagated_vote` expresses
four catalogue-level facts `_eligible_base_queryset` also expresses. It does not call that
function (which bundles workload rules wrong for a propagation target) and that function could
not be refactored to expose them (its own docstring records that tests and
`stream_backstop_sweep` assert against its COMPILED SQL).
`TestPropagationEligibilityMatchesTheBaseQueryset` fails if the two ever disagree.

VERIFICATION - mutation red, restore green (7 mutants, all red; 263 tests green restored):

  M1  md5 tier returns no groups                     3 failed
  M2  source votes read from representatives only    2 failed
  M3  propagation ignores member ineligibility       1 failed
  M4  envelope re-sampling reverted to preflight     2 failed
  M5  visual conjunct never disagrees                1 failed
  M6  no-evidence no longer abstains                 1 failed
  M7  propagation eligibility drops the tag excludes 1 failed (the tripwire)

Suites: test_run_pipeline, test_local_identify_printing_tags,
test_local_calculate_verdicts, test_stage_e_dispatch - 520+ tests, all green.
No model changes, so no migration.

Docs: living pages only, no dated report - docs/identification-pipeline.md (Stage C+ md5
section), docs/features/printing-tags.md (both name-frequency fixes),
docs/features/stage-e-operations.md (mid-pass re-sampling).


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

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant