Fix Call the Spirit Dragons upkeep counters and win rider (#5274) - #5584
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the ForEachCategoryPutCounter effect and parses reflexive counter-placement conditions to support cards like 'Call the Spirit Dragons' (resolving issue #5274). Feedback focuses on strictly adhering to MTG Comprehensive Rules by guarding counter additions and tracked set updates against zero counter quantities in both the auto-apply and choice resolution paths. Additionally, the parser should be enhanced to support reflexive counter-placement conditions where the specific counter type is omitted.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if cards.len() == 1 { | ||
| let object_id = cards[0]; | ||
| let count_val = | ||
| crate::game::quantity::resolve_quantity_with_targets(state, count, ability) | ||
| .max(0) as u32; | ||
| crate::game::effects::counters::apply_counter_addition( | ||
| state, | ||
| ability.controller, | ||
| object_id, | ||
| counter_type.clone(), | ||
| count_val, | ||
| events, | ||
| ); | ||
| publish_tracked_set_unique(state, &[object_id]); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Guard counter addition and tracked set publication against zero counter quantities.
If count_val is 0, no counters are actually placed on the permanent (CR 122.1). Therefore, we should not call apply_counter_addition or add the object to the tracked set of objects that received counters "this way" (CR 608.2c).
if cards.len() == 1 {
let object_id = cards[0];
let count_val =
crate::game::quantity::resolve_quantity_with_targets(state, count, ability)
.max(0) as u32;
if count_val > 0 {
crate::game::effects::counters::apply_counter_addition(
state,
ability.controller,
object_id,
counter_type.clone(),
count_val,
events,
);
publish_tracked_set_unique(state, &[object_id]);
}
continue;
}References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| let count_val = | ||
| crate::game::quantity::resolve_quantity_with_targets(state, count, &ability).max(0) | ||
| as u32; | ||
| for &card_id in chosen { | ||
| crate::game::effects::counters::apply_counter_addition( | ||
| state, | ||
| ability.controller, | ||
| card_id, | ||
| counter_type.clone(), | ||
| count_val, | ||
| events, | ||
| ); | ||
| } | ||
| if !chosen.is_empty() { | ||
| publish_tracked_set_unique(state, chosen); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Guard counter addition and tracked set publication against zero counter quantities in the choice resolution path.
Just like the auto-apply path, if count_val is 0, no counters are placed on the chosen permanents (CR 122.1). We should avoid calling apply_counter_addition and publishing them to the tracked set.
let count_val =
crate::game::quantity::resolve_quantity_with_targets(state, count, &ability).max(0)
as u32;
if count_val > 0 {
for &card_id in chosen {
crate::game::effects::counters::apply_counter_addition(
state,
ability.controller,
card_id,
counter_type.clone(),
count_val,
events,
);
}
if !chosen.is_empty() {
publish_tracked_set_unique(state, chosen);
}
}References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| let (rest, _) = opt(terminated( | ||
| super::primitives::parse_counter_type_typed, | ||
| alt((tag(" counters "), tag(" counter "))), | ||
| )) | ||
| .parse(rest)?; |
There was a problem hiding this comment.
[MEDIUM] Support parsing reflexive counter-placement conditions when the counter type is omitted.
If a card says "If you put counters on five Dragons this way" (omitting the specific counter type), the current parser will fail because opt(...) won't consume the word "counters" or "counter", causing the subsequent tag("on ") to fail. We should consume the word "counters" or "counter" even if the counter type is omitted.
let (rest, _) = alt((
terminated(
super::primitives::parse_counter_type_typed,
alt((tag(" counters "), tag(" counter "))),
),
alt((tag("counters "), tag("counter "))),
))
.parse(rest)?;References
- Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators. (link)
Parse changes introduced by this PR · 3 card(s), 6 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the parse work is sound, but the PR ships a new Effect variant that isn't registered at two required sites, regenerates an unrelated 230k-line data file against a different snapshot, and adds an enum sibling that should be a parameterization. CI is red on 5 checks; two of those are the missing registrations.
🔴 New Effect variant not registered — non-exhaustive matches (CI red)
Effect::ForEachCategoryPutCounter is added to types/ability.rs but two phase-ai matches were never updated:
crates/phase-ai/src/policies/redundancy_avoidance.rs:279—non-exhaustive patterns: &engine::types::Effect::ForEachCategoryPutCounter { .. } not coveredcrates/phase-ai/src/policies/effect_classify.rs:52— same
This is exactly what the add-engine-effect skill checklist exists to prevent — it enumerates every registration point that must move in lockstep (types → parser → resolver → targeting → multiplayer filter → frontend → AI → tests). Please run it end-to-end rather than fixing just the two the compiler named; the compiler only catches the exhaustive matches, and the checklist covers the non-exhaustive registration points it cannot catch.
🔴 known-tokens.toml: −6,015 lines of unrelated data
crates/engine/data/known-tokens.toml goes +20 / −6,015 (230,395 → 224,400 lines). The removals are [[token.source_card_refs]] provenance blocks for cards with nothing to do with this PR — Giant Opportunity, Aven Initiate, Waylay, and ~6,000 lines more.
That file is generated (crates/engine/src/bin/tokens_gen.rs, driven from build.rs / gen-card-data.sh). This looks like a local regeneration against a different or stale Scryfall snapshot, committed by accident. A Call the Spirit Dragons fix must not shrink the token catalog. Please drop the file from the diff entirely (git checkout origin/main -- crates/engine/data/known-tokens.toml) and re-verify.
🔴 ForEachCategoryPutCounter is a sibling that should be a parameterization
Main already has Effect::ForEachCategoryExile (types/ability.rs:11449), and the shared IterationCategory enum (ability.rs:114) is already factored out. Your own doc comment names the relationship: "Category-iteration sibling of Effect::ForEachCategoryExile."
The two variants differ only in the terminal action — both iterate a fixed IterationCategory, both augment a base TargetFilter with the per-member restriction, both carry a Chooser, both accumulate into the chain tracked set. That is a leaf-level parameterization of one structural axis, which CLAUDE.md's "Parameterize, don't proliferate" rule asks you to refactor rather than extend:
Before adding a sibling variant to an enum, ask: is the new variant a leaf-level parameterization of an existing variant's structural axis? … one sibling is cheap, ten siblings make the eventual refactor multi-week as call sites multiply across parser, converter, resolver, and tests.
The second sibling is the cheapest possible moment to fix this. The composable form is a single category-iteration effect carrying the per-member body, so the next card in this class (ForEachCategoryDestroy, ForEachCategoryDraw, …) is a leaf parameter and not another variant plus another eight match arms. Note this is the direct cause of blocker #1 — every new sibling multiplies the registration sites that can be missed.
🟡 Non-blocking
crates/engine/src/game/ability_scan.rs:1202 — clippy: this pattern is unneeded as the .. pattern can match that element.
✅ Clean
The parse work itself reads well, and the CR annotations check out — CR 608.2c, CR 105.1, CR 122.1, CR 205.2a and CR 700.2 are all real and apposite. The four-test plan (parser unit → chained win-rider → reflexive this way gate → five-Dragon integration) is the right shape, and call_the_spirit_dragons_upkeep_puts_counters_on_five_dragons_and_wins is a genuinely discriminating end-to-end test.
Recommendation: restore known-tokens.toml from main, run the add-engine-effect checklist to completion, and — before re-pushing — collapse ForEachCategoryExile + ForEachCategoryPutCounter into one parameterized category-iteration effect. The third item is the one worth doing properly now; the other two are mechanical.
…hase-rs#5274) Collapse ForEachCategoryExile and ForEachCategoryPutCounter into Effect::ForEachCategory { action: ForEachCategoryAction }. Restore known-tokens.toml from main, register phase-ai match arms, and apply review nits (ability_scan player_scope clear, CR 205.3m). Co-authored-by: Cursor <cursoragent@cursor.com>
d1b7ff5 to
676aa9c
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved. All three blockers resolved on 676aa9c608, and the architectural one was resolved the right way rather than the cheap way.
✅ Blocker 3 — parameterized, not proliferated (the one that mattered)
You didn't add ForEachCategoryPutCounter as a sibling. You collapsed the axis:
ForEachCategory { category: IterationCategory, chooser, action: ForEachCategoryAction }
enum ForEachCategoryAction {
ExileFromPool { zone, up_to },
PutCounter { target, counter_type, count },
}The category axis stays shared; only the per-member terminal action varies. That is exactly the parameterization the sibling was hiding, and it's the right cut — IterationCategory was already factored out, so the two variants genuinely differed only in the terminal body.
The payoff is measurable, and it's why blocker 1 disappeared rather than being fixed. Adding the counter behavior required zero new match arms. Every registration site is a +1/−1 rename:
analysis/ability_graph.rs:957·game/effects/mod.rs:3354·game/printed_cards.rs:1160phase-ai/policies/effect_classify.rs:288·phase-ai/policies/redundancy_avoidance.rs:509- plus four
impl Effectarms andeffect_variant_name
A sibling would have needed a new arm at every one of those, and the original head proved the point by missing two of them. This is CLAUDE.md's "one sibling is cheap, ten siblings make the eventual refactor multi-week" — caught at sibling #2, which is the cheapest moment it could have been caught.
The EffectKind mapping is correctly action-aware rather than collapsed:
ForEachCategory { action: PutCounter { .. }, .. } => EffectKind::PutCounter,
ForEachCategory { .. } => EffectKind::ChooseFromZone,✅ Blocker 2 — generated-file contamination gone
known-tokens.toml is absent from the diff entirely (was +20/−6,015). Verified against the raw diff, not the description.
✅ Blocker 1 — registration
Fixed by construction, per above. Paired-seed AI gate passes, which is the right result: the phase-ai changes are pure renames with zero behavior delta.
✅ Evidence
Parse-diff (baseline 7913f7648e) — 3 cards, and it proves the refactor is semantically neutral. Sanar and Portent of Calamity appear as clean ForEachCategoryExile → ForEachCategory removed/added pairs with identical category/zone payloads: a pure rename, no behavior change. Call the Spirit Dragons goes from a bare for — the dropped clause that was the bug — to ForEachCategory (category=color, counter_type=P1P1, target=you control Dragon). Exactly the claimed scope, zero collateral.
The runtime test discriminates. call_the_spirit_dragons_upkeep_puts_counters_on_five_dragons_and_wins asserts real game state on both halves: each of five Dragons carries exactly one +1/+1 counter, and P0 wins. The win rider is the hard part — it depends on the "this way" tracked-set gate, not just on counters landing — and asserting it is what makes this close #5274 rather than merely parse it. mod line registered in tests/integration/main.rs, so no inert false green.
The existing Sanar test wasn't weakened in the migration — zone and up_to are both still pinned; the diff is purely the mechanical reshape into ExileFromPool { .. }.
CRs all grep-verify: 105.1 (five colors), 205.2a (card types), 122.1 (counters), 608.2c (instruction order / "this way" tracked set), 608.2d (up_to optional pick), 104.2b (an effect may state a player wins).
🟡 Non-blocking, and not your bug
The chooser doc carries CR 700.2, which is about modal spells ("two or more options in a bulleted list") — it does not govern "who makes each per-member choice." But you inherited it: it was already on the line you reworded, and it appears 21× in types/ability.rs on main. That's a systemic miscitation of the same class main just started sweeping in 1ad208b4ed. I'll file it separately; don't touch it here.
Enqueueing — Decision-cost perf gate is still pending and merge-when-ready will wait for it.
Closes #5274
Summary
for each color, put a +1/+1 counter on a Dragon you control of that colorasEffect::ForEachCategoryPutCounter(Call the Spirit Dragons upkeep).this waywin gate.If you put +1/+1 counters on five Dragons this way, you win the gameas a gatedWinTheGamesub-ability (FilteredTrackedSetSize >= 5).Test plan
for_each_color_put_counter_on_typed_permanent_of_that_color— upkeep clause →ForEachCategoryPutCountercall_the_spirit_dragons_upkeep_parses_put_counter_and_win_rider— chainedWinTheGamegated byFilteredTrackedSetSize >= 5s07_if_put_counters_on_type_this_way— reflexiveif you put … this waygate strips correctlycall_the_spirit_dragons_upkeep_puts_counters_on_five_dragons_and_wins— integration: five WUBRG Dragons, P0 winsVerification