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

[bohdansolovie]

Summary

• Treat they may tap that permanent as optional in trigger effect parsing
• Route optional tap prompts to the entering permanent's controller (TriggeringSourceController)
• Add parser regression for Charismatic Conqueror execute shape

Fixes #4963

Test plan

• cargo test -p engine --lib trigger_etb_subject_enters_untapped

[gemini-code-assist]

Code Review

This pull request implements support for 'they may tap' zone-change observer triggers (such as Charismatic Conqueror) by updating the optional prompt player resolution and the clause shell parser, along with adding corresponding tests. The feedback identifies incorrect Magic Comprehensive Rules (CR) citations in the newly added comments, specifically recommending the removal of the reference to CR 603.7c as it is not applicable to standard triggered abilities.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[matthewevans]

Requesting changes on two blockers.

First, the runtime behavior introduced in optional_prompt_player needs runtime coverage. The current test only proves that the parser marks the clause optional; it does not prove that the WaitingFor prompt is offered to the entering permanent's controller, nor that accept/decline resolves correctly for Charismatic Conqueror. Please add a GameScenario-style regression that puts an opponent's untapped artifact/creature onto the battlefield and asserts the prompt player is the opponent.

Second, the new comments cite CR 603.7c, but that rule is for delayed triggered abilities. I verified the local CR text: 603.7c is not applicable to this ordinary zone-change trigger. CR 608.2d covers choices during resolution; the trigger context should use the normal trigger rules instead. Please remove or correct that citation.

[github-actions]

Parse changes introduced by this PR · 3 card(s), 3 signature(s) (baseline: main 1cc181725a64)

2 card(s) · ability/Tap · field target: parent target → triggering source

Examples: Charismatic Conqueror, Dragon Turtle

1 card(s) · ability/Tap · field target: untapped you control creature → untapped scoped player controls creature

Examples: Bow to My Command

1 card(s) · ability/Tap · field targets: ∅ → 0+

Examples: Bow to My Command

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

[matthewevans]

Requesting changes on the current head. The prior runtime blocker is still unresolved: the new test only asserts that the parser marked the execute effect optional; it still does not drive the trigger to WaitingFor and prove the prompt player is the entering permanent's controller.

The parse-diff is also far broader than the Charismatic Conqueror class, including Gonti/Razor Demon/Royal Booster/Tibalt's Trickery/Mishra's Command/Tarnation and other unrelated cards. Please narrow the clause-shell route and add a runtime scenario for the optional tap prompt controller.

[matthewevans]

Requesting changes on current head 9ece812226e6f896ccb10c305633c059c3d5f2c5.

[MED] The current parse-diff is still far outside the Charismatic Conqueror optional-tap fix. Evidence: the current coverage-parse-diff sticky changes broad unrelated signatures (ReduceAbilityCost affects fields, static_structure, orphaned_copy_retarget, Ravenous Trap, Suppression Field/Zirda, Teyo), and for the target card it changes Charismatic Conqueror from a concrete Tap (target=parent target) to that. Why it matters: the new runtime tests cover the prompt-player path, but they do not justify or guard this card-data/parser blast radius; the PR can still false-green while rewriting unrelated card parses. Suggested fix: constrain the parser change so the parse-diff is limited to the intended Charismatic Conqueror shape, and add production parse/card-data coverage that would fail if the target card regresses to the vague that/unrelated-signature diff.

[bohdansolovie]

Thanks @matthewevans — addressed on the new head (e16d99014).

The Tap → that regression was a stale sticky. I reproduced the production parse_details (the exact tree the coverage-parse-diff is built from) for the current head via build_parse_details_for_face:

• origin/main: Ability "Tap" target = "parent target"
• this head: Ability "Tap" target = "triggering source"

Both are a concrete, supported Tap — never the vague that (Effect::Unimplemented). The parent target → triggering source change is exactly the intended routing fix so the optional-tap prompt goes to the entering permanent's controller. The that in the earlier sticky came from a prior commit before the lift_parent_target_to_triggering_source + peek(tag("tap")) peel landed.

