fix(mtgish-import): two converter bugs found by reviving the crate's dead test suite - #7230
Conversation
`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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
This crate's tests run nowhere. ci.yml excludes it from the workspace nextest run (added 2026-07-04, c4def2e) 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<T> 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 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR centralizes selection-continuation handling, adds unqualified combat-damage conversion, expands collection ordering metadata, and updates structural golden fixtures for revised effect representations. ChangesMTGish import updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/mtgish-import/src/convert/action.rs`:
- Around line 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.
In `@crates/mtgish-import/src/convert/replacement.rs`:
- Around line 342-351: Extend the existing
would_deal_damage_fixed_actions_convert_to_typed_modifications test to assert
that CombatDamageWouldBeDealt produces damage_source_filter: None,
damage_target_filter: None, and combat_scope:
Some(CombatDamageScope::CombatOnly), preserving the current conversion behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4eb108a-5d2b-44f8-851e-eb0938f3a66d
📒 Files selected for processing (9)
crates/mtgish-import/src/convert/action.rscrates/mtgish-import/src/convert/replacement.rscrates/mtgish-import/src/diff/ordering.rscrates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.jsoncrates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.jsoncrates/mtgish-import/tests/golden/structural/etb_tapped/expected.jsoncrates/mtgish-import/tests/golden/structural/etb_with_counters/expected.jsoncrates/mtgish-import/tests/golden/structural/etb_with_counters_and_trigger/expected.jsoncrates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json
💤 Files with no reviewable changes (4)
- crates/mtgish-import/tests/golden/structural/etb_tapped/expected.json
- crates/mtgish-import/tests/golden/structural/etb_and_ltb_lifegain/expected.json
- crates/mtgish-import/tests/golden/structural/vanilla_etb_trigger/expected.json
- crates/mtgish-import/tests/golden/structural/etb_replacement_plus_trigger/expected.json
| 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, | ||
| } |
There was a problem hiding this comment.
🎯 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.
There was a problem hiding this comment.
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.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Missing discriminating regression assertions for unqualified combat damage. Evidence: crates/mtgish-import/src/convert/replacement.rs:3307-3342 invokes CombatDamageWouldBeDealt, but the test asserts only damage_modification; it never asserts the new semantic output: damage_source_filter: None, damage_target_filter: None, and combat_scope: Some(CombatDamageScope::CombatOnly). Why it matters: an incorrect scope or filter mapping would still leave this regression green. Suggested fix: extend this focused test to assert all three fields for the converted definitions.
Review feedback from @matthewevans on phase-rs#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 <noreply@anthropic.com>
…m/JacobWoodson/phase into claude/wonderful-northcutt-227f91
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
One converter mapping is now covered; the other substantive fix is still blocked on a discriminating regression.
🔴 Blocker
[HIGH] The selection-continuation fix has no test that reaches the changed outer-target rewrite. Evidence: crates/mtgish-import/src/convert/action.rs:252-263 now skips the immediately following effect during Actions::Targeted rewriting, and crates/mtgish-import/src/convert/action.rs:5650-5683 shares that predicate with SearchPlayersLibrary; the PR adds no test for either path. The only new focused regression is crates/mtgish-import/src/convert/replacement.rs:3314-3366, which covers the separate combat-damage mapping. Why it matters: reverting the new selection-continuation predicate restores the reported arbitrary-permanent retargeting behavior while the current PR tests remain green, so this shared converter change is unpinned. Suggested fix: add a converter-level regression that starts with the real Actions::Targeted / SearchPlayersLibrary(Ref_TargetPlayer, ...) shape, reaches SearchLibrary -> ChangeZone, and asserts the outer player filter is applied to the search while the selected-card ChangeZone.target remains TargetFilter::Any.
✅ Clean
The new current-head assertions in replacement.rs:3330-3352 now pin every field introduced by CombatDamageWouldBeDealt; the previous formal finding is resolved. I also confirmed the open CodeRabbit ChangeZoneAll suggestion is inapplicable to this converter: crates/mtgish-import/src/convert/action.rs:648 is its sole reference and no converter producer constructs that effect.
Request changes: add the production-shaped regression for the selection-continuation fix, then I will re-review this head.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — correction to my prior requested-changes review.
My earlier blocker overlooked an existing discriminator: crates/mtgish-import/src/convert/action.rs:7884-7925 constructs the real Actions::Targeted → SearchPlayersLibrary(Ref_TargetPlayer) → SearchLibrary → ChangeZone path and asserts that the selected-card ChangeZone.target remains TargetFilter::Any. The test predates this PR, but it reaches this PR’s changed VariableBindings::rewrite_target_filters seam: reverting the new selection-continuation skip changes that target to the outer typed opponent filter and makes the assertion fail. It therefore pins the reported regression without a duplicate test.
The separate combat-damage mapping has current assertions for its source filter, target filter, and combat scope. I also rechecked the current CodeRabbit feedback: its ChangeZoneAll suggestion is inapplicable because this converter has no ChangeZoneAll producer.
No remaining current-head finding. Approving 400d68325cecf70f90b00374c391dd6538dd6f88.
2 participants
Summary
Two converter bug fixes in
mtgish-import, plus the test-suite maintenance needed to surface them.These started as two unit tests reported as failing on Windows but passing on Linux CI. That premise turned out to be wrong, and the correction is the useful part: CI has never run this crate's tests.
ci.ymlexcludesmtgish-importfrom the workspace nextest run (added 2026-07-04, c4def2e), and the Tiltfile only runsphase-engineandphase-ai. Both failing tests landed 2026-07-07 — three days after the exclusion. They are platform-independent and had simply never executed anywhere.The two real bugs
Bound selection targets clobbered under a
Targetedwrapper.Actions::TargetedrewritesTargetFilter::Anyon inner effects with the wrapper's typed constraint (CR 115.1 + CR 601.2c).apply_player_target_chainalready knew someAnyslots are not target slots — theLibrary → BattlefieldChangeZone after a SearchLibrary is bound to the card the search found — and skipped them. The outer rewrite did not, and clobbered the same slots immediately afterward.For the Acquire class that's a rules break, not a shape mismatch: the ChangeZone would move an arbitrary opponent-controlled permanent onto the battlefield under your control instead of the artifact the search selected. 8 corpus cards hit this shape (Acquire, Bribery, Dichotomancy, Eternal Dominion, Inevitable Betrayal, Mimeofacture, Sphinx Ambassador). Fixed by extracting the predicate both passes need into a single
is_selection_continuationauthority.Bare
CombatDamageWouldBeDealtunhandled.event_to_damage_filtershandled every qualified combat-damage variant but not the unqualified one, which fell through to strict-fail (33 corpus occurrences). The mapping isn't a judgement call — the event names neither source nor recipient, so both filter slots stayNoneand onlycombat_scopenarrows it (CR 510.1a);damage_event_to_prevent_paramsalready maps the same variant that way. This one was a coverage gap, not silent corruption: strict-failure is the crate's designed response to an unhandled variant.Test-suite maintenance
Five weeks of unchecked engine churn had also broken the rest of the suite:
sorcery_speedno longer serializes (replaced byActivationRestriction::AsSorcery; all 10 occurrences werefalse, so nothing semantic is hidden by removing them);AddCounterfolded intoPutCounter;RemoveCounter.countwidenedu32→QuantityExprat the same value.ORDERING_MANIFEST. Unclassified fields fall back toOrderSignificant, 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. Most classifications are mechanical; the judgement calls are documented per entry and in the commit body. All 28 CR citations grepped againstdocs/MagicCompRules.txt.The CI exclusion is deliberately left in place
Re-enabling it would make
manifest_coveragegate every engine PR that adds aVec<T>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 (nomtgishreferences 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. The tradeoff of leaving it: the manifest will start drifting again at the same rate.Happy to add that job in this PR or a follow-up if maintainers prefer.
Verification
cargo test -p mtgish-importis green (149 lib + 11 golden + manifest coverage) andcargo clippy -p mtgish-import --all-targets -- -D warningsis clean.CI will not exercise any of this — the crate is still excluded, so reviewing requires running the above locally. Two caveats on my own verification: it was run on Windows, and
cargo-nextestisn't installed here, so these tests have not been run under nextest. They have no shared global state, so process isolation shouldn't matter.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests