fix(engine): spill unmatched colored cost reductions into generic (#6405) - #6740
Conversation
…ase-rs#6405) Aang, Master of Elements ("Spells you cast cost {W}{U}{B}{R}{G} less to cast. (This can reduce generic costs.)") wasn't reducing generic mana at all. `apply_shard_reduction` removed a matching colored pip when present but silently dropped any reduction unit with no match, instead of spilling it over to reduce generic mana per CR 118.7b/c. Fixed the shared helper (used by both the general ModifyCost::Reduce path and the Defiler cost-reduction path) and added coverage for the spillover, excess-beyond-match, and mismatched-color-never-touched cases.
|
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 (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughColored mana reductions now remove matching colored shards and reduce generic mana when no matching shard is available. Unit and integration tests cover spillover, mismatches, matched colors, and zero-floor behavior. ChangesColored Reduction Handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 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/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs`:
- Around line 8-12: Correct and complete the CR citations and descriptions: in
crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs
lines 8-12, cite CR 118.7b for mismatched-color reductions; in
crates/engine/src/game/casting_tests.rs lines 26057-26059, replace CR 118.7a
with CR 118.7b; add the CR 118.7b citation at lines 98-100; and cite CR 118.7
and CR 118.7b for zero-floor spillover behavior at lines 121-123.
🪄 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: 75b83d55-c26b-4bdd-a809-2f57cf290454
📒 Files selected for processing (5)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rscrates/engine/tests/integration/main.rs
| //! CR 118.7b: a colored reduction unit with no matching component in the cost | ||
| //! converts to reducing generic mana. CR 118.7c: a colored reduction that | ||
| //! exceeds the cost's component of that color reduces the color to nothing, | ||
| //! then spills the excess to generic. CR 118.7a: a reduction never touches a | ||
| //! mismatched color's pip. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct and complete the CR annotations.
CR 118.7a applies only to generic-mana reductions; a colored reduction whose type is absent is governed by CR 118.7b. (media.wizards.com)
crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L8-L12: cite CR 118.7b for the mismatched-color statement.crates/engine/src/game/casting_tests.rs#L26057-L26059: replace CR 118.7a with CR 118.7b.crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L98-L100: add the applicable CR 118.7b citation.crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L121-L123: cite CR 118.7 and CR 118.7b for zero-floor spillover behavior.
As per coding guidelines, rules-related engine code must have a verified CR citation and description.
📍 Affects 2 files
crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L8-L12(this comment)crates/engine/src/game/casting_tests.rs#L26057-L26059crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L98-L100crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs#L121-L123
🤖 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/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs`
around lines 8 - 12, Correct and complete the CR citations and descriptions: in
crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs
lines 8-12, cite CR 118.7b for mismatched-color reductions; in
crates/engine/src/game/casting_tests.rs lines 26057-26059, replace CR 118.7a
with CR 118.7b; add the CR 118.7b citation at lines 98-100; and cite CR 118.7
and CR 118.7b for zero-floor spillover behavior at lines 121-123.
Sources: Coding guidelines, Path instructions
Co-authored-by: jsdevninja <topit89807@gmail.com>
Hold — current-head evidence and test gap requiredMaintainer port Before a new implementation review, please provide the public current-head coverage parse-diff sticky/artifact. The previous engine-source head did not expose that required evidence, and the new run is still being established; pending CI itself is not the hold reason. There is also a substantive test gap in the changed Defiler consumer at No approval or auto-merge is armed. Once the public parse-diff evidence and this regression are present, request a fresh current-head review. |
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
[MED] Defiler's newly changed consumer path lacks a discriminating spillover regression. Evidence: crates/engine/src/game/casting_costs.rs:6119-6124 now delegates every Defiler reduction shard to spillover-aware apply_shard_reduction, but crates/engine/src/game/casting_costs.rs:16824-16887 exercises only a matching {G} shard and asserts life payment plus LifeChanged; those assertions also pass with the former matching-only implementation. Why it matters: an unmatched Defiler colored shard, or an excess shard after its matching pip is removed, could fail to reduce generic mana without any consumer-level test detecting it. Suggested fix: add an accepted-Defiler-payment test that drives handle_defiler_payment and asserts the pending/payment cost after an unmatched colored shard and after excess reduction spills into generic.
[LOW] The CR 118.7a annotations describe colored/colorless spillover behavior that it does not govern. Evidence: crates/engine/src/game/casting.rs:7687-7693 describes matching colored/colorless handling as “118.7a scope”; crates/engine/src/game/casting_tests.rs:26057-26059 assigns the mismatched-color case to 118.7a; and crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs:8-12 does the same. CR 118.7a applies only to generic-mana reductions; CR 118.7b governs an absent colored/colorless type spilling into generic, while 118.7c/d govern excess colored/colorless reduction. Why it matters: rules annotations are a correctness contract, so these references would direct future changes to the wrong rule. Suggested fix: revise the three annotations to attribute each stated behavior to 118.7b and, where applicable, 118.7c/d.
The current-head parse-diff reports “No card-parse changes detected,” so no parse-diff evidence hold remains.
- Correct CR annotations that mislabeled the "no matching colored/colorless component" and "excess reduction" spillover cases as CR 118.7a (which only governs generic-mana reductions). Those cases are CR 118.7b (absent type) and CR 118.7c/d (excess), per docs/MagicCompRules.txt. - Add two consumer-level regression tests driving handle_defiler_payment (not just the private apply_defiler_mana_reduction helper) that assert the post-payment pending cost after an unmatched colored reduction shard and after an excess reduction beyond the matching component, so a future regression that decouples the two can't hide behind the existing matching-only Defiler test.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review complete for the current head.
The shared cost-modification seam now applies CR 118.7b–d spillover uniformly, including Defiler's consumer path. The registered scenario test drives Aang's parsed static through casting, and the added Defiler cases discriminate the prior dropped-reduction behavior. The current-head parse-diff reports no card-parse changes; required CI is green.
|
Current-head maintainer review is approved. I attempted the authorized auto-merge action, but GitHub still reports |
|
Update: GitHub now reports this approved current head in merge-queue position 1 ( |
2 participants
Summary
apply_shard_reduction(crates/engine/src/game/casting.rs) removed one matching colored pip per reduction unit when present, but silently dropped any unit with no match instead of falling back to generic.pub(super)) so both call sites — the generalModifyCost::Reducebattlefield/self-spell path and the Defiler cost-reduction path (apply_defiler_mana_reductionincasting_costs.rs, which had the identical bug) — get the CR-correct behavior for free, per the project's single-authority convention.Closes #6405.
Test plan
cargo test -p engine --lib -- game::casting::tests— 741 passed, including 4 new unit tests covering: no-match spillover (all-generic spell gets the full 5-mana discount), excess-beyond-match spillover, exact-match-only (no spillover), and mismatched-color-never-touched.cargo test -p engine --lib -- defiler— 8 passed, including the Defiler-payment test that exercises the other fixed call site.crates/engine/tests/integration/issue_6405_aang_multicolor_cost_reduction.rs(4 tests) drives Aang's actual parsed static ability end-to-end through the cast pipeline against all-generic, partially-colored, fully-colored, and cheap-spell costs — all pass.cargo clippy -p engine --lib --tests -- -D warnings— clean.cargo fmt --all— no changes needed beyond the edit.Summary by CodeRabbit