fix(engine): re-derive target-dependent cost surcharge on the X+distribute casting route#5556
Merged
matthewevans merged 2 commits intoJul 11, 2026
Conversation
…tion
Fireball ({X}{R}: "This spell costs {1} more to cast for each target
beyond the first. Fireball deals X damage divided evenly, rounded
down, among any number of targets.") never actually paid the surcharge
at runtime, even with strive_cost correctly populated (separate parser
fix, PR phase-rs#5545) — its specific casting route bypassed cost re-derivation
entirely.
Root cause: {X} is announced (CR 601.2b) and the cost locked in via
apply_post_x_cost_modifiers BEFORE targets exist, so the per-target
surcharge loop is a no-op at that point. Targets are chosen after (CR
601.2c), then the "divided evenly among any number of targets" shape
pauses at WaitingFor::DistributeAmong (CR 601.2d) — and the
GameAction::DistributeAmong handler resumed by calling
casting_costs::finalize_cast directly with the stale, pre-target cost,
skipping check_additional_cost_or_pay_with_distribute entirely (the
only place apply_target_dependent_cost_modifiers, the CR 601.2f
surcharge authority, actually runs).
Fix: route through casting_costs::finish_pending_cast_cost_or_pay (the
same authority every other casting route already uses to go from
"targets known" to "cost determined, locked, paid") instead of calling
finalize_cast directly. This is a general seam fix, not Fireball-
specific — Fireball is currently the only real card combining an {X}
cost, a strive-shaped per-target surcharge, and a distribute-among-
targets effect (verified via Scryfall), so any future card with this
shape is covered automatically.
Requires a rollback wrapper: state.pending_cast is taken (set to None)
before the call, and finish_pending_cast_cost_or_pay's downstream chain
has no restore-on-error of its own — so if the recomputed (correctly
higher) cost turns out unpayable, a bare Err would leave
state.waiting_for stale as DistributeAmong while pending_cast is gone,
so a resubmitted DistributeAmong action would silently fall through to
an unrelated resolution-time continuation branch instead of being
cleanly rejected. Mirrors finalize_mana_payment's existing
pending_for_restore clone-and-restore-on-Err pattern (CR 601.2h:
"Unpayable costs can't be paid").
Applied the identical fix to a second call site with the same
anti-pattern, found during review: engine_resolution_choices.rs's
NamedChoice handler's spell-cast resume branch (activation_ability_index
== None) also called finalize_cast directly with a stale cost. No
currently-shipped card was confirmed to reach it with a target-
dependent cost modifier, but the fix is mechanical and the pattern is
identical, so it's fixed defensively rather than left as a known latent
gap.
A second near-miss (finalize_mana_payment's two pending.distribute-
gated blocks) was investigated and found provably unreachable for
every real card today (git-verified: the phase-rs#2856 gate predates and is an
ancestor of the commits shaping those blocks) — documented with a
cross-reference comment, no functional change needed there.
Also corrects DistributionUnit::EvenSplitDamage's doc comment, which
incorrectly claimed it always bypasses WaitingFor::DistributeAmong —
true only for the non-deferred-target-selection flow, not for a
deferred-selection card like Fireball.
Went through 3 rounds of independent plan review (closed: a fabricated
test-file dependency later clarified as belonging to a separate
not-yet-merged PR, a genuinely-investigated near-miss call site, a
corrected card-class table, and a traced-and-fixed state-corruption
risk in the naive version of this fix) plus a review-impl pass.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
rykerwilliams
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 11, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: architecturally correct seam fix — approving on settled green. This routes the {X}+distribute casting resume through the single post-target cost authority instead of paying a stale pre-target cost, and it does so as a general fix, not a Fireball special-case.
✅ Clean
- Right seam (single cost authority).
engine.rs:4836— theDistributeAmongresume previously calledcasting_costs::finalize_castdirectly withpending.cost, the cost locked in atChooseXValuetime before targets existed, so the CR 601.2f per-target surcharge (apply_target_dependent_cost_modifiers) never ran. The fix routes throughfinish_pending_cast_cost_or_pay(casting_costs.rs:3159) — the same authority every post-target-selection path uses. That is the correct consolidation, not a bolt-on. - Softlock-preventing rollback, verified against the cited precedent. Because
state.pending_castis destructively.take()n here (unlikehandle_select_targets, whose pending lives inside theTargetSelectionvariant), a bareErrfrom a recomputed-unpayable cost would strandwaiting_for: DistributeAmongwithpending_cast: None— a resubmittedDistributeAmongwould fall through to the resolution-time continuation branch instead of being cleanly rejected. The clone-and-restore-on-Errmirrorsfinalize_mana_payment'spending_for_restorepattern, which I confirmed lives atcasting_costs.rs:8627/8784and8820/8978. CR 601.2h verbatim: "Unpayable costs can't be paid." - Second call site fixed consistently.
engine_resolution_choices.rs:3970(theNamedChoicespell-cast resume,activation_ability_index == None) carries the identical anti-pattern and gets the identical fix + rollback. Mechanical and correct to fix rather than leave latent. - CR annotations all grep-verified against
docs/MagicCompRules.txt: 207.2c (ability words — "strive" is in the list), 601.2b (X announced), 601.2c (targets), 601.2d (division), 601.2f (total cost locked in), 601.2h (unpayable). Accurate. - General test primitive + discriminating registered test.
scenario.rswith_strive_costis a reusable builder (pinsstrive_costdirectly), not a Fireball hack.fireball_x_cost_surcharge_timing.rsdrives the realChooseX → targets → DistributeAmongsequence and asserts the mana pool reflects the per-target surcharge — fails on the pre-fix single-target cost — and isolates the #5545 parser misparse by using only the verbatim distribution clause.modregistered inmain.rs.
🟡 Non-blocking
- The
casting_costs.rs/game_state.rschanges are documentation-only (cross-reference comment +DistributionUnit::EvenSplitDamagedoc correction) — no functional risk. The "provably-unreachable"finalize_mana_paymentnear-miss is left documented rather than patched, which is the right call for a genuinely unreachable path.
Recommendation: approve + merge as bug once Rust lint + both shards + card data settle green (currently pending on head c068e099). This is a runtime defect fix (Fireball's cost never scaled with targets), so bug, not enhancement.
Parse changes introduced by this PR · 97 card(s), 50 signature(s) (baseline: main
|
matthewevans
requested changes
Jul 11, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Request changes — the engine fix is sound (both test shards pass), but two clippy -D warnings errors in the new test file abort the lint job and cascade the coverage-gate red.
🔴 Blocker — clippy::never_loop at crates/engine/tests/integration/fireball_x_cost_surcharge_timing.rs:122
Every arm of the match inside for _ in 0..40 diverges (return on ChooseXValue, panic! on TargetSelection, _ => return), so the loop body can never reach a second iteration — clippy reports "this loop never actually loops" and the 0..40 bound is dead. Decide the intent:
- If a single check suffices (the state is
ChooseXValueon entry), drop the loop and keep just thematch. - If the intent is to poll until
ChooseXValueappears, the_arm must notreturn— it has to advance the pending state andcontinue, otherwise it silently returns on the first non-matching state.
🔴 Blocker — clippy::manual_is_multiple_of at fireball_x_cost_surcharge_timing.rs:192
assert!(n > 0 && total % n == 0, ...) → replace total % n == 0 with total.is_multiple_of(n) (clippy's suggested fix).
✅ Clean (unchanged from prior review)
The runtime fix itself is right-seam and CR-correct — both Rust test shards (1/2, 2/2) pass on this head. Routing through finish_pending_cast_cost_or_pay with clone-and-restore-on-Err rollback, both call sites fixed, CRs 207.2c / 601.2b–h grep-verified. No architecture changes needed.
Recommendation: fix the two lint errors on the test file (no engine change required); approve and merge as bug on settled green.
…iple_of) drive_choose_x's for-loop never actually looped (every match arm diverges), so clippy correctly flagged it as dead — replaced with a single match. Also replaced total % n == 0 with total.is_multiple_of(n) per clippy's suggestion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K
Contributor
Author
|
Fixed in c8bfd6f.
Re-verified: `cargo clippy -p engine --test integration -- -D warnings` clean, and both tests (`fireball_surcharge_scales_with_target_count_on_distribute_route`, `fireball_unpayable_recomputed_cost_rolls_back_cleanly`) still pass. |
matthewevans
approved these changes
Jul 11, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approving — the two clippy blockers are fixed, and the never_loop fix makes the test strictly more discriminating than before.
✅ Clean
The lint fix is a genuine test hardening, not a lint-silencer. fireball_x_cost_surcharge_timing.rs:122 — the old for _ in 0..40 { match … } had a _ => return arm that silently exited on any unexpected WaitingFor state, so the test could pass without ever driving ChooseX. The replacement makes the catch-all fail closed:
other => panic!("expected ChooseXValue immediately after CastSpell, got {other:?}"),That converts a silent pass into a hard failure, which is exactly the right direction for a test guarding a cost-timing invariant. total % n == 0 → total.is_multiple_of(n) at :185 is semantically identical.
Delta is test-file-only (ahead:1, behind:0, +13/−14, single file), so the engine review from the prior pass carries unchanged:
- Right seam — routes through
finish_pending_cast_cost_or_pay(casting_costs.rs:3159), the single post-target cost authority, rather than callingfinalize_castwith a cost locked in atChooseXValuetime. - Clone-and-restore-on-
Errrollback mirroringfinalize_mana_payment'spending_for_restore— load-bearing, becausepending_castis destructively taken here; without it an unpayable recomputed cost would strandwaiting_for: DistributeAmongwithpending_cast: None. - Both resume call sites fixed (
engine_resolution_choices.rs:3970NamedChoice path). - CR 601.2f (per-target surcharge is part of the total cost) and CR 601.2b–h grep-verified; general
with_strive_costtest builder rather than a Fireball special-case.
CI fully resolved green — 12 SUCCESS, 1 skipped, both Rust shards and the lint job that was previously red.
Approving and enqueuing as bug.
matthewevans
added a commit
to mike-theDude/phase
that referenced
this pull request
Jul 11, 2026
…target-dependent cost re-derivation PR phase-rs#5574 (defer spell sacrifice costs until mana payment) branched from dbac6ad, before phase-rs#5556 (re-derive target-dependent cost surcharge on the X+distribute casting route, f6432a2) landed. Both touch the cost-finalization seam in `finalize_mana_payment` / `finalize_mana_payment_with_phyrexian_choices`, so the branch went stale through no fault of the contributor. This rebase is maintainer-side reconciliation, not a request for changes. The two behaviors are orthogonal and both survive: - phase-rs#5556 routes the `DistributeAmong` resume through `finish_pending_cast_cost_or_pay` so the total cost is re-derived after targets are known (CR 601.2f). That function is byte-identical between main and this branch, so its authority is preserved as-is. - phase-rs#5574 defers a non-mana additional cost (Tinker's "sacrifice an artifact") past the mana window: mana abilities may be activated (CR 601.2g) before costs are paid (CR 601.2h), so the sacrifice is recorded at selection and paid at the payment commit. The one place the two interact is the pool snapshot the distribute branch uses to infer X. On the deferred-sacrifice route, `pay_spell_mana_before_deferred_sacrifice` has already spent the pool by the time that branch runs, so the snapshot is hoisted above it — reading the pool inside the branch would infer X = 0 for a spell that is both {X}+distribute and carries an additional sacrifice cost. The `unwrap_or_else` fallback keeps the original read for every other route, so the hoist is a no-op for every card that currently exists: no card in the 34,632-card MTGJSON AtomicCards pool has {X} in its mana cost *and* a cast-time divided/distributed effect *and* an additional cost. The hoist is therefore latent robustness, not a live fix, and no existing test can discriminate it. This commit documents that ordering constraint at both sites, alongside the CR 601.2g/601.2h rationale for where the pre-payment checks now sit. Comment-only; no functional change. Co-authored-by: mike-theDude <mbriningstool@gmail.com>
andriypolanski
pushed a commit
to andriypolanski/phase
that referenced
this pull request
Jul 11, 2026
* fix(engine): defer spell sacrifice costs until mana payment * fix(engine): address deferred sacrifice review feedback * chore(engine): reconcile Tinker sacrifice-deferral with phase-rs#5556 target-dependent cost re-derivation PR phase-rs#5574 (defer spell sacrifice costs until mana payment) branched from dbac6ad, before phase-rs#5556 (re-derive target-dependent cost surcharge on the X+distribute casting route, f6432a2) landed. Both touch the cost-finalization seam in `finalize_mana_payment` / `finalize_mana_payment_with_phyrexian_choices`, so the branch went stale through no fault of the contributor. This rebase is maintainer-side reconciliation, not a request for changes. The two behaviors are orthogonal and both survive: - phase-rs#5556 routes the `DistributeAmong` resume through `finish_pending_cast_cost_or_pay` so the total cost is re-derived after targets are known (CR 601.2f). That function is byte-identical between main and this branch, so its authority is preserved as-is. - phase-rs#5574 defers a non-mana additional cost (Tinker's "sacrifice an artifact") past the mana window: mana abilities may be activated (CR 601.2g) before costs are paid (CR 601.2h), so the sacrifice is recorded at selection and paid at the payment commit. The one place the two interact is the pool snapshot the distribute branch uses to infer X. On the deferred-sacrifice route, `pay_spell_mana_before_deferred_sacrifice` has already spent the pool by the time that branch runs, so the snapshot is hoisted above it — reading the pool inside the branch would infer X = 0 for a spell that is both {X}+distribute and carries an additional sacrifice cost. The `unwrap_or_else` fallback keeps the original read for every other route, so the hoist is a no-op for every card that currently exists: no card in the 34,632-card MTGJSON AtomicCards pool has {X} in its mana cost *and* a cast-time divided/distributed effect *and* an additional cost. The hoist is therefore latent robustness, not a live fix, and no existing test can discriminate it. This commit documents that ordering constraint at both sites, alongside the CR 601.2g/601.2h rationale for where the pre-payment checks now sit. Comment-only; no functional change. Co-authored-by: mike-theDude <mbriningstool@gmail.com> --------- Co-authored-by: mike-theDude <mbriningstool@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
rykerwilliams
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to #5545 (Fireball's parser misparse — that PR makes
strive_costcorrectly get populated). This PR fixes a separate, independent runtime bug: even withstrive_costpopulated, [[Fireball]]'s ({X}{R}: "This spell costs {1} more to cast for each target beyond the first.\nFireball deals X damage divided evenly, rounded down, among any number of targets.") actual in-game cost never scaled with target count.Root cause
Fireball's specific casting shape — an
{X}cost combined with "divided evenly among any number of targets" — triggers a sequence where:{X}is announced first (CR 601.2b) and the cost gets locked in viaapply_post_x_cost_modifiersbefore any targets exist, so the per-target surcharge loop (for _ in 1..target_count) is a no-op at that point (target_count == 0).WaitingFor::DistributeAmong(CR 601.2d) rather than proceeding straight to payment.GameAction::DistributeAmonghandler resumes by callingcasting_costs::finalize_castdirectly with the stale, pre-target cost — completely skippingcheck_additional_cost_or_pay_with_distribute, the only placeapply_target_dependent_cost_modifiers(the CR 601.2f surcharge authority) actually runs.Fix
Route through
casting_costs::finish_pending_cast_cost_or_pay— the same cost-determination authority every other casting route already uses to go from "targets known" to "cost determined, locked in, paid" — instead of callingfinalize_castdirectly. This is a general seam fix, not hardcoded to Fireball: Fireball is currently the only real card in Magic combining an{X}cost, a strive-shaped per-target surcharge, and a distribute-among-targets effect (verified via Scryfall across two rounds of review), so the fix automatically covers any future card with this shape.This requires a rollback wrapper.
state.pending_castis.take()n (set toNone) before the call, andfinish_pending_cast_cost_or_pay's downstream chain has no restore-on-error of its own. If the recomputed (correctly higher) cost turns out unpayable, a bareErrwould leavestate.waiting_forstale asDistributeAmongwhilepending_castis gone — a resubmittedDistributeAmongaction would then silently fall through to an unrelated resolution-time-continuation branch instead of being cleanly rejected. The fix mirrorsfinalize_mana_payment's existingpending_for_restoreclone-and-restore-on-Errpattern (CR 601.2h: "Unpayable costs can't be paid").Extended defensively to a second call site found during review:
engine_resolution_choices.rs'sNamedChoicehandler has the identical anti-pattern in its spell-cast resume branch (activation_ability_index == None) — also callsfinalize_castdirectly with a stale cost. No currently-shipped card was confirmed to reach it with a target-dependent cost modifier, but the fix is mechanical and the pattern is exactly the same, so it's fixed rather than left as a known latent gap.A second near-miss (
finalize_mana_payment's twopending.distribute-gated blocks) was investigated and found provably unreachable for every real card today — git-verified that the #2856 divide-softlock gate (ability_distribution_pool_needs_chosen_x) predates and is an ancestor of the commits shaping those blocks. Documented with a cross-reference comment; no functional change needed there.Also corrects
DistributionUnit::EvenSplitDamage's doc comment, which incorrectly claimed it always bypassesWaitingFor::DistributeAmong— true only for the non-deferred-target-selection flow, not for a deferred-selection card like Fireball.Test note
The new test pins the surcharge via a new
with_strive_costtest builder rather than parsing Fireball's full real Oracle text, because that text currently mis-parses onorigin/maininto an unrelated, separate bug (a spurious static cost modifier) — that's PR #5545's territory, not this branch's job, and is documented in the test module doc.Files changed
crates/engine/src/game/engine.rs— the primary fix (DistributeAmonghandler)crates/engine/src/game/engine_resolution_choices.rs— the identical fix applied to theNamedChoicehandler's spell-cast resume branchcrates/engine/src/game/casting_costs.rs— documentation-only cross-reference comments (no functional change)crates/engine/src/types/game_state.rs—DistributionUnit::EvenSplitDamagedoc correctioncrates/engine/src/game/scenario.rs—with_strive_costtest buildercrates/engine/tests/integration/fireball_x_cost_surcharge_timing.rs(new) — discriminating runtime testscrates/engine/tests/integration/main.rs— registers the new test moduleCR references
CR 207.2c, CR 601.2b, CR 601.2c, CR 601.2d, CR 601.2f, CR 601.2h
Verification
cargo fmt --allcleancargo clippy -p engine --lib -- -D warningsclean./scripts/check-parser-combinators.sh: no-op (no parser files touched)cargo test -p engine --lib: 16104 passed, 0 failedcargo test -p engine --test integration: 2665 passed, 0 failedNamedChoicecall site, since fixed)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K