Skip to content

feat(engine): implement Fuse keyword runtime — cast both halves of sp… - #2611

Merged
matthewevans merged 5 commits into
phase-rs:mainfrom
dale053:feat/2570-fuse-split-spell-casting
Jun 7, 2026
Merged

feat(engine): implement Fuse keyword runtime — cast both halves of sp…#2611
matthewevans merged 5 commits into
phase-rs:mainfrom
dale053:feat/2570-fuse-split-spell-casting

Conversation

@dale053

@dale053 dale053 commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Closes #2570

  • Implements the Fuse keyword runtime (CR 702.102) — Keyword::Fuse was previously parsed and stored but caused no behaviour change at cast time
  • Adds CastingVariant::Fuse and KeywordKind::Fuse; surfaces the "Cast with Fuse" option in casting_variant_candidates() for split cards in hand whose back face has layout_kind == Some(LayoutKind::Split)
  • Combines both halves' mana costs via restrictions::add_mana_cost (CR 702.102c) and chains the right half's spell ability onto the left's sub-ability so resolution follows left → right (CR 702.102d)
  • Adds right_half_targeting_pending / right_half_target_slots fields to PendingCast (reserved for a future split-targeting UX; existing build_target_slots recursion already covers both halves in a single pass per CR 601.2c)
  • Frontend: adds Fuse: "variantFuse" to CastingVariantModal's variant key map and "variantFuse": "Cast with Fuse" i18n string; adds { type: "Fuse" } to the TypeScript CastingVariant union

Test plan

  • Pick up a Dragon's Maze fuse split card (e.g. Armed // Dangerous, Alive // Well) in hand — confirm the casting variant modal offers "Cast with Fuse" alongside the normal single-half option
  • Verify the mana cost shown for the Fuse option equals both halves' costs combined
  • Cast with Fuse and confirm both halves resolve sequentially (left first, then right) and the spell ends in the graveyard
  • Confirm single-half casting (pick one half) is unaffected
  • Confirm the Fuse option does not appear when the card is in the graveyard or any zone other than hand
  • Run tilt logs clippy and tilt logs test-engine — no new errors

CR coverage

Rule What it governs
CR 702.102a Fuse is available only from hand
CR 702.102c Total cost = left mana cost + right mana cost
CR 702.102d Left half resolves first, then right half
CR 118.9a Fuse is not an alternative cost
CR 601.2c Targets for both halves chosen at cast time

Model: claude-sonnet-4-6

🤖 Generated with Claude Code

@dale053
dale053 requested a review from matthewevans as a code owner June 7, 2026 11:14

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the 'Fuse' casting variant for split cards, adding support across the frontend UI, localization, and the game engine rules (including merging characteristics of both halves on the stack and tracking right-half targeting). Feedback focuses on adhering to repository style guide Rule R2 by refactoring the boolean and vector fields in PendingCast into a single Option<Vec<TargetSelectionSlot>> (and updating tests accordingly), as well as optimizing performance in casting_costs.rs by avoiding cloning the entire CardFace struct.

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.

I am having trouble creating individual review comments. Click here to see my feedback.

crates/engine/src/types/game_state.rs (1106-1113)

high

[HIGH] Avoid boolean fields on structs by using Option<T> or a typed enum. Evidence: crates/engine/src/types/game_state.rs:1109.

Why it matters: Violates rule R2 of the repository style guide, which prohibits bool fields in favor of more expressive types like Option<T> or custom enums to prevent state space explosion.

Suggested fix: Combine both fields into a single Option<Vec<TargetSelectionSlot>> where None indicates no right-half targeting is pending, and Some(slots) holds the stashed slots.

    /// CR 601.2c + CR 702.102d: Stash for right-half target slots while
    /// left-half targeting is in progress during a fused split spell cast.
    /// None indicates no right-half targeting is pending.
    #[serde(default, skip_serializing_if = "Option::is_none")]
    pub right_half_target_slots: Option<Vec<TargetSelectionSlot>>,
