fix(question_feed): phase-C not-official-art routing + md5-expand artist/tag exclusions - #686
Merged
Merged
Conversation
…ist/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).
… 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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:reason_tags.py(item 1): 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— mirroringthe frontend's
NoMatchReasonStrip.tsxNO_MATCH_REASON_TAG_GROUPS. A new test asserts theyare exhaustive and disjoint over
NO_MATCH_REASON_TAGS, the same invariantNoMatchReasonStrip.spec.tsasserts on the frontend side.question_feed.py(item 2, phase C routing): a card carrying a positive(
VotePolarity.APPLY)CardTagVotefor a not-official-art tag has had its artwork questiondeclared unanswerable by a human, so the feed now excludes it from artist-shaped questions in
both
_tier_2_contestedand_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 isunaffected. The exclusion requires a human-backed source
(
vote_consensus.is_human_backed_source), not merely "any positive vote" — see the commitmessage for the reasoning behind that judgment call.
question_feed.py(item 3, md5 expansion):_tier_2_contested's artist and tag own-voteexclusions 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-identicalgroup is not re-asked the identical artist/tag question under a sibling's identifier — the
same convention
_voter_answered_printing_card_idsalready established for the printingtiers. 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 perreview pair) plus one md5 expansion per distinct tag the voter has touched. Per the
engineering brief, this is scoped to
_tier_2_contestedonly —_tier_4_fresh's ownartist/tag own-vote exclusions keep their pre-existing, unwidened form (see Open items below).
Observation (out of scope, no implementation):
local_illustration. _eligible_illustration_cards_queryset(the machine illustration calculator) has no awareness ofNOT_OFFICIAL_ART_REASON_TAGSeither, and plausibly warrants the same exclusion for the samereason phase C does. Left as a follow-up, not implemented here.
Test plan
test_reason_tags.pyrun alone — 10 passed (includes 3 new tests for the partition'sexhaustiveness/disjointness and the
external-ipgroup membership)test_question_feed.pyrun alone — 50 passed (includes 4 new tests inTestPhaseCNotOfficialArtRouting: positive not-official-art excludes the artist question,not-official-printing doesn't, a negative vote doesn't, a machine-cast vote doesn't)
test_md5_group_pooling.pyrun alone — 72 passed (includes 6 new tests inTestPhaseCAndTierTwoMd5Expansioncovering artist/tag md5 widening, siblingnon-re-serving, the per-tag-axis regression
_tier_2_contested's existing comment warnsagainst, 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 Pool printing-consensus votes across md5 identity groups #482
condition f1 test)
Test suite is order-dependent: leaked fetch-failure window trips the envelope across files (8 failures on master) #679's concern)
python -m py_compileon all five changed files — cleanmypy— zero new errors; the two pre-existingquestion_feed.pyhits (lines 158/583) andreason_tags.py's zero-error baseline are both untouched by this change (repo-wide baselineis ~290 django-stubs related-manager errors across 20 files, none introduced here)
pre-commit runover the five changed files — all green (black reformatted one test fileon the first pass; re-ran clean after)
Task-end checks (CLAUDE.md)
docs/reference file describes this feed's routing behavior in a waythat needs updating in place (the design is captured in
question_feed.py's owndocstrings, which this PR extends)
no new user-visible UI or admin action) — no wiki page needs
<change>for auser-invisible routing fix; confirming this judgment stands as owner review proceeds
master, not mergedCI gate fix:
constant_rename_equivalence.pyfalse positive on pure additionsThe "Renamed constants are behaviour-preserving" check was failing on this
PR itself:
scope_modules()computed the set of "renamed/removed" constantnames as a symmetric difference between base and head declarations, so
a constant added at head with nothing removed anywhere (this PR's two new
NOT_OFFICIAL_PRINTING_REASON_TAGS/NOT_OFFICIAL_ART_REASON_TAGS) pulledreason_tags.pyandtest_reason_tags.pyinto the whole-module equivalencecomparison. Since base never declared those names, the modules can never
normalise identically — this failure mode is unpassable by construction for
any PR that merely adds a matching-pattern constant.
Fix: the affected-name set now uses
base − head(names that disappearedfrom base) instead of the symmetric difference, matching the function's own
pre-existing docstring ("modules that touch a constant whose DECLARED NAME
changed"). Renames and removals are still fully scoped and checked — only
pure additions with nothing removed anywhere in the same module drop out.
One real tradeoff surfaced and is accepted, not 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 this PR's
false positive. In practice this capability was only ever exercised when a
companion rename existed in the same module (verified against PR #567's own
history), which remains fully covered; the isolated case is pinned as a
documented, deliberate limitation in
test_an_isolated_extraction_with_a_wrong_value_is_a_known_scope_blind_spot.test_constant_rename_equivalence.pyalone — 49 passed.github/scripts/tests/suite together — 214 passed (order-independence per issue Test suite is order-dependent: leaked fetch-failure window trips the envelope across files (8 failures on master) #679)
python -m py_compileon both touched files — cleanpre-commit runon both touched files — clean (black reformatted thetest file on the first pass; re-ran clean after)