fix(engine): auto-tap self-sacrifice mana abilities like Gold tokens#6230
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request resolves issue #6157 by enabling automatic mana payment to utilize Gold tokens, which feature a self-sacrifice cost without a tap component. It introduces a helper to identify unambiguous self-sacrifice costs so they can be processed on the auto-tap path. The review feedback highlights a correctness bug where tapped or summoning-sick Gold tokens might be incorrectly excluded from auto-tap due to the order of checks in is_active_tap_mana_ability. Additionally, the feedback points out incorrect MTG Comprehensive Rules citations (referencing CR 701.21 instead of CR 701.16 for Sacrifice) in both casting_costs.rs and mana_sources.rs.
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 !has_tap_component(&ability.cost) && !has_unambiguous_self_sacrifice_component(&ability.cost) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
[HIGH] Correctness bug with tapped/summoning-sick Gold tokens. Evidence: crates/engine/src/game/mana_sources.rs:1782.
Why it matters: If is_active_tap_mana_ability performs obj.tapped or summoning sickness checks before verifying if the ability actually has a {T} component, then tapped or summoning-sick Gold tokens (especially when animated) will be incorrectly excluded from auto-tap, violating CR 302.6 and CR 701.16.
Suggested fix: Restructure the function to check has_tap_component and has_unambiguous_self_sacrifice_component first, and only apply the tapped and summoning sickness checks if the ability actually has a {T} component.
References
- Rule R6: Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| cost_has_component(cost, |c| matches!(c, AbilityCost::Untap)) | ||
| } | ||
|
|
||
| /// CR 605.3a + CR 701.21: True when the cost has a `Sacrifice` component that |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR annotation citation. Evidence: crates/engine/src/game/mana_sources.rs:273.
Why it matters: The MTG Comprehensive Rules section for Sacrifice is CR 701.16, whereas CR 701.21 refers to Meld, violating the requirement for verified CR annotations (Rule R6).
Suggested fix: Change CR 701.21 to CR 701.16.
| /// CR 605.3a + CR 701.21: True when the cost has a `Sacrifice` component that | |
| /// CR 605.3a + CR 701.16: True when the cost has a Sacrifice component that |
References
- Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
| .cost(AbilityCost::Tap), | ||
| ); | ||
| } | ||
| // CR 605.3a + CR 701.21: an ambiguous sacrifice cost (choosing among |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR annotation citation. Evidence: crates/engine/src/game/casting_costs.rs:13218.
Why it matters: The MTG Comprehensive Rules section for Sacrifice is CR 701.16, whereas CR 701.21 refers to Meld, violating the requirement for verified CR annotations (Rule R6).
Suggested fix: Change CR 701.21 to CR 701.16.
| // CR 605.3a + CR 701.21: an ambiguous sacrifice cost (choosing among | |
| // CR 605.3a + CR 701.16: an ambiguous sacrifice cost (choosing among |
References
- Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The new self-sacrifice predicate is never reached for a tapped source. auto_tap_mana_options still returns early on obj.tapped before scan_mana_abilities can call is_active_tap_mana_ability; that early gate is only valid for a {T} cost. A tapped Gold token can legally pay “Sacrifice this token: Add one mana of any color”, but the proposed auto-payment path offers no source and the spell remains unpayable. Please make the untapped/summoning-sickness prefilter conditional on the selected ability requiring {T} (or let the existing activation/payability predicate make that decision), and add a tapped-Gold regression test.
Parse changes introduced by this PR✓ No card-parse changes detected. |
Auto-tap source discovery required every scanned mana ability to have a
{T} cost component, so Gold's "Sacrifice this token: Add one mana of any
color." (no {T}) was invisible to CastPaymentMode::Auto even though the
sacrifice targets only the token itself and needs no player choice —
exactly as deterministic as a {T} cost. Treasure worked only because its
cost happens to also include {T}.
Extend the single-authority eligibility check to also accept an
unambiguous self-sacrifice cost (Sacrifice targeting SelfRef, count 1),
and exclude that same shape from the "needs manual payment" fallback path
so it isn't double-counted. Ambiguous sacrifice costs (Krark-Clan
Ironworks' "Sacrifice an artifact") still require a manual choice and are
unaffected.
Closes phase-rs#6157
Address PR review on phase-rs#6230: the self-sacrifice predicate was never reached for a tapped source. Both auto-tap source gates (`auto_tap_mana_options` and `collect_sorted_auto_tap_source_options`) returned early on `obj.tapped` / summoning-sickness before `scan_mana_abilities` could evaluate the ability, so a tapped Gold token ("Sacrifice this token: Add one mana of any color.", no `{T}`) offered no source and the spell stayed unpayable. Make the object-level tapped/summoning-sick prefilter conditional on the source requiring `{T}`: a source is only dropped up front when it lacks a *tapless* unambiguous self-sacrifice mana ability (CR 106.12 / CR 302.6 gate `{T}`/`{Q}` costs only). Sources carrying such an ability fall through to `scan_mana_abilities`, where a cheap per-ability cost/field guard on the auto-tap path still excludes any `{T}`/`{Q}` ability of a tapped or summoning-sick source — so no `{T}` source leaks through and no legality simulation (readiness call) is added, preserving the bulk-activation linearity invariant. Treasure (`{T}, Sacrifice this artifact`) is deliberately excluded from the bypass: once tapped it genuinely cannot activate. Add regression test `tapped_gold_token_still_auto_taps_for_self_sacrifice_mana` proving a tapped, summoning-sick Gold token is still auto-tapped via its self-sacrifice mana ability. Fails on the pre-fix path, passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
d846de2 to
249a857
Compare
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAutomatic mana payment now recognizes deterministic tapless self-sacrifice abilities, including Gold tokens, while preserving restrictions for tapped or summoning-sick tap abilities and choice-bearing composite costs. Integration tests cover Gold, Treasure, and LED-shaped sources. ChangesGold token automatic payment
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant CastingCosts
participant ManaSources
participant GoldToken
Player->>CastingCosts: cast spell with automatic payment
CastingCosts->>ManaSources: scan payable mana abilities
ManaSources->>GoldToken: select tapless self-sacrifice ability
GoldToken-->>CastingCosts: produce mana and sacrifice
CastingCosts-->>Player: resolve spell and draw a card
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Thanks for the precise catch, @matthewevans — you were exactly right that the new predicate was never reached for a tapped source. Fixed and rebased on latest main. What changed
Both now gate the tapped/summoning-sick/can't-tap exclusion behind a new helper Regression test Green locally: |
|
Current-head review completed. The tapped/summoning-sick Gold regression now reaches the per-ability gate, while No approval or enqueue yet: the current Rust and frontend required checks are still running, and CodeRabbit's current-head review is in progress. I will leave this held for those external gates rather than override them. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/mana_sources.rs (1)
273-316: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPredicate only checks presence of a self-sacrifice component, not exclusivity.
has_unambiguous_self_sacrifice_component/object_has_tapless_self_sacrifice_mana_abilitytreat a cost as "unambiguous"/deterministic whenever aSacrifice(SelfRef, count:1)component exists anywhere in the cost — viacost_has_component'scosts.iter().any(&pred)— without checking that every other component of aCompositecost is also choice-free (e.g.Tap). A hypothetical future mana ability shapedComposite{ Sacrifice(SelfRef,1), Discard{ any card } }(orSacrifice(SelfRef,1)alongside a second, ambiguousSacrificetarget) would be classified as deterministic by all three call sites (object_has_tapless_self_sacrifice_mana_ability,has_activatable_non_tap_mana_ability_for_payment,is_active_tap_mana_ability) and pushed onto the auto-tap fast path even though it still requires a player choice (which card to discard / which permanent to sacrifice).Today's two shipped shapes (Gold: bare
Sacrifice(SelfRef,1); Treasure:Composite{Tap, Sacrifice(SelfRef,1)}) are unaffected, but per the "build reusable building blocks for a class of cards" guideline this predicate should assert that the self-sacrifice component is the only non-Tapcomponent before treating the whole cost as choice-free.♻️ Proposed tightening
pub(crate) fn has_unambiguous_self_sacrifice_component(cost: &Option<AbilityCost>) -> bool { - cost_has_component(cost, |c| { - matches!( - c, - AbilityCost::Sacrifice(SacrificeCost { - target: TargetFilter::SelfRef, - requirement: SacrificeRequirement::Count { count: 1 }, - }) - ) - }) + match cost { + Some(AbilityCost::Composite { costs }) => { + costs.iter().any(|c| is_self_sacrifice(c)) + && costs + .iter() + .all(|c| matches!(c, AbilityCost::Tap) || is_self_sacrifice(c)) + } + Some(c) => is_self_sacrifice(c), + None => false, + } }As per path instructions, "Build reusable card-class building blocks rather than one-card special cases... new enum variants, parsers, effects, and filters must support a category of cards."
Also applies to: 1276-1282, 1820-1825
🤖 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_sources.rs` around lines 273 - 316, Update has_unambiguous_self_sacrifice_component to validate the entire AbilityCost structure, not merely detect a matching component: require exactly one Sacrifice(SelfRef, count: 1) and allow only choice-free Tap alongside it, rejecting any additional discard or sacrifice components. Preserve the existing Gold and Treasure cost shapes, so object_has_tapless_self_sacrifice_mana_ability and all callers classify only genuinely deterministic costs.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/engine/src/game/mana_sources.rs`:
- Around line 273-316: Update has_unambiguous_self_sacrifice_component to
validate the entire AbilityCost structure, not merely detect a matching
component: require exactly one Sacrifice(SelfRef, count: 1) and allow only
choice-free Tap alongside it, rejecting any additional discard or sacrifice
components. Preserve the existing Gold and Treasure cost shapes, so
object_has_tapless_self_sacrifice_mana_ability and all callers classify only
genuinely deterministic costs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 07198f57-8863-42c4-b55c-48807f401ecb
📒 Files selected for processing (4)
crates/engine/src/game/casting_costs.rscrates/engine/src/game/mana_sources.rscrates/engine/tests/integration/issue_6157_gold_token_auto_mana_payment.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking: the auto-tap eligibility predicate currently accepts a cost that still requires a player choice.
🔴 Blocker
crates/engine/src/game/mana_sources.rs:282-291 uses cost_has_component, whose Composite implementation is costs.iter().any(...). Consequently, a mana ability with Composite[Discard, Sacrifice(SelfRef, 1)] is classified as an “unambiguous” self-sacrifice merely because one component matches. That classification is consumed both by the manual-payment exclusion at mana_sources.rs:1276-1283 and by the auto-tap eligibility gate at mana_sources.rs:1820-1824, so the ability can be selected automatically even though it still needs a discard choice.
This is a real existing cost shape, not a hypothetical: the Lion's Eye Diamond activation test builds exactly Composite[Discard { selection: Chosen }, Sacrifice(SelfRef, 1)] at crates/engine/src/game/mana_abilities.rs:6209-6223; the activation pipeline then exposes WaitingFor::PayCost { kind: Discard } at mana_abilities.rs:1450-1468 and asserts that prompt at 6240-6256. Auto-tap must not bypass that prompt.
The established authority already models the needed invariant: mana_abilities.rs:3027-3041 is a deny-by-default full-tree whitelist, allowing only Tap, Sacrifice(SelfRef, 1), and composites whose every component is choice-free. Please make the auto-tap predicate share/extract that full-cost authority (or apply the same full-tree invariant), then add a regression proving the LED-shaped composite remains on the manual-payment path while bare Gold and {T} + self-sacrifice Treasure remain eligible.
✅ Clean
The current head fixes the prior tapped/summoning-sick Gold reachability finding; that issue is independent of this composite-cost blocker.
Recommendation: request changes. Tighten eligibility to the full, choice-free cost tree and cover the existing LED shape before approval.
… eligibility (LED composite)
has_unambiguous_self_sacrifice_component matched a self-sacrifice via
cost_has_component (Composite = any), so Lion's Eye Diamond's
Composite[Discard{Chosen}, Sacrifice(SelfRef,1)] was misclassified as
unambiguous self-sacrifice. That let the auto-tap eligibility gate
(mana_sources.rs:1820) and the manual-payment exclusion
(mana_sources.rs:1281) auto-select LED, bypassing its Discard prompt.
Reuse the existing deny-by-default full-tree authority
mana_abilities::cost_component_choice_free (Tap + single-token
self-sacrifice + Composites built solely from those) by making it
pub(crate) and sharing it: the predicate now requires the whole cost
tree be choice-free AND actually contain a self-sacrifice component.
Gold (bare self-sac) and Treasure ({T}+self-sac) stay auto-tap eligible;
LED-shaped composites are rejected and keep their discard prompt.
Add regression led_shaped_discard_sacrifice_stays_off_auto_tap.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ent path
Drive CastPaymentMode::Auto with a LED-shaped mana source
(Composite[Discard{Chosen}, Sacrifice(SelfRef,1)]) alongside a Gold
token: auto-tap must skip the choice-bearing LED and pay the generic-1
spell with Gold, leaving LED on the battlefield (manual-payment path),
while a LED-only board is not auto-payable. Complements the predicate
unit test with a behavioral regression; fails on the pre-fix any-match
predicate (spell becomes unpayable) and passes with the whole-tree
choice-free check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Good catch, @matthewevans — you're right that What changed
Result: LED ( Regression (end-to-end) Green locally: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/issue_6157_gold_token_auto_mana_payment.rs`:
- Around line 255-268: The sanity-floor assertion around
runner.cast(spell).try_resolve() must verify that resolution pauses in the
expected ManaPayment state rather than asserting an error. Replace the is_err()
check with final_waiting_for() or an equivalent positive reach-guard, preserving
the scenario’s intended outcome that the spell remains unresolved because mana
payment requires handling.
🪄 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: 530ae4bb-7f71-4eae-803b-8adfa8a430e9
📒 Files selected for processing (3)
crates/engine/src/game/mana_abilities.rscrates/engine/src/game/mana_sources.rscrates/engine/tests/integration/issue_6157_gold_token_auto_mana_payment.rs
|
Maintainer fixup The current head is held pending its required CI run and CodeRabbit review. No approval or merge-queue enrollment has been performed. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 4656be6: the LED-only regression now drives the real CastSpell action and asserts the manual ManaPayment boundary; Gold/Treasure/tapped-Gold and LED cases remain covered. Current CodeRabbit completed successfully; auto-merge will wait for the remaining required checks.
…hase-rs#6230) * fix(engine): auto-tap self-sacrifice mana abilities like Gold tokens Auto-tap source discovery required every scanned mana ability to have a {T} cost component, so Gold's "Sacrifice this token: Add one mana of any color." (no {T}) was invisible to CastPaymentMode::Auto even though the sacrifice targets only the token itself and needs no player choice — exactly as deterministic as a {T} cost. Treasure worked only because its cost happens to also include {T}. Extend the single-authority eligibility check to also accept an unambiguous self-sacrifice cost (Sacrifice targeting SelfRef, count 1), and exclude that same shape from the "needs manual payment" fallback path so it isn't double-counted. Ambiguous sacrifice costs (Krark-Clan Ironworks' "Sacrifice an artifact") still require a manual choice and are unaffected. Closes phase-rs#6157 * fix(engine): honor tapped/summoning-sick self-sacrifice mana on auto-tap Address PR review on phase-rs#6230: the self-sacrifice predicate was never reached for a tapped source. Both auto-tap source gates (`auto_tap_mana_options` and `collect_sorted_auto_tap_source_options`) returned early on `obj.tapped` / summoning-sickness before `scan_mana_abilities` could evaluate the ability, so a tapped Gold token ("Sacrifice this token: Add one mana of any color.", no `{T}`) offered no source and the spell stayed unpayable. Make the object-level tapped/summoning-sick prefilter conditional on the source requiring `{T}`: a source is only dropped up front when it lacks a *tapless* unambiguous self-sacrifice mana ability (CR 106.12 / CR 302.6 gate `{T}`/`{Q}` costs only). Sources carrying such an ability fall through to `scan_mana_abilities`, where a cheap per-ability cost/field guard on the auto-tap path still excludes any `{T}`/`{Q}` ability of a tapped or summoning-sick source — so no `{T}` source leaks through and no legality simulation (readiness call) is added, preserving the bulk-activation linearity invariant. Treasure (`{T}, Sacrifice this artifact`) is deliberately excluded from the bypass: once tapped it genuinely cannot activate. Add regression test `tapped_gold_token_still_auto_taps_for_self_sacrifice_mana` proving a tapped, summoning-sick Gold token is still auto-tapped via its self-sacrifice mana ability. Fails on the pre-fix path, passes after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(engine): require full choice-free cost tree for self-sac auto-tap eligibility (LED composite) has_unambiguous_self_sacrifice_component matched a self-sacrifice via cost_has_component (Composite = any), so Lion's Eye Diamond's Composite[Discard{Chosen}, Sacrifice(SelfRef,1)] was misclassified as unambiguous self-sacrifice. That let the auto-tap eligibility gate (mana_sources.rs:1820) and the manual-payment exclusion (mana_sources.rs:1281) auto-select LED, bypassing its Discard prompt. Reuse the existing deny-by-default full-tree authority mana_abilities::cost_component_choice_free (Tap + single-token self-sacrifice + Composites built solely from those) by making it pub(crate) and sharing it: the predicate now requires the whole cost tree be choice-free AND actually contain a self-sacrifice component. Gold (bare self-sac) and Treasure ({T}+self-sac) stay auto-tap eligible; LED-shaped composites are rejected and keep their discard prompt. Add regression led_shaped_discard_sacrifice_stays_off_auto_tap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(engine): end-to-end LED composite stays off auto-tap manual-payment path Drive CastPaymentMode::Auto with a LED-shaped mana source (Composite[Discard{Chosen}, Sacrifice(SelfRef,1)]) alongside a Gold token: auto-tap must skip the choice-bearing LED and pay the generic-1 spell with Gold, leaving LED on the battlefield (manual-payment path), while a LED-only board is not auto-payable. Complements the predicate unit test with a behavioral regression; fails on the pre-fix any-match predicate (spell becomes unpayable) and passes with the whole-tree choice-free check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(PR-6230): assert manual payment pause * test(PR-6230): preserve manual mana payment assertion * test(PR-6230): fix manual payment assertion --------- Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
2 participants
fix(engine): auto-tap self-sacrifice mana abilities like Gold tokens
Auto-tap source discovery required every scanned mana ability to have a
{T} cost component, so Gold's "Sacrifice this token: Add one mana of any
color." (no {T}) was invisible to CastPaymentMode::Auto even though the
sacrifice targets only the token itself and needs no player choice —
exactly as deterministic as a {T} cost. Treasure worked only because its
cost happens to also include {T}.
Extend the single-authority eligibility check to also accept an
unambiguous self-sacrifice cost (Sacrifice targeting SelfRef, count 1),
and exclude that same shape from the "needs manual payment" fallback path
so it isn't double-counted. Ambiguous sacrifice costs (Krark-Clan
Ironworks' "Sacrifice an artifact") still require a manual choice and are
unaffected.
Closes #6157
Summary by CodeRabbit
Bug Fixes
Tests