References
  1. Rule R2: No bool fields — parameterize with existing typed enums. A bool field never expresses the design space; the project uses typed enums like ControllerRef, Comparator, PlayerScope, Option, and dedicated discriminated unions instead. (link)

crates/engine/src/types/game_state.rs (1159-1164)

medium

[MEDIUM] Update test initialization to use Option for right_half_target_slots. Evidence: crates/engine/src/types/game_state.rs:1162.

Why it matters: Ensures consistency with the refactored PendingCast struct that replaces the boolean field with Option<Vec<TargetSelectionSlot>>.

Suggested fix: Set right_half_target_slots to None.

            convoked_creatures: Vec::new(),
            cancel_restore_prepared_source: None,
            payment_mode: CastPaymentMode::Auto,
            right_half_target_slots: None,
        }

crates/engine/src/game/casting_costs.rs (4163-4182)

medium

[MEDIUM] Avoid cloning the entire CardFace struct when only specific fields are needed. Evidence: crates/engine/src/game/casting_costs.rs:4165.

Why it matters: CardFace can be a large struct containing Oracle text, parsed abilities, and other heavy fields. Cloning it entirely is inefficient and unnecessary.

Suggested fix: Extract and clone only the core_types and color fields from the reference.

    if casting_variant == CastingVariant::Fuse {
        if let Some(obj) = state.objects.get_mut(&object_id) {
            let extra = obj.back_face.as_ref()
                .filter(|bf| bf.layout_kind == Some(crate::types::card::LayoutKind::Split))
                .map(|bf| (bf.card_types.core_types.clone(), bf.color.clone()));
            if let Some((extra_types, extra_colors)) = extra {
                for ct in extra_types {
                    if !obj.card_types.core_types.contains(&ct) {
                        obj.card_types.core_types.push(ct);
                    }
                }
                for color in extra_colors {
                    if !obj.color.contains(&color) {
                        obj.color.push(color);
                    }
                }
            }
        }
    }

@matthewevans matthewevans added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jun 7, 2026
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@matthewevans

Copy link
Copy Markdown
Member

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: PASS — Fuse logic lives entirely in the engine cast pipeline (casting.rs cost-combine + chain-append, casting_costs.rs stack-characteristic merge); reuses restrictions::add_mana_cost and append_to_ability_def_sub_chain building blocks rather than reinventing. Frontend change is a thin variant-label + i18n map only.
Idiomatic: CONCERN — adds two PendingCast fields that are never read (one is a bare bool); a stray gitlink was committed.
Value: Covers the whole Fuse class (any split card with Keyword::Fuse + Split back face), not a single card — cost combine, merged characteristics, and ordered left→right resolution all route through general chain machinery. Right-half targets are collected automatically via collect_target_slots' existing sub_ability recursion (verified ability_utils.rs:1172 recurses the chain), so the approach genuinely generalizes.

CR verification: All cited numbers grep-confirmed in docs/MagicCompRules.txt — 702.102a–d (Fuse), 709.4b/c/d (combined cost/types/characteristics), 105.2 (colors), 601.2c/f, 118.9a (Fuse is not an alt cost). Annotations are accurate and well-placed.

Reconciled with existing reviews:

  • Gemini [HIGH] R2 bool field (right_half_targeting_pending) — CONFIRMED as a real smell, but Gemini's fix (collapse to Option<Vec<TargetSelectionSlot>>) is wrong-target: both fields are never read anywhere in the tree (only ever written false/Vec::new()), so the correct fix is to delete both, not retype them. See finding below.
  • Gemini [MEDIUM] test init for OptionREFUTED as written (depends on the wrong refactor above); the test-struct field additions are correct for the current shape and would simply be removed alongside the fields.
  • Gemini [MEDIUM] CardFace full clone (casting_costs.rs:4210) — CONFIRMED; obj.back_face.clone() clones the entire face (oracle text + parsed abilities) only to read core_types/color. Low severity (one-shot finalize path, not per-step), but Gemini's borrow-then-extract fix is correct and worth applying.

