From 740a3fe303229b5ac7a034fd1b0165439df13e62 Mon Sep 17 00:00:00 2001 From: Nicholas Tindle Date: Tue, 16 Jun 2026 05:29:35 -0500 Subject: [PATCH 1/2] =?UTF-8?q?fix(parser):=20UNSUPPORTED=20cluster:=20Vil?= =?UTF-8?q?lainous=20choice=20=E2=80=94=20each-opponent=20/=20each-target-?= =?UTF-8?q?controller=20two-mod?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../engine/src/game/effects/choose_one_of.rs | 252 ++++++++++++- crates/engine/src/game/static_abilities.rs | 8 + crates/engine/src/parser/oracle.rs | 32 ++ crates/engine/src/parser/oracle_classifier.rs | 10 + .../engine/src/parser/oracle_effect/lower.rs | 30 +- crates/engine/src/parser/oracle_effect/mod.rs | 334 +++++++++++++++++- .../src/parser/oracle_effect/sequence.rs | 34 ++ .../src/parser/oracle_static/dispatch.rs | 34 ++ crates/engine/src/types/statics.rs | 31 ++ 9 files changed, 732 insertions(+), 33 deletions(-) diff --git a/crates/engine/src/game/effects/choose_one_of.rs b/crates/engine/src/game/effects/choose_one_of.rs index 6317352cc8..cf0bde8f9c 100644 --- a/crates/engine/src/game/effects/choose_one_of.rs +++ b/crates/engine/src/game/effects/choose_one_of.rs @@ -170,16 +170,20 @@ fn choosing_players( let apnap = players::apnap_order(state); - // CR 608.2c + CR 108.3 + CR 109.4: Two chooser filters are anchored to + // CR 608.2c + CR 108.3 + CR 109.4: Three chooser filters are anchored to // resolution-scoped state that `matches_player_scope` cannot see (it carries // no `ResolvedAbility`): `ChosenPlayer` reads the player chosen earlier this - // resolution from `ability.chosen_players`, and `ParentObjectTargetOwner` - // reads the owner of the ability's first object target. Resolve them here — - // this is the one caller that has the ability in scope — and order the - // result in APNAP (CR 701.55d). Both filter out eliminated players (CR - // 104.3a — a player who loses leaves the game and can no longer be a - // chooser) and yield a single chooser, which is correct for the - // villainous-choice patterns these power (The Master, This Is How It Ends). + // resolution from `ability.chosen_players`; `ParentObjectTargetOwner` reads + // the owner of the ability's first object target (CR 108.3); and + // `ParentObjectTargetController` reads its controller (CR 109.4) — the chooser + // for "that creature's controller faces a villainous choice" (Hunted by The + // Family), where the targeted creature's controller (not owner) makes the + // choice and the two differ for a stolen creature. Resolve them here — this + // is the one caller that has the ability in scope — and order the result in + // APNAP (CR 701.55d). All filter out eliminated players (CR 104.3a — a player + // who loses leaves the game and can no longer be a chooser) and yield a + // single chooser, which is correct for the villainous-choice patterns these + // power (The Master, This Is How It Ends, Hunted by The Family). let anchored: Option = match chooser { PlayerFilter::ChosenPlayer { index } => { ability.chosen_players.get(*index as usize).copied() @@ -187,6 +191,9 @@ fn choosing_players( PlayerFilter::ParentObjectTargetOwner => { crate::game::ability_utils::parent_target_owner(ability, state) } + PlayerFilter::ParentObjectTargetController => { + crate::game::ability_utils::parent_target_controller(ability, state) + } _ => None, }; if let Some(player) = anchored { @@ -194,7 +201,8 @@ fn choosing_players( .players .iter() .any(|p| p.id == player && !p.is_eliminated); - return if alive { vec![player] } else { Vec::new() }; + let players = if alive { vec![player] } else { Vec::new() }; + return expand_extra_villainous_instances(state, players); } let targeted: Vec = ability @@ -216,13 +224,14 @@ fn choosing_players( .collect(); if !targeted.is_empty() { - return apnap + let players = apnap .into_iter() .filter(|player| targeted.contains(player)) .collect(); + return expand_extra_villainous_instances(state, players); } - apnap + let players = apnap .into_iter() .filter(|player| { super::matches_player_scope( @@ -233,7 +242,57 @@ fn choosing_players( ability.source_id, ) }) - .collect() + .collect(); + expand_extra_villainous_instances(state, players) +} + +/// CR 701.55c: Count the number of ADDITIONAL villainous-choice instances a +/// `facing` player must perform — one per active `GrantsExtraVillainousChoice` +/// static on a battlefield permanent controlled by an OPPONENT of that player +/// (The Valeyard — "If an opponent would face a villainous choice, they face +/// that choice an additional time."). Returns the additional count (default 0), +/// not `1 + count`: the base instance is already represented by the player's +/// single occurrence in the facing-player list. +/// +/// This is the controller-inverted mirror of `vote::votes_per_session_for` +/// (CR 701.38d), where the source is controlled by the voting player themselves; +/// here the source (the Valeyard) is controlled by the facing player's opponent. +fn villainous_extra_instances_for(state: &GameState, facing: PlayerId) -> u32 { + use crate::game::functioning_abilities::active_static_definitions; + use crate::types::statics::StaticMode; + + let mut extras: u32 = 0; + for &src_id in state.battlefield.iter() { + let Some(obj) = state.objects.get(&src_id) else { + continue; + }; + if !players::is_opponent(state, facing, obj.controller) { + continue; + } + for s in active_static_definitions(state, obj) { + if matches!(s.mode, StaticMode::GrantsExtraVillainousChoice) { + extras = extras.saturating_add(1); + } + } + } + extras +} + +/// CR 701.55c + CR 701.55d: Expand a facing-player list so each player appears +/// once per total instance of the villainous choice they must face — their base +/// occurrence plus `villainous_extra_instances_for` additional copies, inserted +/// consecutively so APNAP order (CR 701.55d) across distinct players is +/// preserved while each player resolves all of their instances one at a time +/// (CR 701.55c). +fn expand_extra_villainous_instances(state: &GameState, players: Vec) -> Vec { + let mut expanded = Vec::with_capacity(players.len()); + for p in players { + expanded.push(p); + for _ in 0..villainous_extra_instances_for(state, p) { + expanded.push(p); + } + } + expanded } fn branch_descriptions(branches: &[AbilityDefinition]) -> Vec { @@ -484,4 +543,173 @@ mod tests { other => panic!("expected ChooseOneOfBranch, got {other:?}"), } } + + /// CR 701.55c (cluster 32, Class D — The Valeyard): A + /// `GrantsExtraVillainousChoice` static on a battlefield permanent + /// controlled by an OPPONENT of the facing player makes that player face the + /// choice one additional time. The facing-player list expands so the player + /// appears twice consecutively (base + 1 extra); without the static they + /// appear exactly once. Tests the building block + /// (`expand_extra_villainous_instances`) via the live resolver, not a card. + #[test] + fn villainous_choice_doubled_when_opponent_controls_extra_instance_static() { + // Player 1 faces the choice; player 0 controls a Valeyard-like source. + let mut state = GameState::new(FormatConfig::commander(), 3, 42); + + let branch = AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ); + + // Sanity baseline: with no extra-instance source, player 1 faces the + // choice exactly once (no remaining players queued for a re-face). + let ability = ResolvedAbility::new( + Effect::ChooseOneOf { + chooser: PlayerFilter::ParentObjectTargetOwner, + branches: vec![branch.clone()], + }, + vec![TargetRef::Object(ObjectId(99))], + ObjectId(1), + PlayerId(0), + ); + // Bind the parent target to an object owned by player 1. + let obj_id = ObjectId(99); + let target_obj = crate::game::game_object::GameObject::new( + obj_id, + crate::types::identifiers::CardId(0), + PlayerId(1), + "Faced Creature".to_string(), + crate::types::zones::Zone::Battlefield, + ); + state.objects.insert(obj_id, target_obj); + + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::ChooseOneOfBranch { + player, + remaining_players, + .. + } => { + assert_eq!(*player, PlayerId(1)); + assert!( + remaining_players.is_empty(), + "without an extra-instance static the facing player faces the choice once" + ); + } + other => panic!("expected ChooseOneOfBranch, got {other:?}"), + } + + // Now add a Valeyard-like permanent controlled by player 0 (an opponent + // of the facing player 1) carrying GrantsExtraVillainousChoice. + let valeyard_id = ObjectId(50); + let mut valeyard = crate::game::game_object::GameObject::new( + valeyard_id, + crate::types::identifiers::CardId(1), + PlayerId(0), + "The Valeyard".to_string(), + crate::types::zones::Zone::Battlefield, + ); + valeyard.static_definitions.push( + crate::types::ability::StaticDefinition::new( + crate::types::statics::StaticMode::GrantsExtraVillainousChoice, + ) + .affected(TargetFilter::Player), + ); + state.objects.insert(valeyard_id, valeyard); + state.battlefield.push_back(valeyard_id); + + let mut events2 = Vec::new(); + resolve(&mut state, &ability, &mut events2).unwrap(); + match &state.waiting_for { + WaitingFor::ChooseOneOfBranch { + player, + remaining_players, + .. + } => { + assert_eq!(*player, PlayerId(1)); + // CR 701.55c: the same player faces the choice one more time, + // queued consecutively right after their first instance. + assert_eq!( + remaining_players.as_slice(), + &[PlayerId(1)], + "the facing player must face the choice twice (base + 1 extra)" + ); + } + other => panic!("expected ChooseOneOfBranch, got {other:?}"), + } + } + + /// CR 701.55c (cluster 32, Class D): an extra-instance source controlled by + /// the FACING player themselves (not an opponent) grants no extra instance — + /// the static reads "if an OPPONENT would face a villainous choice". Guards + /// the controller-inversion in `villainous_extra_instances_for`. + #[test] + fn villainous_choice_not_doubled_by_self_controlled_static() { + let mut state = GameState::new(FormatConfig::commander(), 3, 42); + + let obj_id = ObjectId(99); + let target_obj = crate::game::game_object::GameObject::new( + obj_id, + crate::types::identifiers::CardId(0), + PlayerId(1), + "Faced Creature".to_string(), + crate::types::zones::Zone::Battlefield, + ); + state.objects.insert(obj_id, target_obj); + + // Source controlled by player 1 (the facing player) — must NOT count. + let src_id = ObjectId(50); + let mut src = crate::game::game_object::GameObject::new( + src_id, + crate::types::identifiers::CardId(1), + PlayerId(1), + "Self Valeyard".to_string(), + crate::types::zones::Zone::Battlefield, + ); + src.static_definitions.push( + crate::types::ability::StaticDefinition::new( + crate::types::statics::StaticMode::GrantsExtraVillainousChoice, + ) + .affected(TargetFilter::Player), + ); + state.objects.insert(src_id, src); + state.battlefield.push_back(src_id); + + let branch = AbilityDefinition::new( + AbilityKind::Spell, + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + }, + ); + let ability = ResolvedAbility::new( + Effect::ChooseOneOf { + chooser: PlayerFilter::ParentObjectTargetOwner, + branches: vec![branch], + }, + vec![TargetRef::Object(obj_id)], + ObjectId(1), + PlayerId(0), + ); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + match &state.waiting_for { + WaitingFor::ChooseOneOfBranch { + player, + remaining_players, + .. + } => { + assert_eq!(*player, PlayerId(1)); + assert!( + remaining_players.is_empty(), + "a self-controlled extra-instance static must not double the choice" + ); + } + other => panic!("expected ChooseOneOfBranch, got {other:?}"), + } + } } diff --git a/crates/engine/src/game/static_abilities.rs b/crates/engine/src/game/static_abilities.rs index 923659a366..20539540d9 100644 --- a/crates/engine/src/game/static_abilities.rs +++ b/crates/engine/src/game/static_abilities.rs @@ -298,6 +298,14 @@ pub fn build_static_registry() -> HashMap { // scans active_static_definitions at vote-session start. No continuous-effect // plumbing needed; registered here so coverage marks the card as supported. registry.insert(StaticMode::GrantsExtraVote, handle_rule_mod); + // CR 701.55c: GrantsExtraVillainousChoice — "If an opponent would face a + // villainous choice, they face that choice an additional time." (The + // Valeyard). Runtime enforcement is in + // game/effects/choose_one_of.rs::villainous_extra_instances_for(), which + // scans active_static_definitions when assembling the facing-player list. No + // continuous-effect plumbing needed; registered here so coverage marks the + // card as supported. + registry.insert(StaticMode::GrantsExtraVillainousChoice, handle_rule_mod); // No generic `StaticMode::Other(...)` stubs are currently needed. // diff --git a/crates/engine/src/parser/oracle.rs b/crates/engine/src/parser/oracle.rs index 35c7a24b91..b5c16a366a 100644 --- a/crates/engine/src/parser/oracle.rs +++ b/crates/engine/src/parser/oracle.rs @@ -5493,6 +5493,38 @@ mod tests { assert!(matches!(*r.abilities[0].effect, Effect::DealDamage { .. })); } + /// CR 701.55c (cluster 32, Class D — The Valeyard): "If an opponent would + /// face a villainous choice, they face that choice an additional time." leads + /// with "if …" and contains "would ", so without the classifier redirect it + /// is classified as a replacement and falls through to an + /// `Unimplemented{name:"replacement_structure"}`. The + /// `is_static_compound_pattern` gate must route it to Priority 7 static + /// dispatch, which lowers it to `StaticMode::GrantsExtraVillainousChoice`. + /// Tests the classifier+dispatch building blocks, asserting NO Unimplemented. + #[test] + fn valeyard_grants_extra_villainous_choice_static() { + let r = parse( + "If an opponent would face a villainous choice, they face that choice an additional time. (They can make the same or different choices.)", + "The Valeyard", + &[], + &["Legendary", "Creature"], + &[], + ); + + assert_eq!( + r.statics.len(), + 1, + "expected one extra-villainous-choice static, got {r:#?}" + ); + assert_eq!(r.statics[0].mode, StaticMode::GrantsExtraVillainousChoice); + assert!( + r.abilities + .iter() + .all(|a| !matches!(*a.effect, Effect::Unimplemented { .. })), + "the Valeyard line must not produce an Unimplemented effect, got {r:#?}" + ); + } + #[test] fn mindlock_orb_routes_to_static_search_prohibition() { let r = parse( diff --git a/crates/engine/src/parser/oracle_classifier.rs b/crates/engine/src/parser/oracle_classifier.rs index ed85373fcc..1b929e33e5 100644 --- a/crates/engine/src/parser/oracle_classifier.rs +++ b/crates/engine/src/parser/oracle_classifier.rs @@ -520,6 +520,16 @@ fn is_static_compound_pattern(lower: &str) -> bool { { return true; } + // CR 701.55c: "If an opponent would face a villainous choice, they face that + // choice an additional time." (The Valeyard) leads with "if …" and contains + // "would ", so it is otherwise classified as a replacement and never reaches + // the static parser. It is in fact an extra-instance rule-modifying static + // (`StaticMode::GrantsExtraVillainousChoice`, the CR 701.55c twin of + // `GrantsExtraVote`). Route it to Priority 7 static dispatch — which runs + // before the Priority 8 replacement gate — so it lowers to the static. + if scan_contains(lower, "face a villainous choice") && scan_contains(lower, "additional time") { + return true; + } false } diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 896f11c826..9f9efbb653 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -4214,15 +4214,33 @@ pub(super) fn try_parse_damage_with_remainder<'a>( // CDA quantity parser (`the number of … you control`, `your life total`, // …). Without this fallback the phrase degrades to a raw `Variable`, which // resolves to 0 at runtime — the damage silently no-ops. - let qty = crate::parser::oracle_quantity::parse_event_context_quantity(qty_text) - .or_else(|| { + let qty = + crate::parser::oracle_quantity::parse_event_context_quantity(qty_text).or_else(|| { crate::parser::oracle_quantity::parse_cda_quantity_with_context(qty_text, ctx) - }) - .unwrap_or_else(|| QuantityExpr::Ref { + }); + let qty = match qty { + Some(qty) => qty, + // CR 120.1 + CR 202.3: The typed quantity parsers declined this + // amount. Only the spell variable "X" resolves through the + // `Variable` runtime path (`quantity.rs` — `name == "X"`, or a named + // choice); any OTHER unrecognized phrase ("the total mana value of + // those exiled cards", Ensnared by the Mara) would be stored + // verbatim and silently resolve to 0 damage. Storing raw Oracle text + // as a `Variable` name is the prohibited verbatim-text-in-parser + // smell, so strict-fail instead: return `None` here, letting the + // effect lower to `Effect::Unimplemented` so coverage honestly flags + // the branch as unsupported rather than dealing the wrong (zero) + // amount. Reaching a resolvable model ("those exiled cards" as a + // typed exiled-this-resolution mana-value aggregate) is a future + // building block; until then coverage waits on the strict-failure + // tag rather than masking the gap. + None if qty_text.eq_ignore_ascii_case("x") => QuantityExpr::Ref { qty: QuantityRef::Variable { - name: qty_text.to_string(), + name: "X".to_string(), }, - }); + }, + None => return None, + }; (qty, &amount_text[before_to.len() + 4..]) } else { return None; diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index b8a8a70f1d..d4339672d6 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -2963,45 +2963,62 @@ fn try_parse_choose_one_of_inline( // failed trial pollutes the committed warnings buffer with spurious // gaps from a malformed branch (e.g., "return a red" left half from // splitting "return a red or green creature" at the wrong " or "). + // CR 701.55a: each chosen option performs ALL of its instructions, so a + // branch body may itself be a multi-step chain ("They exile cards ... until + // they exile a nonland card, then you may cast that card", "discards all the + // cards in their hand, then draws that many cards minus one"). Parse each + // half with `parse_effect_chain_with_context` — the same chain builder used + // for top-level effect sequences — so "then"/","-chained second steps are + // lowered into `sub_ability` links instead of being silently dropped by the + // single-clause `parse_effect_clause`. `build_resolved_from_def` consumes the + // chained `AbilityDefinition` via `resolve_ability_chain` at resolution. let diagnostics_snapshot = ctx.diagnostics.len(); - let left_clause; - let right_clause; + let mut left_def; + let mut right_def; if scoped_choice_player { let mut branch_ctx = ctx.clone(); branch_ctx.relative_player_scope = Some(ControllerRef::ScopedPlayer); - left_clause = parse_effect_clause(left_orig, &mut branch_ctx); - right_clause = parse_effect_clause(right_orig, &mut branch_ctx); + left_def = parse_effect_chain_with_context(left_orig, AbilityKind::Spell, &mut branch_ctx); + right_def = + parse_effect_chain_with_context(right_orig, AbilityKind::Spell, &mut branch_ctx); ctx.diagnostics = branch_ctx.diagnostics; } else { - left_clause = parse_effect_clause(left_orig, ctx); - right_clause = parse_effect_clause(right_orig, ctx); + left_def = parse_effect_chain_with_context(left_orig, AbilityKind::Spell, ctx); + right_def = parse_effect_chain_with_context(right_orig, AbilityKind::Spell, ctx); } - // Reject unless BOTH branches produce non-Unimplemented effects. This + // Reject unless BOTH branches produce a non-Unimplemented HEAD effect. This // prevents false positives on noun-phrase disjunctions and "and/or" - // coordinations inside larger imperatives. - if matches!(left_clause.effect, Effect::Unimplemented { .. }) - || matches!(right_clause.effect, Effect::Unimplemented { .. }) + // coordinations inside larger imperatives. Inspect the chain head — a + // multi-step branch's first effect — not a single flattened clause. + if matches!(*left_def.effect, Effect::Unimplemented { .. }) + || matches!(*right_def.effect, Effect::Unimplemented { .. }) { ctx.diagnostics.truncate(diagnostics_snapshot); return None; } - // Also reject if either branch is `TargetOnly` — that's a structural + // Also reject if either branch HEAD is `TargetOnly` — that's a structural // wrapper, not a terminal effect. A real binary choice needs two // executable branches. - if matches!(left_clause.effect, Effect::TargetOnly { .. }) - || matches!(right_clause.effect, Effect::TargetOnly { .. }) + if matches!(*left_def.effect, Effect::TargetOnly { .. }) + || matches!(*right_def.effect, Effect::TargetOnly { .. }) { ctx.diagnostics.truncate(diagnostics_snapshot); return None; } - let mut left_def = ability_definition_from_clause(AbilityKind::Spell, left_clause); left_def.description = Some(left_orig.to_string()); - let mut right_def = ability_definition_from_clause(AbilityKind::Spell, right_clause); right_def.description = Some(right_orig.to_string()); + // CR 608.2c + CR 701.55a: a branch that opens with an anaphoric subject + // ("That creature becomes a 1/1 …", Hunted by The Family) refers to the + // per-target object chosen by the parent spell, not a fresh broadcast set. + // Rebind the becomes/animate `GenericEffect` subject to `ParentTarget` so + // the type-change binds only to the chosen creature at resolution. + rebind_anaphoric_generic_effect_subject_to_parent(&left_orig.to_lowercase(), &mut left_def); + rebind_anaphoric_generic_effect_subject_to_parent(&right_orig.to_lowercase(), &mut right_def); + Some(parsed_clause(Effect::ChooseOneOf { chooser, branches: vec![left_def, right_def], @@ -3010,6 +3027,17 @@ fn try_parse_choose_one_of_inline( fn parse_villainous_choice_chooser_prefix(input: &str) -> OracleResult<'_, PlayerFilter> { alt(( + // CR 109.4 + CR 701.55a: "that creature's controller faces a villainous + // choice — …" (Hunted by The Family) — the chooser is the CONTROLLER of + // the per-target creature (CR 109.4), which can differ from its owner for + // a stolen creature, so this is `ParentObjectTargetController`, not + // `ParentObjectTargetOwner`. Placed first: it is a strict superstring of + // the bare "faces a villainous choice — " arm below, so longest-literal + // ordering is load-bearing. + value( + PlayerFilter::ParentObjectTargetController, + tag("that creature's controller faces a villainous choice — "), + ), value( PlayerFilter::DefendingPlayer, tag("defending player faces a villainous choice — "), @@ -10809,6 +10837,67 @@ fn replace_fight_subject_with_parent_if_anaphoric_subject( } } +/// CR 608.2c + CR 701.55a: Rebind an anaphoric "That creature"/"That +/// permanent"/"It" subject in a villainous-choice (or other inline-choice) +/// branch back to the spell's parent target. +/// +/// A branch such as "That creature becomes a 1/1 white Human creature and loses +/// all abilities" (Hunted by The Family) lowers its type-change to an +/// `Effect::GenericEffect` whose inner `StaticDefinition.affected` is the +/// broadcast `Typed(Creature)` filter the static-ability parser emits for an +/// implicit "a creature" subject. But the anaphoric "That creature" names the +/// per-target creature chosen by the parent spell (CR 608.2c — read the whole +/// text), so the type-change must bind to that one object, not fan out to every +/// creature on the battlefield. Rebind the outer `target` and each inner +/// `affected` to `ParentTarget` so the runtime `generic_effect_application_filter` +/// inherits the parent's chosen target (CR 611.2c) instead of the broadcast +/// filter. +/// +/// Sibling of `replace_fight_subject_with_parent_if_anaphoric_subject`: same +/// anaphoric-subject detection, but for the becomes/animate `GenericEffect` +/// family. Only fires when the branch text begins with the anaphoric subject, +/// so non-anaphoric branches ("you create a token …") are untouched. +fn rebind_anaphoric_generic_effect_subject_to_parent(lower: &str, def: &mut AbilityDefinition) { + let is_anaphoric_subject = alt(( + tag::<_, _, OracleError<'_>>("that creature "), + tag("that permanent "), + tag("it "), + )) + .parse(lower) + .is_ok(); + if !is_anaphoric_subject { + return; + } + + if let Effect::GenericEffect { + target, + static_abilities, + .. + } = &mut *def.effect + { + if target.is_none() { + *target = Some(TargetFilter::ParentTarget); + } + for static_def in static_abilities { + // Only rebind the broadcast/self subject the parser emits for an + // implicit subject. A `ParentTarget`/`ControllerRef`-bearing + // `affected` is already correctly anchored; leave deliberate + // controller-scoped filters intact. + let is_broadcast_subject = matches!( + static_def.affected, + None | Some(TargetFilter::SelfRef) + | Some(TargetFilter::Typed(TypedFilter { + controller: None, + .. + })) + ); + if is_broadcast_subject { + static_def.affected = Some(TargetFilter::ParentTarget); + } + } + } +} + /// Check if an effect has a `Typed(...)` target filter (not SelfRef/ParentTarget/Any). /// Used to guard anaphoric replacement scope — prevents false positives when a /// pronoun clause follows a conditional effect without a typed target. @@ -49578,6 +49667,221 @@ mod tests { typed ); } + + /// CR 701.55a (cluster 32, Class A+B — Ensnared by the Mara): a + /// villainous-choice branch body that is itself a multi-step chain ("They + /// exile cards ... until they exile a nonland card, then you may cast that + /// card ...") must survive as a chained `AbilityDefinition` (head + + /// `sub_ability`), not be bisected by the clause chunker into a failing + /// `Unimplemented{name:"face"}`. Tests the building blocks (sticky choice + /// block + chain-parsed branches), not the card name. + #[test] + fn villainous_choice_keeps_multistep_first_branch() { + let ability = parse_effect_chain( + "Each opponent faces a villainous choice — They exile cards from the top of their library until they exile a nonland card, then you may cast that card without paying its mana cost, or that player exiles the top four cards of their library and ~ deals damage equal to the total mana value of those exiled cards to that player.", + AbilityKind::Spell, + ); + + let Effect::ChooseOneOf { chooser, branches } = &*ability.effect else { + panic!("expected ChooseOneOf, got {:?}", ability.effect); + }; + assert_eq!(*chooser, PlayerFilter::Opponent); + assert_eq!(branches.len(), 2); + // Branch 0 head + chained "then you may cast" second step preserved. + assert!( + matches!(&*branches[0].effect, Effect::ExileFromTopUntil { .. }), + "branch 0 head must be ExileFromTopUntil, got {:?}", + branches[0].effect + ); + assert!( + branches[0].sub_ability.is_some(), + "branch 0 must retain its 'then you may cast' chained second step" + ); + // No Unimplemented anywhere in either branch chain. + for b in branches { + assert!( + !matches!(&*b.effect, Effect::Unimplemented { .. }), + "no branch head may be Unimplemented, got {:?}", + b.effect + ); + } + } + + /// CR 701.55a (cluster 32, Class A+B — Sycorax Commander): the first branch + /// "discards all the cards in their hand, then draws that many cards minus + /// one" must stay a chained `Discard -> Draw` body. Before the fix the + /// chunker severed it at ", then" and the lead-in became `Unimplemented`. + #[test] + fn villainous_choice_keeps_discard_then_draw_branch() { + let ability = parse_effect_chain( + "Each opponent faces a villainous choice — That opponent discards all the cards in their hand, then draws that many cards minus one, or this creature deals damage to that player equal to the number of cards in their hand.", + AbilityKind::Spell, + ); + + let Effect::ChooseOneOf { chooser, branches } = &*ability.effect else { + panic!("expected ChooseOneOf, got {:?}", ability.effect); + }; + assert_eq!(*chooser, PlayerFilter::Opponent); + assert_eq!(branches.len(), 2); + assert!( + matches!(&*branches[0].effect, Effect::Discard { .. }), + "branch 0 head must be Discard, got {:?}", + branches[0].effect + ); + assert!( + branches[0].sub_ability.is_some(), + "branch 0 must retain its 'then draws that many cards minus one' chained step" + ); + assert!( + matches!(&*branches[1].effect, Effect::DealDamage { .. }), + "branch 1 head must be DealDamage, got {:?}", + branches[1].effect + ); + } + + /// CR 109.4 + CR 701.55a (cluster 32, Class C — Hunted by The Family): "that + /// creature's controller faces a villainous choice — …" anchors the chooser + /// to the controller of the per-target creature + /// (`PlayerFilter::ParentObjectTargetController`), distinct from its owner + /// for a stolen creature. Parser-only: the runtime resolution already exists + /// in `choosing_players`. + #[test] + fn that_creatures_controller_faces_villainous_choice_uses_parent_controller_chooser() { + let ability = parse_effect_chain( + "That creature's controller faces a villainous choice — That creature becomes a 1/1 white Human creature and loses all abilities, or you create a token that's a copy of it.", + AbilityKind::Spell, + ); + + let Effect::ChooseOneOf { chooser, branches } = &*ability.effect else { + panic!("expected ChooseOneOf, got {:?}", ability.effect); + }; + assert_eq!( + *chooser, + PlayerFilter::ParentObjectTargetController, + "chooser for \"that creature's controller\" must be ParentObjectTargetController" + ); + assert_eq!(branches.len(), 2); + for b in branches { + assert!( + !matches!(&*b.effect, Effect::Unimplemented { .. }), + "no branch head may be Unimplemented, got {:?}", + b.effect + ); + } + + // CR 608.2c + CR 611.2c: branch 0 ("That creature becomes a 1/1 white + // Human creature and loses all abilities") is anaphoric — the + // type-change must bind to the parent spell's chosen target, NOT fan out + // to every creature on the battlefield. Both the outer `target` and each + // inner static `affected` must be `ParentTarget`. + let Effect::GenericEffect { + target, + static_abilities, + .. + } = &*branches[0].effect + else { + panic!( + "branch 0 must be GenericEffect, got {:?}", + branches[0].effect + ); + }; + assert_eq!( + *target, + Some(TargetFilter::ParentTarget), + "branch 0 GenericEffect.target must be ParentTarget (the chosen creature), got {target:?}" + ); + for static_def in static_abilities { + assert_eq!( + static_def.affected, + Some(TargetFilter::ParentTarget), + "branch 0 static_def.affected must be ParentTarget, not a broadcast creature filter, got {:?}", + static_def.affected + ); + } + } + + /// CR 120.1 + CR 202.3 (cluster 32, Class C — Ensnared by the Mara): the + /// branch-1 damage amount "the total mana value of those exiled cards" is an + /// exiled-this-resolution aggregate the typed quantity parsers don't yet + /// model. It must NOT degrade to a verbatim `QuantityRef::Variable` carrying + /// raw Oracle text — that would silently resolve to 0 damage at runtime + /// (only `Variable{name:"X"}` / named choices resolve). Instead the + /// `DealDamage` step must strict-fail to `Effect::Unimplemented` so coverage + /// honestly flags the gap. The `ExileTop` branch head still parses, keeping + /// the two-branch `ChooseOneOf` intact. + #[test] + fn ensnared_by_the_mara_unresolvable_damage_amount_strict_fails_not_verbatim_variable() { + let ability = parse_effect_chain( + "Each opponent faces a villainous choice — They exile cards from the top of their library until they exile a nonland card, then you may cast that card without paying its mana cost, or that player exiles the top four cards of their library and ~ deals damage equal to the total mana value of those exiled cards to that player.", + AbilityKind::Spell, + ); + + let Effect::ChooseOneOf { chooser, branches } = &*ability.effect else { + panic!("expected ChooseOneOf, got {:?}", ability.effect); + }; + assert_eq!(*chooser, PlayerFilter::Opponent); + assert_eq!(branches.len(), 2); + + // Branch 1 head exiles four cards (still parses); the chained damage + // step strict-fails rather than emitting a verbatim Variable. + let branch1 = &branches[1]; + assert!( + matches!(&*branch1.effect, Effect::ExileTop { .. }), + "branch 1 head must be ExileTop, got {:?}", + branch1.effect + ); + let damage = branch1 + .sub_ability + .as_ref() + .expect("branch 1 must retain its chained damage step"); + assert!( + matches!(&*damage.effect, Effect::Unimplemented { .. }), + "branch 1 damage step must strict-fail to Unimplemented (unresolvable exiled-cards aggregate), got {:?}", + damage.effect + ); + + // Belt-and-braces: no node anywhere in the parsed ability stores the raw + // Oracle aggregate phrase as a `QuantityRef::Variable` name (the + // prohibited verbatim-text-in-parser smell). The phrase legitimately + // survives in human-readable `description`/Unimplemented-trace fields — + // only its appearance as a resolvable `Variable` payload is the defect. + let json = serde_json::to_string(&ability).expect("serialize ability"); + assert!( + !json.contains(r#""Variable","name":"the total mana value"#), + "the verbatim aggregate phrase must not be stored as a QuantityRef::Variable name" + ); + } + + /// Regression pin (cluster 32 — Missy): a single-effect-branch villainous + /// choice ("Each artifact creature you control deals 1 damage to that + /// opponent, or you draw a card") must remain a clean two-branch + /// `ChooseOneOf{chooser: Opponent}`. Guards the Class A sticky latch against + /// over-capturing or bisecting non-multistep choices. + #[test] + fn missy_endstep_villainous_choice_unchanged() { + let ability = parse_effect_chain( + "Each opponent faces a villainous choice — Each artifact creature you control deals 1 damage to that opponent, or you draw a card.", + AbilityKind::Spell, + ); + + let Effect::ChooseOneOf { chooser, branches } = &*ability.effect else { + panic!("expected ChooseOneOf, got {:?}", ability.effect); + }; + assert_eq!(*chooser, PlayerFilter::Opponent); + assert_eq!(branches.len(), 2); + assert!( + matches!(&*branches[1].effect, Effect::Draw { .. }), + "branch 1 must be Draw, got {:?}", + branches[1].effect + ); + for b in branches { + assert!( + !matches!(&*b.effect, Effect::Unimplemented { .. }), + "no branch head may be Unimplemented, got {:?}", + b.effect + ); + } + } } /// Snapshot tests locking current `parse_effect_chain` behavior before the diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 2b5dbfe5c0..6f6663b6da 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -515,6 +515,15 @@ pub(super) fn split_clause_sequence(text: &str) -> Vec { // ("get +2/+0 and gain haste ... and attack this turn if able") are body // delimiters owned by `try_parse_compound_subject_each`, not clause splits. let mut compound_subject_each_sticky = false; + // CR 701.55a + CR 701.55d: once a villainous-choice head ("[subject] face(s) + // a villainous choice — ") is detected, keep the WHOLE choice block intact + // for the rest of the current sentence. Everything after the em-dash is one + // indivisible instruction whose internal "," / " then " / " and " / " or " + // are branch delimiters owned by `try_parse_choose_one_of_inline`, not clause + // splits. Without this latch the chunker bisects a branch body (e.g. Ensnared + // by the Mara: "... until they exile a nonland card, then you may cast that + // card ...") and the lead-in is severed into a failing `Unimplemented{face}`. + let mut villainous_choice_sticky = false; while let Some(ch) = chars.next() { match ch { @@ -548,6 +557,15 @@ pub(super) fn split_clause_sequence(text: &str) -> Vec { } } } + ',' if paren_depth == 0 + && !in_single_quote + && !in_double_quote + // CR 701.55a: inside a latched villainous-choice block, "," is a + // branch delimiter (", then" / ", or"), not a clause boundary. + && villainous_choice_sticky => + { + current.push(ch); + } ',' if paren_depth == 0 && !in_single_quote && !in_double_quote => { let remainder = chars.clone().collect::(); if let Some((boundary, chars_to_skip)) = @@ -567,12 +585,27 @@ pub(super) fn split_clause_sequence(text: &str) -> Vec { push_clause_chunk(&mut chunks, ¤t, Some(ClauseBoundary::Sentence)); current.clear(); compound_subject_each_sticky = false; + // CR 701.55a: a true sentence boundary ends the choice block. + villainous_choice_sticky = false; while matches!(chars.peek(), Some(c) if c.is_whitespace()) { chars.next(); } } _ => { current.push(ch); + // CR 701.55a: latch the villainous-choice block once the chunk + // accumulated so far contains the choice opener "a villainous + // choice — " (em-dash + trailing space). Anchored on the full + // opener so it covers "face"/"faces" and any chooser prefix + // ("that creature's controller faces a villainous choice — "). + if !villainous_choice_sticky + && nom_primitives::scan_contains( + ¤t.to_ascii_lowercase(), + "a villainous choice \u{2014} ", + ) + { + villainous_choice_sticky = true; + } // Detect bare " and " at word boundary followed by an imperative verb. // Handles patterns like "you lose 1 life and create a Treasure token". // Uses a restricted verb list to avoid false positives on noun phrases @@ -782,6 +815,7 @@ pub(super) fn split_clause_sequence(text: &str) -> Vec { || choice_partition_remainder || compound_subject_each || compound_subject_each_sticky // CR 109.5 + CR 115.1: keep the whole compound-subject body intact + || villainous_choice_sticky // CR 701.55a: keep the whole villainous-choice block intact || inside_otherwise_body || have_base_pt_continuation || continuous_modifier_conjunct diff --git a/crates/engine/src/parser/oracle_static/dispatch.rs b/crates/engine/src/parser/oracle_static/dispatch.rs index 418e471d8b..363323ed69 100644 --- a/crates/engine/src/parser/oracle_static/dispatch.rs +++ b/crates/engine/src/parser/oracle_static/dispatch.rs @@ -417,6 +417,40 @@ pub(crate) fn parse_static_line_inner( } } + // CR 701.55c: "If an opponent would face a villainous choice, they face that + // choice an additional time." (The Valeyard) — an extra-instance replacement + // static, the structural twin of `GrantsExtraVote` (CR 701.38d). `affected` + // is `Player` (mirroring `GrantsExtraVote`); the opponent-of-the-facing- + // player scoping is owned by the resolver + // (`choose_one_of::villainous_extra_instances_for`), which counts only + // sources controlled by an opponent of the facing player — `affected` here is + // a coverage/semantic marker, not the scope authority. Reminder text "(They + // can make the same or different choices.)" is already stripped above by + // `strip_reminder_text`. Comma/no-comma variants are alt arms, not flat + // full-sentence equality, matching the GrantsExtraVote pattern. + { + let lower_trim = tp.lower.trim_end_matches('.').trim(); + let res: nom::IResult<&str, (), OracleError<'_>> = nom::combinator::value( + (), + nom::branch::alt(( + nom::bytes::complete::tag( + "if an opponent would face a villainous choice, they face that choice an additional time", + ), + nom::bytes::complete::tag( + "if an opponent would face a villainous choice they face that choice an additional time", + ), + )), + ) + .parse(lower_trim); + if res.is_ok() { + return Some( + StaticDefinition::new(StaticMode::GrantsExtraVillainousChoice) + .affected(TargetFilter::Player) + .description(text.to_string()), + ); + } + } + // CR 401.5 + CR 118.9 + CR 601.2a: "You may [play|cast] [filter] from the // top of your library [rider]." Top-of-library cast permission class // (Realmwalker, Future Sight, Bolas's Citadel, Magus of the Future, Vivien diff --git a/crates/engine/src/types/statics.rs b/crates/engine/src/types/statics.rs index d8a1c64d44..31928b7d3d 100644 --- a/crates/engine/src/types/statics.rs +++ b/crates/engine/src/types/statics.rs @@ -692,6 +692,19 @@ pub enum StaticMode { /// layer 7 — there is no continuous P/T or keyword grant; the static is a /// pure "session-start +1 votes" signal. GrantsExtraVote, + /// CR 701.55c: A replacement effect causes an opponent who would face a + /// villainous choice to face that choice some number of additional times + /// (The Valeyard — "If an opponent would face a villainous choice, they face + /// that choice an additional time."). Each active source controlled by an + /// opponent of the facing player adds +1 additional villainous-choice + /// instance for that player; the whole CR 701.55a process is then performed + /// that many additional times, one at a time (CR 701.55c). Parallel to + /// `GrantsExtraVote` (CR 701.38d), but counts a different keyword action read + /// by a different resolver (`choose_one_of`), so the two are not unifiable + /// (categorical-boundary rule: CR 701.55 vs CR 701.38 are separate sections). + /// Like `GrantsExtraVote`, it does not feed into layer 7 — there is no + /// continuous P/T or keyword grant. + GrantsExtraVillainousChoice, /// CR 702.51a: Grants a keyword to spells during casting. /// Generalized version of CastWithFlash — the `spell_filter` on the StaticDefinition /// determines which spells are affected (e.g., "Creature spells you cast have convoke"). @@ -1571,6 +1584,7 @@ impl StaticMode { | StaticMode::CantCauseSacrificeOrExile { .. } | StaticMode::CastWithFlash | StaticMode::GrantsExtraVote + | StaticMode::GrantsExtraVillainousChoice | StaticMode::CastWithKeyword { .. } | StaticMode::CastWithAlternativeCost { .. } | StaticMode::AlternativeKeywordCost { .. } @@ -1681,6 +1695,9 @@ impl fmt::Display for StaticMode { } StaticMode::CastWithFlash => write!(f, "CastWithFlash"), StaticMode::GrantsExtraVote => write!(f, "GrantsExtraVote"), + StaticMode::GrantsExtraVillainousChoice => { + write!(f, "GrantsExtraVillainousChoice") + } StaticMode::CastWithKeyword { keyword } => { write!(f, "CastWithKeyword({keyword:?})") } @@ -2276,6 +2293,9 @@ impl FromStr for StaticMode { } // CR 701.38d: "While voting, you may vote an additional time." "GrantsExtraVote" => StaticMode::GrantsExtraVote, + // CR 701.55c: "If an opponent would face a villainous choice, they + // face that choice an additional time." (The Valeyard) + "GrantsExtraVillainousChoice" => StaticMode::GrantsExtraVillainousChoice, // Parameterized other => { if let Some(inner) = other @@ -2651,6 +2671,17 @@ mod tests { StaticMode::from_str("IgnoreHexproof").unwrap(), StaticMode::IgnoreHexproof ); + // CR 701.55c: GrantsExtraVillainousChoice (The Valeyard) must round-trip + // through Display/FromStr so card data persists the variant across the + // WASM serde boundary, mirroring its CR 701.38d twin GrantsExtraVote. + assert_eq!( + StaticMode::from_str("GrantsExtraVillainousChoice").unwrap(), + StaticMode::GrantsExtraVillainousChoice + ); + assert_eq!( + StaticMode::GrantsExtraVillainousChoice.to_string(), + "GrantsExtraVillainousChoice" + ); } #[test] From f6814312f1c6acfe374f178fe6b1b028f0b15492 Mon Sep 17 00:00:00 2001 From: Matt Evans Date: Wed, 17 Jun 2026 00:33:17 -0700 Subject: [PATCH 2/2] fix(PR-3489): address Gemini R1 + rebase onto current main - oracle_static/dispatch.rs: compose the optional comma in the GrantsExtraVillainousChoice and GrantsExtraVote phrase recognizers as a single `opt(tag(","))` axis instead of two flat full-sentence `tag()` permutations, per CLAUDE.md "compose combinators, don't enumerate permutations" (Gemini R1). Applied to both twins so they stay consistent. - Merged current origin/main (resolved the oracle_effect/mod.rs conflicts by keeping both sides' independent helpers and tests; #3419/#3461 etc. have since landed on main). Verification: fmt + parser gate clean; clippy -p engine --all-targets -D warnings clean; cargo test -p engine green (0 failed), incl. valeyard_grants_extra_villainous_choice_static. --- .../src/parser/oracle_static/dispatch.rs | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/parser/oracle_static/dispatch.rs b/crates/engine/src/parser/oracle_static/dispatch.rs index 79e612cac1..53716ceb45 100644 --- a/crates/engine/src/parser/oracle_static/dispatch.rs +++ b/crates/engine/src/parser/oracle_static/dispatch.rs @@ -400,12 +400,16 @@ pub(crate) fn parse_static_line_inner( // string-equality checks. { let lower_trim = tp.lower.trim_end_matches('.').trim(); + // The optional comma after "while voting" is a single `opt` axis rather + // than two flat full-sentence permutations (CLAUDE.md: compose + // combinators, don't enumerate permutations). let res: nom::IResult<&str, (), OracleError<'_>> = nom::combinator::value( (), - nom::branch::alt(( - nom::bytes::complete::tag("while voting, you may vote an additional time"), - nom::bytes::complete::tag("while voting you may vote an additional time"), - )), + ( + nom::bytes::complete::tag("while voting"), + nom::combinator::opt(nom::bytes::complete::tag(",")), + nom::bytes::complete::tag(" you may vote an additional time"), + ), ) .parse(lower_trim); if res.is_ok() { @@ -427,19 +431,18 @@ pub(crate) fn parse_static_line_inner( // a coverage/semantic marker, not the scope authority. Reminder text "(They // can make the same or different choices.)" is already stripped above by // `strip_reminder_text`. Comma/no-comma variants are alt arms, not flat - // full-sentence equality, matching the GrantsExtraVote pattern. + // The optional comma after "choice" is a single `opt` axis rather than two + // flat full-sentence permutations (CLAUDE.md: compose combinators, don't + // enumerate permutations). { let lower_trim = tp.lower.trim_end_matches('.').trim(); let res: nom::IResult<&str, (), OracleError<'_>> = nom::combinator::value( (), - nom::branch::alt(( - nom::bytes::complete::tag( - "if an opponent would face a villainous choice, they face that choice an additional time", - ), - nom::bytes::complete::tag( - "if an opponent would face a villainous choice they face that choice an additional time", - ), - )), + ( + nom::bytes::complete::tag("if an opponent would face a villainous choice"), + nom::combinator::opt(nom::bytes::complete::tag(",")), + nom::bytes::complete::tag(" they face that choice an additional time"), + ), ) .parse(lower_trim); if res.is_ok() {