fix(engine): recognize PayCost as a zone-change-ledger-invisible parent for generic "if you can't" riders#5869
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds integration tests for Greenbelt Rampager's ETB trigger (issue #4955) to verify correct behavior when paying or failing to pay the energy cost. The feedback highlights a test adequacy issue where the tests only cover scenarios where the owner and controller are the same player, failing to verify that the creature returns to its owner's hand when controlled by an opponent.
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.
|
|
||
| /// Build a scenario with Greenbelt Rampager in P0's hand, funded for its real | ||
| /// {G} cost, with P0's starting energy set to `starting_energy`. | ||
| fn scenario_with_rampager(starting_energy: u32) -> (engine::game::scenario::GameRunner, ObjectId) { |
There was a problem hiding this comment.
[MEDIUM] Test does not exercise the divergent owner vs. controller case. Evidence: crates/engine/tests/integration/issue_4955_greenbelt_rampager.rs:46.
Why it matters: The test only exercises the case where the owner and controller of the creature are the same player (P0), failing to verify that the creature is correctly returned to its owner's hand (CR 404.2) when controlled by an opponent.
Suggested fix: Add a third test case where an opponent (P1) gains control of the creature before the ETB trigger resolves, and assert that it returns to P0's hand (owner) rather than P1's hand (controller).
References
- player-scoped queries on non-battlefield zones (graveyard / library / hand / exile) filter by obj.owner == player, not obj.controller (CR 404.2). Tests using create_object set controller = owner and cannot exercise the divergent case. (link)
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
…nt for generic "if you can't" riders
Greenbelt Rampager: "When this creature enters, pay {E}{E}. If you can't,
return this creature to its owner's hand and you get {E}." always bounced
the creature, even when the controller had 2+ energy and paid successfully.
The generic "if you can't" rider lowers to
Not { ZoneChangedThisWay { Any } }, a proxy that reads
state.last_zone_changed_ids -- populated only by effects that move an object
between zones. Effect::PayCost deducts a resource (mana/life/energy/loyalty)
and moves no object, so the ledger stayed empty regardless of whether payment
succeeded, making the proxy unconditionally true.
rewrite_cant_rider_for_non_zone_change_parent already carved out this same
class of bug for Effect::TurnFaceUp (Etrata, Deadly Fugitive). Extends the
same match arm to also recognize Effect::PayCost, rewriting the rider to
Not { OptionalEffectPerformed } -- fed by cost_payment_failed_flag via
mandatory_parent_effect_performed's _ => true default for PayCost, which
correctly distinguishes a paid cost from an unpaid one.
Fixes phase-rs#4955
matthewevans
force-pushed
the
fix/issue-4955
branch
from
823e582 to
66c066b
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — cover controller/owner divergence in the Rampager runtime path.
🟡 Finding
[MED] The regression coverage does not exercise a stolen Greenbelt Rampager where controller and owner differ. Why it matters: the affected if you can't behavior can still bind player-relative results to the owner rather than the current controller. Suggested fix: add a real cast/ETB runtime regression with a controller≠owner fixture and assert the cost/rider resolves for the controller.
Recommendation: request-changes.
2 participants
Fixes #4955.
Greenbelt Rampager: "When this creature enters, pay {E}{E}. If you can't, return this creature to its owner's hand and you get {E}." always bounced the creature, even when the controller had 2+ energy and paid successfully.
Root cause & fix
The generic "if you can't" rider lowers to
Not { ZoneChangedThisWay { Any } }, a proxy populated only by effects that move an object between zones.Effect::PayCostdeducts a resource and moves no object, so the ledger stayed empty regardless of payment success, making the proxy unconditionally true.Extends
rewrite_cant_rider_for_non_zone_change_parent(which already carved out this exact bug class forEffect::TurnFaceUp, Etrata's "if you can't") to also recognizeEffect::PayCost, rewriting the rider toNot { OptionalEffectPerformed }— fed bycost_payment_failed_flag, which correctly distinguishes a paid cost from an unpaid one.Testing
Two discriminating tests: paid case stays on battlefield; unpaid case bounces to hand.
Verified locally (rebased onto main): fmt clean, clippy clean, 16672 lib tests pass, 3131 integration tests pass.