Blast radius is bounded to the intended class. Only two production cards contain "they may tap" — Charismatic Conqueror and Bow to My Command — so the clause-shell peel cannot reach the unrelated signatures (ReduceAbilityCost / Teyo / Ravenous Trap / Suppression Field / Zirda) shown in the sticky; those are pre-existing baseline noise, not produced by this diff. I verified Bow to My Command's tap still parses to a concrete supported Tap (adds a targets = 0+ detail for "any number", target filter unchanged).

New production coverage: charismatic_conqueror_parse_details_is_concrete_tap_not_vague_that asserts the trigger's execute ability is a concrete Tap bound to triggering source and that no node anywhere in the tree is the that residual — it fails if the peel regresses. This is alongside the existing runtime accept/decline/prompt-controller scenarios.

[matthewevans]

[MED] SetTapState anaphor lifting now rewrites targeted/reflexive tap effects to the trigger source. Evidence: crates/engine/src/parser/oracle_trigger.rs:1623; PR parse-diff reports Snaremaster Sprite ability/Tap target changed from parent target to triggering source, but its source text is “When you do, tap target creature an opponent controls...” in data/mtgish-cards.json:25417. Why it matters: targeted tap effects can now act on the entering/triggering object instead of the player-chosen target. Suggested fix: stop the lift when the current AbilityDefinition has its own chosen target (valid_target / optional targeting), or restrict SetTapState lifting to the no-target “that permanent” anaphor class; add a Snaremaster parse regression.

[bohdansolovie]

Thanks @matthewevans — fixed on the new head (84a6dc017).

Root cause. The zone-change anaphor lift rewrote every SetTapState ParentTarget → TriggeringSource, so Snaremaster Sprite's reflexive "you may pay {2}. When you do, tap target creature an opponent controls" had its player-chosen target retargeted onto the entering object.

Fix (crates/engine/src/parser/oracle_trigger.rs). SetTapState now lifts only as the trigger's own top-level "that permanent" tap anaphor (Charismatic Conqueror). It is skipped when:

• the tap is a reflexive/chained sub-ability below the top link — Snaremaster's tap lowers under a PayCost sub-ability;
• the link carries its own multi_target/optional_targeting slot (a player-chosen target); or
• the state is Untap — an entering permanent enters untapped, so "untap that creature" (Howl of the Hunt = the enchanted creature) never means the trigger source.

ChangeZone/Sacrifice/CopyTokenOf anaphors still lift at any depth (Tergrid punisher class).

Parse-diff is now tight and fully justified (baseline main 2851cea91), 3 cards / 2 signatures:

• ability/Tap · target: parent target → triggering source — Charismatic Conqueror (the fix) and Dragon Turtle (its genuine ThatEnteringPermanent self-tap; the targeted "up to one target creature an opponent controls" tap keeps its chosen slot).
• ability/Tap · targets: ∅ → 0+ — Bow to My Command (peel keeping the tap verb).

Snaremaster Sprite and Howl of the Hunt both dropped out of the diff (back to parent target).

Regression set_tap_state_lift_skips_reflexive_and_targeted_taps asserts Snaremaster (reflexive targeted tap) and Howl of the Hunt (untap-that-creature) keep ParentTarget, while Charismatic Conqueror's top-level anaphor still lifts to TriggeringSource. Full engine lib (14,814) + integration suites, clippy, fmt, and the parser-combinator gate are green.

[matthewevans]

Requesting changes on current head 84a6dc017e558fe2e08be6c84d413dda9180f203.

[MED] they may tap drops the actor scope. Evidence: crates/engine/src/parser/clause_shell.rs:308. Why it matters: Bow to My Command-style text is now parsed as optional tap, but the peel returns no PlayerFilter, so target choice/prompting can fall back to the scheme controller instead of the opponents named by "they." Suggested fix: thread the "they" actor as a scoped player/triggering player through the optional peel and add a Bow regression.

[MED] Charismatic Conqueror resolves the prompt player from the entering permanent's live controller. Evidence: crates/engine/src/game/effects/mod.rs:4688. Why it matters: if the entering permanent changes controllers before the trigger resolves, "they" should still be the event-time controller recorded by the zone-change trigger context, not whoever controls the object later. Suggested fix: carry/resolve the optional actor from the ZoneChangeRecord/trigger context authority and add a control-change-before-resolution regression.

