From bfbb886017a37f92b465809c7f3ae89a57a44ba2 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 18:26:18 +0000 Subject: [PATCH 1/2] fix(question_feed): phase-C not-official-art routing + md5-expand artist/tag exclusions Two independent routing gaps in the question feed (MPCAutofill/cardpicker/question_feed.py), diagnosed against reason_tags.py's WTC phase B partition and issue #473's md5 identity groups: 1. reason_tags.py: promotes the WTC phase B "not-official-printing" vs. "not-official-art" partition from prose in the module docstring into two real, checked frozensets (NOT_OFFICIAL_PRINTING_REASON_TAGS / NOT_OFFICIAL_ART_REASON_TAGS), mirroring the frontend's NoMatchReasonStrip.tsx NO_MATCH_REASON_TAG_GROUPS. test_reason_tags.py asserts they are exhaustive and disjoint over NO_MATCH_REASON_TAGS. 2. question_feed.py: phase C routing. A card carrying a positive (VotePolarity.APPLY) CardTagVote for a not-official-art tag has had its artwork question declared unanswerable by a human, so the feed now excludes it from artist-shaped questions in both _tier_2_contested and _tier_4_fresh (_not_official_art_card_ids, md5-group-widened, computed once per feed request in get_next_question_feed_item). The printing question is unaffected. Judgment call: the exclusion requires a HUMAN-BACKED source (vote_consensus. is_human_backed_source), not merely "any positive vote". Reason tags are cast by a human through NoMatchReasonStrip today, but nothing in the schema stops a future machine caster from writing one, and this routing signal is meant to represent an actual human declaration that the artwork question is meaningless for the card - a machine-cast source earning the same trust would need its own explicit decision, not a silent inclusion via this change. 3. question_feed.py: _tier_2_contested's artist and tag own-vote exclusions are now md5-group- widened (_voter_answered_artist_card_ids / _voter_answered_tag_card_ids_by_tag), so a voter who answered one member of a byte-identical group is not re-asked the identical artist/tag question under a sibling's identifier - the same convention _voter_answered_printing_card_ids already established for the printing tiers. The tag widening is scoped to the CARD axis only (never the tag axis), preserving _tier_2_contested's existing per-tag granularity, and costs one query total (not one per review pair) plus one md5 expansion per distinct tag the voter has touched. Scoped to _tier_2_contested only, per the engineering brief - _tier_4_fresh's own artist/tag own-vote exclusions keep their pre-existing, unwidened form. Observation (out of scope, no implementation): local_illustration. _eligible_illustration_cards_queryset (the machine illustration calculator) has no awareness of NOT_OFFICIAL_ART_REASON_TAGS either, and plausibly warrants the same exclusion for the same reason phase C does - left as a follow-up. New tests: test_reason_tags.py (partition exhaustiveness/disjointness), test_question_feed.py (TestPhaseCNotOfficialArtRouting - positive not-official-art excludes, not-official-printing doesn't, negative vote doesn't, machine-cast vote doesn't), test_md5_group_pooling.py (TestPhaseCAndTierTwoMd5Expansion - artist/tag md5 widening, sibling non-re-serving, the per-tag-axis regression the existing _tier_2_contested comment warns against, and the not-official-art group-wide exclusion; plus a computed-once-per-feed-request pin for all three new exclusions, extending the existing 2026-07-25 PR #482 condition f1 test). --- MPCAutofill/cardpicker/question_feed.py | 131 +++++++++++++-- MPCAutofill/cardpicker/reason_tags.py | 20 ++- .../tests/test_md5_group_pooling.py | 152 ++++++++++++++++++ .../cardpicker/tests/test_question_feed.py | 58 +++++++ .../cardpicker/tests/test_reason_tags.py | 16 ++ 5 files changed, 364 insertions(+), 13 deletions(-) diff --git a/MPCAutofill/cardpicker/question_feed.py b/MPCAutofill/cardpicker/question_feed.py index 0a5cbc586..db2d268ca 100644 --- a/MPCAutofill/cardpicker/question_feed.py +++ b/MPCAutofill/cardpicker/question_feed.py @@ -83,6 +83,7 @@ from cardpicker.models import ( ArtistVoteStatus, Card, + CardArtistVote, CardPrintingTag, CardScanLog, CardTagVote, @@ -91,6 +92,7 @@ QuestionFeedServedPool, Tag, TagVoteStatus, + VotePolarity, VoteSource, ) from cardpicker.printing_candidates import get_ranked_printing_candidates @@ -100,10 +102,12 @@ group_printing_votes, md5_group_expanded_card_ids, ) +from cardpicker.reason_tags import NOT_OFFICIAL_ART_REASON_TAGS from cardpicker.schema_types import QuestionFeedCounts, QuestionFeedItem, TypeEnum from cardpicker.tag_consensus import get_tag_net_polarity, get_tag_review_queue_pairs from cardpicker.vote_consensus import ( VoteTuple, + is_human_backed_source, resolve_vote_weight, resolve_weighted_consensus, ) @@ -263,6 +267,74 @@ def _voter_answered_printing_card_ids(anonymous_id: str) -> set[int]: return md5_group_expanded_card_ids(voted_card_ids) +def _voter_answered_artist_card_ids(anonymous_id: str) -> set[int]: + """ + The artist-tier analogue of `_voter_answered_printing_card_ids` above: every card this voter + has already cast a `CardArtistVote` on, widened to those cards' full md5 identity groups, so + a voter who answered one member of a byte-identical group is not re-asked the same artist + question under a sibling's identifier (issue #473). Scoped to `_tier_2_contested` only + (2026-08-04 gate on the phase-C/md5 routing brief) - `_tier_4_fresh`'s own artist exclusion + keeps its pre-existing, unwidened `.exclude(artist_votes__anonymous_id=...)` form. + + COMPUTED ONCE PER FEED REQUEST, mirroring `_voter_answered_printing_card_ids`'s own + convention exactly. + """ + voted_card_ids = CardArtistVote.objects.filter(anonymous_id=anonymous_id).values_list("card_id", flat=True) + return md5_group_expanded_card_ids(voted_card_ids) + + +def _voter_answered_tag_card_ids_by_tag(anonymous_id: str) -> dict[str, set[int]]: + """ + For every tag name this voter has cast a `CardTagVote` on, the set of card ids - each widened + to its full md5 identity group - that count as "already answered" for THAT tag. Widening is + on the CARD axis only, never the tag axis: `_tier_2_contested`'s own-vote exclusion is + deliberately scoped to (card, tag, anonymous_id), not (card, anonymous_id) - a card carries + ~11 independent attribute-chip tags, and a card-level exclude would silently hide every other + still-open tag the moment a voter touches any one of them (see that function's own comment). + This applies the identical scoping onto md5 siblings: "has this voter answered THIS tag on + ANY member of this card's md5 group", never "has this voter answered ANY tag on this card's + md5 group". + + ONE query fetches every (tag_name, card_id) pair this voter has ever voted on - cost scales + with this voter's own vote count, not with `get_tag_review_queue_pairs()`'s output, so it + does not multiply per review pair. The md5 expansion then runs once per distinct tag name + this voter has touched (bounded by the fixed attribute-chip taxonomy), never once per pair. + """ + rows = CardTagVote.objects.filter(anonymous_id=anonymous_id).values_list("tag__name", "card_id") + card_ids_by_tag: dict[str, set[int]] = defaultdict(set) + for tag_name, card_id in rows: + card_ids_by_tag[tag_name].add(card_id) + return {tag_name: md5_group_expanded_card_ids(card_ids) for tag_name, card_ids in card_ids_by_tag.items()} + + +def _not_official_art_card_ids() -> set[int]: + """ + Cards a human has declared NOT official art via a positive (`VotePolarity.APPLY`) + `CardTagVote` for one of `reason_tags.NOT_OFFICIAL_ART_REASON_TAGS` - the phase-C routing + signal the WTC phase B partition (`reason_tags.py`'s module docstring) was always meant to + feed. For such a card the artwork question is UNANSWERABLE, so the feed must stop serving + artist-shaped questions for it; the printing question is unaffected (see this function's + call sites, both in the artist half only). Widened to each card's full md5 identity group, + since byte-identical files share the same artwork. + + Human-backed only (`is_human_backed_source`), not "any positive vote": these reason tags are + cast by a human through `NoMatchReasonStrip`, but nothing in the schema stops a machine + caster from writing one in principle, and this routing signal is meant to represent an + actual human declaration that the artwork question is meaningless for this card - a future + machine-cast source earning the same trust would need its own explicit decision, not a + silent inclusion here. + + Unlike `_voter_answered_printing_card_ids`/`_voter_answered_artist_card_ids` above, this is + NOT per-voter: it is a fact about the CARD, so it applies identically to every voter's feed. + Still COMPUTED ONCE PER FEED REQUEST, same convention as the per-voter exclusions. + """ + rows = CardTagVote.objects.filter( + tag__name__in=NOT_OFFICIAL_ART_REASON_TAGS, polarity=VotePolarity.APPLY + ).values_list("card_id", "source") + human_backed_card_ids = {card_id for card_id, source in rows if is_human_backed_source(source)} + return md5_group_expanded_card_ids(human_backed_card_ids) + + def is_likely_resolve_printing(card: Card) -> bool: """ True when ONE hypothetical additional agreeing human vote (`VoteSource.USER` weight) added @@ -394,10 +466,21 @@ def _tier_1_confirm_suggestion( def _tier_2_contested( - anonymous_id: str, answered_card_ids: Optional[set[int]] = None + anonymous_id: str, + answered_card_ids: Optional[set[int]] = None, + answered_artist_card_ids: Optional[set[int]] = None, + answered_tag_card_ids_by_tag: Optional[dict[str, set[int]]] = None, + not_official_art_card_ids: Optional[set[int]] = None, ) -> Optional[tuple[QuestionFeedItem, str]]: if answered_card_ids is None: answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) + if answered_artist_card_ids is None: + answered_artist_card_ids = _voter_answered_artist_card_ids(anonymous_id) + if answered_tag_card_ids_by_tag is None: + answered_tag_card_ids_by_tag = _voter_answered_tag_card_ids_by_tag(anonymous_id) + if not_official_art_card_ids is None: + not_official_art_card_ids = _not_official_art_card_ids() + printing_card = ( Card.objects.filter(printing_tag_status=PrintingTagStatus.UNRESOLVED, pk__in=get_contested_card_ids()) .exclude(pk__in=answered_card_ids) @@ -409,7 +492,8 @@ def _tier_2_contested( artist_card = ( Card.objects.filter(artist_vote_status=ArtistVoteStatus.CONTESTED, pk__in=get_contested_artist_card_ids()) - .exclude(artist_votes__anonymous_id=anonymous_id) + .exclude(pk__in=answered_artist_card_ids) + .exclude(pk__in=not_official_art_card_ids) .order_by("-date_created") .first() ) @@ -417,12 +501,14 @@ def _tier_2_contested( return _artist_item(artist_card), "tier_2_contested_artist" for card_id, tag_name in get_tag_review_queue_pairs(): - # scoped to (card, tag, anonymous_id), not just (card, anonymous_id) - a voter who - # already answered a *different* tag on this card (there are ~11 attribute-chip tags - # per card) must still see this tag if they haven't answered it yet. A card-level - # exclude here would silently hide every other still-open tag on a card the moment - # this voter touches any one tag on it. - if CardTagVote.objects.filter(card_id=card_id, tag__name=tag_name, anonymous_id=anonymous_id).exists(): + # scoped to (card, tag, anonymous_id) widened to the card's md5 group, not just (card, + # anonymous_id) - a voter who already answered a *different* tag on this card (there + # are ~11 attribute-chip tags per card) must still see this tag if they haven't + # answered it yet, and a voter who answered THIS tag on a byte-identical sibling of + # this card must not be re-asked it here either (issue #473). A card-level exclude here + # (dropping the tag axis) would silently hide every other still-open tag on a card the + # moment this voter touches any one tag on it. + if card_id in answered_tag_card_ids_by_tag.get(tag_name, set()): continue card = Card.objects.get(pk=card_id) status = card.tag_vote_statuses.get(tag_name) @@ -446,7 +532,9 @@ def _latest_stage_d_origin_reason_subquery() -> Subquery: def _tier_4_fresh( - anonymous_id: str, answered_card_ids: Optional[set[int]] = None + anonymous_id: str, + answered_card_ids: Optional[set[int]] = None, + not_official_art_card_ids: Optional[set[int]] = None, ) -> Optional[tuple[QuestionFeedItem, str]]: # named "tier 4" (not renumbered to 3) even though moderation's former tier 3 was removed # (see module docstring) - keeps this name stable against every docstring/test/comment @@ -471,6 +559,8 @@ def _tier_4_fresh( # what actually decides ordering among them, not a rarely-reached fallback. if answered_card_ids is None: answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) + if not_official_art_card_ids is None: + not_official_art_card_ids = _not_official_art_card_ids() printing_card = ( Card.objects.filter(printing_tag_status=PrintingTagStatus.UNRESOLVED) .exclude(pk__in=get_contested_card_ids()) @@ -498,6 +588,7 @@ def _tier_4_fresh( artist_card = ( Card.objects.filter(artist_vote_status=ArtistVoteStatus.UNRESOLVED) .exclude(artist_votes__anonymous_id=anonymous_id) + .exclude(pk__in=not_official_art_card_ids) .order_by("-date_created") .first() ) @@ -560,9 +651,19 @@ def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem] The voter's answered-card exclusion set (`_voter_answered_printing_card_ids`, md5-group- expanded per issue #473) is resolved ONCE here and passed to every printing tier below - it cannot change mid-request, and recomputing it per tier was a real per-request regression the - 2026-07-25 PR #482 gate (condition f1) called out. + 2026-07-25 PR #482 gate (condition f1) called out. The same convention now covers two more + request-scoped exclusions (2026-08-04 gate on the phase-C/md5 routing brief): + `_voter_answered_artist_card_ids`/`_voter_answered_tag_card_ids_by_tag` (md5-widened, own- + vote exclusions for `_tier_2_contested`'s artist/tag halves - see those functions' own + docstrings for why this is scoped to that tier only), and `_not_official_art_card_ids` (the + phase-C routing signal: a card a human has declared not-official-art via `reason_tags. + NOT_OFFICIAL_ART_REASON_TAGS` stops being served as an artist-shaped question, in both + `_tier_2_contested` and `_tier_4_fresh` - printing questions are unaffected). """ answered_card_ids = _voter_answered_printing_card_ids(anonymous_id) + answered_artist_card_ids = _voter_answered_artist_card_ids(anonymous_id) + answered_tag_card_ids_by_tag = _voter_answered_tag_card_ids_by_tag(anonymous_id) + not_official_art_card_ids = _not_official_art_card_ids() if _served_mix_ratio(anonymous_id) < settings.QUESTION_FEED_LIKELY_RESOLVE_MIX_RATIO: likely_resolve_card = _likely_resolve_printing_card(anonymous_id, answered_card_ids) @@ -576,12 +677,18 @@ def get_next_question_feed_item(anonymous_id: str) -> Optional[QuestionFeedItem] if tier_1_item is not None: return _log_served(anonymous_id, tier_1_item, QuestionFeedServedPool.REMAINDER, "tier_1_confirm_suggestion") - tier_2_result = _tier_2_contested(anonymous_id, answered_card_ids) + tier_2_result = _tier_2_contested( + anonymous_id, + answered_card_ids, + answered_artist_card_ids=answered_artist_card_ids, + answered_tag_card_ids_by_tag=answered_tag_card_ids_by_tag, + not_official_art_card_ids=not_official_art_card_ids, + ) if tier_2_result is not None: tier_2_item, tier_2_reason = tier_2_result return _log_served(anonymous_id, tier_2_item, QuestionFeedServedPool.REMAINDER, tier_2_reason) - tier_4_result = _tier_4_fresh(anonymous_id, answered_card_ids) + tier_4_result = _tier_4_fresh(anonymous_id, answered_card_ids, not_official_art_card_ids=not_official_art_card_ids) if tier_4_result is not None: tier_4_item, tier_4_reason = tier_4_result return _log_served(anonymous_id, tier_4_item, QuestionFeedServedPool.REMAINDER, tier_4_reason) diff --git a/MPCAutofill/cardpicker/reason_tags.py b/MPCAutofill/cardpicker/reason_tags.py index 27cd64607..172dcbaef 100644 --- a/MPCAutofill/cardpicker/reason_tags.py +++ b/MPCAutofill/cardpicker/reason_tags.py @@ -98,6 +98,19 @@ ), ] +# The WTC phase B partition (see the module docstring above) as real, checked constants rather +# than prose a backend consumer would have to re-derive by hand from the "axis" comments inline +# above. Mirrors `NoMatchReasonStrip.tsx`'s `NO_MATCH_REASON_TAG_GROUPS` tagNames exactly - kept +# as two flat frozensets rather than one grouped structure since, unlike the frontend, nothing +# backend-side needs the group's label/hint copy, only "which side is this tag on" as a routing +# predicate. `test_reason_tags.py` asserts these are exhaustive and disjoint over +# `NO_MATCH_REASON_TAGS`, the same invariant `NoMatchReasonStrip.spec.ts` asserts on the frontend +# side of this same split. +NOT_OFFICIAL_PRINTING_REASON_TAGS: frozenset[str] = frozenset( + {"altered-frame", "upscaled", "no-collector-line", "non-english"} +) +NOT_OFFICIAL_ART_REASON_TAGS: frozenset[str] = frozenset({"custom-art", "ai-art", EXTERNAL_IP_TAG_NAME}) + def seed_no_match_reason_tags() -> dict[str, int]: """ @@ -121,4 +134,9 @@ def seed_no_match_reason_tags() -> dict[str, int]: return {"created": created, "updated": updated} -__all__ = ["seed_no_match_reason_tags", "NO_MATCH_REASON_TAGS"] +__all__ = [ + "seed_no_match_reason_tags", + "NO_MATCH_REASON_TAGS", + "NOT_OFFICIAL_PRINTING_REASON_TAGS", + "NOT_OFFICIAL_ART_REASON_TAGS", +] diff --git a/MPCAutofill/cardpicker/tests/test_md5_group_pooling.py b/MPCAutofill/cardpicker/tests/test_md5_group_pooling.py index b24ade19a..287c18e7e 100644 --- a/MPCAutofill/cardpicker/tests/test_md5_group_pooling.py +++ b/MPCAutofill/cardpicker/tests/test_md5_group_pooling.py @@ -42,9 +42,12 @@ ) from cardpicker.management.commands.consensus_recompute import run_consensus_recompute from cardpicker.models import ( + ArtistVoteStatus, Card, CardPrintingTag, PrintingTagStatus, + TagVoteStatus, + VotePolarity, VoteSource, calculator_family, ) @@ -60,16 +63,25 @@ resolve_printing, ) from cardpicker.question_feed import ( + _not_official_art_card_ids, _tier_1_confirm_suggestion, + _tier_2_contested, + _voter_answered_artist_card_ids, _voter_answered_printing_card_ids, + _voter_answered_tag_card_ids_by_tag, get_next_question_feed_item, is_likely_resolve_printing, ) +from cardpicker.tag_consensus import resolve_and_persist_tag_votes from cardpicker.tests.factories import ( + CanonicalArtistFactory, CanonicalCardFactory, + CardArtistVoteFactory, CardFactory, CardPrintingTagFactory, + CardTagVoteFactory, ImageEvidenceFactory, + TagFactory, ) from cardpicker.vote_consensus import ( VoteTuple, @@ -692,6 +704,146 @@ def test_answered_set_is_computed_exactly_once(self, db, md5_groups): assert spy.call_count == 1 + def test_the_three_new_exclusions_are_each_computed_exactly_once(self, db): + # 2026-08-04 gate on the phase-C/md5 routing brief: the same f1 condition above now also + # covers _voter_answered_artist_card_ids/_voter_answered_tag_card_ids_by_tag/ + # _not_official_art_card_ids - each resolved once in get_next_question_feed_item, not + # once per tier that consults it. + with ( + patch( + "cardpicker.question_feed._voter_answered_artist_card_ids", + side_effect=_voter_answered_artist_card_ids, + ) as artist_spy, + patch( + "cardpicker.question_feed._voter_answered_tag_card_ids_by_tag", + side_effect=_voter_answered_tag_card_ids_by_tag, + ) as tag_spy, + patch( + "cardpicker.question_feed._not_official_art_card_ids", + side_effect=_not_official_art_card_ids, + ) as art_spy, + ): + get_next_question_feed_item("voter-1") + + assert artist_spy.call_count == 1 + assert tag_spy.call_count == 1 + assert art_spy.call_count == 1 + + +class TestPhaseCAndTierTwoMd5Expansion: + """ + 2026-08-04 gate on the phase-C/md5 routing brief. Two independent things, both pinned here: + + - `_not_official_art_card_ids` (phase C): a positive, human-backed no-match-reason vote for + one of `reason_tags.NOT_OFFICIAL_ART_REASON_TAGS` stops the artist question from being + served for that card's whole md5 group. + - `_tier_2_contested`'s artist/tag own-vote exclusions, md5-expanded (issue #473's existing + convention, extended to these two halves - see `_voter_answered_artist_card_ids`/ + `_voter_answered_tag_card_ids_by_tag`'s own docstrings). + """ + + def test_answered_artist_card_ids_expand_to_the_whole_group(self, db, md5_groups): + card_a, card_b = CardFactory(), CardFactory() + md5_groups("same-bytes", card_a, card_b) + CardArtistVoteFactory(card=card_a, anonymous_id="voter-1") + + assert _voter_answered_artist_card_ids("voter-1") == {card_a.pk, card_b.pk} + assert _voter_answered_artist_card_ids("voter-2") == set() + + def test_tier_2_does_not_re_serve_an_artist_question_answered_on_a_sibling(self, db, md5_groups): + card_a = CardFactory(artist_vote_status=ArtistVoteStatus.CONTESTED) + card_b = CardFactory(artist_vote_status=ArtistVoteStatus.CONTESTED) + md5_groups("same-bytes", card_a, card_b) + artist_x, artist_y = CanonicalArtistFactory(), CanonicalArtistFactory() + for card in (card_a, card_b): + CardArtistVoteFactory(card=card, artist=artist_x, anonymous_id="crowd-1") + CardArtistVoteFactory(card=card, artist=artist_y, anonymous_id="crowd-2") + + # voter-1 answers card_a's artist question... + CardArtistVoteFactory(card=card_a, artist=artist_x, anonymous_id="voter-1") + + # ...and must not be re-served the identical question via card_b, its byte-identical + # sibling, even though voter-1 never cast a vote on card_b directly. + assert _tier_2_contested("voter-1") is None + + def test_answered_tag_card_ids_by_tag_expand_to_the_whole_group(self, db, md5_groups): + card_a, card_b = CardFactory(), CardFactory() + md5_groups("same-bytes", card_a, card_b) + tag = TagFactory(name="Full Art") + CardTagVoteFactory(card=card_a, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="voter-1") + + by_tag = _voter_answered_tag_card_ids_by_tag("voter-1") + + assert by_tag[tag.name] == {card_a.pk, card_b.pk} + assert _voter_answered_tag_card_ids_by_tag("voter-2") == {} + + def test_tier_2_does_not_re_serve_a_tag_question_answered_on_a_sibling(self, db, md5_groups): + card_a = CardFactory( + printing_tag_status=PrintingTagStatus.RESOLVED, artist_vote_status=ArtistVoteStatus.RESOLVED + ) + card_b = CardFactory( + printing_tag_status=PrintingTagStatus.RESOLVED, artist_vote_status=ArtistVoteStatus.RESOLVED + ) + md5_groups("same-bytes", card_a, card_b) + tag = TagFactory(name="Full Art") + for card in (card_a, card_b): + CardTagVoteFactory(card=card, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="crowd-1") + CardTagVoteFactory(card=card, tag=tag, polarity=VotePolarity.NOT_APPLICABLE, anonymous_id="crowd-2") + resolve_and_persist_tag_votes(card) + card.refresh_from_db() + assert card_a.tag_vote_statuses[tag.name] == TagVoteStatus.CONTESTED + assert card_b.tag_vote_statuses[tag.name] == TagVoteStatus.CONTESTED + + # voter-1 answers tag on card_a... + CardTagVoteFactory(card=card_a, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="voter-1") + + # ...and must not be re-served the identical (card, tag) question via card_b's own + # otherwise-still-contested pair. + assert _tier_2_contested("voter-1") is None + + def test_a_different_tag_on_the_group_is_still_served_despite_the_widened_exclusion(self, db, md5_groups): + card_a = CardFactory( + printing_tag_status=PrintingTagStatus.RESOLVED, artist_vote_status=ArtistVoteStatus.RESOLVED + ) + card_b = CardFactory( + printing_tag_status=PrintingTagStatus.RESOLVED, artist_vote_status=ArtistVoteStatus.RESOLVED + ) + md5_groups("same-bytes", card_a, card_b) + tag_a = TagFactory(name="Full Art") + tag_b = TagFactory(name="Etched") + for tag in (tag_a, tag_b): + CardTagVoteFactory(card=card_a, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="crowd-1") + CardTagVoteFactory(card=card_a, tag=tag, polarity=VotePolarity.NOT_APPLICABLE, anonymous_id="crowd-2") + resolve_and_persist_tag_votes(card_a) + card_a.refresh_from_db() + + # voter-1 answers tag_a on card_a - widened to the whole md5 group for tag_a specifically... + CardTagVoteFactory(card=card_a, tag=tag_a, polarity=VotePolarity.APPLY, anonymous_id="voter-1") + + # ...but tag_b, untouched, must still be served - the exclusion is per-tag, not + # per-card, even once widened onto md5 siblings (the regression the identical comment + # in _tier_2_contested itself warns against). + result = _tier_2_contested("voter-1") + assert result is not None + item, reason = result + assert reason == "tier_2_contested_tag" + assert item.tagName == tag_b.name + assert item.card.identifier == card_a.identifier + + def test_not_official_art_vote_excludes_the_whole_group_from_artist_questions(self, db, md5_groups): + card_a = CardFactory(artist_vote_status=ArtistVoteStatus.CONTESTED) + card_b = CardFactory(artist_vote_status=ArtistVoteStatus.CONTESTED) + md5_groups("same-bytes", card_a, card_b) + artist_x, artist_y = CanonicalArtistFactory(), CanonicalArtistFactory() + for card in (card_a, card_b): + CardArtistVoteFactory(card=card, artist=artist_x, anonymous_id="crowd-1") + CardArtistVoteFactory(card=card, artist=artist_y, anonymous_id="crowd-2") + tag = TagFactory(name="custom-art") + CardTagVoteFactory(card=card_a, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="reporter-1") + + assert _not_official_art_card_ids() == {card_a.pk, card_b.pk} + assert _tier_2_contested("voter-1") is None + def _join_key_evidence(card, **overrides): defaults = dict( diff --git a/MPCAutofill/cardpicker/tests/test_question_feed.py b/MPCAutofill/cardpicker/tests/test_question_feed.py index 25bffff42..9ace9d2ef 100644 --- a/MPCAutofill/cardpicker/tests/test_question_feed.py +++ b/MPCAutofill/cardpicker/tests/test_question_feed.py @@ -213,6 +213,64 @@ def test_own_vote_exclusion_is_scoped_to_the_specific_tag_not_the_whole_card(sel assert item.tagName == tag_b.name +class TestPhaseCNotOfficialArtRouting: + """ + 2026-08-04 gate on the phase-C/md5 routing brief (item 2): a card carrying a positive, + human-backed no-match-reason vote for one of `reason_tags.NOT_OFFICIAL_ART_REASON_TAGS` has + had its artwork question declared unanswerable by a human, so the feed must stop serving + artist-shaped questions for it - the printing question is a different matter and stays + unaffected. `NOT_OFFICIAL_PRINTING_REASON_TAGS` tags carry no such implication. + """ + + @staticmethod + def _artist_candidate(): + # RESOLVED printing + UNRESOLVED artist isolates this card to tier 4's artist half, + # mirroring test_tier_4_artist_when_no_printing_candidates_remain above. + return CardFactory( + printing_tag_status=PrintingTagStatus.RESOLVED, artist_vote_status=ArtistVoteStatus.UNRESOLVED + ) + + def test_a_not_official_art_vote_excludes_the_card_from_artist_questions(self, db): + card = self._artist_candidate() + tag = TagFactory(name="custom-art") + CardTagVoteFactory(card=card, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="crowd-1") + + assert get_next_question_feed_item("anon-1") is None + + def test_a_not_official_printing_vote_does_not_exclude_the_card(self, db): + card = self._artist_candidate() + tag = TagFactory(name="upscaled") + CardTagVoteFactory(card=card, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="crowd-1") + + item = get_next_question_feed_item("anon-1") + + assert item is not None + assert item.type.value == "artist" + assert item.card.identifier == card.identifier + + def test_a_negative_not_official_art_vote_does_not_exclude_the_card(self, db): + card = self._artist_candidate() + tag = TagFactory(name="external-ip") + CardTagVoteFactory(card=card, tag=tag, polarity=VotePolarity.NOT_APPLICABLE, anonymous_id="crowd-1") + + item = get_next_question_feed_item("anon-1") + + assert item is not None + assert item.card.identifier == card.identifier + + def test_a_machine_cast_not_official_art_vote_does_not_exclude_the_card(self, db): + card = self._artist_candidate() + tag = TagFactory(name="ai-art") + CardTagVoteFactory( + card=card, tag=tag, polarity=VotePolarity.APPLY, anonymous_id="ai-bot", source=VoteSource.DEDUCTION + ) + + item = get_next_question_feed_item("anon-1") + + assert item is not None + assert item.card.identifier == card.identifier + + class TestScryfallIllustrationUrl: """`_scryfall_illustration_url` (WTC artist question re-frame) surfaces the canonical printing's harvested Scryfall art-crop URL on artist-type feed items - see that function's diff --git a/MPCAutofill/cardpicker/tests/test_reason_tags.py b/MPCAutofill/cardpicker/tests/test_reason_tags.py index ef98ed273..71ef8e93a 100644 --- a/MPCAutofill/cardpicker/tests/test_reason_tags.py +++ b/MPCAutofill/cardpicker/tests/test_reason_tags.py @@ -3,6 +3,8 @@ from cardpicker.reason_tags import ( EXTERNAL_IP_TAG_NAME, NO_MATCH_REASON_TAGS, + NOT_OFFICIAL_ART_REASON_TAGS, + NOT_OFFICIAL_PRINTING_REASON_TAGS, seed_no_match_reason_tags, ) @@ -82,3 +84,17 @@ def test_reason_tags_are_case_distinct_from_default_tags(self, db): default_tag_names = {name for name, _aliases, _display_name in DEFAULT_TAGS} reason_tag_names = {name for name, _description, _display_name in NO_MATCH_REASON_TAGS} assert default_tag_names.isdisjoint(reason_tag_names) + + +class TestNoMatchReasonTagGroups: + # Mirrors NoMatchReasonStrip.spec.ts's exhaustiveness/disjointness assertion on the frontend + # side of this same WTC phase B split (reason_tags.py's module docstring). + def test_groups_are_disjoint(self): + assert NOT_OFFICIAL_PRINTING_REASON_TAGS.isdisjoint(NOT_OFFICIAL_ART_REASON_TAGS) + + def test_groups_are_exhaustive_over_the_full_taxonomy(self): + reason_tag_names = {name for name, _description, _display_name in NO_MATCH_REASON_TAGS} + assert NOT_OFFICIAL_PRINTING_REASON_TAGS | NOT_OFFICIAL_ART_REASON_TAGS == reason_tag_names + + def test_external_ip_is_in_the_not_official_art_group(self): + assert EXTERNAL_IP_TAG_NAME in NOT_OFFICIAL_ART_REASON_TAGS From 5cd77218ba3957c710cea7836e52dde8fdd575a4 Mon Sep 17 00:00:00 2001 From: wilfordgrimley <2397930+WilfordGrimley@users.noreply.github.com> Date: Tue, 4 Aug 2026 19:22:38 +0000 Subject: [PATCH 2/2] fix(ci): scope constant-rename gate to base-minus-head, not symmetric diff scope_modules() computed the affected-name set as a symmetric difference between base and head declarations, so a constant ADDED at head with nothing removed anywhere also pulled its module into the whole-module equivalence comparison. Since base never declared the new name, that module can never normalise identically to base - any PR merely adding a constant matching the pattern (SKIP_REASON|ANONYMOUS_ID|_VERSION|_WEIGHT| _THRESHOLD|_PREFIX|_REASON) failed this gate permanently. PR #686's own two new reason_tags.py constants tripped exactly this. Fix: use base - head (names that disappeared from base) instead of the symmetric difference, matching the function's own docstring ('modules that touch a constant whose DECLARED NAME changed'). Renames and removals are unaffected - the old name is still base-only either way, so the same module still gets pulled in and still gets the same whole-module comparison it got before. The removed/head-only reporting split at the print site is dead in its 'head only' branch under the new scope (moved is now always base-only), so it's replaced with a plain removed-name list plus a separate, purely-informational head-only listing an operator can use to see both halves of a rename without it affecting scope. One real tradeoff, surfaced and pinned rather than hidden: an ISOLATED literal-to-constant extraction (no companion rename/removal in the same module) is no longer auto-checked for a wrong extracted value, because it is provably indistinguishable, by name-diffing alone, from a harmless new constant backing brand-new logic - the exact shape that caused PR #686's false positive. Verified against real history (PR #567's LANDS_PHASH_SKIP_REASON_PREFIX) that this capability was only ever exercised when a companion rename existed in the same module, which remains fully covered; the isolated case is now pinned as a documented limitation in test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot rather than silently regressing. Adds three tests distinguishing the fix (pure addition -> nothing to prove, rename -> still scoped and still catches a real diff, removal -> still scoped) plus updates to the extraction-folding tests above. Verified: gate script exits 0 against this PR's actual base/head (was 4); test_constant_rename_equivalence.py alone (49 passed) and the full .github/scripts/tests/ suite together (214 passed, no order-dependence per issue #679); py_compile and pre-commit both clean. --- .../scripts/constant_rename_equivalence.py | 27 ++++++-- .../tests/test_constant_rename_equivalence.py | 69 ++++++++++++++++++- 2 files changed, 88 insertions(+), 8 deletions(-) diff --git a/.github/scripts/constant_rename_equivalence.py b/.github/scripts/constant_rename_equivalence.py index 07d8a27bb..738df5de9 100644 --- a/.github/scripts/constant_rename_equivalence.py +++ b/.github/scripts/constant_rename_equivalence.py @@ -793,10 +793,19 @@ def scope_modules(base: RevisionIndex, head: RevisionIndex, everything: bool) -> """Modules to compare, plus the renamed/removed constants that put them there. Scope is deliberately narrow: the modules that touch a constant whose DECLARED NAME changed between the two revisions. `--all` widens it - to every module mentioning the pattern.""" + to every module mentioning the pattern. + + A pure addition (a name present only at head) is NOT in scope: the + module declaring a brand-new constant necessarily differs from base, + so whole-module AST equality could never hold for it, and additions + were never what this gate proves. A removal or a rename both still + land here, because the old name is base-only either way — for a + rename, pulling the old name into scope compares the SAME module + that now declares the new name, so the equivalence proof still + covers it.""" base_names = base.declared_matching() head_names = head.declared_matching() - moved = {n: base_names.get(n) or head_names.get(n) or "?" for n in set(base_names) ^ set(head_names)} + moved = {n: base_names[n] for n in set(base_names) - set(head_names)} shared = sorted(set(base.sources) & set(head.sources)) if everything: @@ -988,10 +997,18 @@ def main(argv: list[str] | None = None) -> int: return len(findings) if moved: - print(f"Constants whose declared name changed between {base_rev} and {head_rev}:") + print(f"Constants removed or renamed away between {base_rev} and {head_rev}:") for name in sorted(moved): - side = "base only" if name in base.declared_matching() else "head only" - print(f" - {name} ({side}, {moved[name]})") + print(f" - {name} (was declared in {moved[name]})") + # Informational only — NOT part of scope. A rename's new name lands + # here too (it is head-only), so an operator reading a scope failure + # can see both halves; a pure addition unrelated to any removal above + # also lands here and is indistinguishable from one by name alone. + added = sorted(set(head.declared_matching()) - set(base.declared_matching())) + if added: + print(f"\nConstants declared only at {head_rev} (informational, not compared):") + for name in added: + print(f" - {name} ({head.declared_matching()[name]})") print() findings += check_equivalence(base, head, targets) diff --git a/.github/scripts/tests/test_constant_rename_equivalence.py b/.github/scripts/tests/test_constant_rename_equivalence.py index bffcf856d..c85b85f68 100644 --- a/.github/scripts/tests/test_constant_rename_equivalence.py +++ b/.github/scripts/tests/test_constant_rename_equivalence.py @@ -153,16 +153,48 @@ def test_fstring_prefix_extraction_is_clean(self): code, out = compare(root) self.assertEqual(code, 0, out) - def test_fstring_prefix_change_is_caught(self): - base = {"m.py": 'def f(reason):\n return f"phash-{reason}"\n'} + def test_fstring_prefix_change_is_caught_when_bundled_with_a_rename(self): + # An isolated extraction — nothing else matching the pattern renamed + # or removed anywhere in the module — cannot be told apart, by + # declared-name diffing alone, from a harmless brand-new constant for + # brand-new logic (see test_an_isolated_extraction_with_a_wrong_value_ + # is_a_known_scope_blind_spot below), so it needs a companion rename + # in the SAME module to land in scope — exactly how it happens in + # practice: PR #567 introduced LANDS_PHASH_SKIP_REASON_PREFIX in the + # same file as dozens of other real renames. + base = { + "m.py": 'A_SKIP_REASON = "a"\n\n\ndef f(x, reason):\n return A_SKIP_REASON if x else f"phash-{reason}"\n' + } head = { - "m.py": 'PHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\ndef f(reason):\n return f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n' + "m.py": 'B_SKIP_REASON = "a"\nPHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\n' + 'def f(x, reason):\n return B_SKIP_REASON if x else f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n' } with _repo(base, head) as root: code, out = compare(root) self.assertEqual(code, 1, out) self.assertIn("m.py", out) + def test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot(self): + # KNOWN LIMITATION, pinned deliberately, not a bug: base declares no + # matching constant at all in this module, so nothing "disappeared" + # from base and scope_modules correctly treats this exactly like a + # harmless brand-new constant for brand-new logic (see + # test_pure_addition_of_a_new_matching_constant_is_out_of_scope in + # TestScopeAndNotes) — the two shapes are provably identical under + # name-diffing; there is no way to tell them apart without also + # accepting whole-module comparisons for every unrelated new-feature + # PR that happens to add a matching-pattern constant, which is the + # false-positive class this gate exists to avoid. Pinned so a future + # scope_modules change doesn't silently start "catching" this again + # without someone noticing the tradeoff moved back the other way. + base = {"m.py": 'def f(reason):\n return f"phash-{reason}"\n'} + head = { + "m.py": 'PHASH_SKIP_REASON_PREFIX = "phash2-"\n\n\ndef f(reason):\n return f"{PHASH_SKIP_REASON_PREFIX}{reason}"\n' + } + with _repo(base, head) as root: + code, out = compare(root) + self.assertEqual(code, 0, out) + def test_string_concatenation_is_folded(self): base = {"m.py": 'def f():\n return "phash-" + "miss"\n'} head = {"m.py": 'PHASH_MISS_SKIP_REASON = "phash-miss"\n\n\ndef f():\n return PHASH_MISS_SKIP_REASON\n'} @@ -521,6 +553,37 @@ def test_default_base_falls_back_when_there_is_no_origin(self): code, out = run_tool(root, "--head", "HEAD") self.assertEqual(code, 0, out) + def test_pure_addition_of_a_new_matching_constant_is_out_of_scope(self): + # scope_modules used to take a SYMMETRIC difference of declared + # names, so a constant added at head with nothing removed anywhere + # put its own module in scope — and that module's AST can never + # equal base's, since base never declared the new name. Any PR + # merely adding a matching constant would fail this gate permanently. + base = {"m.py": "def f():\n return 1\n"} + head = {"m.py": 'NEW_SKIP_REASON = "x"\n\n\ndef f():\n return NEW_SKIP_REASON\n'} + with _repo(base, head) as root: + code, out = compare(root) + self.assertEqual(code, 0, out) + self.assertIn("nothing to prove", out) + + def test_rename_still_puts_the_module_in_scope_and_catches_a_real_diff(self): + base = {"m.py": 'A_SKIP_REASON = "x"\n\n\ndef f(v):\n return A_SKIP_REASON if v else ""\n'} + head = {"m.py": 'B_SKIP_REASON = "x"\n\n\ndef f(v):\n return B_SKIP_REASON if not v else ""\n'} + with _repo(base, head) as root: + code, out = compare(root) + self.assertEqual(code, 1, out) + self.assertIn("A_SKIP_REASON", out) + self.assertIn("m.py", out) + + def test_pure_removal_still_puts_the_module_in_scope(self): + base = {"m.py": 'A_SKIP_REASON = "x"\n\n\ndef f():\n return A_SKIP_REASON\n'} + head = {"m.py": 'def f():\n return "x"\n'} + with _repo(base, head) as root: + code, out = compare(root) + self.assertEqual(code, 0, out) + self.assertIn("A_SKIP_REASON", out) + self.assertNotIn("nothing to prove", out) + class TestNormaliserUnits(unittest.TestCase): def test_first_difference_locates_a_node_path(self):