fix(effects): offer optional tap on Charismatic Conqueror (#4963)

crates/engine/src/game/coverage.rs

@@ -10693,6 +10693,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();

crates/engine/src/game/effects/change_zone.rs

@@ -5815,6 +5815,193 @@ 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) {
+        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;
+
+        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);
+
+        before_resolve(&mut state, opp_creature);
+
+        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:?}"),
+        }
+    }
+
+    /// 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]
+    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.

crates/engine/src/game/effects/mod.rs

@@ -4982,6 +4982,36 @@ 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.
+    // 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,
+        ..
+    } = &ability.effect
+    {
+        if let Some(player) = crate::game::targeting::resolve_effect_player_ref(
+            state,
+            ability,
+            &TargetFilter::TriggeringPlayer,
+        )
+        .or_else(|| {
+            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(