feat(engine): add player-to-player ExchangeLifeTotals effect#3510
feat(engine): add player-to-player ExchangeLifeTotals effect#3510andriypolanski wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements player-to-player life total exchanges (Effect::ExchangeLifeTotals) in accordance with CR 701.12a, including parser support, targeting, resolution logic, and integration tests. Feedback highlights a critical rules violation where life loss from an exchange is incorrectly treated as damage (violating CR 119.5), which could trigger damage-related abilities or prevention effects; using a non-damage life loss helper is recommended.
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.
| for (player_id, diff) in [(player_a_id, old_b - old_a), (player_b_id, old_a - old_b)] { | ||
| let deferred = match diff.signum() { | ||
| 1 => apply_life_gain(state, player_id, diff as u32, events).err(), | ||
| -1 => apply_damage_life_loss(state, player_id, (-diff) as u32, events).err(), |
There was a problem hiding this comment.
[HIGH] Treating life exchange loss as damage. Evidence: crates/engine/src/game/effects/exchange_life.rs:119.
Why it matters: CR 119.5 states that life loss from an exchange is not damage and cannot be prevented as such. Using apply_damage_life_loss may incorrectly trigger damage-related abilities or apply damage prevention. Suggested fix: Use apply_life_loss or a non-damage life loss helper instead.
| -1 => apply_damage_life_loss(state, player_id, (-diff) as u32, events).err(), | |
| -1 => apply_life_loss(state, player_id, (-diff) as u32, events).err(), |
References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Treating life exchange loss as damage violates CR 119.5. (link)
29701fa to
ec0ecc3
Compare
Partial prefix matching on compound Oracle lines (Profane Transfusion, Mister Negative) left trailing clauses as swallowed diagnostics and failed the card-data coverage regression ratchet (+2 swallowed-clause). Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review — feat(engine): add player-to-player ExchangeLifeTotals effect
Verdict: ready to enqueue once rebased on main (mergeState is BEHIND). One non-blocking CR-precision nit below.
Architecture / seam
Right seam, idiomatic. ExchangeLifeTotals is added as a new Effect enum variant (not a new field on an existing variant), so it slots into every existing exhaustive match without forcing a programmatic escape hatch. Categorical boundary is respected — life-total exchange stays within the CR 119 / 701.12 life-rule section; no cross-section unification with power/toughness.
add-engine-effect lockstep is complete and was verified site-by-site:
- types:
Effectvariant +EffectKind+From<&Effect>+effect_variant_name✔ - parser:
try_parse_exchange_life_totals(nomall_consuming/terminated/alt/value, no string dispatch) + IRImperativeFamilyAst+ chain dispatch ✔ - resolver:
exchange_life::resolve_life_totals, all-or-nothing gate vialife_change_blocked✔ - targeting: both
collect_target_slotsandcollect_target_slot_specswired for the two participant filters ✔ - multiplayer/coverage:
coverage.rseffect_details+printed_cards.rswalk_effect+trigger_index.rs+clause_is_dig_lookback_transparent✔ - AI:
redundancy_avoidanceno-static-redundancy arm ✔
Gemini's HIGH finding is already resolved at HEAD
Gemini flagged "life exchange loss treated as damage (CR 119.5), use apply_life_loss". That is exactly what this diff does: apply_damage_life_loss is split — apply_life_loss is the CR 119.3 non-damage primitive and the exchange uses it; apply_damage_life_loss is retained as a thin CR 120.3 delegator for the actual damage path. Confirmed against HEAD; no further action needed.
Tests discriminate
exchange_life_totals_swaps_two_players / ..._blocked_when_either_player_cant_gain and the issue_3486_* integration tests drive resolve() and would fail if the swap or the all-or-nothing gate regressed. Parser tests pin the swallowed-clause ratchet for the compound Profane Transfusion / Mister Negative lines.
Non-blocking nit — CR sub-rule precision
The new code annotates the life-total exchange as CR 701.12a (the generic "a spell may instruct players to exchange something"). The precise sub-rule for this mechanic is CR 701.12c ("When life totals are exchanged, each player gains or loses the amount of life necessary to equal the other's life total"). Worth tightening 701.12a → 701.12c on the ExchangeLifeTotals doc-comment, the resolver resolve_life_totals doc, and the parser comment. Not a correctness issue — purely annotation precision — so it does not block.
Enqueue note
mergeStateStatus: BEHIND (no textual conflict). Merge origin/main in before enqueue so the queue can speculate cleanly.
matthewevans
left a comment
There was a problem hiding this comment.
Adversarial second-pass — CHANGES REQUESTED (parser-combinator gate red)
Thanks for this — the Effect::ExchangeLifeTotals design is clean and the add-engine-effect lockstep is complete. Two things block the enqueue: a hard parser-gate failure on the current head, and a pervasive CR-annotation error. This review supersedes the prior approval state.
Architecture verdict: PASS (with a CR-annotation fix required)
ExchangeLifeTotalsis a newEffectvariant (not a field), so it's safe against the mtgish wildcard — confirmed.- add-engine-effect lockstep is wired end-to-end: resolver (
exchange_life.rs), targeting (collect_target_slots+collect_target_slot_specs), coverage (effect_details), trigger index, multiplayer/AI (redundancy_avoidance), serde round-trip via#[serde(default)]. Exhaustive match arms all updated. Good. - The
apply_damage_life_loss→apply_life_lossrename + delegation correctly routes the exchange through the CR 119.3 non-damage life-loss path (Gemini's "loss-as-damage" concern is fixed at this head). All-or-nothing pre-check (life_change_blockedfor both players before any mutation) is correct per CR 701.12a + 119.7/119.8. - Runtime tests are discriminating:
exchange_life_totals_swaps_two_playersand..._blocked_when_either_player_cant_gaindriveresolve()and would fail if reverted; integration testissue_3486_exchange_life_totals.rscovers parse + resolve for both phrase shapes.
Blocker 1 (fatal): nom-combinator mandate violation
Rust lint (fmt, clippy, parser gate) is RED. The parser-combinator gate flags an added line:
crates/engine/src/parser/swallow_check.rs:
if stripped.contains("if you lost life this way")
.contains(...) for parsing dispatch violates the hard CLAUDE.md nom-combinator rule. Two acceptable fixes:
- Rewrite the dispatch with a
nom_primitives::scan_contains/ combinator form (preferred), or - If this is genuinely a structural guard on a pre-cleaned string (it is gating on a normalized
strippedchunk), annotate the line with// allow-noncombinator: <reason>per PATTERNS.md §9.
Blocker 2 (must fix): wrong CR subpart throughout
Every annotation in this PR cites CR 701.12a for life-total exchange, but 701.12a is the generic "if the entire exchange can't be completed, no part occurs" rule. The rule that actually governs life-total exchange (gain/lose to equal the other player's previous total, with the can't-gain/can't-lose riders you implement) is CR 701.12c:
701.12c When life totals are exchanged, each player gains or loses the amount of life necessary to equal the other player's previous life total. … A player who can't gain life can't be given a higher life total this way … (see rules 119.7–8).
Please retarget the citations to CR 701.12c (keeping CR 701.12a only where you specifically mean the all-or-nothing completion guard, and CR 119.3/119.5/119.7/119.8 where already used). The reviewer noted this as a non-blocking nit, but since it's pervasive (~15 sites: variant doc-comment, resolver, parser, targeting, tests) and the repo treats CR-annotation accuracy as a hard gate, fold it into the same revision.
To unblock
- Fix the
swallow_check.rscombinator-gate line (rewrite or// allow-noncombinator:). - Sweep
701.12a→701.12cfor the life-total-exchange citations.
Once Rust lint is green on the head, I'll approve and enqueue.
Life-with-stat exchange is CR 701.12g (numerical value exchange), not 701.12a/701.12c. Keep 701.12a only for all-or-nothing completion guards. Regenerate engine-inventory from updated variant docs. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Cards that make two players exchange life totals were unimplemented. The existing
ExchangeLifeWithStateffect covers life ↔ creature stat (Tree of Perdition, Evra), but not player-to-player swaps.This PR adds
Effect::ExchangeLifeTotalswith parser + resolver coverage for:Closes #3486
Root cause
No
Effectvariant or parser arm recognized player-to-player exchange phrases. They fell through toUnimplementedor partial parses with no runtime handler.Changes
crates/engine/src/types/ability.rs—Effect::ExchangeLifeTotals { player_a, player_b }+EffectKind::ExchangeLifeTotalscrates/engine/src/parser/oracle_effect/imperative.rs—try_parse_exchange_life_totalsfor both grammatical shapescrates/engine/src/game/effects/exchange_life.rs—resolve_life_totalswith CR 701.12a all-or-nothing + CR 119.5 gain/loss-to-reach (mirrorsExchangeLifeWithStatcan't-gain/can't-lose handling)crates/engine/src/game/ability_utils.rs— two player target slots (Controller implicit, no slot)crates/engine/tests/integration/issue_3486_exchange_life_totals.rs— parser + resolver regressionsRules
Test plan
cargo test -p engine --test integration issue_3486_exchange_life_totalscargo test -p engine --lib exchange_life_totalsclippy,test-enginegreen