fix(parser): route 'that/the noun's controller gains life' to ParentTargetController - #2741
Conversation
There was a problem hiding this comment.
Code Review
This pull request addresses issue #2382 by updating spell-cast resolution in stack.rs to read loyalty and defense counters directly from the back face of a double-faced card when cast transformed, and adds a regression test in change_zone.rs. Feedback highlights a compilation error in the new test due to a type mismatch on the enter_tapped field, and recommends adding a companion test in stack.rs to properly exercise the modified spell-cast resolution path.
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.
mike-theDude
left a comment
There was a problem hiding this comment.
Architecture Review
The stack.rs fix is correct and properly scoped (this is the clean split-out of #2740's stack-path gap). The production change is sound; the regression test is aimed at the wrong path.
Verified:
- Right seam, reuses the building block. Branching
cast_transformed && back_facetointrinsic_face_counters(back.loyalty, back.defense)mirrors the already-mergedchange_zone.rsfix and reuses the sameintrinsic_face_countershelper rather than re-deriving — seeded before the face swap so the replacement pipeline (Doubling Season etc.) sees the correct count. CR 306.5b (loyalty-enters replacement) and CR 712.14a (cast/put transformed → back face) both verified.
[MED–HIGH] The regression test does not exercise the path this PR changes (confirming Gemini's second finding). Evidence: enter_transformed_seeds_back_face_loyalty_counters builds an Effect::ChangeZone { enter_transformed: true } and calls resolve(...), which runs the change_zone.rs execute_zone_move path — the one already fixed on main. The PR's actual change is in stack.rs's cast_transformed spell-resolution branch, which has zero coverage here; the included test re-validates already-merged code. Why it matters: a fix shipping with no test for the thing it fixes can regress silently. Suggested fix: add a test that drives the spell-cast resolution path — cast a Craft / ExileWithAltCost-transformed DFC (non-PW front, PW-loyalty back), resolve it through stack.rs, and assert the back-face loyalty/defense counters were seeded.
[Refuting Gemini's other HIGH — enter_tapped type mismatch] Against the current base this is a non-issue: enter_tapped is still bool on origin/main (types/ability.rs:5886, and every existing test in the file uses enter_tapped: false), so the test compiles fine. Gemini assumed the bool→EtbTapState refactor (#2713) had landed — it hasn't (still open, and it currently has a duplicate-import compile error). If #2713 merges first, this test and several others will need the EtbTapState update, but that's #2713's rebase burden, not a defect in this PR. No change needed here on that account.
Net: the stack.rs change is correct and idiomatic; the one real gap is that the regression test must target the cast_transformed stack path it actually fixes, not the change_zone path that main already covers. Add that test before merge.
…ntTargetController
try_parse_targeted_controller_gain_life only matched the 'its controller'
pronoun form. Cards like Solitude use 'That creature's controller gains
life equal to its power' — the determiner+noun possessive pattern. Extend
the subject parser with a local nom combinator that matches
'that/the <noun>\'s controller ' so both phrasings resolve to
Effect::GainLife { player: TargetFilter::ParentTargetController }.
Adds regression tests for the 'that <noun>\'s controller' and
'the <noun>\'s controller' phrasing variants.
Closes phase-rs#2381
b3a34b8 to
02d9934
Compare
|
Marking this as The existing maintainer/Gemini review comments on this PR discuss a |
…olitude reclassified) (phase-rs#3009) The ParentTargetController routing fix (phase-rs#2741) lets the anaphoric rebind resolve 'its' to Target (the destroyed/exiled object, CR 608.2c) in 'that X's controller gains life equal to its <stat>' for Crumble and Solitude, and the parser-grammar consolidation (phase-rs#2802) reshaped Sly Spy's variant parse — dropping all three out of ObjectScope::Anaphoric and the frozen count 169 -> 166. The engine reclassification landed in phase-rs#2741/phase-rs#2802/ phase-rs#2803; this re-freezes the card-data drift guard to match (the guard self-skips in CI for lack of client card-data, so it only goes red in local Tilt test-engine — verified green here at 166).
Summary
Fixes #2381 — Solitude (and similar cards) give life to the wrong player.
try_parse_targeted_controller_gain_lifeonly matched theits controllerpronoun form. Cards like Solitude use "That creature's controller gains life equal to its power" — a determiner+noun possessive. This caused the line to fall through, defaulting life gain to the casting player instead of the exiled creature's controller.Root cause
Fix
Extended the subject parser with a local nom combinator that matches
that/the <noun>'s controllerusingtag+take_until+tag, so both phrasings resolve toEffect::GainLife { player: TargetFilter::ParentTargetController }:Coverage
Covers all cards that say
"That <type>'s controller gains life"or"The <type>'s controller gains life"— not just Solitude.Tests added
targeted_controller_gains_life_that_noun_phrasing— Solitude Oracle texttargeted_controller_gains_life_the_noun_phrasing—"the permanent's controller"variant