Findings

  • [BLOCKER] gittensory — a stray git submodule/gitlink (mode 160000, Subproject commit a4429ce8b…) was committed. It is not in origin/main, has no .gitmodules entry, and is unrelated to Fuse. This breaks git submodule/clean checkout for everyone. Remove it (git rm --cached gittensory and drop from the tree) before merge.
  • [MEDIUM] crates/engine/src/types/game_state.rs:1127,1131right_half_targeting_pending: bool and right_half_target_slots: Vec<TargetSelectionSlot> are added to PendingCast but read by nothing (grep: only Default/test initializers write them). The PR's own comment in casting.rs:3614 states they "remain reserved for a future split-targeting UX." This is speculative dead state (violates "don't design for hypothetical future requirements") and the unread bool is exactly the R2 smell Gemini flagged. Delete both fields and their initializers; the right-half targets are already collected via the merged sub_ability chain, so no stash is needed.
  • [MEDIUM] No integration test drives an actual fused split spell through the cast pipeline (the only diff "fuse" hits are pre-existing files matching the substring). For a chore: update coverage stats and badges #1-pillar rules feature with combined cost, merged on-stack characteristics, and ordered left→right resolution, add a card-test-style cast→resolve test (e.g. a real Fuse split card) asserting: combined mana cost, both halves' types/colors visible on the stack object, and both halves' effects resolving in order.
  • [NIT] crates/engine/src/game/casting_costs.rs:4210 — replace obj.back_face.clone() with a borrow-then-extract of just (core_types, color) (Gemini's suggestion) to avoid cloning the whole CardFace.

Resolve maintainer review feedback for PR phase-rs#2611:

- remove the stray gittensory gitlink

- remove unused PendingCast right-half targeting state

- avoid cloning the full right CardFace when merging fused stack characteristics

- offer Normal plus Fuse when a hand split card has Fuse so the cast pipeline cannot silently fall through to normal casting

- add Breaking // Entering cast-pipeline coverage for combined cost, fused stack characteristics, and left-to-right resolution

Verification:

- cargo fmt --all

- git diff --check

- Tilt clippy: ok

- Tilt test-engine: new fuse_runtime test passed; existing anaphoric_scope_allowlist_guard failure is unrelated
@matthewevans

Copy link
Copy Markdown
Member

Pushed follow-up maintainer changes in 804dd9b addressing the review feedback:

  • removed the stray gittensory gitlink
  • removed the unused PendingCast right-half targeting fields and initializers
  • avoided cloning the full right CardFace when merging fused stack characteristics
  • fixed Fuse variant enumeration so hand split cards with Fuse offer Normal + Fuse instead of silently falling through to normal casting
  • added real Breaking // Entering cast-pipeline coverage for combined fused cost, fused stack characteristics, and left-to-right resolution
  • extended the scenario cast harness with explicit cast-variant intent and a stack-commit checkpoint for tests

Verification:

  • cargo fmt --all
  • git diff --check
  • Tilt clippy: ok
  • Tilt test-engine: the new fuse_runtime::fused_breaking_entering_combines_cost_characteristics_and_resolves_left_then_right test passed; the remaining failure is the existing unrelated anaphoric_scope_allowlist_guard::anaphoric_scope_set_is_frozen allowlist drift.

@matthewevans matthewevans added feature Larger-scoped feature ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow and removed needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Jun 7, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after maintainer follow-up. I verified the changes are at the casting variant / fused stack-characteristics seams, removed the unused pending-cast state, and added a discriminating real-card cast-pipeline test for fused cost, stack characteristics, and left-to-right resolution. Broad validation is left to GitHub CI/Tilt; local Tilt clippy is green and test-engine only reports the known unrelated anaphoric allowlist failure.

@matthewevans
matthewevans added this pull request to the merge queue Jun 7, 2026
Merged via the queue into phase-rs:main with commit 4a5c97a Jun 7, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow feature Larger-scoped feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keyword::Fuse is parsed but inert — fused split spells (cast both halves) are unimplemented (CR 702.102)

2 participants