fix(engine): make mana payments atomic - #5793
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements atomic mana payments in the game engine by ensuring that failed payment attempts do not leak partial payments into the player's mana pool, adhering to CR 601.2h. It refactors pay_cost_with_demand_and_choices to use a scratch pool and fall back to a second attempt without hand demand if the first attempt fails due to insufficient mana. Additionally, the PR updates several Magic Comprehensive Rules (CR) references in the documentation, introduces comprehensive unit tests for payment failures and successes, and adds a new integration test module for atomic mana payments. There are no review comments, and the changes are architecturally sound and compliant with the repository's style guide.
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✓ No card-parse changes detected. |
a4788b9 to
e6a578d
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved — mana-payment retries now commit only a complete payment.
✅ Clean
- The transaction boundary is the existing
pay_cost_with_demand_and_choicesauthority atcrates/engine/src/game/mana_payment.rs:1033; every casting and mana-ability caller uses it. - CR 601.2h explicitly forbids partial payments. The scratch-pool first attempt and demand-free fallback preserve the original pool on either failure path.
- The registered
GameRunner::cast(...).resolve()regression atcrates/engine/tests/integration/atomic_mana_payment.rs:17fails without the rollback, while focused tests preserve full unit provenance on failure and commit.
Recommendation: approve and enqueue.
Summary
Makes mana payment atomic when demand-aware hybrid selection has to retry without the demand hint. A failed first spend now rolls back completely before the fallback attempt, so a payable spell is not rejected because the failed attempt consumed part of the caller's pool.
Files changed
CR references
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: codex-5
Thinking: high
Tier: Standard
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
tilt get uiresource clippy- Tilt unavailable in this worktree; used the documented direct fallback.cargo fmt --all- passed.cargo fmt --all -- --check- passed.git diff --check- passed.cargo clippy-strict- passed on heada4788b91e612640b5422382bc8cd81d31c59441a.cargo test -p engine- passed on heada4788b91e612640b5422382bc8cd81d31c59441a: 16573 unit tests passed; 6coverage_parse_difftests passed; 9set_checktests passed; 3041 integration tests passed; doc tests passed with 7 ignored.cargo test -p engine atomic_mana_payment::paradox_surveyor_demand_fallback_casts_and_resolves- passed on heada4788b91e612640b5422382bc8cd81d31c59441a: 1 integration test passed../scripts/gen-card-data.sh- passed on heada4788b91e612640b5422382bc8cd81d31c59441a: generated card data for 35397 faces; coverage summary 31335/35397 cards supported (88.5%); 1072 parser warnings.cargo coverage- passed on heada4788b91e612640b5422382bc8cd81d31c59441a: 31335/35397 cards supported (88.5%).cargo semantic-audit- passed on heada4788b91e612640b5422382bc8cd81d31c59441a: 32389 cards audited, 292 existing findings.Gate A
Gate A PASS head=a4788b91e612640b5422382bc8cd81d31c59441a base=6d7821dced9623609edea342b47dd9c704ff0b36
Anchored on
reduce_cost_by_poolscratch-pool dry run keeps affordability simulation separate from caller pool mutation.pay_cost_with_demand_and_choicespayment authority owns demand-aware hybrid and Phyrexian payment selection.Final review-impl
Final review-impl PASS head=a4788b91e612640b5422382bc8cd81d31c59441a
Claimed parse impact
None.
Validation Failures
Resolved local environment issue: the first direct clippy fallback attempt hit
No space left on devicein the generated harness target. I removed that generated target cache and reran the required checks cleanly.CI Failures
None.