Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions MPCAutofill/cardpicker/local_calculate_verdicts.py
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,17 @@
CandidatePrinting,
generate_run_id,
)

# IMPORTED, NOT DUPLICATED AS A LITERAL (2026-07-30). This module's convention elsewhere is to
# duplicate a sibling engine's constant rather than take a hard import-time dependency on it
# (`JOIN_KEY_CONFIDENCE_BOTH`, `RESOLUTION_FLOOR_DPI`). That convention is deliberately NOT followed
# here, for a reason specific to this constant: `_slow_path_eligible_cards_queryset` uses it to
# EXCLUDE the illustration calculator's own rows, so a copy that drifted from the live identity - a
# `-v3` bump, say - would silently stop matching anything and reopen the exact defect the exclusion
# closes, with no error anywhere. A duplicated literal would need a tripwire test to catch that; an
# import cannot drift at all. No cycle: `local_illustration` imports `local_identify_printing_tags`
# and `image_evidence`, never this module.
from cardpicker.local_illustration import ILLUSTRATION_ANONYMOUS_ID
from cardpicker.local_ocr import (
OcrParseResult,
find_matching_candidates,
Expand Down Expand Up @@ -2186,6 +2197,7 @@ def run_fallback_calculator(
}
)


# The ImageEvidence fields packaged into a SlowPathVerdict's raw_signals for human review - every
# extracted signal a reviewer might use to disambiguate a card with no confident join-key hit,
# EXCLUDING candidate-matching fields (collector_line_set_code/collector_line_collector_number are
Expand Down Expand Up @@ -2288,6 +2300,26 @@ def _slow_path_eligible_cards_queryset(
still has no confident automated hit from either calculator and belongs in the review queue
exactly as before this PR.

AND excludes any card the ILLUSTRATION calculator (`_ILLUSTRATION_ANONYMOUS_ID`, issue #507)
already successfully voted on, on exactly the same reasoning and with exactly the same
`is_no_match=False` qualifier (2026-07-30). This exclusion was ABSENT until then, and this
function's own caller admitted it in a comment - "the slow-path queryset would need an
additional exclusion for this identity's votes, similar to the fallback-voted-card exclusion it
already carries". The management command sequences join-key -> fallback -> illustration ->
slow-path, so without it a card the illustration calculator resolves is routed to a human
reviewer moments later in the SAME invocation. That is wrong human work, not a no-op: the
reviewer is asked to identify a card the pipeline just identified.

The consequence was bounded only because `stage-d-illustration-v2` has never run. The
read-only replay in `docs/pipeline-fidelity-gate.md` projects ~3,233 printing votes, so this
fires on the FIRST `-v2` run - which makes it a pre-fire fix, not a cleanup.

LIKE THE FALLBACK EXCLUSION, THIS ONE IS DELIBERATELY NOT RUN-SCOPED. "The illustration
calculator has a confident vote for this card" is a statement about the catalogue, not about a
run. Scoping it would mean a card illustration resolved in run A gets routed to human review by
slow-path in run B, undoing a solved card - see this function's own opening paragraph and
`_fallback_eligible_cards_queryset`'s docstring for the general form of that argument.

`card_ids` (2026-07-24, Stage E Phase 2): a pure scope narrowing, same convention as
`_eligible_cards_queryset`'s own `card_ids` parameter - see that function's own docstring.
Since 2026-07-29 it is ALSO pushed into all FOUR dependency subqueries below (the two join-key
Expand All @@ -2306,11 +2338,16 @@ def _slow_path_eligible_cards_queryset(
# replaces) PR #579's `card_ids` pushdown applied to the same queryset just below.
already_routed = already_routed.filter(run_id=run_id)
fallback_voted = CardPrintingTag.objects.filter(anonymous_id=STAGE_D_FALLBACK_ANONYMOUS_ID, is_no_match=False)
illustration_voted = CardPrintingTag.objects.filter(anonymous_id=ILLUSTRATION_ANONYMOUS_ID, is_no_match=False)
if card_ids is not None:
already_routed = already_routed.filter(card_id__in=card_ids)
fallback_voted = fallback_voted.filter(card_id__in=card_ids)
# Same PR #579 pushdown as the two above - an uncorrelated `IN (SELECT ...)` over a
# 167k-row table on every 25-card micro-batch otherwise.
illustration_voted = illustration_voted.filter(card_id__in=card_ids)
already_routed_card_ids = already_routed.values_list("card_id", flat=True)
fallback_voted_card_ids = fallback_voted.values_list("card_id", flat=True)
illustration_voted_card_ids = illustration_voted.values_list("card_id", flat=True)
queryset = (
Card.objects.filter(
printing_tag_status=PrintingTagStatus.UNRESOLVED,
Expand All @@ -2320,6 +2357,7 @@ def _slow_path_eligible_cards_queryset(
.filter(Q(pk__in=join_key_no_match_card_ids) | Q(pk__in=join_key_no_hit_scanned_card_ids))
.exclude(pk__in=already_routed_card_ids)
.exclude(pk__in=fallback_voted_card_ids)
.exclude(pk__in=illustration_voted_card_ids)
.distinct()
.select_related("source")
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,12 @@ def handle(self, *args: Any, **kwargs: Any) -> None:
# AFTER the fallback calculator above in the SAME invocation/run_id — uses
# illustration_id relationships to deduce printing identity from artist-OCR hits.
# Sequenced before slow-path routing below: a card this calculator resolves must not
# also get routed to human review in the same invocation (the slow-path queryset
# would need an additional exclusion for this identity's votes, similar to the
# fallback-voted-card exclusion it already carries).
# also get routed to human review in the same invocation. The exclusion that makes
# that sequencing actually take effect now EXISTS (2026-07-30) —
# `_slow_path_eligible_cards_queryset`'s `.exclude(pk__in=illustration_voted_card_ids)`,
# the sibling of the fallback-voted-card exclusion this comment used to say was
# merely needed. Until then the ordering was decorative: slow-path routed the card
# anyway.
illustration_result = run_illustration_calculator(
run_id=run_id,
dry_run=dry_run,
Expand Down
107 changes: 103 additions & 4 deletions MPCAutofill/cardpicker/tests/test_local_calculate_verdicts.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,7 @@
CandidateNameIndex,
CandidatePrinting,
)
from cardpicker.local_illustration import ILLUSTRATION_ANONYMOUS_ID
from cardpicker.models import (
ArchivedCardPrintingTag,
CanonicalPrintingMetadata,
Expand Down Expand Up @@ -3224,16 +3225,16 @@ def test_fallback_bulk_mode_takes_no_scoping_branch(self, db):
# -- slow-path calculator ------------------------------------------------

def test_slow_path_scopes_every_dependency_subquery(self, db):
"""Four subqueries: the two join-key no-hit populations, this calculator's own
already-routed `CardScanLog` exclusion, and the fallback-voted `CardPrintingTag`
exclusion."""
"""Five subqueries: the two join-key no-hit populations, this calculator's own
already-routed `CardScanLog` exclusion, the fallback-voted `CardPrintingTag` exclusion,
and (2026-07-30) the illustration-voted `CardPrintingTag` exclusion."""
card_a = CardFactory(name="Slow Path Scope A")
card_b = CardFactory(name="Slow Path Scope B")
scope = [card_a.pk, card_b.pk]

sql = str(_slow_path_eligible_cards_queryset(card_ids=scope).query)

self._assert_every_subquery_scoped(sql, scope, expected_count=4)
self._assert_every_subquery_scoped(sql, scope, expected_count=5)
assert f'"cardpicker_card"."id" IN ({card_a.pk}, {card_b.pk})' in sql

def test_slow_path_bulk_mode_takes_no_scoping_branch(self, db):
Expand Down Expand Up @@ -3335,3 +3336,101 @@ def test_bulk_mode_eligible_sets_are_unchanged(self, db):

assert set(_fallback_eligible_cards_queryset().values_list("pk", flat=True)) == {card_a.pk, card_b.pk}
assert set(_slow_path_eligible_cards_queryset().values_list("pk", flat=True)) == {card_a.pk, card_b.pk}


class TestSlowPathIllustrationExclusion:
"""
THE ILLUSTRATION -> SLOW-PATH DEPENDENCY (2026-07-30, closing the 2026-07-29 composition
audit's §1 Q2 first bullet). `_slow_path_eligible_cards_queryset` excluded `already_routed` and
`fallback_voted` and NOTHING for the illustration calculator, and
`management/commands/local_calculate_verdicts.py`'s own sequencing comment admitted the gap.

The failure direction is WRONG HUMAN WORK, not a silent no-op: a card the illustration
calculator resolves is routed to a reviewer moments later in the SAME invocation, asking a
human to identify a card the pipeline just identified. Bounded today only because
`stage-d-illustration-v2` has never run; `docs/pipeline-fidelity-gate.md`'s read-only replay
projects ~3,233 printing votes, so it fires on the first `-v2` run.
"""

def _no_hit_card(self, name="Illus Exclusion"):
card = CardFactory(name=name, content_phash=42)
_evidence(card, collector_line_raw_text="garbled")
CardScanLog.objects.create(card=card, anonymous_id=JOIN_KEY_ANONYMOUS_ID, skip_reason="ambiguous")
return card

def _illustration_vote(self, card, *, is_no_match=False, anonymous_id=None):
return CardPrintingTag.objects.create(
card=card,
printing=None if is_no_match else CanonicalCardFactory(name=card.name),
is_no_match=is_no_match,
anonymous_id=anonymous_id or ILLUSTRATION_ANONYMOUS_ID,
source=VoteSource.OCR,
confidence=0.85,
)

def test_a_card_the_illustration_calculator_resolved_is_not_routed_to_a_human(self, db):
"""The defect itself."""
resolved = self._no_hit_card("Illustration Resolved")
self._illustration_vote(resolved)

eligible_ids = set(_slow_path_eligible_cards_queryset().values_list("pk", flat=True))

assert resolved.pk not in eligible_ids

def test_a_card_the_illustration_calculator_abstained_on_is_still_routed(self, db):
"""THE CONTROL, and the half that makes the test able to fail in both directions. The
exclusion qualifies on `is_no_match=False` exactly as the fallback one does: an
illustration `is_no_match` vote is the calculator CONCLUDING it cannot identify the card,
which is precisely a card a reviewer should see. Excluding those too would trade wrong
human work for a silently emptied review queue."""
abstained = self._no_hit_card("Illustration Abstained")
self._illustration_vote(abstained, is_no_match=True)

eligible_ids = set(_slow_path_eligible_cards_queryset().values_list("pk", flat=True))

assert abstained.pk in eligible_ids

def test_a_card_only_the_legacy_v1_identity_voted_on_is_still_routed(self, db):
"""`stage-d-illustration-v1`'s 3 legacy rows were cast by a calculator whose border-colour
gate was wrong (see `local_illustration`'s v1 -> v2 section). They are not evidence the
LIVE calculator resolved anything, so they must not suppress routing."""
legacy = self._no_hit_card("Illustration Legacy")
self._illustration_vote(legacy, anonymous_id="stage-d-illustration-v1")

eligible_ids = set(_slow_path_eligible_cards_queryset().values_list("pk", flat=True))

assert legacy.pk in eligible_ids

def test_an_untouched_no_hit_card_is_still_routed(self, db):
"""The exclusion must not empty the queue wholesale."""
untouched = self._no_hit_card("Illustration Untouched")

assert untouched.pk in set(_slow_path_eligible_cards_queryset().values_list("pk", flat=True))

def test_the_exclusion_is_not_run_scoped(self, db):
"""A card illustration resolved in run A must not be routed to human review by slow-path in
run B - "illustration has a confident vote for this card" is a statement about the
catalogue, not about a run. Same asymmetry `_fallback_eligible_cards_queryset`'s docstring
establishes: `run_id` narrows a calculator's OWN progress, never an upstream verdict."""
resolved = self._no_hit_card("Illustration Cross Run")
vote = self._illustration_vote(resolved)
vote.run_id = "run-a"
vote.save()

eligible_ids = set(_slow_path_eligible_cards_queryset(run_id="run-b").values_list("pk", flat=True))

assert resolved.pk not in eligible_ids

def test_the_full_calculator_writes_no_routing_row_for_an_illustration_resolved_card(self, db):
"""End-to-end through `run_slow_path_calculator`, not just the queryset: the observable
the reviewer actually sees is a `CardScanLog(to-review)` row, and that is what must not
appear."""
resolved = self._no_hit_card("Illustration End To End")
self._illustration_vote(resolved)

result = run_slow_path_calculator(run_id="r1", dry_run=False)

assert result.routed_written == 0
assert not CardScanLog.objects.filter(
card=resolved, anonymous_id=SLOW_PATH_ANONYMOUS_ID, skip_reason=SLOW_PATH_TO_REVIEW_SKIP_REASON
).exists()
Loading
Loading