Fix Tinker artifact mana cost - #5574
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements support for deferring non-mana spell additional-cost sacrifices until the final payment commit (addressing Issue #5252), allowing permanents to be tapped for mana before being sacrificed. It introduces the DeferredSacrificeSelection structure, adds validation and commit-time payment logic, and prevents reserved sacrifice permanents from being double-spent on other costs. Feedback on these changes highlights a potential leak of the TERMINAL_CAST_CANCELLATION_ERROR sentinel string to callers of finalize_cast_with_phyrexian_choices. Additionally, several new functions are missing mandatory CR annotations, violating rule R6 of the style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| finalize_cast_with_phyrexian_choices_inner( | ||
| state, | ||
| player, | ||
| object_id, | ||
| card_id, | ||
| ability, | ||
| cost, | ||
| casting_variant, | ||
| cast_timing_permission, | ||
| origin_zone, | ||
| phyrexian_choices, | ||
| None, | ||
| None, | ||
| events, | ||
| ) |
There was a problem hiding this comment.
[HIGH] Potential leak of the TERMINAL_CAST_CANCELLATION_ERROR sentinel string to other callers of finalize_cast_with_phyrexian_choices.
Why it matters: If a normal cast is cancelled due to target-dependent flash conditions failing, the raw sentinel string __terminal_cast_cancellation__ could leak to the user-facing error message because other callers (like pay_and_push) do not map this sentinel.
Suggested fix: Map the sentinel error to a clean user-facing error message inside finalize_cast_with_phyrexian_choices before returning.
finalize_cast_with_phyrexian_choices_inner(
state,
player,
object_id,
card_id,
ability,
cost,
casting_variant,
cast_timing_permission,
origin_zone,
phyrexian_choices,
None,
None,
events,
).map_err(|err| {
if matches!(
&err,
EngineError::ActionNotAllowed(message)
if message == TERMINAL_CAST_CANCELLATION_ERROR
) {
EngineError::ActionNotAllowed(
"Chosen targets do not satisfy the casting condition".to_string(),
)
} else {
err
}
})There was a problem hiding this comment.
Addressed in 1ff3999. Mapped TERMINAL_CAST_CANCELLATION_ERROR inside finalize_cast_with_phyrexian_choices so callers receive a clean casting-condition error.
| pub(super) fn pay_mana_cost_from_pool_with_choices( | ||
| state: &mut GameState, | ||
| player: PlayerId, | ||
| source_id: ObjectId, | ||
| cost: &crate::types::mana::ManaCost, | ||
| phyrexian_choices: Option<&[crate::types::game_state::ShardChoice]>, | ||
| events: &mut Vec<GameEvent>, | ||
| ) -> Result<u32, EngineError> { |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotations in pay_mana_cost_from_pool_with_choices.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add appropriate CR annotations (such as CR 601.2h for cost payment) to the function.
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in 1ff3999. Added the verified CR 601.2h doc annotation to pay_mana_cost_from_pool_with_choices.
| fn validate_deferred_spell_sacrifices_at_commit( | ||
| state: &GameState, | ||
| player: PlayerId, | ||
| pending: &PendingCast, | ||
| ) -> Result<(), EngineError> { |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotations in validate_deferred_spell_sacrifices_at_commit.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add appropriate CR annotations (such as CR 601.2h for cost payment validation) to the function.
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in 1ff3999. Added the verified CR 601.2h annotation to deferred sacrifice commit validation.
| if deferred_spell_sacrifice_reserved(state, *id) { | ||
| return Err(EngineError::InvalidAction( | ||
| "Selected card is already committed to a spell sacrifice cost".to_string(), | ||
| )); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotations for double-spending checks on reserved spell sacrifice permanents.
Why it matters: Every rules-touching line of engine code must carry a verified CR <number>: <description> comment to ensure strict fidelity to the Magic Comprehensive Rules.
Suggested fix: Add a CR annotation citing CR 118.10 (each payment of a cost applies to only one spell, ability, or effect) to the check.
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in 1ff3999. Added verified CR 118.10 annotations to the reserved-permanent double-spend rejection points.
matthewevans
left a comment
There was a problem hiding this comment.
Architecture, rules-correctness, and test coverage all pass. The sole blocker is mechanical: the branch is DIRTY and must be rebased onto main before this can land.
🔴 Blocker
Merge conflict — mergeStateStatus: DIRTY. This is a real textual conflict, so gh pr update-branch will 422; it needs an author rebase onto current main.
Two consequences worth being explicit about:
- The current green CI is on a stale base. It proves nothing about the post-rebase tree. This PR rewrites shared casting/mana state-machine seams (
casting.rs,casting_costs.rs,mana_abilities.rs), which are high-traffic files — several PRs have landed on them since this branch forked. Re-verify green after the rebase. - Watch for a semantic stale-base break, not just a textual one: a clean-merging rebase can still fail to compile if a signature this PR calls has since changed. Confirm
finalize_mana_payment/finish_pending_cost_or_cast/auto_tap_mana_sources_with_contextstill take the shapes used here.
🟡 Non-blocking
Two full GameState clones on the deferred-sacrifice cast path. sacrifice_selection_needs_replacement_choice (casting_costs.rs) does a state.clone() to simulate, and auto_activate_spell_mana_abilities_before_deferred_sacrifice does another (let mut simulated = state.clone()). Both are correct — the simulate-then-commit pattern is exactly right for rollback safety — but the AI beam search evaluates cast lines, so it will hit this whenever it considers a sacrifice-additional-cost spell. Blast radius is bounded (gated on a non-empty sacrifice selection), so this does not block. Worth a follow-up if Tinker-shaped casts show up in a search profile.
DeferredSacrificeSelection stores a raw ObjectId. Per CR 400.7 a permanent that leaves and re-enters is a new object. This is safe as written — validate_deferred_spell_sacrifices_at_commit re-checks zone/controller and re-matches the filter, so a blinked selection either fails the objects.get lookup or fails re-validation, and the cast errors cleanly rather than sacrificing the wrong object. Noting it only so the invariant is deliberate rather than incidental.
✅ Clean
- Rules-correct, and the CR chain is exactly the right one. CR 601.2g ("Mana abilities must be activated before costs are paid") + CR 601.2h ("The player pays the total cost") is precisely what makes tap-then-sacrifice legal, with the sacrifice being part of the total cost locked in at CR 601.2f. All eight cited rules grep-verified verbatim against
docs/MagicCompRules.txt; every one is on point. Tinker's Oracle text confirmed on Scryfall: "As an additional cost to cast this spell, sacrifice an artifact." - Right seam, and the deferral is correctly scoped.
can_defer_spell_sacrifice_until_mana_paymentonly defers when there is actually mana to pay (cost_has_x || cost.mana_value() > 0) — no mana window needed means no deferral. It also declines to defer when the sacrifice would need a replacement choice, which keeps a genuinely nasty interaction out of the deferred path instead of half-handling it. - Rollback discipline is correct.
auto_activate_spell_mana_abilities_before_deferred_sacrificeclones intosimulated, auto-taps, resolves tap-mana triggers inline (CR 605.4a), validates the deferred selections, and only then commits*state = simulated. A failed validation propagatesErrwithout ever having mutated real state. This is the right pattern for a destructivepending_castpath. - The reservation closes the double-spend hole at every seam. A reserved artifact cannot be consumed by a mana ability's own cost — guarded in
handle_sacrifice_for_mana_ability,handle_exile_for_mana_ability, thesacrifice_cost_choicefilter,exile_cost_choiceeligibility, the exile-choice loop, and bothSacrifice(SelfRef, count 1)arms viacost_sacrifices_reserved_source. The Lotus-Petal case (only artifact is itself a sac-for-mana source) fails closed rather than producing an unpayable cost at commit. - Discriminating runtime tests, properly registered (
mod issue_5252_additional_sacrifice_after_mana_abilities;attests/integration/main.rs:438— not inert). The 14 tests map 1:1 onto the failure modes this design has to survive:deferred_sacrifice_artifact_cannot_be_consumed_by_mana_ability_cost,failed_manual_payment_does_not_consume_deferred_sacrifice,deferred_sacrifice_revalidates_cost_filter_at_commit,finalize_rejection_happens_before_deferred_sacrifice_costs_are_paid,composite_mana_ability_cost_cannot_partially_pay_with_deferred_sacrifice_artifact, anddeferred_sacrifice_artifact_granting_spend_permission_is_paid_before_it_leaves. That last one is a particularly good catch.
Recommendation
Rebase onto main and re-verify green. The design, the rules reasoning, and the test coverage are all approved as-is — I am not asking for any behavioral change. Once the conflict is resolved and CI settles green on the new head, this is a fast approve + merge as bug.
7226f33 to
8530076
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Content still clean — but the branch went DIRTY again, and this rebase has a semantic hazard, not just a textual one.
✅ Clean (new commit 1ff3999e)
The follow-up commit is annotation-only (+20/−0) plus one error-message remap, so the architecture review from the previous pass carries unchanged. Both new CR citations grep-verify verbatim and are precisely on point:
- CR 118.10 (
mana_abilities.rs:1008,:1057) — "Each payment of a cost applies to only one spell, ability, or effect. For example, a player can't sacrifice just one creature to activate the activated abilities of two permanents that each require sacrificing a creature as a cost." This is exactly the rule that justifiesdeferred_spell_sacrifice_reservedgating both the exile-for-mana and sacrifice-for-mana cost paths. Well chosen. - CR 601.2h (
casting.rs:12897,casting_costs.rs:1723) — "...Partial payments are not allowed. Unpayable costs can't be paid." Correct for both paying the locked cost from the pool without reopening a mana window, and for re-validating the deferred selection at commit.
🔴 Blocker: mergeStateStatus: DIRTY — and #5556 is why
#5556 (re-derive target-dependent cost surcharge on the X+distribute casting route) merged at 12:25 today, into the same three files this PR touches:
| file | #5556 | this PR |
|---|---|---|
crates/engine/src/game/casting_costs.rs |
+28/−0 | +657/−146 |
crates/engine/src/types/game_state.rs |
+10/−1 | +14/−0 |
crates/engine/tests/integration/main.rs |
+1/−0 | +1/−0 |
Please do not resolve this as a pure textual conflict. Both PRs change when a spell's cost is finalized, at the same seam:
- #5556 re-derives the total cost after targets are chosen, routing through
finish_pending_cast_cost_or_pay(the post-target cost authority) instead of callingfinalize_castwith a cost locked in atChooseXValuetime. It also added a clone-and-restore-on-Errrollback, becausepending_castis destructively taken at that resume point. - this PR defers the additional sacrifice cost until after mana abilities are activated (CR 601.2g → 601.2h).
These have to compose: a spell that is both {X}-with-distribute and has an additional sacrifice cost must re-derive its surcharge after targets and still defer the sacrifice past the mana window. A conflict resolution that keeps one side's version of the finalize path will silently drop the other's behavior — and CI won't necessarily tell you, since each PR's tests only exercise its own path.
Recommendation: rebase onto current main, reconcile the two cost-finalization paths deliberately (not by picking a side), and confirm both fireball_x_cost_surcharge_timing and issue_5252_additional_sacrifice_after_mana_abilities are green on the rebased head. Approve as bug once that lands — the design, the 7-seam reservation, and the 14 registered runtime tests are already reviewed and sound.
(The two 🟡 notes from the prior review — the two GameState clones on the deferred path, and the raw ObjectId in DeferredSacrificeSelection — remain non-blocking.)
…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>
1ff3999 to
556de94
Compare
|
Hi @mike-theDude — heads up that we rebased and reconciled this branch for you; no action needed on your side, and nothing here is a request for changes. This went stale entirely because of churn we merged after you branched. You branched at What the reconciliation preserved from each side. These two changes are orthogonal and both survive intact:
The only place they interact is the pool snapshot the distribute branch uses to infer X: on your deferred-sacrifice route the pool is already spent by the time that branch runs, so the snapshot is hoisted above the payment. I added a comment-only commit documenting that ordering constraint and the CR rationale. Worth being straight about it, though: I checked all 34,632 cards in MTGJSON's AtomicCards, and zero have Verification. I didn't want to accept a green I hadn't watched go red, so I checked both directions:
Either one-sided resolution silently deletes a behavior, which is exactly why this needed a real merge rather than an Your commits keep your authorship; the reconciliation is a separate commit from us. The branch is now Thanks for this one — the deferred-sacrifice model is the correct read of CR 601.2g/601.2h, and the test file you wrote is genuinely good: it's what let me prove the reconciliation didn't quietly drop either side. |
matthewevans
left a comment
There was a problem hiding this comment.
Approving. As with #5571, the staleness here was ours — churn we merged (#5556's target-dependent cost re-derivation, plus the later cost-surcharge work) invalidated your branch after you opened it, so we rebased and reconciled it rather than asking you to chase it. My earlier CHANGES_REQUESTED is superseded by this review.
Verified before approving:
-
CI is real and green — 12 checks, 11 pass, 0 fail,
CLEAN/MERGEABLE. That includesRust lint (fmt, clippy, parser gate), which was the one gate we could not run locally. -
Both behaviors survive the reconciliation, and this was proven, not assumed. #5574 (defer the sacrifice until mana payment) and #5556 (re-derive target-dependent costs) both touch cost finalization, so an
--ours/--theirsresolution would have silently deleted one — and by construction neither PR's own tests would have caught it. The resolution was checked both ways:resolution #5574's tests #5556's tests take main's side 12 fail 2 pass take the PR's side 13 pass 2 fail the reconciliation 13 pass 2 pass Each side's tests fail precisely when the other side wins, which is what makes the merged result trustworthy rather than merely plausible.
-
Structurally corroborated: the PR doesn't touch
engine.rsorengine_resolution_choices.rs— #5556's seam — andfireball_x_cost_surcharge_timing.rsis untouched and green on your head. The two changes compose at different layers rather than colliding. -
The
additional_cost_decidedearly-return gap is latent, not a live bug. It's a Casualty-era guard (CR 601.2b) that predates this PR and that this PR doesn't touch. Triggering it needs a card with{X}in its mana cost and divided/distributed damage and an additional cost. Measured against the full 34,632-card Atomic pool:{X}∧ divided → 14 cards, none with an additional cost; divided ∧ additional cost → 6 cards (Fire Covenant, Infernal Harvest, Nahiri's Sacrifice, Crashing Wave, Fight with Fire, Pollen Remedy), none with{X}in its mana cost. The triple intersection is zero cards — and the component patterns match 716 and 134 cards independently, so that zero is a real result, not a broken query. -
Parse-diff: no card-parse changes, correct for an engine/casting-cost change.
Thanks for your patience — this sat conflicted through no fault of yours.
Summary
Fixes deferred additional sacrifice costs for Tinker-shaped spell casts so artifact mana sources can be tapped during the mana-ability window and sacrificed only at final cost-payment commit.
Closes #5252.
Files changed
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/types/game_state.rsclient/src/adapter/types.tscrates/engine/tests/integration/issue_5252_additional_sacrifice_after_mana_abilities.rscrates/engine/tests/integration/main.rscrates/engine/src/game/engine_tests.rscrates/engine/src/game/visibility.rsCR references
CR 601.2fCR 601.2gCR 601.2hCR 605.4aCR 603.10aCR 701.21aCR 118.8CR 118.3aTrack
Developer
LLM
Model: codex-5
Thinking: high
Tier: Standard
Anchored on
crates/engine/src/game/casting_costs.rs:1986— existing immediate sacrifice-cost path for non-deferred spell costs.crates/engine/src/game/casting.rs:5761— existing post-additional-cost pending-cast mana-cost recomputation before payment.crates/engine/src/game/mana_abilities.rs:1075— pending-cast reservation check used to prevent double-spending selected sacrifice permanents.Gate A
./scripts/check-parser-combinators.sh— clean (exit 0; no output).Verification
cargo fmt --all— clean./scripts/check-parser-combinators.sh— cleanCARGO_BUILD_JOBS=1 cargo test -p engine --test integration issue_5252_additional_sacrifice_after_mana_abilities -- --nocapture— 13 passedCARGO_BUILD_JOBS=1 cargo test -p engine --test integration morbid_curiosity_draws_equal_to_sacrificed_permanents_mana_value -- --nocapture— 1 passedCARGO_BUILD_JOBS=1 cargo test -p engine accepting_spirit_offering_sacrifices_permanent_and_reduces_cost -- --nocapture— 1 passedCARGO_BUILD_JOBS=1 cargo clippy -p engine --all-targets -- -D warnings— cleanPendingCasttype-sync suggestion addressedScope Expansion
None.
Validation Failures
None.
CI Failures
None.