fix(engine): seed back-face loyalty counters when permanent enters transformed (CR 306.5b + CR 712.14a) - #2740
Conversation
There was a problem hiding this comment.
Code Review
This pull request ensures combat damage uses layer-evaluated power by flushing layers in the combat damage phase, and seeds intrinsic ETB counters from the back face of double-faced cards when they enter transformed. The review feedback correctly identifies a gap regarding CR 712.14a: if a non-double-faced card is instructed to enter transformed, it must enter normal-face up. To support this, intrinsic_etb_counters_for_back_face should return an Option so that callers can fall back to front-face counters when no back face is present.
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.
| pub fn intrinsic_etb_counters_for_back_face(obj: &GameObject) -> Vec<(CounterType, u32)> { | ||
| let Some(back) = obj.back_face.as_ref() else { | ||
| return vec![]; | ||
| }; | ||
| let mut counters = Vec::new(); | ||
| if let Some(loy) = back.loyalty { | ||
| if loy > 0 { | ||
| counters.push((CounterType::Loyalty, loy)); | ||
| } | ||
| } | ||
| if let Some(def) = back.defense { | ||
| if def > 0 { | ||
| counters.push((CounterType::Defense, def)); | ||
| } | ||
| } | ||
| counters | ||
| } |
There was a problem hiding this comment.
[HIGH] Missing fallback to front-face counters for non-double-faced cards when entering transformed.\n\nWhy it matters: According to CR 712.14a, if an effect instructs a player to put a card that isn't a double-faced card onto the battlefield transformed, it enters the battlefield normal-face up. Under the current implementation, if enter_transformed is true but back_face is None, intrinsic_etb_counters_for_back_face returns an empty vector, meaning a non-DFC planeswalker or card with intrinsic counters would enter with zero counters.\n\nSuggested fix: Change intrinsic_etb_counters_for_back_face to return Option<Vec<(CounterType, u32)>> so that callers can fall back to front-face counters if there is no back face.
pub fn intrinsic_etb_counters_for_back_face(obj: &GameObject) -> Option<Vec<(CounterType, u32)>> {
let back = obj.back_face.as_ref()?;
let mut counters = Vec::new();
if let Some(loy) = back.loyalty {
if loy > 0 {
counters.push((CounterType::Loyalty, loy));
}
}
if let Some(def) = back.defense {
if def > 0 {
counters.push((CounterType::Defense, def));
}
}
Some(counters)
}References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| let intrinsic = if enter_transformed { | ||
| crate::game::printed_cards::intrinsic_etb_counters_for_back_face(obj) | ||
| } else { | ||
| crate::game::printed_cards::intrinsic_etb_counters(obj) | ||
| }; |
There was a problem hiding this comment.
[HIGH] Fall back to front-face intrinsic counters if the card is not a double-faced card.\n\nWhy it matters: Under CR 712.14a, a non-double-faced card instructed to enter transformed enters normal-face up instead. We must fall back to front-face counters if intrinsic_etb_counters_for_back_face returns None.\n\nSuggested fix: Use unwrap_or_else to fall back to intrinsic_etb_counters.
let intrinsic = if enter_transformed {
crate::game::printed_cards::intrinsic_etb_counters_for_back_face(obj)
.unwrap_or_else(|| crate::game::printed_cards::intrinsic_etb_counters(obj))
} else {
crate::game::printed_cards::intrinsic_etb_counters(obj)
};References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| let intrinsic = if cast_transformed { | ||
| super::printed_cards::intrinsic_etb_counters_for_back_face(obj) | ||
| } else { | ||
| super::printed_cards::intrinsic_etb_counters(obj) | ||
| }; |
There was a problem hiding this comment.
[HIGH] Fall back to front-face intrinsic counters if the card is not a double-faced card.\n\nWhy it matters: Under CR 712.14a, a non-double-faced card instructed to enter transformed enters normal-face up instead. We must fall back to front-face counters if intrinsic_etb_counters_for_back_face returns None.\n\nSuggested fix: Use unwrap_or_else to fall back to intrinsic_etb_counters.
let intrinsic = if cast_transformed {
super::printed_cards::intrinsic_etb_counters_for_back_face(obj)
.unwrap_or_else(|| super::printed_cards::intrinsic_etb_counters(obj))
} else {
super::printed_cards::intrinsic_etb_counters(obj)
};References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
mike-theDude
left a comment
There was a problem hiding this comment.
Architecture Review
The loyalty-seeding fix itself is correct and well-placed, but the branch is stale and the diff spans two issues — that needs resolving before merge.
[MED] Stale branch duplicating already-merged #2702. Evidence: the head branch is fix/2352-combat-damage-layer-eval-v2 and carries two commits — "flush layers before combat damage…" (combat_damage.rs +69, turns.rs +6; CR 510.1a / issue #2352) and "seed back-face loyalty counters…" (#2382). The combat-damage commit is a v2 of #2702, which already MERGED (2026-06-09 02:31) under the identical title. The PR title/body describe only the loyalty fix ("Three files changed"), but the diff shows 5 files because the merged combat-damage work is still on this branch. Why it matters: merging as-is re-applies a second copy of a landed fix (conflict/duplication risk), and a load-bearing engine change (mid-phase flush_layers in Phase::CombatDamage) rides in undocumented under a loyalty-counter PR. Suggested fix: rebase on current main so the branch reduces to the loyalty-seeding commit only; if this v2 combat-damage commit differs from the merged #2702, call that delta out explicitly rather than bundling it here.
[Refuting Gemini's three HIGHs] Gemini wants a "fall back to front-face counters when the card isn't a DFC entering transformed." That contradicts CR 712.14a, which I verified: "If a player is instructed to put a card that isn't a double-faced card onto the battlefield transformed or converted, that card stays in its current zone." A non-DFC told to enter transformed does not enter at all — so seeding front-face counters would be doubly wrong (it shouldn't be on the battlefield). intrinsic_etb_counters_for_back_face returning vec![] when back_face is None is the correct behavior for its scope. The real (pre-existing, latent, out-of-scope) gap is that the enter_transformed path doesn't enforce 712.14a's no-entry rule for non-DFCs — but that belongs in the zone-entry guard, not this counter-seeding helper, and no front-face fallback should be added.
Loyalty fix (LGTM on its own): Seeding from back_face.loyalty/defense before transform_permanent swaps the face is the correct seam — the replacement pipeline (Doubling Season, Hardened Scales) then sees the right count, exactly as CR 306.5b requires (loyalty-enters as a replacement effect). Both the ChangeZone { enter_transformed } and stack.rs cast_transformed (Craft) paths are covered, and the DFC test (non-PW front + PW back → loyalty 3, layer-derived) is the right shape. CR 306.5b and 712.14a both verified accurate.
[LOW] Test coverage is loyalty/ChangeZone-only. The stack.rs cast_transformed (Craft) path and the helper's defense (battle) branch are unexercised. Suggested: add a Craft-transformed resolution test and a defense-counter assertion so both seeded counter kinds and both entry paths are pinned.
Net: rebase to drop the merged-#2702 combat-damage commit (the headline action), don't add Gemini's CR-712.14a-violating fallback, and the loyalty-seeding change is good to go with the two extra tests.
8eb56c2 to
7fca1a4
Compare
|
Pushed maintainer follow-up in e8613ec:
Local checks: cargo fmt --all, git diff --check, and ./scripts/check-parser-combinators.sh. Broad validation is left to GitHub CI. |
…ansformed (CR 306.5b + CR 712.14a) When Effect::ChangeZone enters a permanent with enter_transformed:true, intrinsic ETB counters (loyalty/defense) were seeded from the current front face before the transform_permanent call swapped to the back face. For DFC planeswalkers like Sorin, Ravenous Neonate (front: Vampire, no loyalty; back: PW loyalty 3), the counter map stayed empty. The layer system then re-derived loyalty = Some(0), killing the planeswalker on its own -1 activation. Fix: add intrinsic_etb_counters_for_back_face in printed_cards.rs that reads loyalty/defense from BackFaceData. Use it in execute_zone_move (change_zone.rs) when enter_transformed is true, and in stack.rs for the cast_transformed (Craft) path, so the replacement pipeline (Doubling Season etc.) sees the correct value. Regression test: enter_transformed_seeds_back_face_loyalty_counters. Fixes phase-rs#2382
…ecified The enter_tapped field on Effect::ChangeZone was changed from bool to EtbTapState in a recent refactor; update the enter_transformed regression test to use the correct typed value.
e8613ec to
be8ab97
Compare
|
Pushed maintainer follow-up in 80a644f:
Focused review-impl confirmation found no issues with the test seam. Local checks: |
… and defense counters Add stack-resolution tests for the cast_transformed path (CR 306.5b + CR 712.14a): a permanent spell cast transformed must seed its loyalty or defense counters from the back face, not the front-face spell object. Covers both the planeswalker (loyalty) and battle (defense) back-face counter variants. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
80a644f to
84a1d0e
Compare
|
Apologies to @matthewevans for the two accidental force-pushes that overwrote your follow-up commits (e8613ec and 80a644f). Both times the branch was force-updated without fetching first to check whether new maintainer work had landed — that was a process mistake on my end. Your changes have been re-applied in full in 84a1d0e:
Your authorship is credited via I will not force-push this branch again without fetching and verifying the remote tip first. |
matthewevans
left a comment
There was a problem hiding this comment.
Approving after maintainer handling and CI.
Evidence checked:
- Maintainer review found the production loyalty/defense seeding fix correct and at the right stack/change-zone seam, with the only real gap being missing stack-path tests.
- Maintainer follow-up added stack-resolution coverage for
cast_transformedback-face loyalty and defense counters after the branch was updated. - Focused review-impl confirmation on the maintainer test seam reported no findings.
- Current head
d5a488d49783b59c1a0ccc08f0a79ec7c9d8f646has green GitHub CI across Rust, parser gate, card data, frontend, WASM, Tauri, and lobby worker checks. - No frontend/runtime screenshot gate applies.
Large-refactor gate: not applicable; this is a narrow engine bug fix plus regression coverage.
Summary
Effect::ChangeZonemoves a permanent onto the battlefield withenter_transformed: true, intrinsic ETB counters (loyalty/defense) were seeded from the current front face beforetransform_permanentswapped to the back face. For DFC planeswalkers like Sorin, Ravenous Neonate (front: Vampire with no loyalty; back: Planeswalker with printed loyalty 3), the loyalty counter map stayed empty. The layer system re-derivedloyalty = Some(0)from the empty map, killing the planeswalker on its own −1 activation.intrinsic_etb_counters_for_back_faceinprinted_cards.rsthat reads loyalty/defense fromBackFaceData. Use it inexecute_zone_move(change_zone.rs) whenenter_transformed: true, and instack.rsfor thecast_transformedpath (Craft-type effects), so the Doubling-Season-class replacement pipeline sees the correct counters.Effect::ChangeZone(triggered exile-and-return effects) or spell-cast Craft alternatives.Design
Three files changed:
printed_cards.rs—intrinsic_etb_counters_for_back_face(obj): readsobj.back_face.loyalty/.defenseinstead of the current face'sobj.loyalty/obj.defense. Annotated CR 306.5b + CR 712.14a.change_zone.rs—execute_zone_move: branch onenter_transformedto call the back-face helper; the replacement pipeline now runs with the correct counter count beforetransform_permanentswaps the face.stack.rs— same branch oncast_transformedfor the Craft/ExileWithAltCost-transformed spell-resolution path.The fix is in the counter-seeding step, which is the correct point: the replacement pipeline must see the final counter value (so Doubling Season, Hardened Scales, etc. apply correctly), and that pipeline runs before the face swap.
Tests
change_zone::tests::enter_transformed_seeds_back_face_loyalty_counters— creates a DFC with a non-PW front face (Vampire, no loyalty) and a PW back face (loyalty 3), runs aChangeZone { enter_transformed: true }effect, flushes layers, and asserts:counters[Loyalty] == 3obj.loyalty == Some(3)(layer-derived)change_zonetests pass.Verification
Fixes #2382