[MED] Current parse-diff evidence still includes unrelated SpellCast watcher changes. Evidence: the coverage-parse-diff sticky updated 2026-07-03T21:05:08Z still changes Menagerie Curator and Volo's trigger/SpellCast.watches signature in addition to the tap-related signatures. Why it matters: that is unexplained parser blast radius for a PR scoped to optional tap handling. Suggested fix: narrow the parser diff, or explain and test why the SpellCast watch changes are intentional in this PR.

[bohdansolovie]

Thanks @matthewevans — all three addressed on the new head (a12967ca7, plus a main merge for the parse-diff point below).

[MED] they may tap dropped the actor scope (clause_shell.rs). The peel now threads "they" out as PlayerFilter::TriggeringPlayer for a tap that names its OWN targets (Bow to My Command: "tap any number of untapped creatures they control with total power 8 or greater") — so the optional prompt and the "creatures they control" choice resolve to the triggering opponent, not the scheme's controller. The bare "that permanent"/"it" self-tap anaphor (Charismatic Conqueror, Dragon Turtle) deliberately keeps player_scope: None: it already lifts to SetTapState { TriggeringSource } and resolves its actor from the zone-change record, and attaching a controller-rebinding player_scope there would leak the triggering player onto the "if they don't, you create a Vampire" tail (verified: doing so mis-created the token under PlayerId(1)). Regressions: peel_optional_slots_they_may_tap_targets_threads_triggering_player, peel_optional_slots_they_may_tap_that_permanent_keeps_no_player_scope, and an end-to-end they_may_tap_targeted_threads_triggering_player_scope parse of the Bow trigger asserting player_scope: TriggeringPlayer while the tap keeps its own target set (never lifts to TriggeringSource).

[MED] Charismatic Conqueror resolved the prompt from the live controller (effects/mod.rs). optional_prompt_player now resolves the SetTapState { TriggeringSource } actor from the EVENT-TIME controller recorded on the trigger's ZoneChangeRecord (TriggeringPlayer → record.controller, CR 603.10a), with TriggeringSourceController kept only as a fallback for non-zone-change shapes. So if the entering permanent changes controllers between the ETB event and the trigger resolving, "they" is still the player who controlled it when it entered. Regression charismatic_conqueror_optional_tap_uses_event_time_controller hands the entering creature to PlayerId(0) after the trigger is on the stack and asserts the prompt still goes to PlayerId(1) (would return PlayerId(0) under the old live-controller read).

