Skip to content

fix(engine): split cards report wrong mana value and colors off the stack (CR 202.3d / 709.4b)#5093

Open
real-venus wants to merge 8 commits into
phase-rs:mainfrom
real-venus:fix/split-card-offstack-mana-value
Open

fix(engine): split cards report wrong mana value and colors off the stack (CR 202.3d / 709.4b)#5093
real-venus wants to merge 8 commits into
phase-rs:mainfrom
real-venus:fix/split-card-offstack-mana-value

Conversation

@real-venus

Copy link
Copy Markdown
Contributor

Summary

A split card (Fire // Ice, Assault // Battery, Wear // Tear — 109 pairs) in any zone other than the stack reported only its FRONT half's mana value and colors instead of the combined value of both halves (CR 202.3d / 709.4b). ~30 off-stack consumers were affected: MV/color search & filters, clash/discover/cascade/collect-evidence, library search, exile-cast legality, protection/hexproof, and the leave-play LKI snapshot (so MV-gated "creature with mana value N dies" look-back triggers misfired). Adds authoritative GameObject::effective_mana_value()/effective_colors() (combine both halves off-stack, single-face otherwise) and migrates the off-stack call sites; on-stack/Fuse/deck-validation left unchanged.

Files changed

  • crates/engine/src/game/game_object.rs (new effective_mana_value/effective_colors helpers + gate)
  • crates/engine/src/game/{filter,quantity,targeting,keywords,zones,players,casting,casting_costs,ability_utils,engine_replacement,engine_resolution_choices}.rs
  • crates/engine/src/game/effects/{cascade,discover,clash,collect_evidence,search_library,exile_from_top_until,free_cast_from_zones,counters,delayed_trigger}.rs
  • crates/engine/tests/integration/split_offstack_mana_value.rs (new) + main.rs

CR references

CR 202.2, CR 202.3d, CR 202.3e, CR 709.4, CR 709.4b, CR 709.4d, CR 709.5, CR 709.5c

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high

Verification

  • cargo fmt --all — clean
  • cargo clippy -p engine --all-targets --features proptest -- -D warnings — clean
  • cargo test -p engine — 15023 lib + 1758 integration pass / 0 fail
  • 10 discriminating runtime tests (combined MV/colors off-stack; filters/searches match by combined value; LKI-MV dies-trigger fires; exile-cast legality uses combined MV; NEGATIVE on-stack split = chosen half; Fuse still combines; battlefield Room not over-combined vs Room-in-hand combines; single-face unchanged) — the combined-MV, on-stack, and Room-direction tests are revert-failing
  • gen-card-data/coverage/semantic-audit — N/A: engine-runtime characteristic fix, no card parsing changed

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.

@real-venus real-venus requested a review from matthewevans as a code owner July 4, 2026 17:07
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@github-actions

github-actions Bot commented Jul 4, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 13 card(s), 16 signature(s) (baseline: main 62b681dac32d)

5 card(s) · ability/attacking · removed: attacking

Examples: Curse of Disturbance, Curse of Opulence, Curse of Verbosity (+2 more)

3 card(s) · ability/static_structure · removed: static_structure

Examples: Luxior and Shadowspear, Luxior, Giada's Gift, Realmwright

2 card(s) · ability/BecomeBlocked · added: BecomeBlocked (target=unblocked attacking creature)

Examples: Curtain of Light, Dazzling Beauty

2 card(s) · static/Continuous · added: Continuous (affects=equipped by self permanent, mods=remove type planeswalker, add type creature)

Examples: Luxior and Shadowspear, Luxior, Giada's Gift

2 card(s) · ability/become · removed: become

Examples: Curtain of Light, Dazzling Beauty

1 card(s) · ability/BecomeBlocked · added: BecomeBlocked (kind=activated, target=unblocked attacking creature)

Examples: Trap Runner

1 card(s) · ability/BecomeBlocked · added: BecomeBlocked (target=attacking creature)

Examples: Fog Patch

1 card(s) · ability/BecomeBlocked · added: BecomeBlocked (target=attacking creature, targets=X-X)

Examples: Choking Vines

1 card(s) · static/Continuous · added: Continuous (affects=you control land, mods=add chosen subtype)

Examples: Realmwright

1 card(s) · ability/Draw · added: Draw

Examples: Curse of Verbosity

1 card(s) · ability/Draw · field conditional: P1P1 counters on target ≥ 1

Examples: Oblivion's Hunger

1 card(s) · ability/GainLife · added: GainLife (amount=2)

Examples: Curse of Vitality

1 card(s) · ability/Token · added: Token (token=+2/+2 Black Zombie (Creature Zombie))

Examples: Curse of Disturbance

1 card(s) · ability/Token · added: Token (token=Gold (Artifact Gold))

Examples: Curse of Opulence

1 card(s) · ability/become · removed: become (kind=activated)

Examples: Trap Runner

1 card(s) · ability/become · removed: become (targets=X-X)

Examples: Choking Vines

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@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.

[MED] Fused split spells on the stack still report only the chosen/front half's mana value. Evidence: crates/engine/src/game/game_object.rs:1213 gates combined split-card mana value to zone != Zone::Stack, while crates/engine/src/game/casting_costs.rs:6145 only unions fused stack object types/colors, not mana cost/value. Why it matters: CR 202.3d, CR 709.4d, and CR 702.102b require a fused split spell's on-stack mana value to be determined from both halves, so stack CMC filters and ObjectManaValue reads can undercount fused spells. Suggested fix: carry an authoritative fused-split state onto the stack object, or otherwise make effective_mana_value() combine both halves for CastingVariant::Fuse while preserving chosen-half behavior for non-fused split casts; add a production-path fused-spell test that asserts combined on-stack mana value.

[MED] AI collect-evidence candidates still use front-only mana value. Evidence: crates/engine/src/ai_support/candidates.rs:95 and crates/engine/src/ai_support/candidates.rs:116 sum/sort with obj.mana_cost.mana_value() instead of the new split-aware helper. Why it matters: the engine resolver now accepts off-stack split cards by combined mana value, but AI production choices for WaitingFor::CollectEvidenceChoice can omit a legal evidence payment. Suggested fix: use obj.effective_mana_value() in collect_evidence_candidate_combos and add an AI candidate test with a graveyard split card whose combined mana value alone satisfies the threshold.

[MED] Exiled-card color mana production still reads only exiled.color. Evidence: crates/engine/src/game/effects/mana.rs:785 iterates raw exiled.color for linked exiled cards. Why it matters: effects parsed as "add mana of any of the exiled card's colors" flow through ChoiceAmongExiledColors, so an exiled split card exposes only its front-half color instead of combined off-stack colors. Suggested fix: iterate exiled.effective_colors() in exiled_color_options and add a production-path mana ability test using a linked exiled split card.

@real-venus

Copy link
Copy Markdown
Contributor Author

[MED] Fused split spells on the stack still report only the chosen/front half's mana value. Evidence: crates/engine/src/game/game_object.rs:1213 gates combined split-card mana value to zone != Zone::Stack, while crates/engine/src/game/casting_costs.rs:6145 only unions fused stack object types/colors, not mana cost/value. Why it matters: CR 202.3d, CR 709.4d, and CR 702.102b require a fused split spell's on-stack mana value to be determined from both halves, so stack CMC filters and ObjectManaValue reads can undercount fused spells. Suggested fix: carry an authoritative fused-split state onto the stack object, or otherwise make effective_mana_value() combine both halves for CastingVariant::Fuse while preserving chosen-half behavior for non-fused split casts; add a production-path fused-spell test that asserts combined on-stack mana value.

[MED] AI collect-evidence candidates still use front-only mana value. Evidence: crates/engine/src/ai_support/candidates.rs:95 and crates/engine/src/ai_support/candidates.rs:116 sum/sort with obj.mana_cost.mana_value() instead of the new split-aware helper. Why it matters: the engine resolver now accepts off-stack split cards by combined mana value, but AI production choices for WaitingFor::CollectEvidenceChoice can omit a legal evidence payment. Suggested fix: use obj.effective_mana_value() in collect_evidence_candidate_combos and add an AI candidate test with a graveyard split card whose combined mana value alone satisfies the threshold.

[MED] Exiled-card color mana production still reads only exiled.color. Evidence: crates/engine/src/game/effects/mana.rs:785 iterates raw exiled.color for linked exiled cards. Why it matters: effects parsed as "add mana of any of the exiled card's colors" flow through ChoiceAmongExiledColors, so an exiled split card exposes only its front-half color instead of combined off-stack colors. Suggested fix: iterate exiled.effective_colors() in exiled_color_options and add a production-path mana ability test using a linked exiled split card.

@matthewevans

Donw.
Please kindly review this.

@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.

Thanks for the follow-up. I re-reviewed the current head and the previous split-card fixes did land for several off-stack paths, but I found two remaining blockers around fused spells:

  1. Fused split spells still use front-half metadata during mana payment. build_spell_meta builds restricted-mana metadata from raw obj.mana_cost/obj.color (crates/engine/src/game/casting.rs:1578-1584), and pay_mana_cost_with_choices uses that before finalize_cast_with_phyrexian_choices marks fused_split_spell (crates/engine/src/game/casting_costs.rs:5867, marker at 6145-6150). Restricted mana such as "spend this mana only to cast a spell with mana value/color count N" can therefore accept/reject fused split spells using only the front half.

  2. Spell-cast history still records fused split spells with front-half mana value. The fuse marker is set before record_spell_cast_from_zone (crates/engine/src/game/casting_costs.rs:6145-6150, call at 6320), but the recorder stores obj.mana_cost.mana_value_with_x(...) instead of obj.effective_mana_value() (crates/engine/src/game/restrictions.rs:233). That means "cast a spell with mana value N" history/turn filters can see Breaking // Entering as MV 2 rather than 8.

Please route both paths through the same authoritative split-aware characteristics and add discriminating tests for fused split spell restricted-mana/payment metadata and spell-cast-history mana value.

@matthewevans

Copy link
Copy Markdown
Member

Thanks for the update. I re-reviewed head 52b768e442a71bb75db137bd5c9a10204a28fe83; the previous fused split blockers look addressed now: restricted-mana metadata routes through spell_mana_value/spell_colors, cast history records the fused value, and the collect-evidence / linked-exiled-color paths use the split-aware off-stack characteristics. Live CI is green on this head.

I can't approve or enqueue this from the sweep yet because GitHub currently reports the PR as conflicting with base (mergeStateStatus=DIRTY, mergeable=CONFLICTING). Please rebase/resolve the conflicts and I'll re-review the final head.

…tack (CR 202.3d / 709.4b)

A split card (Fire // Ice, Assault // Battery, Wear // Tear — 109 pairs) in any
zone other than the stack reported only its FRONT half's mana value and colors
instead of the CR-mandated COMBINED value of both halves. CR 202.3d: a split
card not on the stack has the combined mana value of its halves; CR 709.4/709.4b:
its colors and mana value off the stack are determined from the combined mana
cost. ~30 off-stack consumers read obj.mana_cost directly and got the front half:
mana-value/color search & filter (FilterProp::Cmc/ManaValueParity/HasColor/...),
clash/discover/cascade/collect-evidence, library search, free-cast-from-zone,
exile-cast legality, protection/hexproof source characteristics, and — worst —
the leave-play LKI snapshot, so "whenever a creature with mana value N dies"
look-back triggers misfired. Only the on-stack Fuse path combined.

Adds authoritative GameObject::effective_mana_value() and effective_colors()
that combine both halves (read from the already-populated back_face) when the
card is off the stack and is not a battlefield Room, and otherwise return the
existing single-face value byte-for-byte. Migrates the ~30 off-stack call sites
to the helpers; leaves on-stack/spell, stateless-ManaCost, deck-validation, and
the Fuse merge unchanged (chosen-half is correct on the stack, CR 202.3d).

Room handling (CR 709.5): a Room permanent on the battlefield is characterized by
its unlocked halves, not the naive combine, so the gate excludes
zone==Battlefield && room_unlocks.is_some() — off-battlefield Rooms still combine
(709.4), battlefield Rooms do not (709.5c). (room_unlocks is populated in every
zone by card creation, so the gate is zone-explicit, not room_unlocks-presence.)

Tests: 10 discriminating runtime tests through the real GameScenario/GameRunner
path — combined MV/colors off-stack in library/graveyard/hand/exile; MV/color
filters and searches match by combined value; the leave-play LKI snapshot records
combined MV so MV-gated dies triggers fire; the exile-cast (impulse-draw)
legality gate uses combined MV; NEGATIVE on-stack split reports the chosen half;
Fuse still combines; a battlefield Room is not over-combined while the same Room
in hand is; single-face cards unchanged. Full cargo test -p engine green
(15023 lib + 1758 integration / 0 fail); clippy --all-targets -D warnings clean.

CR 202.2, CR 202.3d, CR 202.3e, CR 709.4, CR 709.4b, CR 709.4d, CR 709.5, CR 709.5c.
…ect-evidence, and exiled-color mana (CR 202.3d/709.4d)
…na payment metadata and spell-cast history (CR 202.3d/702.102b)
@real-venus real-venus force-pushed the fix/split-card-offstack-mana-value branch from 52b768e to bf70e0a Compare July 5, 2026 05:01

@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.

[MED] Fused split spells still use front-half mana value/colors when live spell filters evaluate the spell being cast. Evidence: crates/engine/src/game/filter.rs:2555 builds the live SpellCastRecord from raw spell_obj.color and spell_obj.mana_cost.mana_value_with_x(...), and crates/engine/src/game/casting.rs:15203 duplicates the same raw record for per-turn cast-limit filters. Why it matters: cost modifiers, cast prohibitions, and turn-limit filters can use TargetFilter properties such as Cmc, HasColor, ColorCount, or multicolored quality against the current spell, so a fused Breaking // Entering can still be seen as front-half MV/colors instead of the CR 202.3d + CR 702.102b combined spell. Suggested fix: route live spell-filter records through the same spell_mana_value() / spell_colors() authority after the fuse choice is known, preferably via one shared record constructor, and add a production-path fused split spell test against a mana-value or color spell filter.

@matthewevans matthewevans added the bug Bug fix label Jul 5, 2026
@real-venus

Copy link
Copy Markdown
Contributor Author

[MED] Fused split spells still use front-half mana value/colors when live spell filters evaluate the spell being cast. Evidence: crates/engine/src/game/filter.rs:2555 builds the live SpellCastRecord from raw spell_obj.color and spell_obj.mana_cost.mana_value_with_x(...), and crates/engine/src/game/casting.rs:15203 duplicates the same raw record for per-turn cast-limit filters. Why it matters: cost modifiers, cast prohibitions, and turn-limit filters can use TargetFilter properties such as Cmc, HasColor, ColorCount, or multicolored quality against the current spell, so a fused Breaking // Entering can still be seen as front-half MV/colors instead of the CR 202.3d + CR 702.102b combined spell. Suggested fix: route live spell-filter records through the same spell_mana_value() / spell_colors() authority after the fuse choice is known, preferably via one shared record constructor, and add a production-path fused split spell test against a mana-value or color spell filter.

Now I am fixing on it.

… spell_mana_value/spell_colors authority (CR 202.3d/702.102b)
@real-venus

Copy link
Copy Markdown
Contributor Author

@matthewevans
Done.
Please kindly review this.

@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.

[MED] Fused split spells still use front-half metadata in pre-payment live spell filters because the shared record helper keys on a marker that is set too late. Evidence: crates/engine/src/game/casting.rs:3673-3680 builds Fuse choices by preparing the variant and immediately calling can_cast_prepared_now, which runs cast prohibitions and per-turn limits at crates/engine/src/game/casting.rs:4078-4110; but the fused_split_spell marker that spell_cast_record() now relies on is not set until crates/engine/src/game/casting_costs.rs:5867-5876, inside finalization just before mana payment. The new tests manually set fused_split_spell (crates/engine/src/game/filter.rs:5828-5834, crates/engine/src/game/casting_tests.rs:462-470), so they do not exercise the production pre-payment path that decides whether the fused cast is legal or whether a cost modifier/prohibition applies. Why it matters: a Cmc/HasColor/ColorCount filter used by a cost modifier, cast prohibition, or per-turn limit can still evaluate a fused Breaking // Entering as the front half while the engine is deciding whether the Fuse option is available. Suggested fix: make the live spell-record projection take the selected CastingVariant (or otherwise bind the fuse state before all pre-payment legality/cost-modifier checks) and add a production-path test that casts via Fuse through a spell filter before finalization, without manually setting fused_split_spell.

…ll pre-payment spell filters (CR 202.3d/702.102b)

@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.

Thanks for the update. I think the fused projection is now threaded through the preparation/record seam, but there is still a candidate-enumeration issue that can expose the wrong cast path.

  • [MED] casting_variant_candidates computes is_fuse_capable once for any fuse-capable split card (crates/engine/src/game/casting.rs:3836) and then uses that combined projection for the separate hand alternative-cost candidate gates (crates/engine/src/game/casting.rs:3955, :4005, :4017, :4040, etc.). That means a CastWithKeyword { Dash }/Evoke/Overload/etc. grant whose filter matches only the fused combined characteristics can surface a non-Fuse CastingVariant::Dash/Evoke/Overload option. The later preparation path does not prepare those variants as fused: is_fuse_variant is only true for CastingVariant::Fuse (crates/engine/src/game/casting.rs:4118), and the alternative-cost readers for these variants use the non-fused effective_spell_keywords(...) path (crates/engine/src/game/casting.rs:4595, :4640, :4860). So the option is admitted by fused characteristics but then executes as a separate non-fused/front-half alternative-cost cast, potentially falling back to printed cost when the non-fused keyword read no longer finds the grant.

Please keep non-Fuse alternative-cost candidate discovery on the normal/front-half projection, and route the combined projection only through the actual CastingVariant::Fuse preparation/check path. I would also add regression coverage for both directions: a combined-only grant must not add a separate non-Fuse alternative-cost option, and a front-half-only grant must still be visible to the corresponding non-Fuse cast.

@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.

Thanks for the latest update. The earlier fused split blockers I had called out are mostly addressed, but there is still a fused-stack consumer using the raw front-half mana cost.

[MED] Fused split spells with cascade still cascade from the front-half mana value. Evidence: crates/engine/src/game/effects/cascade.rs:32 computes source_mv with obj.mana_cost.mana_value_with_x(obj.zone, obj.cost_x_paid), even though this PR adds GameObject::spell_mana_value() / spell_mana_value_for(...) for fused stack spells in crates/engine/src/game/game_object.rs. Why it matters: a fused Breaking // Entering-style spell that gains cascade has combined mana value 8, but this resolver would still seed the cascade threshold from the front half and carry that undercount into CastOfferKind::Cascade { source_mv }. Suggested fix: route the cascade source threshold through the fused-aware spell mana-value authority, and add a regression where a fused split spell granted cascade can hit a card whose MV is between the front-half MV and the combined MV.

@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.

[MED] Delayed "that spell's mana value" snapshots still use front-half mana value for fused split spells. Evidence: crates/engine/src/game/effects/delayed_trigger.rs:579. Why it matters: a fused split spell's SpellCast event is created after fused_split_spell is set, but this event-context snapshot bypasses spell_mana_value() and freezes the front half for delayed/reflexive effects that later use "that spell's mana value." Suggested fix: snapshot this branch through the fused-aware spell mana-value helper and add a fused split spell production-path delayed-trigger regression.

[MED] Damage source history still snapshots fused spell mana value/colors from the raw front-half object metadata. Evidence: crates/engine/src/game/effects/deal_damage.rs:737. Why it matters: damage-history source filters reconstruct a synthetic source from DamageRecord::source_mana_value/source_colors (crates/engine/src/game/filter.rs:1311), so mana-value/color-gated "damage dealt by a source" checks can misclassify damage from a fused split spell. Suggested fix: snapshot damage source mana value/colors through the same fused-aware/effective metadata helpers and cover a fused split damage source matched by a mana-value or color source filter.

…ayed-trigger and damage-source snapshots (CR 202.3d/702.102b)
@real-venus

Copy link
Copy Markdown
Contributor Author

@matthewevans
Done.
please check this.

@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.

Thanks for the follow-up. I re-reviewed the current head and there are still split-card off-stack consumers using a single face instead of the combined split-card characteristics.

  • crates/engine/src/database/search.rs still filters and reports split cards through one CardFace for mana value and color search. Off stack, split cards need to use the combined halves, so cmc_max and color filters can still include/exclude the wrong cards and report the wrong mana value.
  • crates/engine/src/game/companion.rs and crates/engine/src/game/deck_validation.rs still validate companion / deck legality using a half's mana value. That can accept or reject split cards incorrectly for companion restrictions and Tiny Leaders-style checks.

Please centralize a split-aware off-stack mana-value/color projection using the sibling face/oracle-id data, then route deck-builder search, CardSearchResult, companion validation, and deck validation through that helper. That keeps the rule fix at one seam instead of leaving parallel single-face paths behind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants