Skip to content

fix(mtgish-import): two converter bugs found by reviving the crate's dead test suite #7230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
matthewevans merged 7 commits into from
Aug 11, 2026
Merged

fix(mtgish-import): two converter bugs found by reviving the crate's dead test suite #7230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
80884ed
fix(mtgish-import): keep bound selection targets under Targeted wrapper
JacobWoodson Aug 11, 2026
1c1dd95
fix(mtgish-import): convert bare CombatDamageWouldBeDealt replacements
JacobWoodson Aug 11, 2026
e8cef27
test(mtgish-import): refresh stale goldens, complete ordering manifest
JacobWoodson Aug 11, 2026
0356328
Merge branch 'main' into claude/wonderful-northcutt-227f91
JacobWoodson Aug 11, 2026
3aa27ae
test(mtgish-import): pin the combat-damage event mapping
JacobWoodson Aug 11, 2026
9118ee2
Merge branch 'claude/wonderful-northcutt-227f91' of https://github.co…
JacobWoodson Aug 11, 2026
400d683
Merge branch 'main' into claude/wonderful-northcutt-227f91
JacobWoodson Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 32 additions & 8 deletions crates/mtgish-import/src/convert/action.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down Expand Up @@ -5644,13 +5653,7 @@ fn apply_player_target_chain(
) -> ConvResult<Vec<Effect>> {
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())?);
Expand All @@ -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,
}
Comment on lines +5678 to +5683

@coderabbitai coderabbitai Bot Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle ChangeZoneAll hand-exile continuations.

When a RevealHand continuation lowers to Effect::ChangeZoneAll, Line 5680 calls is_selected_hand_exile_continuation, but that helper matches only Effect::ChangeZone. The supplied Thought Distortion regression path in crates/engine/src/database/synthesis.rs Lines 10786-10926 uses ChangeZoneAll with an exile destination and a structural TargetFilter::Any. The predicate therefore returns false, so VariableBindings::rewrite_target_filters rewrites the structural target instead of preserving the target-player binding. Extend the shared predicate to cover this shape, including the multi-zone origin: None case, and retain the ControllerRef::TargetPlayer regression assertion.

Proposed fix 
 fn is_selected_hand_exile_continuation(effect: &Effect) -> bool {
     matches!(
         effect,
         Effect::ChangeZone {
             origin: Some(Zone::Hand),
             destination: Zone::Exile,
             target: TargetFilter::Any,
             ..
         }
+        | Effect::ChangeZoneAll {
+            destination: Zone::Exile,
+            target: TargetFilter::Any,
+            ..
+        }
     )
 }
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/mtgish-import/src/convert/action.rs` around lines 5678 - 5683, Extend
is_selected_hand_exile_continuation to recognize ChangeZoneAll effects
representing hand exile, including exile destinations with a structural
TargetFilter::Any and the multi-zone origin: None case. Preserve the existing
ChangeZone behavior and ensure the Thought Distortion regression assertion for
ControllerRef::TargetPlayer remains passing.

@JacobWoodson JacobWoodson Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declining this one — I think it conflates two different producers.

is_selected_hand_exile_continuation only ever sees effect vectors built by this converter: it is reached from apply_player_target_chain and VariableBindings::rewrite_target_filters, both of which operate on the Vec<Effect> that convert_list_with_bindings just constructed. The cited Thought Distortion path in crates/engine/src/database/synthesis.rs is the native parser's synthesis output, which never flows through either function.

The mtgish converter never constructs Effect::ChangeZoneAll. The only reference to it anywhere in the crate is an or-pattern arm in the rewrite consumer at crates/mtgish-import/src/convert/action.rs:648:

$ rg -n 'ChangeZoneAll' crates/mtgish-import/src/
crates/mtgish-import/src/convert/action.rs:648:        | Effect::ChangeZoneAll { ref mut target, .. }

Zero constructions. The RevealHandAndPlayerChoosesACardToExile arm — the only producer of a hand-exile continuation here — emits Effect::ChangeZone { origin: Some(Zone::Hand), destination: Zone::Exile, .. }, which the existing predicate already matches.

There is also no Thought Distortion regression in this crate (rg -n 'Thought Distortion' crates/mtgish-import/ is empty), so the ControllerRef::TargetPlayer assertion the comment asks to keep passing does not exist here.

Applying the proposed diff would add an unreachable match arm. Happy to revisit if someone can point at a converter path that actually emits ChangeZoneAll after a RevealHand — that would be a real gap and I would want it covered.

The other finding on this PR (assert the CombatDamageWouldBeDealt mapping, also raised by @matthewevans) was valid and is fixed in 3aa27ae.

}

fn is_selected_hand_exile_continuation(effect: &Effect) -> bool {
matches!(
effect,
Expand Down
38 changes: 36 additions & 2 deletions crates/mtgish-import/src/convert/replacement.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
E::CombatDamageWouldBeDealtByACreatureToRecipient(_perm, recipient) => DamageEventFilters {
source_filter: None,
target_filter: recipient_to_damage_target_filter(recipient),
Expand Down Expand Up @@ -3287,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;
Expand Down Expand Up @@ -3317,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 })
Expand Down
215 changes: 215 additions & 0 deletions crates/mtgish-import/src/diff/ordering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 0 additions & 2 deletions crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
"duration": null,
"description": null,
"target_prompt": null,
"sorcery_speed": false,
"condition": null,
"optional_targeting": false,
"optional": false,
Expand Down Expand Up @@ -63,7 +62,6 @@
"duration": null,
"description": null,
"target_prompt": null,
"sorcery_speed": false,
"condition": null,
"optional_targeting": false,
"optional": false,
Expand Down
2 changes: 0 additions & 2 deletions crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
"duration": null,
"description": null,
"target_prompt": null,
"sorcery_speed": false,
"condition": null,
"optional_targeting": false,
"optional": false,
Expand Down Expand Up @@ -76,7 +75,6 @@
"duration": null,
"description": null,
"target_prompt": null,
"sorcery_speed": false,
"condition": null,
"optional_targeting": false,
"optional": false,
Expand Down
1 change: 0 additions & 1 deletion crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
"duration": null,
"description": null,
"target_prompt": null,
"sorcery_speed": false,
"condition": null,
"optional_targeting": false,
"optional": false,
Expand Down
Loading
Loading