Support Yurlok of Scorch Thrash - #6717
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds Yurlok-style unspent-mana life-loss parsing and execution, routes life loss through replacement handling, preserves phase-transition state across interactive substitutions, supports deferred life-cost resumes and scoped mana abilities, and expands parser, replacement, mana-drain, and integration coverage. ChangesYurlok unspent-mana life-loss flow
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PhaseTransition
participant EmptyManaPool
participant ApplyLifeLoss
participant ReplacementChoice
participant ContinuationResume
PhaseTransition->>EmptyManaPool: drain unspent mana
EmptyManaPool->>ApplyLifeLoss: apply equivalent life loss
ApplyLifeLoss->>ReplacementChoice: resolve replacement or substitution choice
ReplacementChoice->>ContinuationResume: resume deferred continuation
ContinuationResume->>PhaseTransition: continue phase drain
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/game/effects/life.rs (1)
115-161: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winDeferred substitution outcome is silently dropped across the GainLife pipeline (and
Double), unlike the fixed-up LoseLife pipeline. This refactor correctly madeapply_life_loss/resolve_losepropagateSubstitutionDrainOutcome::DeferredasErr(ReplacementDeferred::SubstitutionContinuation), but three sibling sites on the gain side were not updated to match, so a substitute effect that pauses for player input (e.g. a Lich-class "instead draw N cards" whose draw itself needs a choice) is silently treated as fully resolved.
crates/engine/src/game/effects/life.rs#L115-L161: matchdrain_substitution_continuation's outcome in both theExecuteandPreventedarms ofapply_life_gainand returnErr(ReplacementDeferred::SubstitutionContinuation)onDeferred, mirroringapply_life_loss.crates/engine/src/game/effects/life.rs#L84-L93: inresolve_gain'sPreventedarm, check the drain outcome and return early (skipping the trailingGameEvent::EffectResolvedpush) when it defers, mirroringresolve_lose's.is_err()early return.crates/engine/src/game/effects/double.rs#L150-L166: check theResultreturned by bothapply_life_gainandapply_life_lossinresolve_double_lifeand skip pushingGameEvent::EffectResolvedwhen either call returnsErr, instead of discarding both vialet _ = ....🤖 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/game/effects/life.rs` around lines 115 - 161, Propagate deferred substitution outcomes instead of discarding them: in crates/engine/src/game/effects/life.rs lines 115-161, update apply_life_gain’s Execute and Prevented arms to return Err(ReplacementDeferred::SubstitutionContinuation) when drain_substitution_continuation defers; in crates/engine/src/game/effects/life.rs lines 84-93, make resolve_gain’s Prevented arm return early on a deferred drain and skip GameEvent::EffectResolved; in crates/engine/src/game/effects/double.rs lines 150-166, have resolve_double_life handle Err from both apply_life_gain and apply_life_loss and skip GameEvent::EffectResolved rather than discarding the results.crates/engine/src/game/life_costs.rs (1)
134-151: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Err(_)wildcard conflates two distinctReplacementDeferredvariants with different mutation states.
NeedsChoice/ReplacementChoicefires before any life mutation, butSubstitutionContinuationcan fire inapply_life_loss'sExecutearm after life has already been deducted viaapply_life_loss_after_replacement. Mapping both toInsufficientLifeunder oneErr(_)arm means the reported cost outcome can mismatch the actual state change, and the accompanyingdebug_assert!(false, ...)will panic in debug/test builds if this path is ever reached — contradicting the "avoid silently half-applying" rationale in the comment above it, since the life loss may already be half-applied by that point.As per coding guidelines, "Use exhaustive matches without wildcard fallbacks for known enums so the compiler detects missing variants" —
turns.rs::apply_empty_mana_pool_eventalready demonstrates the idiomatic exhaustive form for this exact enum.🛡️ Proposed exhaustive match
- match apply_life_loss(state, player, amount, events) { - Ok(_) => PayLifeCostResult::Paid { amount }, - Err(_) => { - debug_assert!( - false, - "pay_life_as_cost: unexpected ReplacementDeferred during cost payment" - ); - PayLifeCostResult::InsufficientLife - } - } + match apply_life_loss(state, player, amount, events) { + Ok(_) => PayLifeCostResult::Paid { amount }, + Err(ReplacementDeferred::ReplacementChoice) => { + debug_assert!( + false, + "pay_life_as_cost: unexpected competing-replacement choice during cost payment" + ); + PayLifeCostResult::InsufficientLife + } + Err(ReplacementDeferred::SubstitutionContinuation) => { + // Life has already been deducted at this point; this cost path + // needs its own WaitingFor round-trip rather than reporting a + // mismatched InsufficientLife after the mutation already happened. + todo!("thread a pay-life-cost continuation through the substitution drain") + } + }🤖 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/game/life_costs.rs` around lines 134 - 151, Replace the wildcard error handling in the pay-life cost match around apply_life_loss with an exhaustive match over each ReplacementDeferred variant. Handle NeedsChoice/ReplacementChoice as pre-mutation failures and SubstitutionContinuation according to its post-deduction state, without an unconditional debug_assert panic; preserve the existing Paid result for successful application and ensure each branch reports a state-consistent PayLifeCostResult.Source: Coding guidelines
🧹 Nitpick comments (1)
crates/engine/src/game/replacement.rs (1)
17599-17605: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a first-class turn condition instead of a tautological
OnlyIfQuantity.
Fixed(0) EQ Fixed(0)is an inert comparison whose only purpose is to carryactive_player_req.ReplacementConditionhas no plain "only during your turn" arm (onlyUnlessYourTurnandDuringDrawStep { active_player_req }), so this idiom will get copied. A dedicated arm — or promotingactive_player_reqto its own condition — would express Bloodletter's gate without the dead operands, and would apply to the parser-side emission too.Same shape at Line 17775.
🤖 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/game/replacement.rs` around lines 17599 - 17605, Add a first-class turn-gating condition to ReplacementCondition for “only during your turn,” rather than encoding it through tautological OnlyIfQuantity operands. Update the Bloodletter replacement definitions at both occurrences and the parser-side emission to use this condition while preserving ControllerRef::You semantics. Remove the inert Fixed(0) comparison from these gates.
🤖 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.
Inline comments:
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 428-433: Update the comment above the mana-ability player
iteration to cite CR 101.4 for APNAP ordering; retain CR 608.2c only for
resolving instructions in order and CR 605.3b for immediate mana-ability
resolution.
- Around line 447-494: Update the produced_for_tap_event assignment in the
recipients loop so it captures the mana resolved for the acting player,
identified by player, rather than the first APNAP recipient. Preserve each
recipient’s individual resolution for mana creation, and ensure the
TappedForMana payload’s player_id and produced list always refer to the same
player scope.
In `@crates/engine/src/game/turns.rs`:
- Around line 309-312: Correct the rule citations in
crates/engine/src/game/turns.rs:309-312 to retain CR 106.4 and cite CR
500.4/703.4q for mana loss, removing CR 119.3 because the code causes direct
life loss; update the continuation helper at
crates/engine/src/game/turns.rs:357-361 to retain only citations governing
replacement continuation and remove CR 500.5’s duration-expiry reference.
In `@crates/engine/src/types/mana.rs`:
- Around line 2083-2092: Update Mana::clear_expired_retention_markers to keep
EndOfTurn retention through the End→Cleanup transition, clear it during the
cleanup action, and route cleanup exit through the normal empty-pool pipeline;
preserve EndOfCombat expiry at combat-phase end. In
crates/engine/src/types/mana.rs:2083-2092 change the expiry timing, in
crates/engine/src/types/mana.rs:2430-2454 update assertions for survival through
End→Cleanup and loss/life-loss at cleanup exit, and in
crates/engine/src/types/proposed_event.rs:665-668 distinguish EndOfCombat expiry
from EndOfTurn cleanup expiry.
---
Outside diff comments:
In `@crates/engine/src/game/effects/life.rs`:
- Around line 115-161: Propagate deferred substitution outcomes instead of
discarding them: in crates/engine/src/game/effects/life.rs lines 115-161, update
apply_life_gain’s Execute and Prevented arms to return
Err(ReplacementDeferred::SubstitutionContinuation) when
drain_substitution_continuation defers; in
crates/engine/src/game/effects/life.rs lines 84-93, make resolve_gain’s
Prevented arm return early on a deferred drain and skip
GameEvent::EffectResolved; in crates/engine/src/game/effects/double.rs lines
150-166, have resolve_double_life handle Err from both apply_life_gain and
apply_life_loss and skip GameEvent::EffectResolved rather than discarding the
results.
In `@crates/engine/src/game/life_costs.rs`:
- Around line 134-151: Replace the wildcard error handling in the pay-life cost
match around apply_life_loss with an exhaustive match over each
ReplacementDeferred variant. Handle NeedsChoice/ReplacementChoice as
pre-mutation failures and SubstitutionContinuation according to its
post-deduction state, without an unconditional debug_assert panic; preserve the
existing Paid result for successful application and ensure each branch reports a
state-consistent PayLifeCostResult.
---
Nitpick comments:
In `@crates/engine/src/game/replacement.rs`:
- Around line 17599-17605: Add a first-class turn-gating condition to
ReplacementCondition for “only during your turn,” rather than encoding it
through tautological OnlyIfQuantity operands. Update the Bloodletter replacement
definitions at both occurrences and the parser-side emission to use this
condition while preserving ControllerRef::You semantics. Remove the inert
Fixed(0) comparison from these gates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ab454dc2-ec00-4ea9-912c-49d02690e02f
📒 Files selected for processing (25)
crates/engine/src/analysis/resource.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/exchange_life.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/life_costs.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/replacement.rscrates/engine/src/game/static_abilities.rscrates/engine/src/game/turns.rscrates/engine/src/parser/oracle_classifier.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_static/dispatch.rscrates/engine/src/parser/oracle_static/mana_transform.rscrates/engine/src/parser/oracle_static/mod.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mana.rscrates/engine/src/types/proposed_event.rscrates/engine/src/types/statics.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/necrodominance_pay_any_life_draw.rscrates/engine/tests/integration/yurlok_of_scorch_thrash.rs
| let mut produced_for_tap_event = Vec::new(); | ||
| for recipient in recipients { | ||
| let mut scoped = resolved_for_quantity.clone(); | ||
| scoped.set_original_controller_recursive(player); | ||
| scoped.set_controller_recursive(recipient); | ||
| scoped.set_scoped_player_recursive(recipient); | ||
|
|
||
| // CR 106.6: Resolve spend-restriction templates, grants, and expiry so | ||
| // they attach to each produced `ManaUnit`. | ||
| let (produced_mana, restrictions, grants, expiry, source_could_produce_two_or_more_colors) = | ||
| match &scoped.effect { | ||
| Effect::Mana { | ||
| produced, | ||
| restrictions, | ||
| grants, | ||
| expiry, | ||
| target: None, | ||
| } => { | ||
| let mana = match color_override.clone() { | ||
| // `Combination` is pre-chosen — skip `resolve_mana_types` | ||
| // so the exact sequence lands in the pool (CR 605.3b). | ||
| Some(ProductionOverride::Combination(types)) => types, | ||
| Some(ProductionOverride::SingleColor(color)) => { | ||
| resolve_single_color_override(state, produced, &scoped, color) | ||
| } | ||
| None => super::effects::mana::resolve_mana_types_for_ability( | ||
| produced, state, &scoped, | ||
| ), | ||
| }; | ||
| let concrete = resolve_restrictions(restrictions, state, source_id); | ||
| let source_could_produce_two_or_more_colors = | ||
| mana_sources::source_could_produce_two_or_more_colors( | ||
| state, source_id, player, | ||
| ); | ||
| ( | ||
| mana, | ||
| concrete, | ||
| grants.clone(), | ||
| *expiry, | ||
| source_could_produce_two_or_more_colors, | ||
| ) | ||
| } | ||
| _ => (Vec::new(), Vec::new(), Vec::new(), None, false), | ||
| }; | ||
|
|
||
| if produced_for_tap_event.is_empty() { | ||
| produced_for_tap_event.clone_from(&produced_mana); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
produced_for_tap_event carries the first recipient's mana, not the acting player's.
recipients is built in APNAP order, so with a player_scope the first element is the active player, which need not be player (the ability's controller). The TappedForMana event then reports player_id: player alongside a produced list that was resolved in a different player's scope. That diverges whenever per-recipient resolution can differ (resolve_mana_types_for_ability on "any color"/choice-driven produced, or scope-sensitive restrictions), and "Whenever you tap ~ for mana…" matchers read that list. For Yurlok every recipient gets the same BRG, so the tests can't distinguish the two — the bug is latent, not absent.
Either capture the list resolved for player specifically, or rename and document the field as a representative sample and justify why divergence is out of scope.
🐛 Suggested fix: bind the tap-event payload to the acting player's scope
- if produced_for_tap_event.is_empty() {
+ // CR 106.12a: the "tapped for mana" event describes the mana this
+ // permanent's controller produced, so report the acting player's
+ // resolution rather than whichever recipient came first in APNAP order.
+ if recipient == player {
produced_for_tap_event.clone_from(&produced_mana);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut produced_for_tap_event = Vec::new(); | |
| for recipient in recipients { | |
| let mut scoped = resolved_for_quantity.clone(); | |
| scoped.set_original_controller_recursive(player); | |
| scoped.set_controller_recursive(recipient); | |
| scoped.set_scoped_player_recursive(recipient); | |
| // CR 106.6: Resolve spend-restriction templates, grants, and expiry so | |
| // they attach to each produced `ManaUnit`. | |
| let (produced_mana, restrictions, grants, expiry, source_could_produce_two_or_more_colors) = | |
| match &scoped.effect { | |
| Effect::Mana { | |
| produced, | |
| restrictions, | |
| grants, | |
| expiry, | |
| target: None, | |
| } => { | |
| let mana = match color_override.clone() { | |
| // `Combination` is pre-chosen — skip `resolve_mana_types` | |
| // so the exact sequence lands in the pool (CR 605.3b). | |
| Some(ProductionOverride::Combination(types)) => types, | |
| Some(ProductionOverride::SingleColor(color)) => { | |
| resolve_single_color_override(state, produced, &scoped, color) | |
| } | |
| None => super::effects::mana::resolve_mana_types_for_ability( | |
| produced, state, &scoped, | |
| ), | |
| }; | |
| let concrete = resolve_restrictions(restrictions, state, source_id); | |
| let source_could_produce_two_or_more_colors = | |
| mana_sources::source_could_produce_two_or_more_colors( | |
| state, source_id, player, | |
| ); | |
| ( | |
| mana, | |
| concrete, | |
| grants.clone(), | |
| *expiry, | |
| source_could_produce_two_or_more_colors, | |
| ) | |
| } | |
| _ => (Vec::new(), Vec::new(), Vec::new(), None, false), | |
| }; | |
| if produced_for_tap_event.is_empty() { | |
| produced_for_tap_event.clone_from(&produced_mana); | |
| } | |
| let mut produced_for_tap_event = Vec::new(); | |
| for recipient in recipients { | |
| let mut scoped = resolved_for_quantity.clone(); | |
| scoped.set_original_controller_recursive(player); | |
| scoped.set_controller_recursive(recipient); | |
| scoped.set_scoped_player_recursive(recipient); | |
| // CR 106.6: Resolve spend-restriction templates, grants, and expiry so | |
| // they attach to each produced `ManaUnit`. | |
| let (produced_mana, restrictions, grants, expiry, source_could_produce_two_or_more_colors) = | |
| match &scoped.effect { | |
| Effect::Mana { | |
| produced, | |
| restrictions, | |
| grants, | |
| expiry, | |
| target: None, | |
| } => { | |
| let mana = match color_override.clone() { | |
| // `Combination` is pre-chosen — skip `resolve_mana_types` | |
| // so the exact sequence lands in the pool (CR 605.3b). | |
| Some(ProductionOverride::Combination(types)) => types, | |
| Some(ProductionOverride::SingleColor(color)) => { | |
| resolve_single_color_override(state, produced, &scoped, color) | |
| } | |
| None => super::effects::mana::resolve_mana_types_for_ability( | |
| produced, state, &scoped, | |
| ), | |
| }; | |
| let concrete = resolve_restrictions(restrictions, state, source_id); | |
| let source_could_produce_two_or_more_colors = | |
| mana_sources::source_could_produce_two_or_more_colors( | |
| state, source_id, player, | |
| ); | |
| ( | |
| mana, | |
| concrete, | |
| grants.clone(), | |
| *expiry, | |
| source_could_produce_two_or_more_colors, | |
| ) | |
| } | |
| _ => (Vec::new(), Vec::new(), Vec::new(), None, false), | |
| }; | |
| // CR 106.12a: the "tapped for mana" event describes the mana this | |
| // permanent's controller produced, so report the acting player's | |
| // resolution rather than whichever recipient came first in APNAP order. | |
| if recipient == player { | |
| produced_for_tap_event.clone_from(&produced_mana); | |
| } |
🤖 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/game/mana_abilities.rs` around lines 447 - 494, Update the
produced_for_tap_event assignment in the recipients loop so it captures the mana
resolved for the acting player, identified by player, rather than the first
APNAP recipient. Preserve each recipient’s individual resolution for mana
creation, and ensure the TappedForMana payload’s player_id and produced list
always refer to the same player scope.
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed at head f99b52bceca32e9acbb7e100338a8b74830b901d. The parser/static marker is on the right broad seam, but the shared phase, life, and mana-event paths still have four correctness blockers.
-
[BLOCKING]
until end of turnretained mana is lost while entering cleanup.crates/engine/src/game/turns.rs:479-489clearsManaExpiry::EndOfTurnwhenevernext == Phase::Cleanup;crates/engine/src/types/mana.rs:2083-2092then makes that mana eligible for the same End→Cleanup empty-pool event. The regression test atcrates/engine/tests/integration/yurlok_of_scorch_thrash.rs:947-972asserts that premature life loss. I re-verified the official current CR: 500.5b sends “until end of turn” durations to CR 514.2, and CR 514.2 ends them during cleanup. Thus the retention must survive the end-step emptying; only after cleanup expires it may the normal empty-pool action occur at the cleanup boundary. Model that timing explicitly in the phase/cleanup authority, keepEndOfCombatdistinct, and replace the current test with a discriminating End→Cleanup-survives / cleanup-exit-loses case. -
[BLOCKING] Deferred life-gain substitutes are dropped.
crates/engine/src/game/effects/life.rs:80-106and115-161discarddrain_substitution_continuation’sDeferredresult and still emitEffectResolved;crates/engine/src/game/effects/double.rs:150-175similarly discards both life-helper results. The sibling loss path now correctly returnsReplacementDeferred::SubstitutionContinuationatlife.rs:272-300. Propagate that same typed continuation outcome through gain and Double, and emit completion only after the replacement continuation has terminally drained. Add an interactive gain-substitution test that would fail if the outcome is ignored. -
[BLOCKING] Paying life can report an unpaid cost after life has changed.
crates/engine/src/game/life_costs.rs:134-150maps everyapply_life_losserror toInsufficientLife, but the newSubstitutionContinuationpath can deduct life before returningErr(crates/engine/src/game/effects/life.rs:272-281). This leaves an impossible cost result (and the unconditional debug assertion panics in debug/test). MatchReplacementDeferredexhaustively and thread the post-mutation case through the typed cost/continuation authority; do not translate it to insufficient life. Cover the state/result pairing with a replacement that pauses after the life-loss mutation. -
[BLOCKING]
TappedForManacombines one player with another recipient’s mana.crates/engine/src/game/mana_abilities.rs:447-494snapshots the first APNAP recipient’sproduced_mana, then:537-543emits it asplayer_id: player(the activator). Recipient-specific color/choice/restriction resolution can therefore make TapsForMana consumers observe a mismatched player/payload. Bind the payload to the same recipient scope as its player id (or define and implement an explicitly source-level aggregate event), including the scope-excludes-activator case. The Yurlok test only uses fixed BRG (yurlok_of_scorch_thrash.rs:46-82), so add a discriminating scoped, recipient-dependent fixture.
Existing green CI does not cover these paths. CodeRabbit independently raised the continuation, pay-life, and TappedForMana issues; I confirmed them against this exact head.
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head cf7fef8ec4f7746004548cad631540145eff28cb.
[BLOCKING] Deferred pay-life substitutions still let several outer actions complete while the substitute is waiting for player input. PayLifeCostResult::PaidWithDeferredSubstitution correctly distinguishes the post-mutation pause in crates/engine/src/game/life_costs.rs:147-151, but crates/engine/src/game/casting.rs:14348-14368 and :14830-14844, crates/engine/src/game/casting_costs.rs:6007-6020, and crates/engine/src/game/engine_resolution_choices.rs:2492-2510 group it with Paid and immediately continue the cast, payment, or continuation. crates/engine/src/game/mana_abilities.rs:3123-3139 also returns Ok for that result; its outer cursor happens to detect a changed waiting_for, but the spell/effect paths do not. Why it matters: after a life-loss replacement deducts life then opens an interactive substitute, the spell or action can be pushed/advanced while WaitingFor still belongs to the substitute—the cost/result state is still incoherent. The added regression reaches only pay_life_as_cost, not those outer production paths.
Please carry a typed paused-cost outcome through every affected cast/Defiler/effect-PayAmount/shared-payment caller, persist the appropriate continuation, and add at least one GameAction pipeline regression proving the outer spell/action is not finalized until the substitute choice resolves.
The other original blockers are resolved on this head: end-of-turn retained mana survives End→Cleanup and expires during cleanup; gain/Double propagate deferred substitutions; and scoped mana uses an explicit source-level aggregate TappedForMana payload with recipient-dependent coverage. The current parse-diff artifact is accounted for.
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head 360b346b96a2ca29307cd0ccda69f852a695416a.
The earlier paused-cost propagation paths are improved, and the current parse-diff artifact is accounted for. Two blockers remain.
-
[BLOCKING] A deferred resolution-time payment can skip later composite cost legs.
crates/engine/src/game/costs.rs:388-450preserves the full concreteManaAbilityResume::EffectPayCost::cost; the construction at:788-800deliberately includes the paid mana leaf and the remaining composite suffix. But after an interactive Phyrexian/life replacement settles,crates/engine/src/game/mana_abilities.rs:2702-2711ignorescostand immediately resolves the effect-pay-cost rider. Thus a composite payment whose Phyrexian/life mana prefix pauses can resolve its rider without paying a later cost leg.Please persist and resume the exact post-prefix continuation rather than replaying spent mana or treating the whole payment as complete. Add a real
GameActionregression that includes a remaining suffix and proves the rider stays parked until that suffix is paid after the replacement choice. Audit the parallelUnlessPaymentcompletion atmana_abilities.rs:2692-2701in the same design: it also retains a cost but completes as though every component has already been paid. -
[BLOCKING] Current CI is red because the new game-state write family is absent from the CR733 authority matrix. Rust shard 2 job
90191041650failscrates/engine/tests/integration/cr733_resolved_commands_p0.rs:313withpending_deferred_life_cost_resumemissing from the authority matrix (12,230 / 12,231 tests pass). The field was added toGameStateatcrates/engine/src/types/game_state.rs:12987-12992, but this push does not update the corresponding CR733 authority-matrix fixture. Please classify/register the field and restore a green merged-head run.
The branch is also BEHIND current main; after both fixes, update/revalidate against current main before requesting re-review. The PR remains on hold and must not be enqueued.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/engine/src/game/casting.rs (2)
14380-14402: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the repeated Phyrexian life-payment loop into one shared helper. All three sites walk
life_payments, pay each component, and onPaidWithDeferredSubstitutioncapture the unpaid tail and break — differing only in whichpay_life_as_cast*function is invoked.
crates/engine/src/game/casting.rs#L14380-L14402: replace this loop inpay_mana_cost_from_pool_with_choiceswith a call to a sharedpay_life_components(state, player, &life_payments, events, super::life_costs::pay_life_as_cast_or_activation_cost)helper.crates/engine/src/game/casting.rs#L14934-L14947: replace this loop inpay_non_cast_mana_costwith the same helper, passingsuper::life_costs::pay_life_as_costas the pay function.crates/engine/src/game/casting.rs#L15115-L15129: replace this loop inauto_tap_and_pay_cost_excludingwith the same helper, passingsuper::life_costs::pay_life_as_cast_or_activation_cost.As per coding guidelines: "Reuse existing shared building blocks before adding utility or inline extraction logic; test the building block and its parameter range rather than a single card case."
🤖 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/game/casting.rs` around lines 14380 - 14402, The repeated Phyrexian life-payment loops should use one shared helper. In crates/engine/src/game/casting.rs:14380-14402, add or reuse pay_life_components and replace the loop in pay_mana_cost_from_pool_with_choices, passing pay_life_as_cast_or_activation_cost; make the corresponding replacement at 14934-14947 in pay_non_cast_mana_cost with pay_life_as_cost, and at 15115-15129 in auto_tap_and_pay_cost_excluding with pay_life_as_cast_or_activation_cost. Preserve the existing unpaid-tail and error behavior through the helper.Source: Coding guidelines
14551-14610: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThread deferred-life resumes through the activation wrapper.
pay_ability_mana_cost_excludingstill turnsManaCostPayment::PausedintoInvalidAction, but the activation path can hit a Phyrexian/life-loss defer. That dropsstate.pending_deferred_life_cost_resumeafter mana/tap work has already run, so the activation can’t resume through the replacement choice. Use the resume-aware helper here, or persist the resume state before returning.🤖 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/game/casting.rs` around lines 14551 - 14610, Update pay_ability_mana_cost_excluding to use the resume-aware activation/payment helper so a ManaCostPayment::Paused result preserves state.pending_deferred_life_cost_resume through deferred Phyrexian/life-loss replacement choices. Do not convert this paused result into InvalidAction after mana or tap work has already occurred; ensure the activation path can resume via the existing continuation flow.crates/engine/src/game/engine_resolution_choices.rs (1)
2492-2514: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the
_ =>wildcard with explicitInsufficientLife | Prohibitedarms.This
matchonPayLifeCostResultwas modified by this diff to insert the newPaidWithDeferredSubstitutionarm, but the pre-existing_ => { return Err(...) }catch-all remains for the other two variants. The sibling match on the same enum incosts.rs(AbilityCost::PayLifearm) enumeratesInsufficientLife | Prohibitedexplicitly instead of using a wildcard. Since this match block is part of the touched code, please align it with the sibling and the repo's exhaustive-match convention so a future new variant is caught by the compiler here too.🐛 Suggested fix
- _ => { + crate::game::life_costs::PayLifeCostResult::InsufficientLife + | crate::game::life_costs::PayLifeCostResult::Prohibited => { return Err(EngineError::InvalidAction(format!( "Player {player:?} cannot pay {amount} life" ))) }As per coding guidelines: "Use exhaustive
matchover wildcard defaults" and as per path instructions: "wildcard_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants" is a finding undercrates/**/*.rs.🤖 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/game/engine_resolution_choices.rs` around lines 2492 - 2514, Replace the wildcard arm in the PayLifeCostResult match within the resolution-choice flow with an explicit InsufficientLife | Prohibited arm, preserving the existing InvalidAction error behavior. Keep the Paid and PaidWithDeferredSubstitution arms unchanged so future enum variants trigger a compiler error.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (6)
crates/engine/src/game/casting_costs.rs (3)
6110-6115: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing
cost_shard_matches_reductionfor shard cancellation.
apply_offering_cost_reductionusessuper::casting::cost_shard_matches_reductionso a colored reduction can cancel a hybrid/Phyrexian pip. This helper matches shards by exact equality, so a{G/W}pip would not be reduced by a{G}Defiler reduction. Not reachable with today's Defiler printings, but sharing the one matcher keeps the two reduction sites from diverging.🤖 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/game/casting_costs.rs` around lines 6110 - 6115, Update the shard-cancellation loop in apply_offering_cost_reduction to use super::casting::cost_shard_matches_reduction instead of exact shard equality, while preserving removal of only the first matching spell shard for each reduction shard.
8326-8336: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFallback
PendingCastdrops cast state that a real resume would need.The synthesized pending omits
base_cost,distribute,payment_mode,convoked_creatures, andpinned_pool_units. Today only the#[cfg(test)]wrapper passesdeferred_life_resume_pending: None, so this is latent — but if a production caller ever does, the resumed cast silently loses its X/base-cost and distribution state. Adebug_assert!(deferred_life_resume_pending.is_some())(or making the parameter non-optional and having the test wrapper build the pending) would keep the trap from being reachable.🤖 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/game/casting_costs.rs` around lines 8326 - 8336, Prevent the fallback in the deferred life-resume flow from synthesizing an incomplete PendingCast: require deferred_life_resume_pending to be present, preferably by making the parameter non-optional and updating the #[cfg(test)] wrapper to construct and pass the pending state. If the optional API must remain, add a debug assertion before the fallback and preserve base_cost, distribute, payment_mode, convoked_creatures, and pinned_pool_units when constructing it.
11767-11799: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider unifying
finalize_mana_payment_with_resumeandfinalize_mana_payment_with_phyrexian_choices.These two bodies are now near-identical, differing only by
Option<&[ShardChoice]>and theManaAbilityResumevariant. This change adds four more copies of the sameManaCostPayment::Pausedhandling (activation, spell, distribute), so any future fix to the deferred-life resume contract has to be applied in both places or they drift. Threading the choices/resume as parameters into one shared body would collapse the duplication.Also applies to: 11853-11866, 11894-11941
🤖 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/game/casting_costs.rs` around lines 11767 - 11799, Unify finalize_mana_payment_with_resume and finalize_mana_payment_with_phyrexian_choices into one shared finalization path parameterized by Option<&[ShardChoice]> and the appropriate ManaAbilityResume variant. Route the activation, spell, and distribute callers through that shared body, preserving the existing ManaCostPayment::Paused deferred-life resume behavior and each caller’s specific choices/resume values.crates/engine/src/game/engine_payment_choices.rs (1)
1240-1259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated settle-tail between
handle_unless_payment's decline path and the newfinish_successful_unless_payment.The
set_active_priority→resume_pending_continuation_if_priority→ conditionalrun_post_action_pipeline_fromsequence at the bottom ofhandle_unless_payment(1240-1259) is now duplicated verbatim inside the newly extractedfinish_successful_unless_payment(1330-1349). Consider factoring this shared "settle after an unless-payment branch" tail into its own helper takingpost_action_event_start: Option<usize>and returning the settledWaitingFor, called from both the decline path andfinish_successful_unless_payment, so a future fix to the settle sequence doesn't need to be made in two places.Also applies to: 1330-1349
🤖 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/game/engine_payment_choices.rs` around lines 1240 - 1259, Extract the duplicated settle-after-unless-payment sequence from handle_unless_payment and finish_successful_unless_payment into a shared helper accepting post_action_event_start: Option<usize> and returning the settled WaitingFor. Have both call sites use this helper, preserving the existing priority activation, continuation resumption, and conditional post-action pipeline behavior.crates/engine/src/game/engine.rs (2)
3266-3296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew deferred-life gating logic lacks a CR citation.
The new boundary-gating logic in
resume_pending_continuation_if_priorityimplements the same CR 118.3b/119.4/616.1 sequencing thatdrain_pending_deferred_life_cost_resume(3319-3321) explicitly cites, but this segment has no inline comment/citation of its own, unlike the surrounding CR 614.6/605.3b blocks (3297-3314) in the same function. As per path instructions, rules-touching engine code should carry a verifiedCR <number>: <description>annotation.Based on learnings and as per path instructions: "rules-touching code with no verified
CR <number>: <description>annotation" is a finding undercrates/engine/**.🤖 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/game/engine.rs` around lines 3266 - 3296, Add a verified inline CR 118.3b/119.4/616.1 annotation to the deferred-life boundary-gating block in resume_pending_continuation_if_priority, covering the drain_pending_continuation and resume_resolution_frames sequencing. Keep the existing logic unchanged and match the surrounding CR citation style.Source: Path instructions
3261-3296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant double drain in the common (no-deferred-life-cost) path.
Block at 3266-3272 already runs
drain_pending_continuationwheneverdeferred_life_boundary.is_none_or(...), which istruefor the common case wherepending_deferred_life_cost_resumeisNone— and likewise 3273-3277 runsresume_resolution_frames. Then block 3289-3296 re-runs the identical pair wheneverpending_deferred_life_cost_resume.is_none(), which is the same condition in the no-Yurlok-in-flight case. So on every ordinary priority settle (the overwhelming majority of calls to this very hot function),drain_pending_continuationandresume_resolution_frameseach run twice back-to-back. If they're idempotent no-ops when nothing is queued this is just wasted cycles, but the duplication also makes the intended control flow (drain → maybe-resume-deferred-life → drain-again-only-if-that-path-ran) hard to follow and easy to break with a future edit.Consider tracking whether the deferred-life branch (3278-3288) actually ran, and only re-draining in that case, e.g.:
♻️ Suggested restructure
- if deferred_life_boundary.is_none_or(|boundary| state.resolution_stack.len() > boundary) { - effects::drain_pending_continuation(state, events); - } - if matches!(state.waiting_for, WaitingFor::Priority { .. }) - && deferred_life_boundary.is_none_or(|boundary| state.resolution_stack.len() > boundary) - { - effects::resume_resolution_frames(state, events); - } - if matches!(state.waiting_for, WaitingFor::Priority { .. }) - && state - .pending_deferred_life_cost_resume - .as_ref() - .is_some_and(|resume| { - state.resolution_stack.len() <= resume.resume_at_resolution_depth() - }) - { - let waiting_for = drain_pending_deferred_life_cost_resume(state, events)?; - state.waiting_for = waiting_for; - } - if matches!(state.waiting_for, WaitingFor::Priority { .. }) - && state.pending_deferred_life_cost_resume.is_none() - { - effects::drain_pending_continuation(state, events); - if matches!(state.waiting_for, WaitingFor::Priority { .. }) { - effects::resume_resolution_frames(state, events); - } - } + let boundary_not_yet_reached = state + .pending_deferred_life_cost_resume + .as_ref() + .is_some_and(|resume| state.resolution_stack.len() > resume.resume_at_resolution_depth()); + if boundary_not_yet_reached || state.pending_deferred_life_cost_resume.is_none() { + effects::drain_pending_continuation(state, events); + if matches!(state.waiting_for, WaitingFor::Priority { .. }) { + effects::resume_resolution_frames(state, events); + } + } + if matches!(state.waiting_for, WaitingFor::Priority { .. }) { + if let Some(resume) = state.pending_deferred_life_cost_resume.as_ref() { + if state.resolution_stack.len() <= resume.resume_at_resolution_depth() { + state.waiting_for = drain_pending_deferred_life_cost_resume(state, events)?; + if matches!(state.waiting_for, WaitingFor::Priority { .. }) { + effects::drain_pending_continuation(state, events); + if matches!(state.waiting_for, WaitingFor::Priority { .. }) { + effects::resume_resolution_frames(state, events); + } + } + } + } + }🤖 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/game/engine.rs` around lines 3261 - 3296, Update resume_pending_continuation_if_priority so the final drain_pending_continuation and resume_resolution_frames calls do not repeat the initial pair on the ordinary no-deferred-life-cost path. Track whether drain_pending_deferred_life_cost ran in the deferred-life branch, and only perform the final drain-and-resume sequence when that branch actually executed and priority is still pending.
🤖 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.
Inline comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 2380-2407: The resumed cast branch in the payment handling must
re-run deferred-sacrifice validation before committing sacrifices. In the
ManaCostPayment::Paused path, invoke
validate_deferred_spell_sacrifices_at_commit with the resumed pending cast state
before pay_deferred_spell_sacrifices_at_commit, ensuring the chosen permanent is
still valid while preserving the existing pause/resume bookkeeping.
In `@crates/engine/src/game/engine.rs`:
- Around line 3319-3409: Update drain_pending_deferred_life_cost_resume to
preserve the taken DeferredLifeCostResume and restore it to
state.pending_deferred_life_cost_resume whenever the drain returns Err,
including Cast and ManaRoot payment failures. Use a clone-and-restore-on-error
guard around the existing resume processing while keeping successful completion
and waiting-state behavior unchanged.
In `@crates/engine/src/game/life_costs.rs`:
- Around line 146-150: Update the apply_life_cost result handling around
apply_life_loss so ReplacementDeferred::ReplacementChoice returns a distinct
deferred outcome instead of InsufficientLife. Preserve the pending
WaitingFor::ReplacementChoice state and cost-resume context, and ensure the
caller resumes payment after the replacement choice, while leaving the existing
success and substitution-continuation branches unchanged.
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 2756-2778: Update the deferred life-payment drain loop around
pause_mana_ability_cost_payment so PaidWithDeferredSubstitution preserves and
passes the relevant mana-ability context instead of unconditionally supplying
None, avoiding the same panic risk handled in the sibling flow. Add a verified
CR 118.3 + CR 119.4 + CR 616.1 annotation for this rules-relevant segment,
matching the file’s existing documentation convention.
In `@crates/engine/src/types/game_state.rs`:
- Around line 12987-12993: Update filter_state_for_viewer to clear
pending_deferred_life_cost_resume when producing viewer-facing state, matching
the existing pending_cost_move_resume redaction. Ensure the Cast variant’s
embedded PendingCast continuation data is not retained in serialized snapshots
visible to opponents.
---
Outside diff comments:
In `@crates/engine/src/game/casting.rs`:
- Around line 14380-14402: The repeated Phyrexian life-payment loops should use
one shared helper. In crates/engine/src/game/casting.rs:14380-14402, add or
reuse pay_life_components and replace the loop in
pay_mana_cost_from_pool_with_choices, passing
pay_life_as_cast_or_activation_cost; make the corresponding replacement at
14934-14947 in pay_non_cast_mana_cost with pay_life_as_cost, and at 15115-15129
in auto_tap_and_pay_cost_excluding with pay_life_as_cast_or_activation_cost.
Preserve the existing unpaid-tail and error behavior through the helper.
- Around line 14551-14610: Update pay_ability_mana_cost_excluding to use the
resume-aware activation/payment helper so a ManaCostPayment::Paused result
preserves state.pending_deferred_life_cost_resume through deferred
Phyrexian/life-loss replacement choices. Do not convert this paused result into
InvalidAction after mana or tap work has already occurred; ensure the activation
path can resume via the existing continuation flow.
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 2492-2514: Replace the wildcard arm in the PayLifeCostResult match
within the resolution-choice flow with an explicit InsufficientLife | Prohibited
arm, preserving the existing InvalidAction error behavior. Keep the Paid and
PaidWithDeferredSubstitution arms unchanged so future enum variants trigger a
compiler error.
---
Nitpick comments:
In `@crates/engine/src/game/casting_costs.rs`:
- Around line 6110-6115: Update the shard-cancellation loop in
apply_offering_cost_reduction to use
super::casting::cost_shard_matches_reduction instead of exact shard equality,
while preserving removal of only the first matching spell shard for each
reduction shard.
- Around line 8326-8336: Prevent the fallback in the deferred life-resume flow
from synthesizing an incomplete PendingCast: require
deferred_life_resume_pending to be present, preferably by making the parameter
non-optional and updating the #[cfg(test)] wrapper to construct and pass the
pending state. If the optional API must remain, add a debug assertion before the
fallback and preserve base_cost, distribute, payment_mode, convoked_creatures,
and pinned_pool_units when constructing it.
- Around line 11767-11799: Unify finalize_mana_payment_with_resume and
finalize_mana_payment_with_phyrexian_choices into one shared finalization path
parameterized by Option<&[ShardChoice]> and the appropriate ManaAbilityResume
variant. Route the activation, spell, and distribute callers through that shared
body, preserving the existing ManaCostPayment::Paused deferred-life resume
behavior and each caller’s specific choices/resume values.
In `@crates/engine/src/game/engine_payment_choices.rs`:
- Around line 1240-1259: Extract the duplicated settle-after-unless-payment
sequence from handle_unless_payment and finish_successful_unless_payment into a
shared helper accepting post_action_event_start: Option<usize> and returning the
settled WaitingFor. Have both call sites use this helper, preserving the
existing priority activation, continuation resumption, and conditional
post-action pipeline behavior.
In `@crates/engine/src/game/engine.rs`:
- Around line 3266-3296: Add a verified inline CR 118.3b/119.4/616.1 annotation
to the deferred-life boundary-gating block in
resume_pending_continuation_if_priority, covering the drain_pending_continuation
and resume_resolution_frames sequencing. Keep the existing logic unchanged and
match the surrounding CR citation style.
- Around line 3261-3296: Update resume_pending_continuation_if_priority so the
final drain_pending_continuation and resume_resolution_frames calls do not
repeat the initial pair on the ordinary no-deferred-life-cost path. Track
whether drain_pending_deferred_life_cost ran in the deferred-life branch, and
only perform the final drain-and-resume sequence when that branch actually
executed and priority is still pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a17123f4-32ff-44e9-ae04-ae3b9bf11515
📒 Files selected for processing (21)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/companion.rscrates/engine/src/game/costs.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/pay.rscrates/engine/src/game/end_continuous_effect.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_payment_choices.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/life_costs.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/replacement.rscrates/engine/src/game/turns.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mana.rscrates/engine/src/types/proposed_event.rscrates/engine/tests/integration/yurlok_of_scorch_thrash.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/src/types/proposed_event.rs
- crates/engine/src/game/replacement.rs
- crates/engine/src/game/effects/life.rs
- crates/engine/tests/integration/yurlok_of_scorch_thrash.rs
| while let Some(amount) = cursor.remaining_life_payments.first().copied() { | ||
| cursor.remaining_life_payments.remove(0); | ||
| match life_costs::pay_life_as_cast_or_activation_cost(state, pending.player, amount, events) | ||
| { | ||
| PayLifeCostResult::Paid { .. } => {} | ||
| PayLifeCostResult::PaidWithDeferredSubstitution { .. } => { | ||
| pause_mana_ability_cost_payment( | ||
| state, | ||
| None, | ||
| pending, | ||
| cursor, | ||
| events, | ||
| cost_event_start, | ||
| ); | ||
| return Ok(state.waiting_for.clone()); | ||
| } | ||
| PayLifeCostResult::InsufficientLife | PayLifeCostResult::Prohibited => { | ||
| return Err(EngineError::ActionNotAllowed( | ||
| "Cannot complete deferred Phyrexian life cost for mana ability".to_string(), | ||
| )); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Same unconditional None panic risk in the deferred life-payment drain loop.
This new loop drains cursor.remaining_life_payments after a resume and, on PaidWithDeferredSubstitution, also calls pause_mana_ability_cost_payment(state, None, ...) unconditionally — the same pattern flagged at Lines 2317-2372. See the consolidated comment.
Separately: this segment has no CR citation, unlike sibling life-payment code in this file (pay_life_cost cites CR 118.3 + CR 119.4 + CR 119.8; finish_mana_root_after_deferred_life_payment cites CR 118.3b + CR 119.4 + CR 616.1). CR 119.4 governs paying life as a cost, so an annotation here (e.g. CR 118.3 + CR 119.4 + CR 616.1) would keep this file's documentation convention intact for a new rules-relevant control-flow segment.
As per path instructions: "flag rules-touching code with no verified CR <number>: <description> annotation."
📝 Suggested annotation
let ability_def = mana_ability_definition(state, &pending)?;
if cost_sacrifices_reserved_source(state, pending.source_id, &ability_def.cost) {
return Err(EngineError::ActionNotAllowed(
"This permanent is already committed to a spell sacrifice cost".to_string(),
));
}
+ // CR 118.3 + CR 119.4 + CR 616.1: Resume paying any Phyrexian-style life
+ // payments left queued from a prior pause before advancing the other cost
+ // components, so partial payment order is preserved across a substitution.
while let Some(amount) = cursor.remaining_life_payments.first().copied() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while let Some(amount) = cursor.remaining_life_payments.first().copied() { | |
| cursor.remaining_life_payments.remove(0); | |
| match life_costs::pay_life_as_cast_or_activation_cost(state, pending.player, amount, events) | |
| { | |
| PayLifeCostResult::Paid { .. } => {} | |
| PayLifeCostResult::PaidWithDeferredSubstitution { .. } => { | |
| pause_mana_ability_cost_payment( | |
| state, | |
| None, | |
| pending, | |
| cursor, | |
| events, | |
| cost_event_start, | |
| ); | |
| return Ok(state.waiting_for.clone()); | |
| } | |
| PayLifeCostResult::InsufficientLife | PayLifeCostResult::Prohibited => { | |
| return Err(EngineError::ActionNotAllowed( | |
| "Cannot complete deferred Phyrexian life cost for mana ability".to_string(), | |
| )); | |
| } | |
| } | |
| } | |
| let ability_def = mana_ability_definition(state, &pending)?; | |
| if cost_sacrifices_reserved_source(state, pending.source_id, &ability_def.cost) { | |
| return Err(EngineError::ActionNotAllowed( | |
| "This permanent is already committed to a spell sacrifice cost".to_string(), | |
| )); | |
| } | |
| // CR 118.3 + CR 119.4 + CR 616.1: Resume paying any Phyrexian-style life | |
| // payments left queued from a prior pause before advancing the other cost | |
| // components, so partial payment order is preserved across a substitution. | |
| while let Some(amount) = cursor.remaining_life_payments.first().copied() { |
🤖 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/game/mana_abilities.rs` around lines 2756 - 2778, Update
the deferred life-payment drain loop around pause_mana_ability_cost_payment so
PaidWithDeferredSubstitution preserves and passes the relevant mana-ability
context instead of unconditionally supplying None, avoiding the same panic risk
handled in the sibling flow. Add a verified CR 118.3 + CR 119.4 + CR 616.1
annotation for this rules-relevant segment, matching the file’s existing
documentation convention.
Source: Path instructions
360b346 to
7fa8eea
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed at current head 7fa8eea77246190add01dbdcf5b3e37442807b02.
[BLOCKING] The required Rust suite is red because the paused auto-tap/unless-payment continuation no longer has one coherent cost representation. GitHub Actions run 30374984207 fails cost_zone_pipeline::auto_tap_cost_move_redirect_preserves_outer_unless_payment at crates/engine/tests/integration/cost_zone_pipeline.rs:3506 (12,251 passed, 1 failed). The assertion expects the original Composite[Green, generic(1)] in ManaAbilityResume::UnlessPayment.cost.
costs.rs:437-453 intentionally stores the concrete cost passed to the resume root, while engine_payment_choices.rs:958-975 canonicalizes all-mana composite unless costs into one combined mana cost before payment. Those can be valid semantics, but the parked ManaAbilityResume::UnlessPayment.cost, its caller, and the regression currently disagree about which representation is authoritative.
Please choose and preserve one canonical representation through WaitingFor::UnlessPayment, ManaAbilityResume::UnlessPayment, and resume (and update the assertion accordingly). If the normalized root remains canonical, add a runtime GameAction regression covering the replacement/deferred-life round trip that proves it still pays the exact original {G}{1} obligation and that the outer unless payment cannot complete early. Do not merely weaken or remove the structural assertion.
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer review complete. The parked UnlessPayment resume now preserves the authoritative waiting-state cost; the registered GameAction regression covers auto-tap pause, serialization/restore, replacement choice, resumption, exact {G}{1} payment, and no early outer completion. No further findings.
What changed
{1}, {T}ability.Why
Yurlok's activated ability already parsed as a mana ability, but the mana-ability fast path ignored its all-player scope. Its static ability also lacked a parser/runtime representation and required rules-correct coordination between mana emptying, life-loss replacements, and phase advancement.
Impact
Yurlok now parses and functions in multiplayer games. The shared life-loss and mana-expiry paths also handle replacement choices, prevention, interactive substitutions, retained/transformed mana, and legacy serialized phase-transition state consistently.
Validation
cargo fmt --allSummary by CodeRabbit