fix(engine): resolve player-scope bugs in Mercenaries and Total War - #5646
Conversation
Both cards fall under misparse-backlog root-cause category phase-rs#9 (wrong player/controller scope): - Mercenaries ("Any player may activate this ability" + a one-shot "would deal damage to you" prevention shield): the shield's recipient scope was hardcoded to `Any` rather than binding to the activator (CR 602.2a: an activated ability's controller is whoever activated it). This also affected ~27 sibling cards using the same one-shot prevention shape (Circle/Rune of Protection and others), all of which previously had zero recipient restriction on their shield at runtime. Fixed by adding a "to you" recognizer to the shared damage-recipient combinator and threading it through the prevention branch instead of discarding it. - Total War ("whenever a player attacks... destroy all ... creatures that player controls..."): the destroy effect's controller filter was hardcoded to the enchantment's own controller instead of the attacking player who triggered it. Fixed by adding a new dispatch arm recognizing "a player attacks with" as introducing the existing ControllerRef::TriggeringPlayer scope for the effect body's "that player" anaphor. - Typhoon ("deals damage to each opponent equal to the number of Islands that player controls"): verified already correct end-to-end (per-opponent ScopedPlayer binding resolves correctly in the runtime fan-out); added regression tests only, no code change. Total War's separate "except for creatures the player hasn't controlled continuously since the beginning of the turn" exemption clause remains unimplemented (a distinct dropped-modifier-clause root cause, not category phase-rs#9) and is documented as a known gap at the parse site for follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 37 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Approved. This is what "build for the class, not the card" looks like when it's done right — and the parse-diff proves it rather than asserting it.
The architecture is the point
You didn't add a Mercenaries branch. You added a "to you" arm to the shared damage_target_controller() combinator — and because the durable-replacement path already called that combinator, it improved too, for free, without you touching it. That's the difference between a fix and a building block.
What the measurement actually says
Sticky on 0549521709, baseline main e5871f5694 — I verified that baseline is a genuine ancestor of current main (2 commits back, so no staleness inflation). 37 cards, 3 signatures, every one of them in the same class and every one in the correct direction:
| # | signature | direction |
|---|---|---|
| 33 | PreventDamage · target |
any target → controller |
| 3 | CreateDamageReplacement · target_filter |
∅ → Player { Controller } |
| 1 | DestroyAll · filter (Total War) |
you control → triggering player controls |
The 33 Circles/Runes of Protection, Bone Mask, Righteous Aura et al. previously had zero recipient restriction — the shield would prevent damage to any player. The 3 durable ones (Beacon of Destiny, General's Regalia, Nova Pentacle) were unscoped entirely (∅). Those three are collateral you didn't call out; they're correct, and they're the combinator paying you back.
One note on the count: the body says "27 sibling cards." The sticky computes 33 for that signature (37 total). You undercounted rather than overclaimed, and you were explicit that it wasn't a closure claim — so this costs you nothing on correctness. But it's why I'm not adding " isn't a measurement when the sticky renders on your own PR and will tell you the exact figure.quality: that label requires the body's number to be the measured number, and "
CR verification
CR 602.2a is governing, not merely real — I grepped it: "Its controller is the player who activated the ability." That is precisely what makes ControllerRef::Controller the correct binding for Mercenaries and for a Circle of Protection simultaneously: for the Circle you're the activator, for Mercenaries under "any player may activate" the opponent is. One binding, both semantics, because the CR already unified them.
The test that earns it
mercenaries_any_player_activate_prevention_scope.rs has the opponent activate the ability through apply() — the real action path — and asserts the shield protects the activator, not Mercenaries' controller. That is the discriminating assertion; a test that activated it yourself would pass under the old broken Any scope too.
typhoon_per_opponent_island_count.rs is test-only and you said so plainly ("verified already correct end-to-end; no code change"), which is exactly why its absence from the parse-diff is a non-event rather than an inert-fix flag. It pins per-opponent ScopedPlayer fan-out — the same player-scope axis the other two fixes move — so it guards the blast radius. It earns its place.
All three land in tests/integration/ with mod lines in main.rs, not as new top-level binaries. Labeled bug: unscoped → scoped is wrong-to-right.
…lter Total War — "Whenever a player attacks with one or more creatures, destroy all untapped non-Wall creatures that player controls that didn't attack, except for creatures the player hasn't controlled continuously since the beginning of the turn." — silently dropped its trailing continuity exemption: the DestroyAll filter parsed only as [Untapped, Not(AttackedThisTurn)], so it also destroyed creatures the attacking player gained this turn (CR 302.6 + CR 508.1a). This was a documented KNOWN GAP: the existing continuity recognizer (parse_continuity_exemption_clause) only covers Siren's Call's "ignore this effect for each creature ... didn't control continuously ..." ActivePlayerPunisher shape. Total War phrases the same exemption as a trailing "except for creatures the player hasn't controlled continuously ..." on the target population. Add parse_except_continuity_exemption_suffix on the target-filter parse path (oracle_target.rs), a sibling of the "except for <type-list>" exclusion, placed after the controller / "didn't attack" relative clauses it trails. It attaches the existing FilterProp::ControlledContinuouslySinceTurnBegan (the same restriction Siren's Call reaches by its own path), so only creatures the player has controlled continuously since the turn began are destroyed. Reuses the existing FilterProp; no new engine variant. Blast radius: Total War alone (sole card with this phrasing). The player-scope half of Total War was fixed separately in phase-rs#5646.
…lter Total War — "Whenever a player attacks with one or more creatures, destroy all untapped non-Wall creatures that player controls that didn't attack, except for creatures the player hasn't controlled continuously since the beginning of the turn." — silently dropped its trailing continuity exemption: the DestroyAll filter parsed only as [Untapped, Not(AttackedThisTurn)], so it also destroyed creatures the attacking player gained this turn (CR 302.6 + CR 508.1a). This was a documented KNOWN GAP: the existing continuity recognizer (parse_continuity_exemption_clause) only covers Siren's Call's "ignore this effect for each creature ... didn't control continuously ..." ActivePlayerPunisher shape. Total War phrases the same exemption as a trailing "except for creatures the player hasn't controlled continuously ..." on the target population. Add parse_except_continuity_exemption_suffix on the target-filter parse path (oracle_target.rs), a sibling of the "except for <type-list>" exclusion, placed after the controller / "didn't attack" relative clauses it trails. It attaches the existing FilterProp::ControlledContinuouslySinceTurnBegan (the same restriction Siren's Call reaches by its own path), so only creatures the player has controlled continuously since the turn began are destroyed. Reuses the existing FilterProp; no new engine variant. Blast radius: Total War alone (sole card with this phrasing). The player-scope half of Total War was fixed separately in phase-rs#5646.
…lter Total War — "Whenever a player attacks with one or more creatures, destroy all untapped non-Wall creatures that player controls that didn't attack, except for creatures the player hasn't controlled continuously since the beginning of the turn." — silently dropped its trailing continuity exemption: the DestroyAll filter parsed only as [Untapped, Not(AttackedThisTurn)], so it also destroyed creatures the attacking player gained this turn (CR 302.6 + CR 508.1a). This was a documented KNOWN GAP: the existing continuity recognizer (parse_continuity_exemption_clause) only covers Siren's Call's "ignore this effect for each creature ... didn't control continuously ..." ActivePlayerPunisher shape. Total War phrases the same exemption as a trailing "except for creatures the player hasn't controlled continuously ..." on the target population. Add parse_except_continuity_exemption_suffix on the target-filter parse path (oracle_target.rs), a sibling of the "except for <type-list>" exclusion, placed after the controller / "didn't attack" relative clauses it trails. It attaches the existing FilterProp::ControlledContinuouslySinceTurnBegan (the same restriction Siren's Call reaches by its own path), so only creatures the player has controlled continuously since the turn began are destroyed. Reuses the existing FilterProp; no new engine variant. Blast radius: Total War alone (sole card with this phrasing). The player-scope half of Total War was fixed separately in phase-rs#5646.
…lter Total War — "Whenever a player attacks with one or more creatures, destroy all untapped non-Wall creatures that player controls that didn't attack, except for creatures the player hasn't controlled continuously since the beginning of the turn." — silently dropped its trailing continuity exemption: the DestroyAll filter parsed only as [Untapped, Not(AttackedThisTurn)], so it also destroyed creatures the attacking player gained this turn (CR 302.6 + CR 508.1a). This was a documented KNOWN GAP: the existing continuity recognizer (parse_continuity_exemption_clause) only covers Siren's Call's "ignore this effect for each creature ... didn't control continuously ..." ActivePlayerPunisher shape. Total War phrases the same exemption as a trailing "except for creatures the player hasn't controlled continuously ..." on the target population. Add parse_except_continuity_exemption_suffix on the target-filter parse path (oracle_target.rs), a sibling of the "except for <type-list>" exclusion, placed after the controller / "didn't attack" relative clauses it trails. It attaches the existing FilterProp::ControlledContinuouslySinceTurnBegan (the same restriction Siren's Call reaches by its own path), so only creatures the player has controlled continuously since the turn began are destroyed. Reuses the existing FilterProp; no new engine variant. Blast radius: Total War alone (sole card with this phrasing). The player-scope half of Total War was fixed separately in phase-rs#5646.
…lter (phase-rs#5787) Total War — "Whenever a player attacks with one or more creatures, destroy all untapped non-Wall creatures that player controls that didn't attack, except for creatures the player hasn't controlled continuously since the beginning of the turn." — silently dropped its trailing continuity exemption: the DestroyAll filter parsed only as [Untapped, Not(AttackedThisTurn)], so it also destroyed creatures the attacking player gained this turn (CR 302.6 + CR 508.1a). This was a documented KNOWN GAP: the existing continuity recognizer (parse_continuity_exemption_clause) only covers Siren's Call's "ignore this effect for each creature ... didn't control continuously ..." ActivePlayerPunisher shape. Total War phrases the same exemption as a trailing "except for creatures the player hasn't controlled continuously ..." on the target population. Add parse_except_continuity_exemption_suffix on the target-filter parse path (oracle_target.rs), a sibling of the "except for <type-list>" exclusion, placed after the controller / "didn't attack" relative clauses it trails. It attaches the existing FilterProp::ControlledContinuouslySinceTurnBegan (the same restriction Siren's Call reaches by its own path), so only creatures the player has controlled continuously since the turn began are destroyed. Reuses the existing FilterProp; no new engine variant. Blast radius: Total War alone (sole card with this phrasing). The player-scope half of Total War was fixed separately in phase-rs#5646. Co-authored-by: jaytbarimbao-collab <300663773+jaytbarimbao-collab@users.noreply.github.com>
Summary
Three old-school cards from misparse-backlog root-cause category #9 ("wrong player/controller scope"):
Anyinstead of binding to the ability's activator (CR 602.2a: an activated ability's controller is whoever activated it — relevant here because "any player may activate this ability"). Fixed by adding a "to you" recognizer to the shared damage-recipient combinator (damage_target_controller(), already used correctly by the durable-replacement path) and threading it through the one-shot prevention branch instead of discarding the parsed recipient scope.None/unscoped forTargetFilter::Any). Most of these aren't tracked under category chore: update coverage stats and badges #9 specifically; this is a collateral correctness fix from building the general combinator, not a claim of closing that count.ControllerRef::TriggeringPlayerscope (already used elsewhere, already correctly resolved at runtime via theAttackersDeclaredevent) for the effect body's "that player" anaphor.sub_ability_is_continuity_exemption/parse_continuity_exemption_clauseinoracle.rs) for a future follow-up.ScopedPlayerbinding resolves correctly in the runtime fan-out (each opponent's damage scales to their own Island count, not a shared/aggregate count). No code change; added regression tests only.Test plan
mercenaries_any_player_activate_prevention_scope: an opponent (not Mercenaries' controller) activates the ability and is the one protected; a negative sibling confirms the shield does NOT also block damage to Mercenaries' own controller in the same window (proves activator-scoping, not blanket prevention).total_war_attacking_player_scope: 3-player fixture — the attacking player's idle non-Wall untapped creature is destroyed; the enchantment's controller's and a bystander third player's creatures survive untouched; Wall and tapped creatures of the attacker also survive (filter axes independently verified).typhoon_per_opponent_island_count: 3-player fixture with differentiated Island counts (2 vs 5 vs the caster's own 3) proves per-opponent scaling, not an aggregate/shared count.cargo clippy -p engine --all-targets -- -D warningscleancargo fmt --all🤖 Generated with Claude Code
https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K