[MED] parse-diff included unrelated SpellCast watcher changes (Menagerie Curator, Volo). Those were baseline pollution, not this PR: the branch had merged an older main (2851cea) that predated the Volo fix (#5013, d9437f068), so the diff was reverting Volo's trigger/SpellCast.watches signature against the newer baseline. This PR touches none of that code (git diff main...HEAD is confined to the tap files). I've merged current origin/main, so head and baseline now agree on Volo/Menagerie Curator and the SpellCast signatures drop out — the parse-diff is back to the intended tap class (Charismatic Conqueror, Dragon Turtle, Bow to My Command; Bow additionally now carries the player_scope: TriggeringPlayer from the fix above).

Full engine lib suite (14,879) is green; fmt, clippy, the parser-combinator gate, and the engine-authority gate all pass.

[matthewevans]

Current-head review on 69e675b068ecb887c9a03e602719dffc069f75f2:

The prior implementation blockers appear addressed in the current code, but the parser blast-radius proof is still stale for this head. The coverage-parse-diff sticky predates the current push and still includes unrelated trigger signature changes, while the current card-data/coverage check had not completed when reviewed. Please wait for/regenerate the parse-diff sticky on this exact head and confirm that only the intended tap signatures remain.

[bohdansolovie]

Thanks @matthewevans — confirmed on this exact head 69e675b068ecb887c9a03e602719dffc069f75f2 (no new push). You reviewed at 00:26:06Z while the card-data/coverage job was still running; that job has since finished and the coverage-parse-diff sticky regenerated at 00:39:41Z (≈13 min after your review) from the CI run on this head. It's now fresh, and only the intended tap signatures remain:

### Parse changes introduced by this PR · 3 card(s), 3 signature(s)  (baseline: main 2f8bb7e537f5)

#### 2 card(s) · ability/Tap · field target: parent target → triggering source
Examples: Charismatic Conqueror, Dragon Turtle

#### 1 card(s) · ability/Tap · field target: untapped you control creature → untapped scoped player controls creature
Examples: Bow to My Command

#### 1 card(s) · ability/Tap · field targets: ∅ → 0+
Examples: Bow to My Command

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

All three signatures are ability/Tap in the intended "they may tap" class — no SpellCast/ReduceAbilityCost/static_structure/Ravenous Trap/Suppression Field/Zirda/Teyo/Menagerie Curator/Volo entries. Two independent proofs that this sticky is for the current head, not a stale one:

• The untapped you control creature → untapped scoped player controls creature signature on Bow to My Command is produced by the PlayerFilter::TriggeringPlayer threading added in a12967ca7 — it does not exist on any earlier head, so its presence means the diff was rebuilt after that commit.
• The baseline is main 2f8bb7e5 (current tip, post the origin/main merge in 69e675b0); the earlier stale sticky was against 2851cea9, which is what leaked the Volo SpellCast.watches signature.

All CI checks are green on this head (card-data/coverage, Rust lint, both Rust test shards, WASM/Tauri/frontend). Happy to re-run CI if you'd like a second regenerated sticky to double-confirm.

[matthewevans]

Requesting changes on current head 69e675b068ecb887c9a03e602719dffc069f75f2.

[MED] The new optional_prompt_player comment still cites CR 603.10a for the Charismatic Conqueror ETB optional-tap path. Evidence: crates/engine/src/game/effects/mod.rs:4715; the local Comprehensive Rules text for 603.10a covers look-back zone-change triggers such as leaves-the-battlefield, sacrifice, leaves-graveyard, and visible-object-to-hand/library triggers. It does not describe an object entering the battlefield.

The implementation may still be using the right engine provenance (ZoneChangeRecord event-time controller), and CR 608.2d is still relevant for making choices during resolution. Please remove CR 603.10a from this ETB prompt-controller comment and describe the event-time controller lookup as engine trigger-context provenance rather than citing that rule here.

[matthewevans]

Requesting changes on the current head.

[HIGH] The new they may tap parser branch still uses ad-hoc string dispatch and allocations. Evidence: crates/engine/src/parser/clause_shell.rs:307-330 calls to_lowercase() twice and classifies the remainder with starts_with() over a string array. Why it matters: parser dispatch must use the existing nom combinator building blocks; this also adds avoidable allocation to a shared clause-peeling path. Suggested fix: compose the anaphor alternatives with tag/alt (through the existing lowercase bridge) and return the original remainder without manual prefix scanning.

[HIGH] Wrong Comprehensive Rules citation remains in the ordinary-trigger path. Evidence: crates/engine/src/game/effects/mod.rs:415 and crates/engine/src/parser/oracle_trigger.rs:1660 cite CR 603.7c, which the current CR text defines for delayed triggered abilities; these functions handle normal trigger-event provenance. Suggested fix: remove the delayed-trigger citation and use the verified ordinary trigger/zone-change/anaphor rules (CR 603.2, CR 603.6, and/or CR 608.2k) that describe the implemented behavior.

[MED] Bow to My Command has only parser-shape coverage for the new triggering-player scope. Evidence: crates/engine/src/parser/oracle_trigger_tests.rs:22827-22871 asserts PlayerFilter::TriggeringPlayer, but no runtime scenario drives the optional prompt and target selection through WaitingFor/GameAction. Why it matters: the generalized player_scope at crates/engine/src/parser/clause_shell.rs:329 can be structurally correct while the resolving prompt or selected creatures still use the scheme controller. Suggested fix: add a runtime regression with an opponent-triggered Bow-like ability that verifies the opponent receives the prompt and controls the selected creatures.