fix(parser): support "attacking the last chosen player" statics (Triarch Stalker)#5393
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements a mechanism to reconcile and persist player choices when they are later referenced by continuous static abilities (such as 'Creatures attacking the last chosen player...'). It introduces parser support for 'the last chosen player' and 'the chosen player' scopes, along with comprehensive unit tests. The review feedback highlights two key areas for improvement: first, a high-severity issue where the detection of source-chosen player references misses checking triggered abilities, and second, a medium-severity issue regarding the incomplete coverage of player-targeting effects in effect_targets_source_chosen_player which currently only matches damage effects.
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.
| let references_source_chosen_player = result | ||
| .statics | ||
| .iter() | ||
| .any(static_references_source_chosen_player) | ||
| || result | ||
| .abilities | ||
| .iter() | ||
| .any(ability_references_source_chosen_player); |
There was a problem hiding this comment.
[HIGH] Missing trigger check in references_source_chosen_player detection. Evidence: crates/engine/src/parser/oracle.rs:1305-1312.
Why it matters: If a card has a triggered ability that references the chosen player but has no statics or activated abilities doing so, the choice won't be persisted.
Suggested fix: Include a check for result.triggers using ability_references_source_chosen_player on trigger.execute.
let references_source_chosen_player = result
.statics
.iter()
.any(static_references_source_chosen_player)
|| result
.abilities
.iter()
.any(ability_references_source_chosen_player)
|| result
.triggers
.iter()
.any(|t| {
t.execute
.as_ref()
.is_some_and(|a| ability_references_source_chosen_player(a))
});| fn effect_targets_source_chosen_player(effect: &Effect) -> bool { | ||
| match effect { | ||
| Effect::DealDamage { target, .. } => filter_references_source_chosen_player(target), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Incomplete effect coverage in effect_targets_source_chosen_player. Evidence: crates/engine/src/parser/oracle.rs:1345-1350.
Why it matters: Matching only Effect::DealDamage is a single-case implementation that fails to cover other common player-targeting effects (such as life loss, life gain, drawing, discarding, or milling) that might reference the chosen player.
Suggested fix: Expand the match to cover other player-targeting Effect variants, or add a TODO/comment to ensure they are added when those cards are implemented.
Parse changes introduced by this PR · 2 card(s), 4 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
[MED] SourceChosenPlayer references inside triggers do not make the preceding player choice persistent. Evidence: crates/engine/src/parser/oracle.rs:1305 only scans result.statics and result.abilities before flipping trigger choices to persist: true; result.triggers is only visited later as a writer at crates/engine/src/parser/oracle.rs:1319, and existing trigger parsing can produce TargetFilter::SourceChosenPlayer (crates/engine/src/parser/oracle_trigger.rs:11740, covered by crates/engine/src/parser/oracle_trigger_tests.rs:13285). Why it matters: a card that chooses a player and later has a triggered ability keyed to “the chosen player” would still lose the chosen player after the choose instruction resolves. Suggested fix: include trigger execute abilities in the SourceChosenPlayer-reference scan, with a regression that fails before the trigger scan is added.
[MED] The new effect detector reimplements target discovery for only DealDamage instead of using the existing effect target API. Evidence: crates/engine/src/parser/oracle.rs:1345 only checks Effect::DealDamage { target, .. }, while Effect::target_filter() already surfaces many target-bearing player/object effects such as Draw, GainLife, LoseLife, Token owner, SearchLibrary, etc. at crates/engine/src/types/ability.rs:13042. Why it matters: the persistence pass now silently misses any non-damage ability that reads TargetFilter::SourceChosenPlayer, so the helper is a one-effect special case despite being wired as the general source-chosen-player detector. Suggested fix: drive effect_targets_source_chosen_player through effect.target_filter().is_some_and(filter_references_source_chosen_player) and add at least one non-damage sibling regression.
…e-rs#5391) Triarch Stalker and Beckoning Will-o'-Wisp pair a combat trigger ("At the beginning of combat on your turn, choose an opponent") with a continuous static ("Creatures attacking the last chosen player have menace" / "get +1/+0") that reads the chosen player. Neither worked: the static line failed to parse, and the choice was not persisted for a continuous ability to read. Two parser changes route this to existing, tested runtime: 1. `parse_attacking_defender_scope` gains "the last chosen player" / "the chosen player" -> `ControllerRef::SourceChosenPlayer`, alongside the existing you / your opponents / enchanted-player scopes. The static now parses to `FilterProp::Attacking { defender: Some(SourceChosenPlayer) }` + the menace / +1/+0 modification, resolved by the existing `attacking_defender_matches` -> `controller_ref_player` -> `source_chosen_player` path (no runtime change). 2. New `reconcile_persisted_player_choice_for_source_chosen_ref` post-pass flips a `choose a player` / `choose an opponent` effect to `persist: true` when the card carries a durable `SourceChosenPlayer` reference, so the choice is stored as `ChosenAttribute::Player` for the static to read. Targeted: a same-resolution opponent choice (Ruhan, which reads the resolution-scoped `ChosenPlayer { index }`) is left `persist: false`. Tests: static scope parsing (both phrasings), full-card persist flip, negative Ruhan-class regression, and runtime `attacking_defender_matches` resolution against a persisted chosen player. CR 508.1b + CR 613.1 + CR 608.2c.
…5391) Address review: the SourceChosenPlayer-reference detection scanned only statics and activated abilities, so a card whose ONLY chosen-player reference is a triggered ability (a phase trigger scoped to "the chosen player's upkeep", or a trigger effect keyed to the chosen player) would not have its preceding "choose a player" / "choose an opponent" flipped to persist — the choice would be lost after it resolved. - `reconcile_persisted_player_choice_for_source_chosen_ref` now also scans `result.triggers` as readers via a new `trigger_references_source_chosen_player`, which checks both the trigger's own `valid_target` (the phase-trigger scope) and its executed effect chain. - `effect_targets_source_chosen_player` now uses the generic `Effect::target_filter()` accessor instead of matching only `Effect::DealDamage`, so every player-targeting effect variant (life loss/gain, draw, discard, mill, ...) is covered. Regression test: a card whose chosen-player reference lives only in a phase trigger flips the ETB choose to persist (fails before the trigger reader scan is added). CR 613.1 + CR 608.2c + CR 503.1a.
8b3c407 to
d72814b
Compare
|
Thanks @matthewevans — good catch, both points addressed in the latest push. [HIGH/MED] Triggers not scanned as readers: [MEDIUM] Single-case effect coverage: Regression (fails before the trigger-reader scan): Full engine lib suite (15,820) + |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: current head resolves the trigger-reader and generic target-filter findings; parse diff is scoped to the chosen-player attacker statics, and required checks are green.
Closes #5391.
What
Makes Triarch Stalker and Beckoning Will-o'-Wisp work. Both pair a combat trigger "At the beginning of combat on your turn, choose an opponent" with a continuous static "Creatures attacking the last chosen player have menace / get +1/+0". Neither functioned: the static line failed to parse, and the opponent choice was not persisted for a continuous ability to read.
How (routes to existing runtime, no engine changes)
1. Static scope —
parse_attacking_defender_scopegains"the last chosen player"/"the chosen player"→ControllerRef::SourceChosenPlayer, alongside the existing you / your-opponents / enchanted-player scopes (samealt, one new axis value). The static parses toFilterProp::Attacking { defender: Some(SourceChosenPlayer) }+ the modification, resolved by the existingattacking_defender_matches→controller_ref_player→source_chosen_playerpath.2. Durable persist — new
reconcile_persisted_player_choice_for_source_chosen_refpost-pass flips achoose a player/choose an opponenteffect topersist: trueonly when the card carries a durableSourceChosenPlayerreference, so the choice is stored asChosenAttribute::Player. This mirrors the "persist when referred back to" intent documented onEffect::Choose, and the runtime already re-runs layers for persistedPlayer/Opponentchoices.Targeted: a same-resolution opponent choice (Ruhan — reads the resolution-scoped
ChosenPlayer { index }) has no durable reference and is leftpersist: false.Tests
Attacking { defender: SourceChosenPlayer }+ menace / +1/+0.Choose { Opponent, persist: true }.persist: false(pass stays targeted).attacking_defender_matchesresolves against a persisted chosen player (matches the chosen player, rejects others).CR
CR 508.1b (attacking-player scope) + CR 613.1 / CR 608.2c (persisted "chosen player" read by a continuous ability).
Verification
cargo fmt --all✓ · engine lib suite 15,812 pass / 0 fail ✓ ·clippy -p engine --all-targets -D warningsclean ✓