From 85c2a47b591aaed206e3e956d85d30896f184caa Mon Sep 17 00:00:00 2001 From: root Date: Fri, 3 Jul 2026 16:40:36 +0200 Subject: [PATCH 1/7] fix(effects): offer optional tap on Charismatic Conqueror (#4963) Treat "they may tap that permanent" as an optional effect and route the prompt to the entering permanent's controller instead of auto-tapping. Co-authored-by: Cursor --- crates/engine/src/game/effects/mod.rs | 16 ++++++++++++ crates/engine/src/parser/clause_shell.rs | 8 ++++++ .../engine/src/parser/oracle_trigger_tests.rs | 26 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index c6ac20d81d..62a6cb6dc0 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -4677,6 +4677,22 @@ fn optional_prompt_player(state: &GameState, ability: &ResolvedAbility) -> Playe return player; } } + // CR 603.7c + CR 608.2d: "they may tap that permanent" on zone-change + // observer triggers (Charismatic Conqueror) — the entering object's + // controller decides whether to tap it, not the ability's controller. + if let Effect::SetTapState { + target: TargetFilter::TriggeringSource, + .. + } = &ability.effect + { + if let Some(player) = crate::game::targeting::resolve_effect_player_ref( + state, + ability, + &TargetFilter::TriggeringSourceController, + ) { + return player; + } + } if let Effect::Sacrifice { target, .. } = &ability.effect { if target_filter_controller_scope(target) == Some(ControllerRef::ParentTargetController) { if let Some(player) = crate::game::targeting::resolve_effect_player_ref( diff --git a/crates/engine/src/parser/clause_shell.rs b/crates/engine/src/parser/clause_shell.rs index 6f8469ce7a..b9f4a84918 100644 --- a/crates/engine/src/parser/clause_shell.rs +++ b/crates/engine/src/parser/clause_shell.rs @@ -286,6 +286,14 @@ pub(crate) fn peel_optional_slots( if let Some((scope, player_scope, rest)) = try_peel_opponent_may_prefix(text) { return (true, scope, player_scope, rest); } + // CR 603.7c + CR 608.2d: "they may …" on zone-change observer triggers + // (Charismatic Conqueror: the entering permanent's controller may tap it). + // The optional prompt player is resolved at runtime via + // `optional_prompt_player` for `SetTapState { TriggeringSource }` effects. + let lower = text.to_lowercase(); + if let Some((_, rest)) = nom_on_lower(text, &lower, |i| value((), tag("they may ")).parse(i)) { + return (true, None, None, rest.to_string()); + } if let Some(rest) = peel_you_may_prefix(text, YouMayBlocklist::ChunkLoop) { return (true, None, None, rest); } diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 2ace721a14..41116643e4 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -198,6 +198,27 @@ fn assert_owned_by_opponent(filter: &TargetFilter) { } } +#[test] +fn parse_post_spell_modifier_creature_type_does_not_share_reference() { + use crate::types::ability::{FilterProp, SharedQuality, SharedQualityRelation, TargetFilter}; + + let filter = parse_post_spell_modifier( + "that doesn't share a creature type with a creature you control or a creature card in your graveyard", + ) + .expect("expected SharesQuality post-spell modifier"); + let TargetFilter::Typed(tf) = filter else { + panic!("expected Typed filter, got {filter:?}"); + }; + assert!(tf.properties.iter().any(|p| matches!( + p, + FilterProp::SharesQuality { + quality: SharedQuality::CreatureType, + relation: SharedQualityRelation::DoesNotShare, + reference: Some(_), + } + ))); +} + #[test] fn parse_post_spell_modifier_cast_origin_from_nonhand() { // CR 601.2a: "from anywhere other than your hand" → InAnyZone over the @@ -1527,6 +1548,11 @@ fn trigger_etb_subject_enters_untapped_attaches_negated_condition() { condition: Box::new(TriggerCondition::ZoneChangeObjectIsTapped) }) ); + let execute = def.execute.as_ref().expect("execute present"); + assert!( + execute.optional, + "they may tap must be optional, got execute={execute:?}" + ); } // Guard: a bare "enters" (no tapped-state rider) must NOT attach a From 1f0555ec8aff074014ec10dc321284e4356295b9 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 3 Jul 2026 17:58:03 +0200 Subject: [PATCH 2/7] fix(parser): narrow optional tap peel and fix CI (#4963) Peel only "they may tap" so Gonti-style "they may play" grants stay non-optional, drop Volo-only parser test from this branch, and cite CR 608.2d. Co-authored-by: Cursor --- crates/engine/src/game/effects/mod.rs | 6 +++--- crates/engine/src/parser/clause_shell.rs | 6 ++++-- .../engine/src/parser/oracle_trigger_tests.rs | 21 ------------------- 3 files changed, 7 insertions(+), 26 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 62a6cb6dc0..1fcafa5e1d 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -4677,9 +4677,9 @@ fn optional_prompt_player(state: &GameState, ability: &ResolvedAbility) -> Playe return player; } } - // CR 603.7c + CR 608.2d: "they may tap that permanent" on zone-change - // observer triggers (Charismatic Conqueror) — the entering object's - // controller decides whether to tap it, not the ability's controller. + // CR 608.2d: "they may tap that permanent" on zone-change observer + // triggers (Charismatic Conqueror) — the entering object's controller + // decides whether to tap it, not the ability's controller. if let Effect::SetTapState { target: TargetFilter::TriggeringSource, .. diff --git a/crates/engine/src/parser/clause_shell.rs b/crates/engine/src/parser/clause_shell.rs index b9f4a84918..200e9d4234 100644 --- a/crates/engine/src/parser/clause_shell.rs +++ b/crates/engine/src/parser/clause_shell.rs @@ -286,12 +286,14 @@ pub(crate) fn peel_optional_slots( if let Some((scope, player_scope, rest)) = try_peel_opponent_may_prefix(text) { return (true, scope, player_scope, rest); } - // CR 603.7c + CR 608.2d: "they may …" on zone-change observer triggers + // CR 608.2d: "they may tap …" on zone-change observer triggers // (Charismatic Conqueror: the entering permanent's controller may tap it). // The optional prompt player is resolved at runtime via // `optional_prompt_player` for `SetTapState { TriggeringSource }` effects. let lower = text.to_lowercase(); - if let Some((_, rest)) = nom_on_lower(text, &lower, |i| value((), tag("they may ")).parse(i)) { + if let Some((_, rest)) = + nom_on_lower(text, &lower, |i| value((), tag("they may tap ")).parse(i)) + { return (true, None, None, rest.to_string()); } if let Some(rest) = peel_you_may_prefix(text, YouMayBlocklist::ChunkLoop) { diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 41116643e4..181667813f 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -198,27 +198,6 @@ fn assert_owned_by_opponent(filter: &TargetFilter) { } } -#[test] -fn parse_post_spell_modifier_creature_type_does_not_share_reference() { - use crate::types::ability::{FilterProp, SharedQuality, SharedQualityRelation, TargetFilter}; - - let filter = parse_post_spell_modifier( - "that doesn't share a creature type with a creature you control or a creature card in your graveyard", - ) - .expect("expected SharesQuality post-spell modifier"); - let TargetFilter::Typed(tf) = filter else { - panic!("expected Typed filter, got {filter:?}"); - }; - assert!(tf.properties.iter().any(|p| matches!( - p, - FilterProp::SharesQuality { - quality: SharedQuality::CreatureType, - relation: SharedQualityRelation::DoesNotShare, - reference: Some(_), - } - ))); -} - #[test] fn parse_post_spell_modifier_cast_origin_from_nonhand() { // CR 601.2a: "from anywhere other than your hand" → InAnyZone over the From 9ece812226e6f896ccb10c305633c059c3d5f2c5 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 3 Jul 2026 19:20:11 +0200 Subject: [PATCH 3/7] fix(parser/effects): scope optional-tap peel + prove prompt controller (#4963) Peel only the "they may " optional marker (keeping the "tap" verb) so Charismatic Conqueror and Bow to My Command keep their tap effects while unrelated "they may play/cast" grants are untouched. Lift SetTapState's ParentTarget to TriggeringSource on zone-change triggers so the tap and its optional-prompt player resolve off the entering object, and add a runtime scenario proving the opponent is prompted and accept taps / decline makes the Vampire token. Co-authored-by: Cursor --- crates/engine/src/game/effects/change_zone.rs | 149 ++++++++++++++++++ crates/engine/src/parser/clause_shell.rs | 27 +++- crates/engine/src/parser/oracle_trigger.rs | 6 + .../engine/src/parser/oracle_trigger_tests.rs | 15 ++ 4 files changed, 189 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/game/effects/change_zone.rs b/crates/engine/src/game/effects/change_zone.rs index 1c816a9c14..8b90a3e7d5 100644 --- a/crates/engine/src/game/effects/change_zone.rs +++ b/crates/engine/src/game/effects/change_zone.rs @@ -5602,6 +5602,155 @@ mod tests { ); } + /// Issue #4963 (runtime): set up Charismatic Conqueror under `PlayerId(0)` + /// with its trigger parsed from Oracle text, make an opponent's creature + /// enter untapped, then resolve the stacked trigger to its optional prompt. + /// Returns the state paused at the `OptionalEffectChoice`, plus the + /// conqueror and the entering permanent ids. + fn fire_charismatic_conqueror_optional_tap() -> (GameState, ObjectId, ObjectId) { + use crate::game::triggers::process_triggers; + use crate::parser::oracle_trigger::parse_trigger_line; + + let mut state = GameState::new_two_player(4963); + + let conqueror = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Charismatic Conqueror".to_string(), + Zone::Battlefield, + ); + let trigger = parse_trigger_line( + "Whenever an artifact or creature an opponent controls enters untapped, they may tap that permanent. If they don't, you create a 1/1 white Vampire creature token with lifelink.", + "Charismatic Conqueror", + ); + { + let obj = state.objects.get_mut(&conqueror).unwrap(); + obj.card_types.core_types.push(CoreType::Creature); + obj.trigger_definitions.push(trigger); + } + + let opp_creature = create_object( + &mut state, + CardId(2), + PlayerId(1), + "Opponent Creature".to_string(), + Zone::Hand, + ); + state + .objects + .get_mut(&opp_creature) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); + + let ability = ResolvedAbility::new( + Effect::ChangeZone { + origin: Some(Zone::Hand), + destination: Zone::Battlefield, + target: TargetFilter::Any, + owner_library: false, + enter_transformed: false, + enters_under: Some(ControllerRef::You), + enter_tapped: crate::types::zones::EtbTapState::Unspecified, + enters_attacking: false, + up_to: false, + enter_with_counters: vec![], + conditional_enter_with_counters: vec![], + face_down_profile: None, + enters_modified_if: None, + }, + vec![TargetRef::Object(opp_creature)], + ObjectId(999), + PlayerId(1), + ); + let mut events = Vec::new(); + resolve(&mut state, &ability, &mut events).unwrap(); + process_triggers(&mut state, &events); + + let mut resolve_events = Vec::new(); + crate::game::stack::resolve_top(&mut state, &mut resolve_events); + + (state, conqueror, opp_creature) + } + + /// CR 608.2d (issue #4963): the "they may tap that permanent" optional + /// decision is offered to the ENTERING permanent's controller (the + /// opponent), not to Charismatic Conqueror's controller. + #[test] + fn charismatic_conqueror_optional_tap_prompts_entering_controller() { + let (state, _conqueror, _opp) = fire_charismatic_conqueror_optional_tap(); + match &state.waiting_for { + crate::types::game_state::WaitingFor::OptionalEffectChoice { player, .. } => { + assert_eq!( + *player, + PlayerId(1), + "the entering permanent's controller (opponent) must decide the optional tap" + ); + } + other => panic!("expected an OptionalEffectChoice prompt, got {other:?}"), + } + } + + /// Accepting taps the entering permanent (and, per the `Not(OptionalEffect + /// Performed)` gate, no Vampire token is created). + #[test] + fn charismatic_conqueror_optional_tap_accept_taps_entering_permanent() { + let (mut state, _conqueror, opp) = fire_charismatic_conqueror_optional_tap(); + let mut events = Vec::new(); + crate::game::engine_payment_choices::handle_optional_effect_choice( + &mut state, + true, + &mut events, + ) + .unwrap(); + assert!( + state.objects[&opp].tapped, + "accepting the optional tap must tap the entering permanent" + ); + assert!( + !state + .battlefield + .iter() + .any(|id| state.objects.get(id).is_some_and(|o| o.name == "Vampire")), + "accepting must NOT create the 'if they don't' Vampire token" + ); + } + + /// Declining leaves the permanent untapped and creates the 1/1 white Vampire + /// token with lifelink under Charismatic Conqueror's controller. + #[test] + fn charismatic_conqueror_optional_tap_decline_creates_vampire_token() { + let (mut state, _conqueror, opp) = fire_charismatic_conqueror_optional_tap(); + let mut events = Vec::new(); + crate::game::engine_payment_choices::handle_optional_effect_choice( + &mut state, + false, + &mut events, + ) + .unwrap(); + assert!( + !state.objects[&opp].tapped, + "declining the optional tap must leave the entering permanent untapped" + ); + let vampire = state + .battlefield + .iter() + .filter_map(|id| state.objects.get(id)) + .find(|o| o.name == "Vampire") + .expect("declining must create the Vampire token"); + assert_eq!( + vampire.controller, + PlayerId(0), + "the token is created under Charismatic Conqueror's controller ('you')" + ); + assert!( + vampire.has_keyword(&Keyword::Lifelink), + "the created Vampire token must have lifelink" + ); + } + /// CR 400.6 + CR 608.2c: `ChangeZoneAll` must set `last_effect_count` to /// the number of objects moved so downstream sub-abilities referring to /// "that many" (via `QuantityRef::EventContextAmount`) resolve correctly. diff --git a/crates/engine/src/parser/clause_shell.rs b/crates/engine/src/parser/clause_shell.rs index 200e9d4234..75f0194ee8 100644 --- a/crates/engine/src/parser/clause_shell.rs +++ b/crates/engine/src/parser/clause_shell.rs @@ -69,7 +69,8 @@ use crate::parser::oracle_nom::error::OracleError; use nom::branch::alt; use nom::bytes::complete::{tag, take_until}; -use nom::combinator::{opt, value}; +use nom::combinator::{opt, peek, value}; +use nom::sequence::terminated; use nom::Parser; use super::oracle_effect::conditions::{ @@ -286,14 +287,24 @@ pub(crate) fn peel_optional_slots( if let Some((scope, player_scope, rest)) = try_peel_opponent_may_prefix(text) { return (true, scope, player_scope, rest); } - // CR 608.2d: "they may tap …" on zone-change observer triggers - // (Charismatic Conqueror: the entering permanent's controller may tap it). - // The optional prompt player is resolved at runtime via - // `optional_prompt_player` for `SetTapState { TriggeringSource }` effects. + // CR 608.2d: "they may tap …" third-person optional tap on a triggered + // ability whose actor is the trigger's subject player, not the ability's + // controller (Charismatic Conqueror: the entering permanent's controller may + // tap it; Bow to My Command: your opponents may tap their own creatures). + // Peel ONLY the "they may " optional marker and leave the "tap …" verb for + // the effect parser — consuming the verb would strand a bare noun phrase. + // Scoped to a following "tap" so unrelated "they may play/cast/…" grants + // (Gonti and siblings) keep their specialized routes. The prompt player is + // resolved at runtime by `optional_prompt_player` for + // `SetTapState { TriggeringSource }` effects. let lower = text.to_lowercase(); - if let Some((_, rest)) = - nom_on_lower(text, &lower, |i| value((), tag("they may tap ")).parse(i)) - { + if let Some((_, rest)) = nom_on_lower(text, &lower, |i| { + value( + (), + terminated(tag::<_, _, OracleError<'_>>("they may "), peek(tag("tap"))), + ) + .parse(i) + }) { return (true, None, None, rest.to_string()); } if let Some(rest) = peel_you_may_prefix(text, YouMayBlocklist::ChunkLoop) { diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index 2a65587a0c..d63e194f73 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -1590,6 +1590,12 @@ fn lift_parent_target_to_triggering_source(effect: &mut Effect) { // "create a token that's a copy of that creature" (Necroduality) — the // copy source is the entering object, not the trigger's own source. Effect::CopyTokenOf { target, .. } => target, + // CR 608.2k: "they may tap that permanent" on a single-object zone-change + // trigger (Charismatic Conqueror) — "that permanent" is the entering + // object. Binding to `TriggeringSource` makes both the tap target and the + // runtime optional-prompt player (its controller) resolve off the event + // object instead of the ability's own source/controller. + Effect::SetTapState { target, .. } => target, _ => return, }; if matches!(target, TargetFilter::ParentTarget) { diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index 181667813f..cbef6cf01f 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -1532,6 +1532,21 @@ fn trigger_etb_subject_enters_untapped_attaches_negated_condition() { execute.optional, "they may tap must be optional, got execute={execute:?}" ); + // CR 608.2k: "tap that permanent" must keep the tap verb (peel only the + // "they may " optional marker) and bind "that permanent" to the entering + // object via `TriggeringSource` — not strand a bare noun phrase. + assert!( + matches!( + execute.effect.as_ref(), + Effect::SetTapState { + target: TargetFilter::TriggeringSource, + state: TapStateChange::Tap, + .. + } + ), + "expected optional tap on the triggering source, got {:?}", + execute.effect + ); } // Guard: a bare "enters" (no tapped-state rider) must NOT attach a From e16d9901479df14ae62b5e0065c008ac53d54ee9 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 3 Jul 2026 20:27:23 +0200 Subject: [PATCH 4/7] test(coverage): lock Charismatic Conqueror concrete-tap parse-details (#4963) Add a production parse_details regression (the exact tree the coverage-parse-diff sticky is built from) asserting the optional-tap trigger lowers to a concrete Tap ability bound to the entering object ("triggering source"), never the vague `that` bare noun phrase. Confirms the clause-shell peel keeps the tap verb; the peel's blast radius is only the two production cards with "they may tap" (Charismatic Conqueror, Bow to My Command). Co-authored-by: Cursor --- crates/engine/src/game/coverage.rs | 64 ++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index 24ae0a4776..44d87c1b69 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -9928,6 +9928,70 @@ mod tests { } } + /// CR 608.2d + issue #4963 (matthewevans review): production `parse_details` + /// regression for Charismatic Conqueror. This is the exact tree the + /// `coverage-parse-diff` sticky is built from. It must show the trigger's + /// optional tap as a *concrete* `Tap` ability bound to the entering object + /// ("triggering source"), NOT the vague `that` bare-noun-phrase + /// (`Effect::Unimplemented { name: "that" }`, which surfaces as an + /// unsupported `label == "that"`). Guards the clause-shell peel so it keeps + /// the "tap" verb and does not strand a noun phrase. + #[test] + fn charismatic_conqueror_parse_details_is_concrete_tap_not_vague_that() { + let mut face = make_face(); + face.name = "Charismatic Conqueror".to_string(); + face.oracle_text = Some( + "Vigilance\nWhenever an artifact or creature an opponent controls enters untapped, they may tap that permanent. If they don't, you create a 1/1 white Vampire creature token with lifelink.".to_string(), + ); + face.triggers = vec![crate::parser::oracle_trigger::parse_trigger_line( + "Whenever an artifact or creature an opponent controls enters untapped, they may tap that permanent. If they don't, you create a 1/1 white Vampire creature token with lifelink.", + "Charismatic Conqueror", + )]; + + let details = build_parse_details_for_face(&face); + let trigger = details + .iter() + .find(|item| item.category == ParseCategory::Trigger) + .expect("Charismatic Conqueror must produce a trigger parse item"); + assert!( + trigger.supported, + "the zone-change trigger must be supported, got {trigger:?}" + ); + let tap = trigger + .children + .first() + .expect("trigger must carry its execute ability"); + assert_eq!( + tap.label, "Tap", + "the optional effect must stay a concrete Tap (never the vague \ + `that` bare noun phrase), got label {:?}", + tap.label + ); + assert!( + tap.supported, + "the tap ability must be supported, got {tap:?}" + ); + assert!( + tap.details + .iter() + .any(|(k, v)| k == "target" && v == "triggering source"), + "the tap must bind to the entering object (`triggering source`), \ + not the ability's own parent target, got {:?}", + tap.details + ); + // Belt-and-suspenders: no node anywhere in the tree may be the vague + // `that` unimplemented residual the earlier peel produced. + fn none_is_vague_that(items: &[ParsedItem]) -> bool { + items + .iter() + .all(|it| it.label != "that" && none_is_vague_that(&it.children)) + } + assert!( + none_is_vague_that(&details), + "no parse node may be the vague `that` residual, got {details:?}" + ); + } + #[test] fn card_face_with_nested_mode_unimplemented_is_detected() { let mut face = make_face(); From 84a6dc017e558fe2e08be6c84d413dda9180f203 Mon Sep 17 00:00:00 2001 From: root Date: Fri, 3 Jul 2026 22:51:13 +0200 Subject: [PATCH 5/7] fix(parser): scope SetTapState anaphor lift to top-level "that permanent" tap (#4963) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The zone-change anaphor lift rewrote *every* SetTapState ParentTarget to TriggeringSource, which retargeted player-chosen taps onto the entering object. Restrict the SetTapState lift to the trigger's OWN top-level "that permanent" tap anaphor (Charismatic Conqueror): - skip it below the top link (reflexive "you may pay {2}. When you do, tap target creature an opponent controls" — Snaremaster Sprite), - skip it when the link carries its own multi_target/optional_targeting slot, and - only lift TapStateChange::Tap — an entering permanent enters untapped, so "untap that creature" (Howl of the Hunt = the enchanted creature) never refers to the trigger source. Charismatic Conqueror and Dragon Turtle's genuine "that entering permanent" self-tap still lift. Add parse regressions for Snaremaster, Howl of the Hunt, and Charismatic Conqueror. Co-authored-by: Cursor --- crates/engine/src/parser/oracle_trigger.rs | 40 ++++++++-- .../engine/src/parser/oracle_trigger_tests.rs | 74 +++++++++++++++++++ 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index d63e194f73..61a86ac0c8 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -43,8 +43,8 @@ use crate::types::ability::{ DestinationConstraint, DieResultFilter, Effect, FilterProp, ObjectScope, OriginConstraint, ParsedCondition, PlayerFilter, PlayerScope, PtStat, PtValueScope, QuantityExpr, QuantityRef, RenownSubject, SacrificeAggregateStat, SacrificeCost, SacrificeRequirement, StaticCondition, - TapCreaturesRequirement, TargetFilter, TriggerCondition, TriggerConstraint, TriggerDefinition, - TypeFilter, TypedFilter, UnlessPayModifier, ZoneChangeClause, + TapCreaturesRequirement, TapStateChange, TargetFilter, TriggerCondition, TriggerConstraint, + TriggerDefinition, TypeFilter, TypedFilter, UnlessPayModifier, ZoneChangeClause, }; use crate::types::card_type::{is_land_subtype, CoreType}; use crate::types::counter::CounterType; @@ -1581,7 +1581,7 @@ fn mode_carries_event_source_object(mode: &TriggerMode) -> bool { /// TargetFilter` and whose runtime semantics make sense against the event /// object (e.g. `ChangeZone` operating on the just-discarded card). Other /// effect variants are left untouched. -fn lift_parent_target_to_triggering_source(effect: &mut Effect) { +fn lift_parent_target_to_triggering_source(effect: &mut Effect, allow_set_tap_lift: bool) { // CR 608.2k: each variant carries a top-level `target` that, when the // surface anaphor was "that ", refers to the event object. let target = match effect { @@ -1595,7 +1595,25 @@ fn lift_parent_target_to_triggering_source(effect: &mut Effect) { // object. Binding to `TriggeringSource` makes both the tap target and the // runtime optional-prompt player (its controller) resolve off the event // object instead of the ability's own source/controller. - Effect::SetTapState { target, .. } => target, + // + // Restricted to `TapStateChange::Tap`: a permanent enters *untapped* + // (Charismatic Conqueror keys off "enters untapped"), so the "that + // permanent" anaphor is only ever *tapped*, never untapped. An `Untap` + // that survives to here refers to some other object (e.g. Howl of the + // Hunt's "untap that creature" = the *enchanted* creature, not the + // entering Aura) and must not be lifted onto the trigger source. + // + // CR 608.2c: also gated on `allow_set_tap_lift` (see the caller) so a + // *targeted* or *reflexive* tap keeps its player-chosen target — e.g. + // Snaremaster Sprite's "you may pay {2}. When you do, tap target creature + // an opponent controls" lowers the tap under a `PayCost` sub-ability and + // any multi-target/optional tap carries its own chosen slot. Their + // `ParentTarget` refers to the chosen creature, not the event object. + Effect::SetTapState { + target, + state: TapStateChange::Tap, + .. + } if allow_set_tap_lift => target, _ => return, }; if matches!(target, TargetFilter::ParentTarget) { @@ -1619,12 +1637,24 @@ fn lift_parent_target_to_triggering_source_in_ability(ability: &mut AbilityDefin // Necroduality (top-level `CopyTokenOf` with no prior choice) and Tergrid // ("put that card …, then create a token") still lift correctly. let mut node = Some(ability); + let mut is_top_level = true; while let Some(link) = node { if introduces_chosen_object_target(link.effect.as_ref()) { break; } - lift_parent_target_to_triggering_source(link.effect.as_mut()); + // CR 608.2c + CR 608.2k: `SetTapState` only lifts as the trigger's OWN + // top-level "that permanent" anaphor (Charismatic Conqueror). It must not + // lift when the tap is a reflexive "when you do" sub-ability (reached + // below the top link) or carries its own player-chosen target slot + // (`multi_target` / `optional_targeting`) — those `ParentTarget`s refer to + // the chosen creature, not the entering object. The other anaphoric + // variants (`ChangeZone`/`Sacrifice`/`CopyTokenOf`) still lift at any + // depth for the Tergrid punisher class. + let allow_set_tap_lift = + is_top_level && link.multi_target.is_none() && !link.optional_targeting; + lift_parent_target_to_triggering_source(link.effect.as_mut(), allow_set_tap_lift); node = link.sub_ability.as_deref_mut(); + is_top_level = false; } } diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index cbef6cf01f..e2f8e39f5d 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -21245,3 +21245,77 @@ fn opponent_attacks_you_with_two_or_more_dinosaurs_carries_type_filter() { ), } } + +#[test] +fn set_tap_state_lift_skips_reflexive_and_targeted_taps() { + // Walk an ability chain and return the first `SetTapState` effect (so tests + // can inspect both its tap/untap `state` and its `target`). + fn find_set_tap(a: &AbilityDefinition) -> Option<&Effect> { + if matches!(a.effect.as_ref(), Effect::SetTapState { .. }) { + return Some(a.effect.as_ref()); + } + a.sub_ability.as_deref().and_then(find_set_tap) + } + + // CR 608.2c: Snaremaster Sprite lowers "you may pay {2}. When you do, tap + // target creature an opponent controls" as a reflexive tap under a `PayCost` + // sub-ability. Its `ParentTarget` is the player-chosen creature, so the + // anaphor lift must leave it alone (it must NOT become `TriggeringSource`, + // which would retarget the tap onto the entering object). + let snare = parse_trigger_line( + "When Snaremaster Sprite enters, you may pay {2}. When you do, tap target creature an opponent controls and put a stun counter on it.", + "Snaremaster Sprite", + ); + match snare.execute.as_deref().and_then(find_set_tap) { + Some(Effect::SetTapState { target, state, .. }) => { + assert_eq!(*state, TapStateChange::Tap); + assert_eq!( + *target, + TargetFilter::ParentTarget, + "a reflexive/targeted tap must keep its player-chosen target, not \ + lift to TriggeringSource, got {target:?}" + ); + } + other => panic!("Snaremaster Sprite must lower a SetTapState tap, got {other:?}"), + } + + // CR 608.2k: Charismatic Conqueror's top-level "they may tap that permanent" + // anaphor still lifts to `TriggeringSource` so the tap and its optional + // prompt resolve off the entering object's controller. + let charis = parse_trigger_line( + "Whenever an artifact or creature an opponent controls enters untapped, they may tap that permanent. If they don't, you create a 1/1 white Vampire creature token with lifelink.", + "Charismatic Conqueror", + ); + match charis.execute.as_deref().and_then(find_set_tap) { + Some(Effect::SetTapState { target, state, .. }) => { + assert_eq!(*state, TapStateChange::Tap); + assert_eq!( + *target, + TargetFilter::TriggeringSource, + "the top-level 'that permanent' anaphor must lift to TriggeringSource, \ + got {target:?}" + ); + } + other => panic!("Charismatic Conqueror must lower a SetTapState tap, got {other:?}"), + } + + // CR 608.2c: Howl of the Hunt's "untap that creature" refers to the + // *enchanted* creature (the condition's subject), not the entering Aura, so + // the anaphor lift is restricted to `Tap` and must leave this `Untap` alone. + let howl = parse_trigger_line( + "When Howl of the Hunt enters, if enchanted creature is a Wolf or Werewolf, untap that creature.", + "Howl of the Hunt", + ); + match howl.execute.as_deref().and_then(find_set_tap) { + Some(Effect::SetTapState { target, state, .. }) => { + assert_eq!(*state, TapStateChange::Untap); + assert_eq!( + *target, + TargetFilter::ParentTarget, + "an untap-that-creature anaphor must not lift to TriggeringSource, \ + got {target:?}" + ); + } + other => panic!("Howl of the Hunt must lower a SetTapState untap, got {other:?}"), + } +} From a12967ca785d573de12562c59edddbc7247a5c48 Mon Sep 17 00:00:00 2001 From: root Date: Sat, 4 Jul 2026 02:10:58 +0200 Subject: [PATCH 6/7] fix(parser/effects): scope "they may tap" actor to the triggering player (#4963) Address matthewevans' review on the optional-tap fix: - clause_shell "they may tap" peel now threads the trigger's subject player out as PlayerFilter::TriggeringPlayer, so a Bow to My Command-style tap of "any number of untapped creatures they control" routes its optional prompt and target choice to "they" (the triggering opponent), not the scheme's controller. The bare "that permanent"/"it" self-tap anaphor (Charismatic Conqueror) deliberately keeps no player_scope: it lifts to SetTapState { TriggeringSource } and would otherwise leak the triggering player onto the "if they don't, you create ..." tail. - optional_prompt_player resolves the SetTapState { TriggeringSource } actor from the EVENT-TIME controller on the trigger's ZoneChangeRecord (TriggeringPlayer -> record.controller, CR 603.10a) instead of the object's live controller, so a control change between ETB and the trigger resolving still prompts the player who controlled the permanent when it entered. TriggeringSourceController remains a fallback. Regressions: peel-level Bow vs Charismatic Conqueror actor-scope tests, an end-to-end Bow parse asserting player_scope: TriggeringPlayer, and a control-change-before-resolution runtime scenario. Co-authored-by: Cursor --- crates/engine/src/game/effects/change_zone.rs | 38 +++++++++ crates/engine/src/game/effects/mod.rs | 24 ++++-- crates/engine/src/parser/clause_shell.rs | 80 ++++++++++++++++--- .../engine/src/parser/oracle_trigger_tests.rs | 50 ++++++++++++ 4 files changed, 176 insertions(+), 16 deletions(-) diff --git a/crates/engine/src/game/effects/change_zone.rs b/crates/engine/src/game/effects/change_zone.rs index 8b90a3e7d5..165734ac45 100644 --- a/crates/engine/src/game/effects/change_zone.rs +++ b/crates/engine/src/game/effects/change_zone.rs @@ -5608,6 +5608,16 @@ mod tests { /// Returns the state paused at the `OptionalEffectChoice`, plus the /// conqueror and the entering permanent ids. fn fire_charismatic_conqueror_optional_tap() -> (GameState, ObjectId, ObjectId) { + fire_charismatic_conqueror_optional_tap_with(|_, _| {}) + } + + /// Like `fire_charismatic_conqueror_optional_tap`, but runs `before_resolve` + /// with the entering permanent id AFTER the trigger has been placed on the + /// stack (post `process_triggers`) and BEFORE it resolves — the seam a + /// control-change-before-resolution regression needs. + fn fire_charismatic_conqueror_optional_tap_with( + before_resolve: impl FnOnce(&mut GameState, ObjectId), + ) -> (GameState, ObjectId, ObjectId) { use crate::game::triggers::process_triggers; use crate::parser::oracle_trigger::parse_trigger_line; @@ -5669,6 +5679,8 @@ mod tests { resolve(&mut state, &ability, &mut events).unwrap(); process_triggers(&mut state, &events); + before_resolve(&mut state, opp_creature); + let mut resolve_events = Vec::new(); crate::game::stack::resolve_top(&mut state, &mut resolve_events); @@ -5693,6 +5705,32 @@ mod tests { } } + /// CR 603.10a (issue #4963 review): if the entering permanent CHANGES + /// controllers after it entered but before the trigger resolves, "they" is + /// still the player who controlled it at the zone-change event — read from + /// the `ZoneChangeRecord` — not whoever controls it live at resolution. Here + /// the entering creature is handed to `PlayerId(0)` (Charismatic Conqueror's + /// controller) after the trigger is on the stack; the optional tap must still + /// be offered to `PlayerId(1)`, the event-time controller. + #[test] + fn charismatic_conqueror_optional_tap_uses_event_time_controller() { + let (state, _conqueror, _opp) = + fire_charismatic_conqueror_optional_tap_with(|state, opp| { + state.objects.get_mut(&opp).unwrap().controller = PlayerId(0); + }); + match &state.waiting_for { + crate::types::game_state::WaitingFor::OptionalEffectChoice { player, .. } => { + assert_eq!( + *player, + PlayerId(1), + "the event-time controller (recorded on the ZoneChangeRecord) must decide \ + the optional tap, even after the permanent changed controllers" + ); + } + other => panic!("expected an OptionalEffectChoice prompt, got {other:?}"), + } + } + /// Accepting taps the entering permanent (and, per the `Not(OptionalEffect /// Performed)` gate, no Vampire token is created). #[test] diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 1fcafa5e1d..a306ffda3a 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -4677,9 +4677,16 @@ fn optional_prompt_player(state: &GameState, ability: &ResolvedAbility) -> Playe return player; } } - // CR 608.2d: "they may tap that permanent" on zone-change observer - // triggers (Charismatic Conqueror) — the entering object's controller - // decides whether to tap it, not the ability's controller. + // CR 608.2d + CR 603.10a: "they may tap that permanent" on zone-change + // observer triggers (Charismatic Conqueror) — the entering object's + // controller decides whether to tap it, not the ability's controller. + // Resolve the actor from the EVENT-TIME controller recorded on the trigger's + // `ZoneChangeRecord` (`TriggeringPlayer` → `record.controller`), NOT the + // object's live controller: if the entering permanent changes controllers + // between the ETB event and this trigger resolving, "they" is still the + // player who controlled it when it entered. `TriggeringSourceController` + // (live/LKI controller) is only the fallback for non-zone-change events that + // reach this shape without a recorded triggering player. if let Effect::SetTapState { target: TargetFilter::TriggeringSource, .. @@ -4688,8 +4695,15 @@ fn optional_prompt_player(state: &GameState, ability: &ResolvedAbility) -> Playe if let Some(player) = crate::game::targeting::resolve_effect_player_ref( state, ability, - &TargetFilter::TriggeringSourceController, - ) { + &TargetFilter::TriggeringPlayer, + ) + .or_else(|| { + crate::game::targeting::resolve_effect_player_ref( + state, + ability, + &TargetFilter::TriggeringSourceController, + ) + }) { return player; } } diff --git a/crates/engine/src/parser/clause_shell.rs b/crates/engine/src/parser/clause_shell.rs index 75f0194ee8..dee862c71c 100644 --- a/crates/engine/src/parser/clause_shell.rs +++ b/crates/engine/src/parser/clause_shell.rs @@ -287,16 +287,23 @@ pub(crate) fn peel_optional_slots( if let Some((scope, player_scope, rest)) = try_peel_opponent_may_prefix(text) { return (true, scope, player_scope, rest); } - // CR 608.2d: "they may tap …" third-person optional tap on a triggered - // ability whose actor is the trigger's subject player, not the ability's - // controller (Charismatic Conqueror: the entering permanent's controller may - // tap it; Bow to My Command: your opponents may tap their own creatures). - // Peel ONLY the "they may " optional marker and leave the "tap …" verb for - // the effect parser — consuming the verb would strand a bare noun phrase. - // Scoped to a following "tap" so unrelated "they may play/cast/…" grants - // (Gonti and siblings) keep their specialized routes. The prompt player is - // resolved at runtime by `optional_prompt_player` for - // `SetTapState { TriggeringSource }` effects. + // CR 608.2d + CR 603.2: "they may tap …" third-person optional tap on a + // triggered ability whose actor is the trigger's subject player, not the + // ability's controller (Charismatic Conqueror: the entering permanent's + // controller may tap it; Bow to My Command: the triggering opponent may tap + // their own creatures). Peel ONLY the "they may " optional marker and leave + // the "tap …" verb for the effect parser — consuming the verb would strand a + // bare noun phrase. Scoped to a following "tap" so unrelated "they may + // play/cast/…" grants (Gonti and siblings) keep their specialized routes. + // + // Thread "they" out as `PlayerFilter::TriggeringPlayer` so the optional + // prompt (and any "creatures they control" target choice) resolves to the + // triggering player — NOT the ability's controller. Without this, a Bow to + // My Command-style tap of "creatures they control" would fall back to the + // scheme's controller. For the "that permanent" anaphor class (Charismatic + // Conqueror) the tap independently lifts to `SetTapState { TriggeringSource }` + // and `optional_prompt_player` resolves the same triggering player from the + // zone-change record, so the two paths agree. let lower = text.to_lowercase(); if let Some((_, rest)) = nom_on_lower(text, &lower, |i| { value( @@ -305,7 +312,22 @@ pub(crate) fn peel_optional_slots( ) .parse(i) }) { - return (true, None, None, rest.to_string()); + // CR 603.2: "tap that permanent" / "tap it" taps the trigger's OWN source + // (Charismatic Conqueror, Dragon Turtle). The parser lifts that anaphor to + // `SetTapState { TriggeringSource }` and `optional_prompt_player` already + // resolves its actor from the zone-change record, so it must NOT also carry + // a controller-rebinding `player_scope` — that would leak the triggering + // player onto the following "if they don't, you …" tail (the token would be + // created under the wrong player). Only a tap that names its OWN targets + // ("any number of untapped creatures they control" — Bow to My Command) + // threads the triggering player so its prompt and "creatures they control" + // choice resolve to "they", not the ability's (scheme's) controller. + let rest_lower = rest.to_lowercase(); + let taps_trigger_source_anaphor = ["tap that ", "tap it", "tap those ", "tap them"] + .iter() + .any(|prefix| rest_lower.starts_with(prefix)); + let player_scope = (!taps_trigger_source_anaphor).then_some(PlayerFilter::TriggeringPlayer); + return (true, None, player_scope, rest.to_string()); } if let Some(rest) = peel_you_may_prefix(text, YouMayBlocklist::ChunkLoop) { return (true, None, None, rest); @@ -888,6 +910,42 @@ mod tests { assert_eq!(rest, "cast the exiled card without paying its mana cost"); } + /// CR 608.2d + CR 603.2 (issue #4963 review): "they may tap " + /// (Bow to My Command) threads the triggering player out as the optional + /// actor so the prompt and the "creatures they control" choice resolve to + /// "they", not the ability's (scheme's) controller. The "tap" verb is kept. + #[test] + fn peel_optional_slots_they_may_tap_targets_threads_triggering_player() { + let (is_optional, opponent_scope, player_scope, rest) = peel_optional_slots( + "they may tap any number of untapped creatures they control with total power 8 or greater", + ); + assert!(is_optional); + assert!(opponent_scope.is_none()); + assert_eq!(player_scope, Some(PlayerFilter::TriggeringPlayer)); + assert_eq!( + rest, + "tap any number of untapped creatures they control with total power 8 or greater" + ); + } + + /// CR 603.2 (issue #4963 review): the "they may tap that permanent" / "tap + /// it" anaphor taps the trigger's OWN source (Charismatic Conqueror). It + /// lifts to `SetTapState { TriggeringSource }` and its prompt resolves from + /// the zone-change record, so it must NOT thread a controller-rebinding + /// `player_scope` (that would leak onto the "if they don't, you …" tail). + #[test] + fn peel_optional_slots_they_may_tap_that_permanent_keeps_no_player_scope() { + let (is_optional, opponent_scope, player_scope, rest) = + peel_optional_slots("they may tap that permanent"); + assert!(is_optional); + assert!(opponent_scope.is_none()); + assert!( + player_scope.is_none(), + "the 'that permanent' self-tap anaphor must not carry a player scope, got {player_scope:?}" + ); + assert_eq!(rest, "tap that permanent"); + } + #[test] fn is_you_may_pay_to_end_effect_phrase_matches_body_and_full_clause() { assert!(is_you_may_pay_to_end_effect_phrase( diff --git a/crates/engine/src/parser/oracle_trigger_tests.rs b/crates/engine/src/parser/oracle_trigger_tests.rs index e2f8e39f5d..5fc784d55e 100644 --- a/crates/engine/src/parser/oracle_trigger_tests.rs +++ b/crates/engine/src/parser/oracle_trigger_tests.rs @@ -21319,3 +21319,53 @@ fn set_tap_state_lift_skips_reflexive_and_targeted_taps() { other => panic!("Howl of the Hunt must lower a SetTapState untap, got {other:?}"), } } + +/// CR 608.2d + CR 603.2 (issue #4963 review): Bow to My Command's +/// "they may tap any number of untapped creatures they control ..." threads the +/// triggering player out as the optional actor (`player_scope: TriggeringPlayer`) +/// so the optional prompt AND the "creatures they control" choice resolve to the +/// triggering opponent named by "they", not the scheme's controller. Unlike +/// Charismatic Conqueror's "that permanent" self-tap, this tap keeps its OWN +/// target set — it must NOT lift to `TriggeringSource`. +#[test] +fn they_may_tap_targeted_threads_triggering_player_scope() { + fn find_tap_ability(a: &AbilityDefinition) -> Option<&AbilityDefinition> { + if matches!(a.effect.as_ref(), Effect::SetTapState { .. }) { + return Some(a); + } + a.sub_ability.as_deref().and_then(find_tap_ability) + } + fn chain_has_triggering_player_scope(a: &AbilityDefinition) -> bool { + a.player_scope == Some(PlayerFilter::TriggeringPlayer) + || a.sub_ability + .as_deref() + .is_some_and(chain_has_triggering_player_scope) + } + + let bow = parse_trigger_line( + "At the beginning of each opponent's end step, they may tap any number of untapped creatures they control with total power 8 or greater.", + "Bow to My Command", + ); + let execute = bow + .execute + .as_deref() + .expect("Bow to My Command trigger must have an execute body"); + let tap = find_tap_ability(execute).expect("Bow to My Command must lower a SetTapState tap"); + match tap.effect.as_ref() { + Effect::SetTapState { target, state, .. } => { + assert_eq!(*state, TapStateChange::Tap); + assert_ne!( + *target, + TargetFilter::TriggeringSource, + "Bow's tap names its own targets; it must NOT lift to TriggeringSource, \ + got {target:?}" + ); + } + other => panic!("expected SetTapState, got {other:?}"), + } + assert!( + chain_has_triggering_player_scope(execute), + "the 'they may tap ' optional actor must thread as \ + player_scope: TriggeringPlayer, got execute={execute:?}" + ); +} From c3b16f1fd71e7ac10d6f247aa9c405db5b9eb954 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Thu, 9 Jul 2026 16:07:03 -0700 Subject: [PATCH 7/7] fix(PR-5014): correct optional tap CR citation Remove the inapplicable CR 603.10a citation from optional_prompt_player's Charismatic Conqueror comment. CR 608.2d was verified locally as the relevant choices-during-resolution rule; the event-time controller lookup remains described as engine trigger-context provenance. --- crates/engine/src/game/effects/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index 99a9f628d7..a90ac1278c 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -4712,9 +4712,9 @@ fn optional_prompt_player(state: &GameState, ability: &ResolvedAbility) -> Playe return player; } } - // CR 608.2d + CR 603.10a: "they may tap that permanent" on zone-change - // observer triggers (Charismatic Conqueror) — the entering object's - // controller decides whether to tap it, not the ability's controller. + // CR 608.2d: "they may tap that permanent" on zone-change observer + // triggers (Charismatic Conqueror) — the entering object's controller + // decides whether to tap it, not the ability's controller. // Resolve the actor from the EVENT-TIME controller recorded on the trigger's // `ZoneChangeRecord` (`TriggeringPlayer` → `record.controller`), NOT the // object's live controller: if the entering permanent changes controllers