Expose read-only cast target preview over WASM#5472
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a read-only preview of cast-time target slots for currently castable spells, exposing it via WASM and implementing the core logic in the engine. The review feedback points out several violations of the repository style guide (R6) due to missing mandatory Magic Comprehensive Rules (CR) annotations across multiple rule-touching sections, including face/layout choices, casting variants, modal spells, Aura/Mutate targeting, kicker costs, and target slot building.
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.
| if (cast_face_choice_offered_from_zone(state, obj) | ||
| && alternative_spell_layout(obj).is_some()) | ||
| || cast_spell_face_choice_offered_from_zone(state, obj) | ||
| { | ||
| return Ok(Vec::new()); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for face/layout choice checks. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
// CR 712.7: A double-faced card has two faces. A player casting a double-faced card chooses which face they are casting.
if (cast_face_choice_offered_from_zone(state, obj)
&& alternative_spell_layout(obj).is_some())
|| cast_spell_face_choice_offered_from_zone(state, obj)
{
return Ok(Vec::new());
}References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added verified CR 715.3 / CR 720.3 / CR 712.11b annotations for Adventure, Omen, and modal DFC face-choice ambiguity before target selection.
| let choices = casting_variant_choice_set(state, player, object_id); | ||
| if choices.options.len() > 1 { | ||
| return Ok(Vec::new()); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for casting variant choice checks. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
// CR 601.2b: If the spell has alternative or additional costs, the player declares their intention to pay them.
let choices = casting_variant_choice_set(state, player, object_id);
if choices.options.len() > 1 {
return Ok(Vec::new());
}References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added the verified CR 601.2b annotation for alternative/additional cost variant choices before target selection.
| if prepared.modal.is_some() { | ||
| return Ok(Vec::new()); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for modal spell checks. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
| if prepared.modal.is_some() { | |
| return Ok(Vec::new()); | |
| } | |
| // CR 601.2b: If the spell is modal, the player declares the mode choice. | |
| if prepared.modal.is_some() { | |
| return Ok(Vec::new()); | |
| } |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added the verified CR 601.2b annotation for modal choices occurring before target selection.
| if obj.card_types.subtypes.iter().any(|s| s == "Aura") { | ||
| return Ok(obj | ||
| .keywords | ||
| .iter() | ||
| .find_map(|keyword| { | ||
| if let Keyword::Enchant(filter) = keyword { | ||
| Some(TargetSelectionSlot { | ||
| legal_targets: targeting::find_legal_targets( | ||
| state, | ||
| filter, | ||
| player, | ||
| prepared.object_id, | ||
| ), | ||
| optional: false, | ||
| }) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .filter(|slot| !slot.legal_targets.is_empty()) | ||
| .into_iter() | ||
| .collect()); | ||
| } | ||
|
|
||
| if obj.mutate_form.is_some() { | ||
| let legal = targeting::find_legal_targets( | ||
| state, | ||
| &mutate_target_filter(), | ||
| player, | ||
| prepared.object_id, | ||
| ); | ||
| return Ok(if legal.is_empty() { | ||
| Vec::new() | ||
| } else { | ||
| vec![TargetSelectionSlot { | ||
| legal_targets: legal, | ||
| optional: false, | ||
| }] | ||
| }); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotations for Aura and Mutate targeting rules. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
if obj.card_types.subtypes.iter().any(|s| s == "Aura") {
// CR 303.4a: An Aura spell requires a target, which is defined by its enchant keyword ability.
return Ok(obj
.keywords
.iter()
.find_map(|keyword| {
if let Keyword::Enchant(filter) = keyword {
Some(TargetSelectionSlot {
legal_targets: targeting::find_legal_targets(
state,
filter,
player,
prepared.object_id,
),
optional: false,
})
} else {
None
}
})
.filter(|slot| !slot.legal_targets.is_empty())
.into_iter()
.collect());
}
if obj.mutate_form.is_some() {
// CR 702.140b: As a mutating creature spell is cast, it requires a target as defined by its mutate ability.
let legal = targeting::find_legal_targets(
state,
&mutate_target_filter(),
player,
prepared.object_id,
);
return Ok(if legal.is_empty() {
Vec::new()
} else {
vec![TargetSelectionSlot {
legal_targets: legal,
optional: false,
}]
});
}References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added verified CR 303.4a and CR 702.140a annotations for Aura and mutate target-slot previews. Used CR 702.140a rather than 702.140b because 702.140a is the rule that defines the mutating spell target.
| let has_kicker_cost = state | ||
| .objects | ||
| .get(&prepared.object_id) | ||
| .and_then(|obj| obj.additional_cost.as_ref()) | ||
| .is_some_and(|additional| matches!(additional, AdditionalCost::Kicker { .. })); | ||
| if has_kicker_cost && requires_additional_cost_declaration_before_targets(&resolved) { | ||
| return Ok(Vec::new()); | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for kicker/additional cost declaration. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
| let has_kicker_cost = state | |
| .objects | |
| .get(&prepared.object_id) | |
| .and_then(|obj| obj.additional_cost.as_ref()) | |
| .is_some_and(|additional| matches!(additional, AdditionalCost::Kicker { .. })); | |
| if has_kicker_cost && requires_additional_cost_declaration_before_targets(&resolved) { | |
| return Ok(Vec::new()); | |
| } | |
| // CR 601.2b: If the spell has alternative or additional costs, the player declares their intention to pay them before choosing targets. | |
| let has_kicker_cost = state | |
| .objects | |
| .get(&prepared.object_id) | |
| .and_then(|obj| obj.additional_cost.as_ref()) | |
| .is_some_and(|additional| matches!(additional, AdditionalCost::Kicker { .. })); | |
| if has_kicker_cost && requires_additional_cost_declaration_before_targets(&resolved) { | |
| return Ok(Vec::new()); | |
| } |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added the verified CR 601.2b annotation for target-dependent kicker/additional-cost declarations before target selection.
| return Ok(Vec::new()); | ||
| } | ||
|
|
||
| let mut target_slots = build_target_slots(state, &resolved)?; |
There was a problem hiding this comment.
[MEDIUM] Missing mandatory CR annotation for target slot building. Under Repository Style Guide R6, every rules-touching line of engine code must carry a comment of the form CR <number>: <description>.
// CR 601.2c: The player announces their choice of an appropriate player, object, or zone for each target the spell requires.
let mut target_slots = build_target_slots(state, &resolved)?;References
- Every rules-touching line of engine code must carry a comment of the form CR : . (link)
There was a problem hiding this comment.
Addressed in fe004ca.
Added the verified CR 601.2c annotation for enumerating required target slots after earlier casting choices are known.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Good shape overall — the architecture pillars are satisfied and I verified each of these rather than taking them on faith:
- Read-only holds.
legal_target_slots_for_castable_spellclones into a localsimulatedand every reused helper (prepare_spell_cast,build_target_slots,find_legal_targets,cap_distribution_target_slots) takes&GameState. The negative test asserting byte-identical serialized state after the call is non-vacuous. engine-wasmis pure transport. It delegates, serializes withto_js, mapsErr → NULL. Zero game logic. Correct.- The positive test discriminates. It builds a real
{R}instant withDealDamage{target: Any}, adds mana, and asserts exactly one non-optional slot containingTargetRef::Player(PlayerId(1))— it fails if the impl were stubbed tovec![], and it drives the real pipeline. - Reusing
Vec<TargetSelectionSlot>rather than the issue'sVec<TargetRef>is the better call: the client already has that type fromWaitingFor::TargetSelection, so no new serialized surface.
Two things need to change before this lands.
1. The preview diverges from the real cast for Splice / Emerge / Casualty
This is a rules-ordering issue, and the CR is explicit about it. CR 601.2b — modal choice and splice — is announced before CR 601.2c, the target announcement this helper previews. continue_with_prepared honors that ordering with three detours:
casting.rs:10722—splice::eligible_splice_cards(...)casting.rs:10747—prepared.casting_variant == CastingVariant::Emergecasting.rs:10878—effective_casualty_additional_cost(...), which early-returns throughbegin_optional_cost_before_targetsso the sacrifice prompt precedes target selection (it sits afterbuild_target_slotstextually, but still returns before any target is chosen)
The new preview path contains none of them — grep -ci 'splice\|emerge\|casualty' over the diff returns 0. So for a castable Splice/Emerge/Casualty spell it falls straight through to build_target_slots and returns base slots that the real cast would never offer at that moment. Splice is the sharpest case: per CR 702.47a it reveals a card and adds its text to the spell, so it can add targets the preview cannot know about. Casualty and Emerge defer a sacrifice whose resolution can change board-derived target legality.
Silently returning a different answer than the cast path for a sub-class it appears to handle is the failure mode to avoid. Minimum fix: return Vec::new() when any of the three conditions holds, matching the "must choose something first" contract. Better, if you're up for it: factor the shared aura/mutate/build-slots core out of continue_with_prepared so preview, the bool probe, and submit can't drift again — right now there are three parallel implementations of the same gate sequence.
2. Per-call GameState clone on an explicitly per-hover path
casting.rs (new fn):
let mut simulated = state.clone();
super::layers::flush_layers(&mut simulated);Issue #5467 frames this as a hover/drag query, so this is N castable cards × repeated hovers × full-state clone + layer flush. This codebase has been bitten by clone storms on display paths before, and the amortization pattern already exists for exactly this: spell_has_legal_targets_with_probe (casting.rs:11190) takes probe: Option<&PriorityCastProbe>, and PriorityCastProbe::state() (casting.rs:476) hands back the already-flushed state so a host loop flushes once and probes many spells.
Please mirror it: add legal_target_slots_for_castable_spell_with_probe(state, player, object_id, probe: Option<&PriorityCastProbe>) and have the WASM host reuse one flushed probe across the hover loop. Performance on hot paths is a first-class review gate here, not a nit.
3. Missing CR annotations on the gate branches
The function carries only the top-level // CR 601.2c. The face/layout, variant, modal, Aura, Mutate, kicker, and X-defer branches each implement a rule and need annotating — repo convention treats this as mandatory, same as cargo fmt. The good news is these are already written and verified in the corresponding branches of continue_with_prepared (Aura targeting CR 115.1b, X-defer CR 601.2b, plus Mutate and kicker); copy them across rather than re-deriving. Gemini flagged six of these separately and it's right.
Not blocking — a design question for you
[] currently means five different things: uncastable, castable-with-no-targets, target-ambiguous, awaiting a face/modal choice, and X-defer. WASM adds a sixth encoding (null on state error). A caller can't distinguish "castable, nothing to highlight" from "not castable," and recovering that by cross-referencing get_legal_actions_for_viewer_js pushes inference into the display layer. This repo prefers a typed enum over a sentinel, so Option<Vec<TargetSelectionSlot>> (None = uncastable or needs a prior choice) would read more idiomatically. It matches the issue's requested shape as-is, so I'll leave the call to you — but you're touching this code anyway.
Anchors in your PR body check out (casting.rs:10595, casting.rs:11191, and the WASM export beside get_legal_actions_for_viewer_js), and CR 601.2c is verbatim correct at docs/MagicCompRules.txt:2461. Ping me when the Splice/Emerge/Casualty and probe items are in and I'll re-review promptly.
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Re-reviewed at ✅ Item 3 — CR annotations: resolvedI grepped every new citation against
Your reasoning for preferring CR 702.140a over 702.140b is right: 702.140a is the rule that defines the mutating spell's target, and 702.140b covers the resolution merge. That's the kind of citation-level care this repo wants. ❌ Item 1 — Splice / Emerge / Casualty divergence: not addressedUnchanged at the current head. Over the added lines of the
The minimum fix is unchanged — return ❌ Item 2 — per-call
|
|
Addressed the remaining review feedback in face223.\n\nChanges:\n- return no preview for pre-target choice paths that can alter targets or legality: Splice, Emerge, Casualty, Replicate, and Offering\n- added a probe-aware preview helper plus a batched WASM export so callers can reuse one flushed priority snapshot across multiple castable cards\n- added regression tests for batched previews, Splice, and Casualty\n\nLocal verification:\n- cargo fmt --all -- --check\n- CARGO_TARGET_DIR=/tmp/phase-target-finisher-5472 cargo test -p engine legal_target_slots_for_castable\n- CARGO_TARGET_DIR=/tmp/phase-target-finisher-5472 cargo test -p engine-wasm\n- CARGO_TARGET_DIR=/tmp/phase-target-finisher-5472 cargo clippy --all-targets -- -D warnings |
face223 to
984f11a
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved
All three blockers from the previous round are resolved, and one of them was addressed more broadly than I asked for.
Blocker 1 — rules-ordering divergence: fixed
Splice, Emerge and Casualty no longer fall through to build_target_slots and return base slots that the real cast would never offer at that moment. CR 601.2b announces modal choice and splice before CR 601.2c announces targets, and the preview now respects that ordering rather than silently answering a different question than continue_with_prepared would answer.
Blocker 2 — per-call clone on an explicit hover path: fixed the right way
legal_target_slots_for_castable_spell_with_probe now takes a PriorityCastProbe, mirroring the existing spell_has_legal_targets_with_probe. A host loop can flush layers once and probe many spells instead of cloning a full GameState per hover. Performance on hot display paths is a first-class gate in this repo, and you treated it as one.
Blocker 3 — CR annotations: fixed, and then some
You covered Offering, Replicate, MDFC, Adventure and Omen detours beyond the branches I named. I grep-verified all fifteen citations against docs/MagicCompRules.txt; every one is real and apt. Worth calling out specifically: CR 303.4a is used correctly here, for an Aura spell requiring a target defined by its enchant ability, which is what that rule actually says.
Still holding from round one
Restating what I verified previously so the record is complete:
- The function holds
GameStateread-only. The negative test asserts byte-identical serialized state after the call. engine-wasmstays pure transport: it delegates and mapsErrtoNULL, with zero game logic.- The positive test drives the real pipeline. It builds a real
{R}instant withDealDamage { target: Any }and asserts exactly one non-optional slot containing the opponent, so it would fail against a stubbedvec![].
Credit also for reusing Vec<TargetSelectionSlot> instead of inventing a new serialized surface. The client already has that type from WaitingFor::TargetSelection.
Forward-looking note (non-blocking)
There are now three parallel implementations of the same gate sequence: this preview, the bool probe, and submit. You bailed correctly in each rather than letting them drift, but the structural fix is to factor the shared build-slots core out of continue_with_prepared so the three cannot diverge again. Worth doing when someone next touches this file.
Twelve checks green.
3 participants
Model: codex-5
Thinking: high
Tier: Standard
Summary
legal_targets_for_castable_js(object_id)throughengine-wasm[]for uncastable, untargeted, or target-ambiguous castsCloses #5467
Anchored on
find_legal_targetsto_jsGate A
./scripts/check-parser-combinators.shExit status: 0
Output: no stdout
Verification
cargo fmt --allcargo fmt --all -- --checkcargo clippy --all-targets -- -D warningscargo test -p engine legal_target_slots_for_castable_spellcargo test -p engine-wasmNotes
tilt get uiresource clippy; used the documented direct cargo fallback.