fix(engine): honor effective Dash keyword for Ragavan dash cast (Fixes #566)

[kiannidev]

Summary

• Read Keyword::Dash through effective_spell_keywords in candidate generation, cost preparation, and AlternativeCastChoice dispatch (matching Blitz/Evoke).
• Add integration regression tests for Ragavan-shaped {R} / dash {1}{R} alternative-cast offer and end-to-end dash resolution.

Fixes #566

Test plan

• cargo test -p engine --test integration issue_566
• cargo fmt --all

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

Review: PR #2798 — honor effective Dash keyword (Fixes #566)

Verdict: Approve with reservations. The engine change is correct, idiomatic, and at the right seam — it brings Dash in line with the established effective_spell_keywords granted-keyword pattern (Evoke/Overload/Emerge/Blitz already use it; Dash + Spectacle were the laggards). My one substantive concern is the test: it does not discriminate the fix and the actual #566 card never needed it.

Seam / correctness (lens a) — PASS

All three Dash cast-path reads now route through effective_spell_keywords(state, player, object_id) — the canonical authority that merges printed obj.keywords with statics-granted keywords (granted_spell_keywords + flashback), the same function Evoke (casting.rs:2752), Overload (:2764), Emerge (:2775), and Blitz (:2801, :7209) already consume. player is in scope at all three sites (casting_variant_candidates param, prepare_spell_cast_with_variant_override_inner param, handle_cast_spell_with_payment_mode param). Post-resolution riders (haste, next-end-step return) are installed by CastingVariant::Dash via prepare_spell_cast_with_variant_override, untouched and correct.

Idiomatic (lens b) — PASS

Typed Keyword::Dash(_) match, reuse of the shared building block rather than re-reading obj.keywords, consistent with siblings. CR annotations are accurate (grep-verified against docs/MagicCompRules.txt): 702.109a (Dash = static abilities, justifying the CR 604.1 "granted by a static" framing), 601.2f (cost determination), 118.9 (alternative costs). No +// malformations.

Value / test (lens c) — the reservation

[MED] The new integration test does not discriminate the fix, and Ragavan never exercised the bug this PR targets. Evidence: client/public/card-data.json carries Ragavan's Dash as a printed keyword ("keywords":[{"Dash":{...}}]), which lands in obj.keywords. origin/main's pre-fix code read obj.keywords directly (casting.rs:2786-2790 on origin/main), so it already surfaced AlternativeCastChoice(Dash) for Ragavan — effective_spell_keywords is a superset that includes printed keywords, so the behavior is identical for printed Dash. The test setup_ragavan_in_hand likewise pushes Dash onto obj.keywords via .with_keyword(...), so it passes against both old and new code and proves neither the fix nor a #566 regression. Why it matters: the change this PR actually makes — honoring granted Dash (a static adding Dash via CastWithKeyword) — ships with no discriminating coverage; a future regression to the granted path would not be caught. Suggested fix: add a test where Dash is granted by a continuous effect / CastWithKeyword static (not pushed onto obj.keywords), assert the offer appears — that is the only fixture shape that fails on origin/main and passes here.

[LOW] New test is largely redundant with existing inline coverage. casting.rs:22662 dash_creature_offers_dash_variant and :22704 dash_full_cast_installs_riders_on_resolution already cover printed-Dash offer + full-cast-to-battlefield + haste + riders (using Zurgo Bellstriker). The new integration test re-covers the same printed path with a different card name. Not harmful, but it does not add coverage beyond renaming the fixture.

[LOW] Spectacle is the remaining un-converted sibling. Evidence: casting.rs:2812 still reads obj.keywords directly in casting_variant_candidates for Keyword::Spectacle(_), unlike all other alternative-cost keywords. Out of scope for #566/Dash, but it is the one remaining inconsistency in this exact class — worth a follow-up so granted Spectacle is honored too.

Note

The fix is genuinely class-general and consistent with the other alternative-cost keywords — that part is good. The gap is purely on the verification side: the bug report (#566, printed-Dash Ragavan) and the regression test both live on a path that already worked, while the code change targets the granted-Dash path that has no test. Recommend tightening the test before merge so the green check actually defends the new behavior.

[natefinch]

Independent review (review-impl lenses) — PR #2798

Seam / correctness (lens a) — PASS. All three Dash cast-path reads now route through effective_spell_keywords(state, player, object_id), the canonical authority that merges printed obj.keywords with statics-granted keywords (casting.rs:1111, clones obj.keywords then upserts granted_spell_keywords). This is the correct seam and brings Dash in line with its siblings — Evoke (:2752), Overload (:2764), Emerge (:2775), Blitz (:2801) already consume effective_spell_keywords. player/object_id are in scope at all three sites. CR annotations grep-verify against docs/MagicCompRules.txt (702.109a, 601.2f); the CR 604.1 "granted by a static" framing is accurate.

Idiomatic (lens b) — PASS. Typed Keyword::Dash(_) match, shared building block reused rather than re-reading obj.keywords.

Findings

[MED] The regression test does not discriminate the fix. Evidence: setup_ragavan_in_hand injects Dash via .with_keyword(...) → push_keyword → obj.keywords.push(kw) (scenario.rs:703-707). effective_spell_keywords begins let mut keywords = obj.keywords.clone(); (casting.rs:1120), so the .any(|k| matches!(k, Keyword::Dash(_))) check finds Dash identically in the pre-fix code (obj.keywords.iter()) and the post-fix code. Both tests therefore pass on origin/main with the src change reverted — I verified this analytically and confirmed the fixture path (could not run cargo: toolchain not on PATH in this env). Why it matters: the behavior this PR actually changes — honoring Dash granted by a CastWithKeyword static, with no Dash in obj.keywords — ships with zero discriminating coverage, so a future regression to the granted path would not be caught; and Ragavan's printed Dash already reached obj.keywords on main, so the #566 card never exercised this path. Suggested fix: add a test that grants Dash via a CastWithKeyword static (Dash absent from obj.keywords, mirroring the WebSlinging fixture in derived_views.rs:1069) and assert the offer appears — the only fixture shape that fails on origin/main and passes here.

[LOW] Spectacle is the remaining un-converted sibling. Evidence: casting.rs:2812 and :3093 still read obj.keywords directly for Keyword::Spectacle(_), unlike every other alternative-cost keyword. Out of scope for #566, but it is the one remaining inconsistency in this exact class; worth a follow-up so granted Spectacle is honored too.

This confirms and folds in the reservation already raised by @matthewevans. The engine change is sound, class-general, and safe to merge; the gap is purely verification — recommend strengthening the test so the green check defends the granted-Dash path the fix targets.

VERDICT: approve with comments

[matthewevans]

Maintainer note: pushed aa14b7f addressing the MED from both reviews — new granted_dash_from_static_offers_dash_choice test grants Dash via a StaticMode::CastWithKeyword battlefield static with no printed Dash, empirically verified to FAIL against origin/main casting.rs and pass with this PR's fix (the existing Ragavan tests pass on both, since printed Dash reaches obj.keywords either way). Also converted the two remaining Spectacle keyword reads to effective_spell_keywords, closing out the LOW (Spectacle was the last unconverted alternative-cost sibling; the granted-merge seam it relies on is pinned by the new Dash test).

[matthewevans]

Maintainer sign-off: correct seam (effective_spell_keywords is the canonical granted-keyword authority; Evoke/Overload/Emerge/Blitz already route through it), discriminating granted-Dash test added and revert-verified, Spectacle sibling converted. Two independent review-impl approvals on the engine change.