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.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
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.
…r ETB Addresses matthewevans's CHANGES_REQUESTED review on PR phase-rs#5869: the prior tests only exercised the case where owner and controller are the same player (P0). Adds two integration tests driving a real reanimated Greenbelt Rampager (P0 owns it, P1 controls it after casting a real Necromantic-Summons-derived reanimation spell) through the production cast/ETB pipeline: - controller_pays_when_owner_has_no_energy: the {E}{E} cost must bind to the controller (P1), not the owner (P0), even when the owner has none to spare. - cant_pay_bounces_to_owner_hand_reward_to_controller: an unpayable cost must bounce to the OWNER's hand (per the Oracle text's explicit "owner's hand" wording, CR 110.2) while the "you get {E}" reward still goes to the CONTROLLER (CR 109.5, 603.3d).
78c8bf9 to
cce08b5
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — make the PayCost rider outcome resolution-local.
🔴 Blocker
[HIGH] A successful mandatory PayCost can inherit a failure from an earlier, unrelated resolution. Evidence: crates/engine/src/game/effects/pay.rs:35-42 and :250-272 set cost_payment_failed_flag only on failure; its PaymentOutcome::Paid path does not clear it. The new rewrite at crates/engine/src/parser/oracle_effect/mod.rs:348-364 changes the rider to Not { OptionalEffectPerformed }, while crates/engine/src/game/effects/mod.rs:8505-8521 only seeds that outcome when !state.cost_payment_failed_flag. Thus, after any prior unpayable cost, a later paid Rampager cost leaves the stale flag set, no outcome is seeded, and the newly rewritten "If you can't" bounce still runs. The added tests initialize a fresh state and do not exercise that preceding-failure path.
Suggested fix: make the success/failure signal for this PayCost instance resolution-local at the payment authority/chain handoff (clearing or replacing the stale global signal before the current mandatory payment), then add a runtime regression that first resolves an unpayable payment and subsequently pays the Rampager ETB cost successfully.
Recommendation: request changes.
…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
…r ETB Addresses matthewevans's CHANGES_REQUESTED review on PR phase-rs#5869: the prior tests only exercised the case where owner and controller are the same player (P0). Adds two integration tests driving a real reanimated Greenbelt Rampager (P0 owns it, P1 controls it after casting a real Necromantic-Summons-derived reanimation spell) through the production cast/ETB pipeline: - controller_pays_when_owner_has_no_energy: the {E}{E} cost must bind to the controller (P1), not the owner (P0), even when the owner has none to spare. - cant_pay_bounces_to_owner_hand_reward_to_controller: an unpayable cost must bounce to the OWNER's hand (per the Oracle text's explicit "owner's hand" wording, CR 110.2) while the "you get {E}" reward still goes to the CONTROLLER (CR 109.5, 603.3d).
Review follow-up (phase-rs#4955): a successful mandatory PayCost could inherit a failure from an earlier, unrelated resolution. cost_payment_failed_flag is only ever SET on failure; pay::resolve never cleared it, so a stale true left by any prior unpayable cost survived into a later payment that succeeded, suppressed the mandatory-rider seed's optional_effect_ performed write (the seed guards on !cost_payment_failed_flag), and the Not { OptionalEffectPerformed } 'If you can't' bounce fired despite the cost having been paid. pay::resolve now clears the flag at this PayCost instance's chain handoff, making the signal resolution-local — mirroring the established per-iteration clear in the repeated-payment driver ('clear the resolution failure flag before THIS payment so a prior iteration's failure can't be misread as this payment's outcome'). Regressions: a production-path integration test casts the same Rampager twice — first with an unpayable {E}{E} (bounces, grants {E}, leaves the stale flag) then, funded, successfully (must STAY on the battlefield with both energy spent and no rider {E}); plus a pay.rs unit test that a pre-set stale flag is replaced by a successful payment's outcome, and a strengthened assertion on the existing stale-flag stamp test. Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17516 passed; engine integration 3753 passed.
cce08b5 to
ccef396
Compare
|
Addressed the stale-flag blocker.
Added the requested regression: a production-path integration test casts the same Rampager twice — the first cast's {E}{E} is unpayable (bounces, grants {E}, and leaves the global flag set), then, funded to exactly {E}{E}, the second cast pays successfully and must STAY on the battlefield with both energy spent and no rider {E}. Fails against the previous head. Also added a Verified locally in an isolated target dir: fmt clean, workspace clippy |
📝 WalkthroughWalkthrough
ChangesGreenbelt Rampager payment resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
353-366: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBroadened match covers all
PayCostshapes, but only the Energy case is regression-tested.The rewrite now applies
Not { OptionalEffectPerformed }to any precedingEffect::PayCost, regardless of the innerAbilityCost(Mana, Life, Energy, Discard, Composite, Speed, Loyalty). That's the right class-level fix per the project's "building blocks over one-card special cases" principle, but the only new coverage (issue_4955_greenbelt_rampager.rs) exercisesAbilityCost::PayEnergy. ADiscard/Compositecost payment does cause its own zone change (the discard), so swapping the "if you can't" signal away fromZoneChangedThisWayfor those shapes deserves at least one additional integration test (e.g., a mana- or life-cost "if you can't" card) to confirmcost_payment_failed_flagthreads correctly end-to-end through the rewritten condition for non-Energy shapes.🤖 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/engine/src/parser/oracle_effect/mod.rs` around lines 353 - 366, Add an integration regression test covering a non-Energy preceding Effect::PayCost, such as a mana- or life-cost “if you can’t” card, and verify the rewritten optional-effect condition uses cost_payment_failed_flag correctly end to end. Keep the broadened matching logic unchanged and follow the existing issue_4955_greenbelt_rampager.rs test structure.Source: Path instructions
🤖 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.
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 353-366: Add an integration regression test covering a non-Energy
preceding Effect::PayCost, such as a mana- or life-cost “if you can’t” card, and
verify the rewritten optional-effect condition uses cost_payment_failed_flag
correctly end to end. Keep the broadened matching logic unchanged and follow the
existing issue_4955_greenbelt_rampager.rs test structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9bf2d4ef-7584-42c0-bb59-de310f0b5398
📒 Files selected for processing (4)
crates/engine/src/game/effects/pay.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/tests/integration/issue_4955_greenbelt_rampager.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review: prior blockers are addressed; holding for the independent pass.
✅ Clean
pay::resolve now clears the resolution-local failure flag before this PayCost, and the real Rampager recast regression proves a later successful payment does not inherit an earlier failure. The controller/owner regressions also exercise both payment ownership and the owner-hand bounce destination.
Recommendation: hold until the current CI run and CodeRabbit review complete, then recheck before approval.
|
CodeRabbit's non-Energy coverage note is not a blocking gap. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the current head makes the payment outcome resolution-local and covers paid, unpaid, stale-state, and controller/owner runtime paths.
…nt for generic "if you can't" riders (phase-rs#5869) * fix(engine): recognize PayCost as a zone-change-ledger-invisible parent 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 * test(engine): cover controller/owner divergence for Greenbelt Rampager ETB Addresses matthewevans's CHANGES_REQUESTED review on PR phase-rs#5869: the prior tests only exercised the case where owner and controller are the same player (P0). Adds two integration tests driving a real reanimated Greenbelt Rampager (P0 owns it, P1 controls it after casting a real Necromantic-Summons-derived reanimation spell) through the production cast/ETB pipeline: - controller_pays_when_owner_has_no_energy: the {E}{E} cost must bind to the controller (P1), not the owner (P0), even when the owner has none to spare. - cant_pay_bounces_to_owner_hand_reward_to_controller: an unpayable cost must bounce to the OWNER's hand (per the Oracle text's explicit "owner's hand" wording, CR 110.2) while the "you get {E}" reward still goes to the CONTROLLER (CR 109.5, 603.3d). * fix(engine): make the PayCost failure signal resolution-local Review follow-up (phase-rs#4955): a successful mandatory PayCost could inherit a failure from an earlier, unrelated resolution. cost_payment_failed_flag is only ever SET on failure; pay::resolve never cleared it, so a stale true left by any prior unpayable cost survived into a later payment that succeeded, suppressed the mandatory-rider seed's optional_effect_ performed write (the seed guards on !cost_payment_failed_flag), and the Not { OptionalEffectPerformed } 'If you can't' bounce fired despite the cost having been paid. pay::resolve now clears the flag at this PayCost instance's chain handoff, making the signal resolution-local — mirroring the established per-iteration clear in the repeated-payment driver ('clear the resolution failure flag before THIS payment so a prior iteration's failure can't be misread as this payment's outcome'). Regressions: a production-path integration test casts the same Rampager twice — first with an unpayable {E}{E} (bounces, grants {E}, leaves the stale flag) then, funded, successfully (must STAY on the battlefield with both energy spent and no rider {E}); plus a pay.rs unit test that a pre-set stale flag is replaced by a successful payment's outcome, and a strengthened assertion on the existing stale-flag stamp test. Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17516 passed; engine integration 3753 passed.
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.
Summary by CodeRabbit
Bug Fixes
Tests