ship/fix castability fallback for tap mana - #7016
Conversation
|
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)
📝 WalkthroughWalkthroughThe change adds exact simulation for producer-to-filter-land payment routes. It prevents filter-land activation mana from being counted twice. Casting and auto-payment checks now accept only routes that leave the spell payable. ChangesFilter-land casting validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Casting
participant ManaSources
participant PaymentSimulation
participant Spell
Casting->>ManaSources: inspect activatable mana profiles
ManaSources-->>Casting: exclude mana-funded tap abilities
Casting->>PaymentSimulation: simulate producer and filter-land activations
PaymentSimulation->>Spell: apply exact auto-tap payment
Spell-->>PaymentSimulation: report remaining payable cost
PaymentSimulation-->>Casting: return payment-path result
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/game/casting_tests.rsast-grep timed out on this file Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/src/game/mana_sources.rs`:
- Around line 2190-2200: Update the residual mana-feasibility logic around
has_tap_component and mana_sub_cost_of so mana-consuming tap sources are
represented as dependency-aware activation plans rather than unconditionally
excluded. Preserve the exact auto-tap probe while allowing a manually activated
source such as Blood Pet to satisfy the filter land’s activation cost before the
spell cost is paid, and add a regression test covering that sequence under the
existing mana-payment test helpers.
🪄 Autofix
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: 532a582e-40d2-434a-a7da-67aa1d243594
📒 Files selected for processing (3)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/mana_sources.rs
| // CR 601.2g: A tap mana ability that itself needs mana (such as | ||
| // a filter land) must stay with the exact auto-tap payment probe. | ||
| // A standalone profile cannot represent the mana it consumes to | ||
| // activate, and would let that mana cover the spell as well. Plain | ||
| // tap sources remain here so they can combine with a manual-choice | ||
| // mana ability during the same cost-payment step. | ||
| if has_tap_component(&ability.cost) | ||
| && mana_abilities::mana_sub_cost_of(&ability.cost).is_some() | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve filter-land sequences after manual mana activation.
Lines 2196-2200 remove the filter land from residual feasibility. This rejects legal casts that require a manual mana ability before the filter land.
For example, a player can activate Blood Pet for {B}, then activate a {B}, {T}: Add {B}{R} filter land while casting a {B}{R} spell. The exact auto-tap probe runs before the manual activation. The fallback can see Blood Pet but cannot combine it with the excluded filter land.
Model mana-consuming sources as dependency-aware activation plans instead of removing them from all residual profiles. Add a regression test for this sequence. CR 601.2g and CR 605.3a permit activating mana abilities during casting before costs are paid. (media.wizards.com)
As per coding guidelines, compose reusable typed building blocks; as per path instructions, engine behavior must remain faithful to the Comprehensive Rules.
🤖 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 2190 - 2200, Update the
residual mana-feasibility logic around has_tap_component and mana_sub_cost_of so
mana-consuming tap sources are represented as dependency-aware activation plans
rather than unconditionally excluded. Preserve the exact auto-tap probe while
allowing a manually activated source such as Blood Pet to satisfy the filter
land’s activation cost before the spell cost is paid, and add a regression test
covering that sequence under the existing mana-payment test helpers.
Sources: Coding guidelines, Path instructions
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 8664-8665: Add a concise Comprehensive Rules annotation beside the
combined payment-feasibility and manual-payment-path checks in the casting
legality flow, referencing CR 601.2h and its rule about unpayable total costs.
Keep the existing conditions and behavior unchanged.
In `@crates/engine/src/game/casting_tests.rs`:
- Around line 386-406: Update both successful filter-land activation routes in
crates/engine/src/game/casting_tests.rs:386-406 and
crates/engine/src/game/casting_tests.rs:511-530 by asserting immediately after
each apply_as_current TapLandForMana call that filter_land is tapped, while
preserving the existing mana-resolution and cast assertions.
- Around line 214-252: Add a verified CR 601.2g and CR 605.1a/605.3a annotation
to create_black_red_filter_land, describing that its activated mana ability may
be activated while paying a spell cost. Keep the existing ability definition and
behavior unchanged.
In `@crates/engine/src/game/casting.rs`:
- Around line 14752-14796: The exact filter-land witness is recomputed across
has_manual_mana_payment_path_for_spell and
can_feasibly_pay_mana_cost_without_x_with_probe. Compute or memoize
has_exact_filter_land_payment_witness once for each concrete (source_id, cost)
predicate before those branches, then reuse the cached result while preserving
the existing fallback behavior.
🪄 Autofix
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: c91a3f84-0bc8-4ffe-b640-a33592630cce
📒 Files selected for processing (3)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rs
| && super::casting::can_feasibly_pay_mana_cost(state, player, Some(object_id), cost) | ||
| && super::casting::has_manual_mana_payment_path_for_spell(state, player, object_id, cost); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols"
wc -l crates/engine/src/game/casting_costs.rs
rg -n "pub fn|fn .*mana|has_manual_mana_payment_path_for_spell|can_feasibly_pay_mana_cost|#[CR]" crates/engine/src/game/casting_costs.rs | head -200
echo
echo "Context around lines 8630-8685"
sed -n '8630,8685p' crates/engine/src/game/casting_costs.rsRepository: phase-rs/phase
Length of output: 6736
🌐 Web query:
Magic Comprehensive Rules 601.2g total cost legality before casting spell
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, specifically rule 601.2g, if a spell's total cost includes a mana payment, the player is granted the opportunity to activate mana abilities after the total cost has been determined but before those costs are actually paid [1][2][3]. To clarify the timeline of casting a spell and cost determination: 1. Determination of Total Cost (601.2f): The player first determines the total cost of the spell, accounting for mana costs, alternative/additional costs, and any cost increases or reductions [1][2][3]. Once determined, this total cost is locked in, meaning subsequent effects that would change the cost will not apply [1][2][3]. 2. Legality Check (601.2e): Prior to this, the game checks if the spell can be legally cast [4]. If it cannot, the action is illegal and the game state is reversed [1][4]. 3. Mana Ability Activation (601.2g): Once the total cost is locked in, if that cost includes mana, the player may activate mana abilities to generate the necessary resources [1][2][3]. These mana abilities must be activated before the player proceeds to pay the total cost [1][2][3]. Rule 601.2g serves as a specific window within the sequence of casting a spell (outlined in rules 601.2a–h) to ensure the player can generate the required mana to satisfy the previously determined total cost [1][2].
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/casting-spells.html
- 2: https://mtg-rules.vercel.app/chapters/601
- 3: https://magic-rulebook.vercel.app/rules/601
- 4: https://apps.magicjudges.org/forum/topic/45414/
🌐 Web query:
Magic Comprehensive Rules 601.2e determine costs legality check total cost
💡 Result:
In the Magic: The Gathering Comprehensive Rules, section 601.2e is a specific step in the process of casting a spell [1][2][3]. This rule states that the game checks to see if the proposed spell can legally be cast [1][2][3]. If the spell is found to be illegal, the game process is reversed, and the game returns to the moment immediately before the casting of that spell was proposed [1][2][3]. To clarify the relationship between this step and cost determination: 1. Process Order: The casting of a spell is a multi-step procedure (601.2a–h). Rule 601.2e occurs after the initial proposal steps (such as announcing the spell and choosing modes/targets) and before the player determines the total cost of the spell, which is handled in rule 601.2f [1][2][3]. 2. Legality Check: At step 601.2e, the game verifies if the spell is legal to cast based on the choices made up to that point [1][2][3]. If, for instance, a player is attempting to cast a spell without being able to meet a required condition (such as a timing restriction or a required target), the casting is illegal, and the game state is reset to before the proposal [1][2][3]. 3. Cost Determination (601.2f): This rule follows the legality check [1][2][3]. During this step, the player calculates the total cost, which includes the mana cost or an alternative cost, plus any additional costs and cost increases, minus any cost reductions [2][4][3]. Once this total cost is finalized, it becomes "locked in" [2][4][3]. In summary, 601.2e acts as a validation gate for the actions taken during the proposal phase (601.2a–d) before the player proceeds to calculate and pay the costs associated with the spell in 601.2f–h [1][2][3].
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/casting-spells.html
- 2: https://mtg-rules.vercel.app/chapters/601
- 3: https://mtg.fandom.com/wiki/Casting_spells
- 4: https://magic-rulebook.vercel.app/rules/601
🌐 Web query:
site:apps.magicjudges.org/ComprehensiveRules 601.2o "can't be paid" total cost
💡 Result:
The rule is likely 601.2h, not 601.2o—current Comprehensive Rules skip the letters l and o.
In short:
- 601.2f: Determine and lock in the spell’s total cost.
- 601.2g: Activate mana abilities.
- 601.2h: Pay the total cost.
- If any part of the total cost is impossible to pay, the spell cannot be cast; costs must be paid fully, not partially. (blogs.magicjudges.org)
Citations:
🌐 Web query:
MTG Comprehensive Rules 601.2h pay total cost if any cost can't be paid
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rule 601.2h governs the payment of the total cost for a spell. The rule states: "The player pays the total cost in any order. Partial payments are not allowed. Unpayable costs can’t be paid" [1][2]. This subrule is part of the broader process of casting a spell (rule 601) [3][1]. The process dictates that a player must be legally allowed to cast a spell to begin the process, and if they are unable to comply with any requirements during the steps of casting (which include determining and paying the total cost), the casting of the spell is considered illegal [1]. Consequently, the game state is returned to the moment before the spell was proposed [1]. Under rule 601.2h, because partial payments are prohibited and unpayable costs cannot be satisfied, the player must be able to pay the entire locked-in total cost as determined in rule 601.2f [1][4]. If any portion of that total cost cannot be paid, the entire action of casting the spell is illegal and is rolled back [1].
Citations:
- 1: https://mtg-rules.vercel.app/chapters/601
- 2: https://boardgames.stackexchange.com/questions/27392/how-does-the-example-of-rule-601-2h-work
- 3: https://media.wizards.com/2026/downloads/MagicCompRules%2020260116.pdf
- 4: https://ancestral.vision/spells-abilities-and-effects/casting-spells.html
Add the Comprehensive Rules annotation for spell legality.
These checks gate whether a proposed cast proceeds to the payment prompt, so document the applicable casting-legality rule (for example, CR 601.2h regarding unpayable total costs).
🤖 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 8664 - 8665, Add a
concise Comprehensive Rules annotation beside the combined payment-feasibility
and manual-payment-path checks in the casting legality flow, referencing CR
601.2h and its rule about unpayable total costs. Keep the existing conditions
and behavior unchanged.
Source: Path instructions
| fn create_black_red_filter_land(state: &mut GameState, card_id: u32) -> ObjectId { | ||
| let filter_land = create_object( | ||
| state, | ||
| CardId(card_id), | ||
| PlayerId(0), | ||
| "Black-Red Filter Land".to_string(), | ||
| Zone::Battlefield, | ||
| ); | ||
| let obj = state.objects.get_mut(&filter_land).unwrap(); | ||
| obj.card_types.core_types.push(CoreType::Land); | ||
| Arc::make_mut(&mut obj.abilities).push( | ||
| AbilityDefinition::new( | ||
| AbilityKind::Activated, | ||
| Effect::Mana { | ||
| produced: ManaProduction::ChoiceAmongCombinations { | ||
| options: vec![ | ||
| vec![ManaColor::Black, ManaColor::Black], | ||
| vec![ManaColor::Black, ManaColor::Red], | ||
| vec![ManaColor::Red, ManaColor::Red], | ||
| ], | ||
| }, | ||
| restrictions: vec![], | ||
| grants: vec![], | ||
| expiry: None, | ||
| target: None, | ||
| }, | ||
| ) | ||
| .cost(AbilityCost::Composite { | ||
| costs: vec![ | ||
| AbilityCost::Mana { | ||
| cost: ManaCost::Cost { | ||
| shards: vec![ManaCostShard::Black], | ||
| generic: 0, | ||
| }, | ||
| }, | ||
| AbilityCost::Tap, | ||
| ], | ||
| }), | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required CR annotation.
create_black_red_filter_land models an activated mana ability during spell payment. Add a verified CR 601.2g + CR 605.1a/605.3a annotation that describes this behavior. The rules permit mana-ability activation while paying a spell cost. (media.wizards.com)
As per coding guidelines, “rules-related code” must include a verified CR number and description.
🤖 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_tests.rs` around lines 214 - 252, Add a
verified CR 601.2g and CR 605.1a/605.3a annotation to
create_black_red_filter_land, describing that its activated mana ability may be
activated while paying a spell cost. Keep the existing ability definition and
behavior unchanged.
Sources: Coding guidelines, Path instructions
| fn has_exact_filter_land_payment_successor( | ||
| state: &GameState, | ||
| player: PlayerId, | ||
| mut accepts: impl FnMut(&GameState) -> bool, | ||
| ) -> bool { | ||
| for producer in super::mana_sources::activatable_mana_source_selections(state, player) { | ||
| if is_costed_tap_mana_selection(state, &producer) { | ||
| continue; | ||
| } | ||
|
|
||
| for after_producer in exact_mana_ability_successors(state.clone(), player, &producer) { | ||
| for filter in | ||
| super::mana_sources::activatable_mana_source_selections(&after_producer, player) | ||
| { | ||
| if filter.source == producer.source | ||
| || !is_costed_tap_mana_selection(&after_producer, &filter) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| for after_filter in | ||
| exact_mana_ability_successors(after_producer.clone(), player, &filter) | ||
| { | ||
| if accepts(&after_filter) { | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| false | ||
| } | ||
|
|
||
| /// Finds a two-step producer -> filter-land route that leaves the spell | ||
| /// payable under the ordinary exact auto-tap authority. | ||
| fn has_exact_filter_land_payment_witness( | ||
| state: &GameState, | ||
| player: PlayerId, | ||
| source_id: ObjectId, | ||
| cost: &ManaCost, | ||
| ) -> bool { | ||
| has_exact_filter_land_payment_successor(state, player, |after_filter| { | ||
| can_pay_cost_after_auto_tap_with_probe(after_filter, player, source_id, cost, None) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers of the castability predicate and the new witness to
# establish whether the exact route search runs once or per candidate card.
set -euo pipefail
rg -n -C 6 'can_feasibly_pay_mana_cost_without_x' --type=rust
echo '--- callers of has_manual_mana_payment_path_for_spell ---'
rg -n -C 6 'has_manual_mana_payment_path_for_spell' --type=rust
echo '--- enumeration entrypoints that gate on castability ---'
rg -n -C 4 'fn (legal_actions|available_actions|castable|enumerate_.*cast)' --type=rustRepository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- repo files around casting ==='
fd -a 'casting\.rs$' . | sed 's#^\./##'
echo '--- symbols of interest ==='
rg -n 'can_feasibly_pay_mana_cost_without_x|has_manual_mana_payment_path_for_spell|has_exact_filter_land_payment_witness|has_exact_filter_land_payment_successor|activatable_mana_source_selections|settle_exact_mana_ability_prompts|exact_mana_ability_successors' crates --type=rust || true
echo '--- caller context exact lines from casting.rs ==='
for name in has_manual_mana_payment_path_for_spell has_exact_filter_land_payment_successor has_exact_filter_land_payment_witness settle_exact_mana_ability_prompts exact_mana_ability_successors is_costed_tap_mana_selection; do
line=$(rg -n "fn $name" crates/engine/src/game/casting.rs | head -1 | cut -d: -f1)
echo "--- $name around line ${line:-?} ---"
if [ -n "${line:-}" ]; then
start=$((line-25)); [ "$start" -lt 1 ] && start=1
end=$((line+120))
sed -n "${start},${end}p" crates/engine/src/game/casting.rs | nl -ba -v "$start"
fi
doneRepository: phase-rs/phase
Length of output: 4112
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- casting.rs caller/function contexts ---'
for start in 14580 14620 14752 14785 14835 14850; do
end=$((start+120))
echo "===== lines ${start}-${end} ====="
sed -n "${start},${end}p" crates/engine/src/game/casting.rs | awk '{printf "%7d\t%s\n", NR+'"$start"', $0}'
done
echo '--- casting_costs.rs context around 8645-8735 ---'
sed -n '8645,8735p' crates/engine/src/game/casting_costs.rs | awk '{printf "%7d\t%s\n", NR+8644, $0}'
echo '--- manual_mana/search references ---'
rg -n 'has_manual_mana_payment_path_for_spell|can_feasibly_pay_mana_cost_without_x_with_probe|can_feasibly_pay_mana_cost_without_x' --type=rust
echo '--- castability/affordability callers ---'
rg -n 'can_feasibly_pay_mana_cost_without_x|feasibly_pay_mana|afford|can_afford|legal_actions|available_actions' crates --type=rust -g '!*.lock' | head -200Repository: phase-rs/phase
Length of output: 39663
Avoid recomputing the exact filter-land route for the same spell/payment predicate.
has_manual_mana_payment_path_for_spell calls has_exact_filter_land_payment_witness directly, and can_feasibly_pay_mana_cost_without_x_with_probe calls it again when it has failed the preceding affordability checks. Both paths then run the same nested clone-and-simulate search before the final shimler/non-tap-source fallback. In castability workflows that evaluate many candidates, memoize the predicate result on the concrete (object_id, cost) before entering those branches, or compute the witness once and reuse it across both callers.
🤖 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 14752 - 14796, The exact
filter-land witness is recomputed across has_manual_mana_payment_path_for_spell
and can_feasibly_pay_mana_cost_without_x_with_probe. Compute or memoize
has_exact_filter_land_payment_witness once for each concrete (source_id, cost)
predicate before those branches, then reuse the cached result while preserving
the existing fallback behavior.
1 participant
Summary by CodeRabbit
Bug Fixes
Tests