From 80884edb4c2ff22b5ffa61dd34f04babfc47ac7e Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:33:27 -0500 Subject: [PATCH 1/4] fix(mtgish-import): keep bound selection targets under Targeted wrapper `Actions::Targeted` rewrites `TargetFilter::Any` on inner effects with the wrapper's typed constraint (CR 115.1 + CR 601.2c), so the engine can surface a proper target slot at cast time. That rewrite walked every effect blindly. `apply_player_target_chain` already knew some `Any` slots are not target slots: the `Library -> Battlefield` ChangeZone after a SearchLibrary is bound to the card the search found, and the `Hand -> Exile` ChangeZone after a RevealHand is bound to the card the player chose. It skipped those. The outer rewrite did not, and clobbered the same slots right after. For the Acquire class that is a rules break, not just a shape change: the ChangeZone would move an arbitrary opponent-controlled permanent onto the battlefield under your control instead of the artifact the search selected. 8 cards in the corpus hit this shape (Acquire, Bribery, Dichotomancy, Eternal Dominion, Inevitable Betrayal, Mimeofacture, Sphinx Ambassador). Extract the predicate both passes need into `is_selection_continuation` so there is one authority for "this effect's `Any` is bound by the preceding effect", and route the outer rewrite through it. Found by running `cargo test -p mtgish-import --lib`, which CI does not do (see the ordering-manifest commit for why). Co-Authored-By: Claude Opus 5 --- crates/mtgish-import/src/convert/action.rs | 40 +++++++++++++++++----- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/crates/mtgish-import/src/convert/action.rs b/crates/mtgish-import/src/convert/action.rs index dcebaddee3..323f1d54c3 100644 --- a/crates/mtgish-import/src/convert/action.rs +++ b/crates/mtgish-import/src/convert/action.rs @@ -245,11 +245,20 @@ impl VariableBindings { /// typed constraint from the outer `Actions::Targeted` wrapper. This /// preserves the typed target slot so the engine can prompt for targets /// at cast/activation time (CR 601.2c). + /// + /// Selection continuations (`is_selection_continuation`) are skipped: their + /// `TargetFilter::Any` is bound by the preceding effect's choice, not a + /// target slot the outer wrapper may constrain. fn rewrite_target_filters(&self, effects: &mut [Effect]) { let Some(typed) = &self.target_filter else { return; }; - for effect in effects.iter_mut() { + for idx in 0..effects.len() { + let (preceding, from_here) = effects.split_at_mut(idx); + let effect = &mut from_here[0]; + if is_selection_continuation(preceding.last(), effect) { + continue; + } rewrite_any_target_filter_in_effect(effect, typed); } } @@ -5644,13 +5653,7 @@ fn apply_player_target_chain( ) -> ConvResult> { let mut out = Vec::with_capacity(effects.len()); for effect in effects { - let hand_selection_continuation = matches!(out.last(), Some(Effect::RevealHand { .. })) - && is_selected_hand_exile_continuation(&effect); - let library_selection_continuation = - matches!(out.last(), Some(Effect::SearchLibrary { .. })) - && is_search_library_change_zone_continuation(&effect); - - if hand_selection_continuation || library_selection_continuation { + if is_selection_continuation(out.last(), &effect) { out.push(effect); } else { out.push(apply_player_target(effect, target_filter.clone())?); @@ -5659,6 +5662,27 @@ fn apply_player_target_chain( Ok(out) } +/// CR 115.1 + CR 601.2c: Does `effect` consume a selection already bound by the +/// immediately preceding effect — the card found by `Effect::SearchLibrary`, the +/// card chosen by `Effect::RevealHand` — rather than declare a target slot of +/// its own? +/// +/// Such an effect's `TargetFilter::Any` is structural: the engine resolves it +/// from the continuation the previous effect installed, so no player or typed +/// constraint from an enclosing wrapper may overwrite it. Both rebinding passes +/// (`apply_player_target_chain` for the `SearchPlayersLibrary` player axis and +/// `VariableBindings::rewrite_target_filters` for the outer `Actions::Targeted` +/// typed axis) route through this single predicate — an outer wrapper that +/// clobbered the slot would retarget the move at an arbitrary permanent the +/// bound player controls instead of the card the search selected. +fn is_selection_continuation(preceding: Option<&Effect>, effect: &Effect) -> bool { + match preceding { + Some(Effect::RevealHand { .. }) => is_selected_hand_exile_continuation(effect), + Some(Effect::SearchLibrary { .. }) => is_search_library_change_zone_continuation(effect), + _ => false, + } +} + fn is_selected_hand_exile_continuation(effect: &Effect) -> bool { matches!( effect, From 1c1dd95097412055150122b9fd779dedf67d75cb Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:33:36 -0500 Subject: [PATCH 2/4] fix(mtgish-import): convert bare CombatDamageWouldBeDealt replacements `event_to_damage_filters` handled every qualified combat-damage variant (...ToRecipient, ...ByACreature, ...ByACreatureToRecipient, and so on) but not the unqualified `CombatDamageWouldBeDealt`, which fell through to the strict-fail arm. 33 occurrences in the corpus. The mapping is not a judgement call: the event names neither a source nor a recipient, so both filter slots stay `None` and only `combat_scope` narrows the replacement (CR 510.1a). `damage_event_to_prevent_params` already maps the same variant that way for the prevention path; this brings the damage-modification path in line. Note this was a coverage gap, not silent corruption. Strict-failure is the crate's designed response to an unhandled variant, so these cards were reported unsupported rather than converted wrongly. CR 510.1a and CR 614.1a verified against docs/MagicCompRules.txt. Co-Authored-By: Claude Opus 5 --- crates/mtgish-import/src/convert/replacement.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index a11778a778..f52ccf08be 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -339,6 +339,16 @@ fn event_to_damage_filters( }, // Combat-only / noncombat-only restrictors set `combat_scope` per // CR 614.1a. + // + // CR 510.1a: The unqualified "combat damage would be dealt" event names + // neither a source nor a recipient, so both filter slots stay `None` + // and only the combat scope narrows the replacement. Mirrors the same + // variant's handling in `damage_event_to_prevent_params`. + E::CombatDamageWouldBeDealt => DamageEventFilters { + source_filter: None, + target_filter: None, + combat_scope: Some(CombatDamageScope::CombatOnly), + }, E::CombatDamageWouldBeDealtByACreatureToRecipient(_perm, recipient) => DamageEventFilters { source_filter: None, target_filter: recipient_to_damage_target_filter(recipient), From e8cef273737c0d36898fadbba2e9191a782a9042 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Tue, 11 Aug 2026 00:33:57 -0500 Subject: [PATCH 3/4] test(mtgish-import): refresh stale goldens, complete ordering manifest This crate's tests run nowhere. ci.yml excludes it from the workspace nextest run (added 2026-07-04, c4def2e8) and the Tiltfile only runs phase-engine and phase-ai. The two unit tests that prompted this work landed 2026-07-07, three days after the exclusion, so they have never executed on any runner -- they were reported as Windows-only failures, but they are platform-independent and simply had never been run. Five weeks of engine churn went unchecked behind that. Three stale shapes in the structural goldens, each a serialization change with no behavioural component: - `sorcery_speed: false` dropped -- the field was replaced by `ActivationRestriction::AsSorcery` and no longer serializes. All 10 occurrences were `false`, i.e. "no AsSorcery restriction", which is now the absence of the restriction. Nothing semantic is hidden by removing them. - `AddCounter` -> `PutCounter` -- duplicate variant folded into one, with `#[serde(alias = "AddCounter")]` kept for persisted snapshots. - `RemoveCounter.count` 1 -> `{Fixed, value: 1}` -- widened from u32 to QuantityExpr to mirror PutCounter.count. Same value. And 37 engine list fields had accumulated with no ORDERING_MANIFEST entry. Unclassified fields fall back to OrderSignificant, so the diff tool was reporting spurious reorder divergences on set-like lists -- false positives for anyone using it to hunt native-parser silent failures. Classifications are mostly mechanical (type sets, zone unions, membership tests). The ones that needed a call: - ModalChoice::mode_pawprints is positional -- index-parallel with the modes, so reordering reprices every mode (CR 700.2i). - ResolvedAbility::target_incarnations is positional -- pin i guards target i, and `targets` is already positional. - selected_mode_labels is positional ("printed instruction order") but SpellContext::chosen_modes is not: it is stored ascending, so the order is a normalization and the multiset is the meaning. - ResolutionCastSuccessAction::remaining_hits is a set -- CR 702.60a lets Ripple cast any number of the reveals and bottoms the rest "in any order". - TriggerOccurrenceState::active_grants is a keyed set -- every access is by producer key or instance id, never by index. Mirror types (AbilityDefinitionDe, ResolvedAbility) get classes identical to what they reconstruct, or the same list would diff differently depending on which shape the JSON deserialized through. All 28 CR citations grepped against docs/MagicCompRules.txt. `cargo test -p mtgish-import` is now green (149 lib + 11 golden + manifest coverage) and clippy is clean. The CI exclusion is deliberately left in place. Re-enabling it would make manifest_coverage gate every engine PR that adds a Vec field to the five core type files -- roughly 7-8 PRs a week at the rate this backlog accumulated -- in service of a crate nothing in the product path consumes (no mtgish references in the engine, the WASM bridge, the card-data pipeline, or any script or workflow). If the rot is worth catching, a non-blocking or scheduled job is the better shape. Consequence of leaving it: the manifest will start drifting again at the same rate. Co-Authored-By: Claude Opus 5 --- crates/mtgish-import/src/diff/ordering.rs | 215 ++++++++++++++++++ .../etb_and_ltb_lifegain/expected.json | 2 - .../expected.json | 2 - .../structural/etb_tapped/expected.json | 1 - .../etb_with_counters/expected.json | 3 +- .../expected.json | 15 +- .../vanilla_etb_trigger/expected.json | 1 - 7 files changed, 225 insertions(+), 14 deletions(-) diff --git a/crates/mtgish-import/src/diff/ordering.rs b/crates/mtgish-import/src/diff/ordering.rs index 02ca2a336c..d55f15254f 100644 --- a/crates/mtgish-import/src/diff/ordering.rs +++ b/crates/mtgish-import/src/diff/ordering.rs @@ -376,6 +376,221 @@ pub const ORDERING_MANIFEST: &[((&str, &str), OrderingClass)] = &[ ), // ----- Trigger cause filters ----- (("TriggerCause", "core_types"), OrderingClass::SetEquivalent), + // ----- AbilityBlockReason ----- + // CR 602.5: the field's own contract is "sorted, deduped permanents" — a + // canonicalized set of independently-prohibiting sources. Set. + ( + ("AbilityBlockReason", "sources"), + OrderingClass::SetEquivalent, + ), + // ----- AbilityCondition (RevealedHasCardType) ----- + // CR 205.1a: disjunctive core-type match against the revealed card — only + // membership decides the outcome. Matches `QuantityRef::card_types`. Set. + ( + ("AbilityCondition", "card_types"), + OrderingClass::SetEquivalent, + ), + // ----- AbilityDefinitionDe (serde mirror of AbilityDefinition) ----- + // The deserialization mirror must classify identically to the type it + // reconstructs, or the same list would diff differently depending on which + // shape the JSON deserialized through. + ( + ("AbilityDefinitionDe", "activation_restrictions"), + OrderingClass::SetEquivalent, + ), + ( + ("AbilityDefinitionDe", "mode_abilities"), + OrderingClass::OrderSignificant, + ), + ( + ("AbilityDefinitionDe", "target_constraints"), + OrderingClass::SetEquivalent, + ), + // ----- CardFace / CardMetadata catalog lists ----- + // CR 717.1: the set of lit-up roll numbers on an Attraction variant. Which + // numbers are lit decides the roll; their listing order does not. Set. + ( + ("CardFace", "attraction_lights"), + OrderingClass::SetEquivalent, + ), + // Alchemy spellbook — the fixed candidate pool a draft effect selects from. + // Membership defines the pool; MTGJSON's listing order is incidental. Set. + (("CardMetadata", "spellbook"), OrderingClass::SetEquivalent), + // ----- CastingPermission ----- + // CR 613.1d: enters-with continuous modifications carried on a cast grant. + // Layer assignment is by modification variant, so the layer system re-sorts + // them at apply time. Mirrors `StaticDefinition::modifications`. Set. + ( + ("CastingPermission", "enters_with_modifications"), + OrderingClass::SetEquivalent, + ), + // ----- Effect embedded lists (continued) ----- + // CR 122.1: entry-time counters gated per-filter. Counter placement is + // cumulative and filter-keyed, so reordering the riders cannot change the + // resulting counter set. Mirrors `Effect::enter_with_counters`. Set. + ( + ("Effect", "conditional_enter_with_counters"), + OrderingClass::SetEquivalent, + ), + // CR 707.9: "except …" modifications applied to a created copy. Same + // layer-resorted rationale as `StaticDefinition::modifications`. Set. + ( + ("Effect", "copy_modifications"), + OrderingClass::SetEquivalent, + ), + (("Effect", "modifications"), OrderingClass::SetEquivalent), + // Zone union searched for candidate cards — membership, not order. Set. + (("Effect", "zones"), OrderingClass::SetEquivalent), + // ----- FaceDownProfile ----- + // CR 708.2a + CR 205.1a: the core types and subtypes a face-down permanent + // is given. Type/subtype sets are unordered. Set. + ( + ("FaceDownProfile", "extra_core_types"), + OrderingClass::SetEquivalent, + ), + ( + ("FaceDownProfile", "subtypes"), + OrderingClass::SetEquivalent, + ), + // ----- ManaSpendRestriction ----- + // CR 106.6: the field's own contract is a DISJUNCTION of cost criteria — + // any one match satisfies it, so order cannot matter. Set. + ( + ("ManaSpendRestriction", "criteria"), + OrderingClass::SetEquivalent, + ), + // ----- ModalChoice ----- + // CR 700.2i: per-mode pawprint weights are index-parallel with the modes + // themselves — `mode_pawprints[i]` is the cost of mode `i`. Reordering + // reprices every mode. Positional. + ( + ("ModalChoice", "mode_pawprints"), + OrderingClass::OrderSignificant, + ), + // ----- NotedManaPayment ----- + // CR 106.6: the multiset of mana types spent on one activation. Consumers + // ask "was type T among them", never "which came first". Set. + (("NotedManaPayment", "types"), OrderingClass::SetEquivalent), + // ----- PerpetualModification ----- + // CR 205.1a + CR 113.2c: granted keyword and creature-subtype sets. Each + // keyword instance functions independently (CR 113.2c). Set. + ( + ("PerpetualModification", "creature_subtypes"), + OrderingClass::SetEquivalent, + ), + ( + ("PerpetualModification", "keywords"), + OrderingClass::SetEquivalent, + ), + // ----- QuantityRef ----- + // CR 305.2a: origin zones narrowing a land-play count — a membership test + // over the play's recorded origin. Set. + (("QuantityRef", "from_zones"), OrderingClass::SetEquivalent), + // ----- ReplacementCondition ----- + // CR 111.1: core types the proposed token must overlap. Overlap is + // symmetric in the listing order. Set. + ( + ("ReplacementCondition", "core_types"), + OrderingClass::SetEquivalent, + ), + // ----- During-resolution cast state (Cascade / Discover / Ripple) ----- + // CR 608.2g: dig cards that were not the hit. CR 702.60a bottoms the + // non-cast reveals "in any order", so their recorded order is not + // rules-meaningful. Mirrors `CastPermissionConstraint::exiled_misses`. Set. + ( + ("ResolutionCastCleanup", "exiled_misses"), + OrderingClass::SetEquivalent, + ), + // CR 607.2a: the "exiled this way" batch a re-offer's candidate set is + // confined to — a membership restriction. Set. + ( + ("ResolutionCastSuccessAction", "member_pool"), + OrderingClass::SetEquivalent, + ), + // CR 702.60a: Ripple may cast ANY NUMBER of the same-named reveals, so the + // remaining pool is a candidate set, not a queue with meaningful order. Set. + ( + ("ResolutionCastSuccessAction", "remaining_hits"), + OrderingClass::SetEquivalent, + ), + // Zone union searched for free-cast candidates. Matches `Effect::zones`. Set. + ( + ("ResolutionCastSuccessAction", "zones"), + OrderingClass::SetEquivalent, + ), + // ----- ResolvedAbility (continued) ----- + // Ids stripped from this ability's own candidate lists — read as a + // membership test, never as a positional referent. Set. + ( + ("ResolvedAbility", "cost_paid_object_ids"), + OrderingClass::SetEquivalent, + ), + // CR 700.2b: one definition per mode; mode order is the player-facing + // label order. Mirrors `AbilityDefinition::mode_abilities`. Positional. + ( + ("ResolvedAbility", "mode_abilities"), + OrderingClass::OrderSignificant, + ), + // CR 700.2d: the field's own contract is "in the printed instruction order + // used to resolve them" — order IS the resolution sequence. Positional. + ( + ("ResolvedAbility", "selected_mode_labels"), + OrderingClass::OrderSignificant, + ), + // CR 115.1 + CR 601.2c: independent legality predicates ANDed together. + // Mirrors `AbilityDefinition::target_constraints`. Set. + ( + ("ResolvedAbility", "target_constraints"), + OrderingClass::SetEquivalent, + ), + // CR 400.7 + CR 603.7c: incarnation pins for the referents in `targets`, + // which is itself positional — pin `i` guards target `i`. Reordering + // re-binds each pin to a different target. Positional. + ( + ("ResolvedAbility", "target_incarnations"), + OrderingClass::OrderSignificant, + ), + // ----- SpellContext (continued) ----- + // CR 113.2c + CR 601.2b: the non-kicker analogue of `kickers_paid`, read + // the same way (was cost C paid, how many times). Set. + ( + ("SpellContext", "additional_cost_payments"), + OrderingClass::SetEquivalent, + ), + // CR 700.2d: mode indices are stored ASCENDING with repeats — the ordering + // is a normalization, and the multiset of chosen modes is the meaning. + // (Player-facing resolution order lives in `selected_mode_labels`.) Set. + ( + ("SpellContext", "chosen_modes"), + OrderingClass::SetEquivalent, + ), + // ----- StaticMode (continued) ----- + // CR 702.122a: which crew-like keyword actions a power/toughness + // contribution modifier applies to. Membership. Set. + (("StaticMode", "actions"), OrderingClass::SetEquivalent), + // CR 122.2: destination zones excluded from counter persistence — the + // "any zone other than [zones]" set. Set. + ( + ("StaticMode", "excluded_zones"), + OrderingClass::SetEquivalent, + ), + // ----- TriggerCause / TriggerDefinition ----- + // CR 603.6a: the field's own contract marks these qualifiers disjunctive. Set. + (("TriggerCause", "qualifiers"), OrderingClass::SetEquivalent), + // CR 106.1: the produced-mana set a "taps for {C}" trigger requires at + // least one match in — an overlap test. Set. + ( + ("TriggerDefinition", "taps_for_mana_produced"), + OrderingClass::SetEquivalent, + ), + // ----- TriggerOccurrenceState ----- + // Grant instances are addressed exclusively by `producer` key (duplicate + // producers are rejected outright) and by `instance` id — never by index. + // A keyed set. Set. + ( + ("TriggerOccurrenceState", "active_grants"), + OrderingClass::SetEquivalent, + ), ]; /// Look up the ordering class for a `(carrier, field)` pair. diff --git a/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json b/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json index b459b3a084..0a26eb0d28 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json @@ -24,7 +24,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -63,7 +62,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json index e75ceb8d14..11b8db5202 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json @@ -19,7 +19,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -76,7 +75,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json b/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json index befd644010..078fb9463c 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json @@ -26,7 +26,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json b/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json index ba4c386200..e7465927b8 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_with_counters/expected.json @@ -10,7 +10,7 @@ "execute": { "kind": "Spell", "effect": { - "type": "AddCounter", + "type": "PutCounter", "counter_type": "P1P1", "count": { "type": "Ref", @@ -27,7 +27,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json index ac8c832bf7..6a1b2c91c5 100644 --- a/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.json @@ -12,7 +12,10 @@ "effect": { "type": "RemoveCounter", "counter_type": "M1M1", - "count": 1, + "count": { + "type": "Fixed", + "value": 1 + }, "target": { "type": "SelfRef" } @@ -22,7 +25,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -50,7 +52,10 @@ "effect": { "type": "RemoveCounter", "counter_type": "M1M1", - "count": 1, + "count": { + "type": "Fixed", + "value": 1 + }, "target": { "type": "SelfRef" } @@ -60,7 +65,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, @@ -89,7 +93,7 @@ "execute": { "kind": "Spell", "effect": { - "type": "AddCounter", + "type": "PutCounter", "counter_type": "M1M1", "count": { "type": "Fixed", @@ -104,7 +108,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, diff --git a/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json b/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json index cd0a74aea5..7c3a00a2bb 100644 --- a/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json +++ b/crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json @@ -24,7 +24,6 @@ "duration": null, "description": null, "target_prompt": null, - "sorcery_speed": false, "condition": null, "optional_targeting": false, "optional": false, From 3aa27ae46de0c5aff0c569e473ad8c9b51b07f78 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Tue, 11 Aug 2026 10:12:06 -0500 Subject: [PATCH 4/4] test(mtgish-import): pin the combat-damage event mapping Review feedback from @matthewevans on #7230: the test covering the new `CombatDamageWouldBeDealt` arm asserted only `damage_modification`, never the mapping the arm actually introduces. Making the conversion stop erroring was enough to turn it green, so a wrong scope or an over-narrow filter would have shipped undetected. Assert all three event-derived fields on every produced definition: `damage_source_filter: None`, `damage_target_filter: None`, `combat_scope: Some(CombatOnly)` (CR 510.1a -- the unqualified event names neither source nor recipient, so scope is its only contribution). Verified non-vacuous: flipping the arm to `NoncombatOnly` fails the test with a discriminating message. Co-Authored-By: Claude Opus 5 --- .../mtgish-import/src/convert/replacement.rs | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/crates/mtgish-import/src/convert/replacement.rs b/crates/mtgish-import/src/convert/replacement.rs index f52ccf08be..88eca1baf6 100644 --- a/crates/mtgish-import/src/convert/replacement.rs +++ b/crates/mtgish-import/src/convert/replacement.rs @@ -3297,8 +3297,8 @@ fn expiration_tag(e: &Expiration) -> String { #[cfg(test)] mod tests { use engine::types::ability::{ - AbilityCost, ContinuousModification, DamageModification, Duration, Effect, QuantityExpr, - ReplacementMode, TargetFilter, + AbilityCost, CombatDamageScope, ContinuousModification, DamageModification, Duration, + Effect, QuantityExpr, ReplacementMode, TargetFilter, }; use engine::types::card_type::{CoreType, Supertype}; use engine::types::keywords::Keyword; @@ -3327,6 +3327,30 @@ mod tests { ) .expect("fixed damage replacement actions should convert"); + assert_eq!(defs.len(), 3); + + // CR 510.1a: the unqualified event names neither a damage source nor a + // recipient, so both filter slots must stay unset and the combat scope + // is the ONLY narrowing the event contributes. Asserted on every + // definition the action list produced, since they all share one event. + // Without this, a wrong scope or an over-narrow filter would still + // leave the `damage_modification` assertions below green. + for (idx, def) in defs.iter().enumerate() { + assert_eq!( + def.damage_source_filter, None, + "defs[{idx}]: unqualified combat event must not narrow the damage source" + ); + assert_eq!( + def.damage_target_filter, None, + "defs[{idx}]: unqualified combat event must not narrow the damage target" + ); + assert_eq!( + def.combat_scope, + Some(CombatDamageScope::CombatOnly), + "defs[{idx}]: combat-damage event must restrict the replacement to combat damage" + ); + } + assert!(matches!( defs[0].damage_modification.as_ref(), Some(DamageModification::PreventionMinus { value: u32::MAX })