refactor(engine): migrate the Delve fuel cost mover onto zone_pipeline#6031
Conversation
Route the Delve graveyard-fuel exile (engine.rs delve payment arm) through
zone_pipeline::move_object with a typed PendingCostMoveResume::DelveManaPayment
continuation, removing the last raw cost-side zone mover.
- Resume re-installs the exact Delve WaitingFor::ManaPayment state (generic-only
marker included) rather than routing through the generic cast resume, which
would wrongly finish the pending cast (CR 601.2h + 602.2b).
- The exile link uses ExileLinkSpec { duration: None, tracking: TrackBySource }
so Murktide Regent-style 'cards exiled with this source' counts survive
(issue #1322); the delivery tail links only fuel actually delivered to Exile —
redirected fuel is not linked but still pays its generic component
(CR 702.66a + CR 614.1 + CR 616.1).
- Watched-red witnesses: neutering the resume dispatch reds both delve
redirect witnesses on resume loss; dropping TrackBySource reds the Murktide
link witness (left: [] right: [ObjectId(2)]).
- zone-authority baseline regenerated: exactly the engine.rs apply_action
mover row removed (Gate B: 59 hits / 40 rows).
There was a problem hiding this comment.
Code Review
This pull request implements the resume logic for Delve mana payments after graveyard-to-exile cost moves, ensuring that replacement effects (such as redirects or preventions) are correctly handled and that only cards actually reaching exile are linked to the spell. The feedback recommends improving Rust idiomaticity by replacing duplicate if guards with a single match block when resuming pending continuations, and removing a redundant manual assignment to state.waiting_for in the replacement choice handler.
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.
| if matches!(state.waiting_for, WaitingFor::Priority { .. }) | ||
| && matches!( | ||
| state.pending_cost_move_resume, | ||
| Some(crate::types::game_state::PendingCostMoveResume::ManaAbilityPayment { .. }) | ||
| Some(PendingCostMoveResume::DelveManaPayment { .. }) | ||
| ) | ||
| { | ||
| state.waiting_for = resume_delve_mana_payment(state); | ||
| } | ||
| if matches!(state.waiting_for, WaitingFor::Priority { .. }) | ||
| && matches!( | ||
| state.pending_cost_move_resume, | ||
| Some(PendingCostMoveResume::ManaAbilityPayment { .. }) | ||
| ) | ||
| { | ||
| state.waiting_for = mana_abilities::resume_mana_ability_cost_move(state, events)?; |
There was a problem hiding this comment.
[MEDIUM] Use a match or else if instead of multiple independent if statements with duplicate guards.
Why it matters: It avoids redundant checks on state.waiting_for and makes the mutually exclusive nature of the resume paths explicit and idiomatic.
Suggested fix:
if matches!(state.waiting_for, WaitingFor::Priority { .. }) {
match &state.pending_cost_move_resume {
Some(PendingCostMoveResume::DelveManaPayment { .. }) => {
state.waiting_for = resume_delve_mana_payment(state);
}
Some(PendingCostMoveResume::ManaAbilityPayment { .. }) => {
state.waiting_for = mana_abilities::resume_mana_ability_cost_move(state, events)?;
}
_ => {}
}
} if matches!(state.waiting_for, WaitingFor::Priority { .. }) {
match &state.pending_cost_move_resume {
Some(PendingCostMoveResume::DelveManaPayment { .. }) => {
state.waiting_for = resume_delve_mana_payment(state);
}
Some(PendingCostMoveResume::ManaAbilityPayment { .. }) => {
state.waiting_for = mana_abilities::resume_mana_ability_cost_move(state, events)?;
}
_ => {}
}
}| if matches!( | ||
| state.pending_cost_move_resume, | ||
| Some(PendingCostMoveResume::DelveManaPayment { .. }) | ||
| ) { | ||
| let waiting_for = super::engine::resume_delve_mana_payment(state); | ||
| state.waiting_for = waiting_for.clone(); | ||
| return Ok(waiting_for); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid redundant manual assignment to state.waiting_for before returning.
Why it matters: The caller of handle_replacement_choice already assigns the returned WaitingFor to state.waiting_for. Manually assigning it here is redundant and inconsistent with other resume blocks (such as the Foretell block right above it).
Suggested fix:
if matches!(
state.pending_cost_move_resume,
Some(PendingCostMoveResume::DelveManaPayment { .. })
) {
return Ok(super::engine::resume_delve_mana_payment(state));
} if matches!(
state.pending_cost_move_resume,
Some(PendingCostMoveResume::DelveManaPayment { .. })
) {
return Ok(super::engine::resume_delve_mana_payment(state));
}
Parse changes introduced by this PRBaseline pending for |
Route the Delve graveyard-fuel exile (engine.rs delve payment arm) through
zone_pipeline::move_object with a typed PendingCostMoveResume::DelveManaPayment
continuation, removing the last raw cost-side zone mover.
marker included) rather than routing through the generic cast resume, which
would wrongly finish the pending cast (CR 601.2h + 602.2b).
so Murktide Regent-style 'cards exiled with this source' counts survive
(issue Delve (Murktide Regent) — Murktide Regent can't be delved. #1322); the delivery tail links only fuel actually delivered to Exile —
redirected fuel is not linked but still pays its generic component
(CR 702.66a + CR 614.1 + CR 616.1).
redirect witnesses on resume loss; dropping TrackBySource reds the Murktide
link witness (left: [] right: [ObjectId(2)]).
mover row removed (Gate B: 59 hits / 40 rows).