fix(parser): UNSUPPORTED cluster: EachDealsDamage variant — 'each <object-class filter> you control #4625
Conversation
…ject-class filter> you control' Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
…-unsupported-cluster-eachdealsdamage-variant
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the EachSourceDealsDamage effect to support MTG rules where multiple matching objects independently deal a fixed amount of damage to a recipient. It includes the core resolution logic, parser integration, comprehensive unit tests, and updates to the AI threat classifier. Feedback on the changes highlights two key improvements: first, restricting the target-slot exclusion wildcard match in ability.rs to only EachController recipients so that Shared recipients can correctly surface their target slots; second, expanding the AI threat classifier's direct damage detection to include other player-resolving target filters like Player, Opponent, ParentTarget, and Controller.
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.
| // CR 109.4 + CR 120.3a: `EachController` and other non-`Shared` | ||
| // recipients resolve per source / context at the resolver, with no | ||
| // player-selectable target slot. | ||
| | Effect::EachSourceDealsDamage { .. } |
There was a problem hiding this comment.
The wildcard match Effect::EachSourceDealsDamage { .. } incorrectly matches both EachController and Shared recipients. However, Shared recipients (such as Princess Snowfall's 'any target') do have player-selectable target slots that should be surfaced by target_filter(). Matching all EachSourceDealsDamage here bypasses target slot generation for Shared recipients, contradicting the comment on lines 11995-11997. We should restrict this match to only EachController recipients.
| | Effect::EachSourceDealsDamage { .. } | |
| | Effect::EachSourceDealsDamage { recipient: EachDamageRecipient::EachController, .. } |
| if matches!( | ||
| recipient, | ||
| engine::types::ability::EachDamageRecipient::EachController | ||
| | engine::types::ability::EachDamageRecipient::Shared( | ||
| engine::types::ability::TargetFilter::Any | ||
| ) | ||
| ) { |
There was a problem hiding this comment.
The threat classifier only sets direct_damage = 1.0 for EachController and Shared(TargetFilter::Any). However, other Shared target filters like Player, Opponent, ParentTarget (e.g., Missy's 'that opponent'), and Controller also resolve to players and represent direct damage/burn. We should expand the matches to include these filters to ensure complete classifier coverage.
if matches!(
recipient,
engine::types::ability::EachDamageRecipient::EachController
| engine::types::ability::EachDamageRecipient::Shared(
engine::types::ability::TargetFilter::Any
| engine::types::ability::TargetFilter::Player
| engine::types::ability::TargetFilter::Opponent
| engine::types::ability::TargetFilter::ParentTarget
| engine::types::ability::TargetFilter::Controller
)
) {
Parse changes introduced by this PR · 5 card(s), 10 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] EachSourceDealsDamage pauses a simultaneous damage batch as fresh sequential DealDamage continuations. Evidence: crates/engine/src/game/effects/deal_damage.rs:1990 applies each source one at a time, and crates/engine/src/game/effects/deal_damage.rs:2001 stashes the remaining entries through stash_remaining_each_source_damage, which builds new DealDamage nodes. Why it matters: apply_damage_after_replacement can return NeedsChoice after damage was already dealt for lifelink/life-gain replacement (crates/engine/src/game/effects/deal_damage.rs:683-690), so the remaining sources get delayed into later resolution instead of preserving one simultaneous damage batch. Suggested fix: mirror the existing post-replacement survivor approach: collect/replace/apply the batch in phases and stash already-replaced survivors as ApplyPostReplacementDamage continuations when Phase C pauses.
[LOW] The AI threat classifier misses player-bound shared recipients introduced by this PR. Evidence: crates/phase-ai/src/threat_profile.rs:158-167 marks direct damage only for EachController or Shared(Any), while the new parser test shows Missy/Case parse player-bound recipients as Shared(ParentTarget) at crates/engine/src/parser/oracle_effect/subject.rs:5358-5377. Why it matters: those effects can burn a player but will not raise direct_damage, so AI threat probabilities undercount the new class. Suggested fix: either classify player-resolving shared recipients as direct damage or preserve recipient-kind context so ParentTarget can be distinguished by player/object binding.
2 participants
Summary
Fixes a parser misparse affecting 6 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED cluster: EachDealsDamage variant — 'each you control deals N damage to ' (NON-targeted class source, fixed/own amount)
Cards corrected
Fix
Implemented cluster 80 (
EachSourceDealsDamage— non-targeted class-source fixed-amount damage) on upstream/main worktree phase-main-base. Added a newEffect::EachSourceDealsDamage { sources: TargetFilter, amount: QuantityExpr, recipient: EachDamageRecipient }variant plus a typedEachDamageRecipient { Shared(TargetFilter) | EachController }enum (no booleans), a resolver that iterates the source class at resolution (CR 608.2) dealing per-source damage (CR 120.1), and a combinator-only parser dispatch. All 6 cluster cards now parse correctly in their nested locations: Missy (ChooseOneOf branch → Shared(ParentTarget)), Sarkhan the Masterless (Attacks trigger → Shared(TriggeringSource)), Case of the Gateway Express (ETB sub_ability → Shared(TriggeringSource), runtime-resolves to the chosen creature via SequentialSibling target propagation), Princess Snowfall (both triggers' sub_ability → Shared(Any)), Rakdos Charm mode 3 (EachController), Aura Barbs clause 1 (each enchantment → EachController). Aura Barbs clause 2 (attachment-host recipient) is converted from a LIVE misparse (DealDamage{Fixed 2, ParentTarget}) into an honest fail-closed Unimplemented "each_source_attached_damage" (CR 303.4 deferred). card-data regen confirms EXACTLY these 6 cards capture the variant — zero over-capture.Two runtime-critical fixes were discovered via card-tests and are NOT in the original plan but are required for correctness: (1) added EachSourceDealsDamage to extract_event_context_filter (effects/mod.rs) — without it the Shared(TriggeringSource)/Shared(ParentTarget) recipients of Sarkhan/Missy never bind at resolution and deal zero damage; (2) added it to the Any-target-slot guard (triggers.rs) so Princess Snowfall's "any target" generates a target slot. I also added three over-capture guards beyond the plan's too-broad starts_with_subject_prefix gate (require subject to begin with "each ", require predicate main verb to be "deals"/"deal", require a Fixed source-independent amount); these eliminated 14+ wrong captures (Enslave, Flametongue Kavu Avatar, Stensia's granted ability, Self-Destruct, Thing Swing, Polliwallop, Goblin Tinkerer, Bomb Squad, etc.). Reused the existing stash_remaining_each_source_damage helper instead of building the plan's proposed new stash function (building-block reuse), and extracted a shared resolve_effect_recipients authority used by both DealDamage::resolve and the new resolver.
Verification (cargo run directly, worktree not under Tilt): cargo fmt clean; cargo clippy --workspace --all-targets clean (no errors/warnings); cargo test -p engine full suite passes; cargo test -p phase-ai passes; 16 new tests pass (13 building-block parser tests including 7 negatives/deferred + 3 resolver/hydration tests, one a full resolve_ability_chain hydration round-trip proving Sarkhan's TriggeringSource recipient binds); parser diff gate on my added lines is clean; cargo semantic-audit shows 0 findings for the 6 cluster cards. All 14 CR numbers in the diff verified against docs/MagicCompRules.txt. Generated data artifacts (known-tokens.toml, oracle-subtypes.json) regenerated as a side-effect of gen-card-data were restored to HEAD; gitignored card-data.json was regenerated. Did NOT commit. No stop-and-return — the deferred Aura Barbs clause 2 is documented in scopeExpansion.
Files changed
CR references
Verification
cargo fmt --all— pass — clean, no files changed (no fmt commit needed)./scripts/check-parser-combinators.sh <upstream/main merge-base 03cdf89c5>— pass — exit 0cargo clippy -p engine --all-targets -- -D warnings— pass — exit 0, fully clean; no warnings in fix-changed files (no pre-existing debt surfaced for -p engine)cargo test -p engine— pass — all tests pass, zero failures (includes new resolve_each_source_deals_damage tests: Shared accumulate, EachController, Shared(TriggeringSource) hydration)cargo run --profile tool --features cli --bin oracle-gen -- data --filter 'missy|aura barbs|rakdos charm|sarkhan the masterless|case of the gateway express|princess snowfall'— pass — all 6 cards generated; each EachSourceDealsDamage clause inspected vs oracle text and confirmed correctCards confirmed re-parsed correctly: Rakdos Charm — mode 3 'Each creature deals 1 damage to its controller' → EachSourceDealsDamage{sources: Creature (all, controller=null), amount: 1, recipient: EachController}. Correct., Missy — end-step villainous-choice branch 1 'Each artifact creature you control deals 1 damage to that opponent' → EachSourceDealsDamage{sources: Artifact+Creature/You, amount: 1, recipient: Shared(ParentTarget)}. Correct (that opponent → ParentTarget)., Sarkhan the Masterless — attack trigger 'each Dragon you control deals 1 damage to that creature' → EachSourceDealsDamage{sources: Subtype Dragon/You, amount: 1, recipient: Shared(TriggeringSource)}. Correct (binds attacker via event hydration; covered by new test)., Case of the Gateway Express — ETB TargetOnly(creature you don't control) + SequentialSibling EachSourceDealsDamage{sources: Creature/You, amount: 1, recipient: Shared(TriggeringSource)} = 'Each creature you control deals 1 damage to that creature'. Correct (sibling chain pre-populates targets → chosen target). To-solve/Solved static parse fine., Princess Snowfall — both ETB and upkeep triggers: CopyTokenOf + sub EachSourceDealsDamage{sources: Subtype Dwarf/You, amount: 1, recipient: Shared(Any)} = 'each Dwarf you control deals 1 damage to any target'. Cluster clause correct. OUT-OF-SCOPE residual: TargetFallback warning on the 'Seven Dwarves' CopyTokenOf target (named-card-copy limitation), and the card is an un-set (UNK) printing — neither relates to the EachSourceDealsDamage cluster., Aura Barbs — clause 1 'Each enchantment deals 2 damage to its controller' → EachSourceDealsDamage{sources: Enchantment, amount: 2, recipient: EachController}. Correct. Clause 2 'each Aura...deals 2 damage to the creature it's attached to' → Unimplemented('each_source_attached_damage'): the explicitly-DEFERRED AttachedPermanent recipient (CR 303.4 §9), failing CLOSED by design rather than mis-dealing — documented in both the EachDamageRecipient enum and try_parse_each_source_deals_damage. Intentional, in-scope behavior of this fix.
🤖 Generated with Claude Code