fix(engine): Coalition Victory only wins if you control a land of each basic type and a creature of each color#5049
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The new Coalition Victory comments cite the wrong Comprehensive Rules for this effect-based win condition. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:2855, crates/engine/src/parser/oracle_nom/condition.rs:2872, crates/engine/src/parser/oracle_nom/condition.rs:2889, and crates/engine/src/parser/oracle_nom/condition.rs:2912 cite CR 603.4, which docs/MagicCompRules.txt defines only for triggered abilities with intervening “if” clauses; crates/engine/src/parser/oracle_nom/condition.rs:2889 and crates/engine/src/parser/oracle_nom/condition.rs:2912 also cite CR 104.2a, but that rule is last-player-standing while CR 104.2b is the effect-states-you-win rule. Why it matters: this repo requires verified CR annotations, and wrong rule citations create false evidence around rules-sensitive parser/engine code. Suggested fix: remove CR 603.4 here and cite the effect/resolution rules that actually apply, e.g. CR 104.2b plus CR 608.2c/CR 608.2h for evaluating the conditional game-state check during resolution.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
|
Thanks — you're right, and I verified against
Updated in
Comment-only change; no logic or test behavior changed. |
|
Reviewed current head I’m still holding approval/enqueue because this PR is missing the current AI-CONTRIBUTOR proof/template sections that the maintainer review loop requires before queueing. Please fill in the template sections for files changed / track / LLM disclosure / verification, including the exact test evidence you already summarized here. Once that proof is in the PR body, this can be reconsidered without another code-path blocker from me. |
…h basic type and a creature of each color
Coalition Victory's Oracle text is "You win the game if you control a land of
each basic land type and a creature of each color." The trailing "if …"
condition was not parsed, so the spell reduced to a bare Effect::WinTheGame and
casting it won the game immediately regardless of the board.
Teach the condition parser the two "of each …" clauses and compose them:
parse_control_conditions now recognizes "you control a land of each basic land
type and a creature of each color" and yields
And[ BasicLandTypeCount{You} >= 5,
DistinctColorsAmongPermanents{creatures you control} >= 5 ]
reusing the existing domain and distinct-color quantities. The two sub-clauses
are factored into standalone combinators (parse_land_of_each_basic_land_type,
parse_creature_of_each_color) so they cover every "of each basic land type" /
"of each color" clause, not just this card. WinTheGame is already
trailing-condition-eligible, so the parsed condition attaches to the spell and
is checked once on resolution (CR 104.2a + CR 603.4).
Tests (runtime, real cast pipeline):
- coalition_victory_does_not_win_without_the_board: casting with an empty board
does NOT win (condition unmet). Fails on the old bare-WinTheGame parse, which
won unconditionally (revert-probed).
- coalition_victory_wins_with_full_domain_and_all_colors: controlling a land of
each basic type and a creature of each color, casting wins the game.
Coalition Victory's effect-based win condition is a spell, not a triggered ability, so CR 603.4 (the intervening-"if" rule, which applies only to triggered abilities) does not apply, and CR 104.2a is the last-player-standing rule rather than the effect-states-you-win rule. - Win-condition sites now cite CR 104.2b (an effect may state that a player wins the game) + CR 608.2h (game-state information is determined once, as the spell resolves). - The "of each basic land type" / "of each color" sub-combinators keep only their subject-matter rules (CR 205.3i basic land types, CR 105.2 colors) and drop the erroneous CR 603.4. Comment-only change; no logic or test behavior changes.
05b6a39 to
82e1e7a
Compare
|
Current-head review on I rechecked the latest push. The implementation still looks scoped and compositional: it parses the Coalition Victory condition by composing reusable “land of each basic land type” and “creature of each color” condition helpers, and the runtime regression covers both the false board and the satisfied board through the cast pipeline. I am still holding approval/enqueue because the AI-contributor proof/template sections are missing and checks were not complete for this exact head when reviewed. Please add the current-head summary/files/verification/LLM disclosure and let CI finish. |
|
Friendly nudge: the CHANGES_REQUESTED on the CR annotations was addressed in |
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 82e1e7a3a142eafb45897cc33b377720739db738.
The prior CR blocker is addressed: the Coalition Victory win condition cites the effect-based win and resolution-time information rules, and the of each condition helpers keep their color/basic-land-type CR grounding. The implementation is scoped to the condition parser/runtime seam and composes the existing conditional WinTheGame path rather than adding card-specific resolution logic.
Parse-diff evidence is scoped to Coalition Victory only, changing the WinTheGame.conditional field from absent to the expected “basic land types among lands you control >= 5 and colors among creatures you control >= 5” condition. The runtime regression covers the false board and satisfied board through the cast pipeline. The PR body still has template gaps, but the current comments and green current-head checks provide enough concrete proof for this maintainer review.
Problem
Coalition Victory (Time Spiral) reads:
The trailing "if …" condition was not parsed, so the spell reduced to a bare
Effect::WinTheGame(condition: null) — casting Coalition Victory won the game immediately, regardless of the board.Fix
Teach the condition parser the two "of each …" clauses and compose them.
parse_control_conditionsnow recognizes "you control a land of each basic land type and a creature of each color" and yields:reusing the existing domain (
BasicLandTypeCount) and distinct-color (DistinctColorsAmongPermanents) quantities — no new condition types. The two sub-clauses are factored into standalone combinators (parse_land_of_each_basic_land_type,parse_creature_of_each_color) so they cover every "of each basic land type" / "of each color" clause, not just this card (build-for-the-class).WinTheGameis already trailing-condition-eligible, so the parsed condition attaches to the spell and is checked once on resolution.Tests (runtime, real cast pipeline)
coalition_victory_does_not_win_without_the_board— casting with an empty board does not win (condition unmet). Fails on the old bare-WinTheGameparse, which won unconditionally (revert-probed locally: the assertion fails when the combinator arm is removed).coalition_victory_wins_with_full_domain_and_all_colors— controlling a land of each basic type (Plains/Island/Swamp/Mountain/Forest) and a creature of each color (W/U/B/R/G), casting wins the game.