Skip to content

fix(engine): Winter Soldier attack trigger and Hero counter rider (#4560)#4580

Merged
matthewevans merged 18 commits into
phase-rs:mainfrom
kiannidev:fix/4560-winter-soldier-attack-trigger
Jun 30, 2026
Merged

fix(engine): Winter Soldier attack trigger and Hero counter rider (#4560)#4580
matthewevans merged 18 commits into
phase-rs:mainfrom
kiannidev:fix/4560-winter-soldier-attack-trigger

Conversation

@kiannidev

Copy link
Copy Markdown
Contributor

Summary

  • Fix misclassification of Winter Soldier, Reborn Avenger's oracle text as an enters-with replacement; trigger lines with reflexive "enters this way" counter riders now route through trigger dispatch.
  • Parse present-tense "enters this way" zone-change gates and the "it enters with an additional +1/+1 counter" rider on graveyard reanimation.
  • Add parser and integration tests covering parse output and in-game Hero reanimation with an extra counter.

Fixes #4560

Test plan

  • cargo test -p engine --lib winter_soldier_reborn_avenger
  • cargo test -p engine --test integration issue_4560

Made with Cursor

…er (phase-rs#4560)

Route trigger lines with reflexive "enters this way" counter riders through trigger dispatch instead of the enters-with replacement interceptor, and parse the Hero +1/+1 rider on graveyard reanimation.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev kiannidev requested a review from matthewevans as a code owner June 29, 2026 01:15
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 23 card(s), 28 signature(s) (baseline: main 315c1235db94)

5 card(s) · ability/ApplyPerpetual · added: ApplyPerpetual

Examples: Blooming Cactusfolk, Charged Conjuration, Fearsome Whelp (+2 more)

3 card(s) · static/Continuous · field conditional: unrecognizedcards in hand (you) > cards in hand (max of opponents)

Examples: Descendant of Kiyomaro, Okina Nightwatch, Secretkeeper

3 card(s) · trigger/Phase · field condition: cards in hand (you) > cards in hand (max of opponents)

Examples: Akuta, Born of Ash, Death of a Thousand Stings, Exile into Darkness

2 card(s) · ability/Attach · added: Attach (conditional=Equipment changed zones this way, target=you control creature)

Examples: Adaptive Armorer, Masterpiece Vault

2 card(s) · ability/CreateDelayedTrigger · removed: CreateDelayedTrigger (when=when any target enters)

Examples: Adaptive Armorer, Masterpiece Vault

1 card(s) · ability/ApplyPerpetual · added: ApplyPerpetual (conditional=additional cost was paid)

Examples: Talion's Throneguard

1 card(s) · trigger/Attacks · added: Attacks (active in=battlefield, watches=self)

Examples: Winter Soldier, Reborn Avenger

1 card(s) · static/Continuous · added: Continuous (affects=you control creature Human, mods=add chosen keyword)

Examples: Greymond, Avacyn's Stalwart

1 card(s) · static/Continuous · field conditional: unrecognizednot (in battlefield opponent controls creature is present)

Examples: Erebos's Titan

1 card(s) · replacement/CreateToken · added: CreateToken

Examples: Jinnie Fay, Jetmir's Second

1 card(s) · replacement/Moved · added: Moved

Examples: Greymond, Avacyn's Stalwart

1 card(s) · replacement/Moved · removed: Moved

Examples: Winter Soldier, Reborn Avenger

1 card(s) · trigger/Phase · field condition: # of 1+ bounty counters in battlefield opponent controls permanent = 0

Examples: Chevill, Bane of Monsters

1 card(s) · trigger/Phase · field condition: # of in battlefield opponent controls creature = 0

Examples: Kezzerdrix

1 card(s) · trigger/Phase · field condition: life lost this turn (you) ≥ 2

Examples: The Book of Vile Darkness

1 card(s) · ability/PutCounter · added: PutCounter (conditional=revealed is creature, counter=2 P1P1, target=parent target)

Examples: What Must Be Done

1 card(s) · ability/RaiseCost · removed: RaiseCost (affects=any target, conditional=additional cost was paid, duration=until end of turn, grants=RaiseCost, target=parent target)

Examples: Talion's Throneguard

1 card(s) · ability/RaiseCost · removed: RaiseCost (affects=any target, duration=until end of turn, grants=RaiseCost, target=parent target)

Examples: Paths of Tuinvale

1 card(s) · ability/ReduceCost · removed: ReduceCost (affects=any target, duration=until end of turn, grants=ReduceCost, target=parent target)

Examples: Paths of Tuinvale

1 card(s) · ability/ReduceCost · removed: ReduceCost (affects=in hand you control Dragon, grants=ReduceCost)

Examples: Fearsome Whelp

1 card(s) · ability/creature · removed: creature

Examples: Fountainport Charmer

1 card(s) · ability/instant · removed: instant

Examples: Charged Conjuration

1 card(s) · ability/it · field conditional: land changed zones this way

Examples: Silver Surfer, Cosmic Voyager

1 card(s) · ability/it · removed: it (conditional=revealed is creature)

Examples: What Must Be Done

1 card(s) · ability/nonland · removed: nonland

Examples: Blooming Cactusfolk

… 3 more signature(s) (3 card-changes) — see parse-diff.json
  • 1 card(s) · ability/replacement_structure · removed: replacement_structure
  • 1 card(s) · ability/static_structure · removed: static_structure
  • 1 card(s) · ability/unknown · removed: unknown

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] The Hero "enters with" rider is modeled as a later counter effect instead of an entry-time counter. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:884 emits Effect::PutCounter, which resolves through the normal counter effect path (crates/engine/src/game/effects/counters.rs:1021) after the zone move, while actual "enters with" counters are applied inside the zone-change entry pipeline (crates/engine/src/game/zone_pipeline.rs:1758). Why it matters: "it enters with an additional +1/+1 counter" is an entry replacement-style property of the battlefield entry, so entry-time observers/replacements can differ even though the final counter count test passes. Suggested fix: carry the conditional Hero rider into the ChangeZone/entry pipeline as conditional enter_with_counters (or an equivalent entry-time structure), and add a test with an entry-time observer that distinguishes entering with the counter from receiving it afterward.

Resolve integration test module conflict after phase-rs#4575 merge.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] The Hero "enters with" rider is still modeled as a post-entry counter effect. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:884 still emits Effect::PutCounter { target: TargetFilter::ParentTarget } for "it enters with an additional +1/+1 counter on it; that runs after the parent ChangeZonehas already moved the object, instead of carrying the counter into the entry pipeline viaChangeZone.enter_with_counters. Why it matters: entry-time observers/replacements can distinguish entering with the counter from receiving the counter afterward, so the final counter-count integration test does not prove the rules behavior. Suggested fix: carry the conditional Hero rider into the ChangeZone/entry pipeline as conditional entry counters, and add a regression that distinguishes entry-time counters from a later PutCounter`.

bohdansolovie and others added 2 commits June 29, 2026 15:51
…phase-rs#4560)

Carry conditional Hero riders as ChangeZone.conditional_enter_with_counters
so counters apply in the zone-change entry pipeline instead of a post-move
PutCounter sub-ability, addressing review feedback on entry-time semantics.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] The regression coverage still does not distinguish entry-time counters from a post-entry counter effect. Evidence: crates/engine/tests/integration/issue_4560_winter_soldier.rs:183 only asserts the returned Hero's final battlefield zone and final +1/+1 counter count, while the prior failure mode could produce the same final state with a later PutCounter; the parse assertion at crates/engine/tests/integration/issue_4560_winter_soldier.rs:112 proves the AST shape but does not exercise an entry-time observer/replacement through the runtime pipeline. Why it matters: the original bug class was timing-sensitive: "enters with" must be visible during battlefield entry, not merely after resolution. Suggested fix: add a runtime regression with an entry-time observer/replacement/counter-modifier interaction that fails if the Hero counter is applied after the zone move rather than inside the entry pipeline.

Keep both issue_4560_winter_soldier and issue_4564_captain_america modules.

Co-authored-by: Cursor <cursoragent@cursor.com>
@kiannidev

Copy link
Copy Markdown
Contributor Author

Merged latest main, resolved main.rs conflict. Hero counter rider uses conditional_enter_with_counters on ChangeZone (not post-entry PutCounter); parse + integration tests updated accordingly. CI re-running — ready for re-review.

bohdansolovie and others added 2 commits June 30, 2026 04:41
Co-authored-by: Cursor <cursoragent@cursor.com>
Fill missing struct fields in change_zone tests and iteration ctx after
the Winter Soldier entry-counter schema extension.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] Resolution-choice ChangeZone paths drop the new conditional entry counters. Evidence: crates/engine/src/game/effects/change_zone.rs:587 creates WaitingFor::EffectZoneChoice without carrying conditional_enter_with_counters, and crates/engine/src/game/engine_resolution_choices.rs:2795 resumes with enter_with_counters: vec![] / conditional_enter_with_counters: vec![]. Why it matters: any resolution-time "return/put a card ... If a [type] enters this way, it enters with ..." shape can parse into the new field but enter without the rider when it goes through the choice path. Suggested fix: carry the conditional specs or resolved per-object counters through EffectZoneChoice, then compute/merge per-object counters before process_one_zone_move.

[MED] The prior timing-test blocker is still not fixed. Evidence: crates/engine/tests/integration/issue_4560_winter_soldier.rs:183 only asserts the final battlefield zone, and crates/engine/tests/integration/issue_4560_winter_soldier.rs:188 only asserts the final counter count. Why it matters: a post-entry PutCounter implementation can satisfy the same final-state assertion, so the regression still does not prove the Hero counter is applied as an entry-time counter rather than after the zone move. Suggested fix: add a runtime test with an ETB counter replacement/observer that fails if the Hero counter is applied after the object enters.

bohdansolovie and others added 3 commits June 30, 2026 10:26
…#4560)

Carry unconditional and conditional enter-with-counter specs across the
EffectZoneChoice round-trip and merge them per chosen object before zone
moves. Add a runtime regression that proves the Winter Soldier Hero rider
folds into ChangeZone entry counters instead of resolving a separate
PutCounter effect afterward.

Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] The EffectZoneChoice conditional-entry-counter path still has no discriminating regression. Evidence: crates/engine/src/game/engine_resolution_choices.rs:2648 now carries conditional_enter_with_counters through WaitingFor::EffectZoneChoice, and crates/engine/src/game/engine_resolution_choices.rs:2806-2816 applies it when a selected card is moved, but the new Winter Soldier regression drives the normal targeted ChangeZone trigger path (crates/engine/tests/integration/issue_4560_winter_soldier.rs:249) and the existing change_zone.rs test fixtures still construct EffectZoneChoice/ChangeZone cases with conditional_enter_with_counters: vec![]. Why it matters: the last blocker was specifically that resolution-choice ChangeZone paths dropped this rider; without a choice-path fixture, a future regression can erase the newly threaded field while the Winter Soldier tests still pass. Suggested fix: add a focused regression that reaches the EffectZoneChoice SelectCards branch with a non-empty conditional_enter_with_counters rider and verifies the selected matching card enters with the counter while a non-matching card does not.

…#4560)

Merge upstream/main and drop conditional_enter_with_counters from ChangeZoneAll
test literals while filling the field on remaining ChangeZone initializers.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] Missing regression for the previously blocked EffectZoneChoice conditional-counter path.

The production path now carries conditional_enter_with_counters into WaitingFor::EffectZoneChoice (crates/engine/src/game/effects/change_zone.rs) and applies it in the SelectCards branch (crates/engine/src/game/engine_resolution_choices.rs), but the new EffectZoneChoice regression still initializes conditional_enter_with_counters: vec![]. The Winter Soldier integration tests cover the targeted attack-trigger path and final Hero counter result, but they do not discriminate the specific interactive EffectZoneChoice SelectCards branch that previously dropped this field.

Please add a focused regression that parks or reaches WaitingFor::EffectZoneChoice with a non-empty conditional_enter_with_counters rider, selects at least one matching Hero and one non-matching creature/card through GameAction::SelectCards, and asserts only the matching selected object enters with the extra counter. That would fail if the field were dropped during the EffectZoneChoice round trip.

…hase-rs#4560)

Add a focused change_zone test that parks EffectZoneChoice with a non-empty
conditional_enter_with_counters rider and verifies SelectCards applies the
counter only to matching Heroes. Fill the new ChangeZone field across
phase-ai and mtgish-import initializers to fix workspace compile failures.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this head. CI is failing in the PR's own Winter Soldier parser coverage: panics at with . Because that failure is directly in the touched behavior, I can't treat it as merge-when-ready noise. Please update the parser/test path so the current Winter Soldier representation is internally consistent and the new entry-time counter modeling remains covered.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding this head. CI is failing in the PR Winter Soldier parser coverage: parser::oracle_trigger::tests::winter_soldier_reborn_avenger_attack_reanimation_trigger panics at crates/engine/src/parser/oracle_trigger.rs:14474 with Hero counter rider. Because that failure is directly in the touched behavior, I cannot treat it as merge-when-ready noise. Please update the parser/test path so the current Winter Soldier representation is internally consistent and the new entry-time counter modeling remains covered.

…ers (phase-rs#4560)

Update oracle_trigger coverage to expect the Hero counter rider folded into
ChangeZone.conditional_enter_with_counters instead of a PutCounter sub-ability.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] The EffectZoneChoice resume path still drops conditional entry counters in production. Evidence: crates/engine/src/game/engine_resolution_choices.rs:2797 looks up the resolving ability only by scanning state.stack, but resolve_top pops the entry before resolving and stores it in state.resolving_stack_entry (crates/engine/src/game/stack.rs:192, crates/engine/src/game/stack.rs:460). The new regression masks this by manually pushing a fake stack entry before SelectCards (crates/engine/src/game/effects/change_zone.rs:4457), so it exercises a state shape production resolution does not have. Why it matters: when a real resolving ChangeZone pauses for EffectZoneChoice, stack_ability is None, so the branch falls back to enter_with_counters.clone() and silently ignores conditional_enter_with_counters; the Hero selected through this path will not get the entry-time counter. Suggested fix: resolve the ability from the active resolving entry as well as the live stack, and change the regression to model production by using state.resolving_stack_entry rather than pushing a fake live stack entry.

@matthewevans matthewevans added bug Bug fix area:engine Core rules engine labels Jun 30, 2026
@matthewevans matthewevans added the area:parser Oracle text parser label Jun 30, 2026
…entry (phase-rs#4560)

EffectZoneChoice resume looked up the resolving ability only on the live
stack, but resolve_top pops the entry into resolving_stack_entry first.
Fall back to that entry when applying conditional_enter_with_counters, and
model production in the regression via resolving_stack_entry instead of a
fake live stack push.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] Conditional entry counters are lost or copied incorrectly after a selected zone move pauses. Evidence: crates/engine/src/game/engine_resolution_choices.rs:2812 computes per_obj_enter_counters for the current selected card, but the pause stashes only that resolved vector into PendingChangeZoneIteration.enter_with_counters (crates/engine/src/game/engine_resolution_choices.rs:2863, crates/engine/src/game/engine_resolution_choices.rs:2895); the drain path then reconstructs ChangeZoneIterationCtx with conditional_enter_with_counters: vec![] (crates/engine/src/game/effects/mod.rs:744) and reuses the same enter_with_counters for every remaining object. Why it matters: if the first selected object pauses for an Aura attachment or replacement/as-enters choice, later selected objects no longer get per-object conditional evaluation, so later Heroes can miss the counter or non-Heroes can inherit the paused object's counter. Suggested fix: carry the conditional counter specs plus resolving ability provenance through PendingChangeZoneIteration and recompute enter_with_counters_for_object for each remaining object during drain, or store per-object resolved counters for the remaining queue.

…ase-rs#4560)

Thread conditional_enter_with_counters specs through PendingChangeZoneIteration
instead of stashing one object's resolved counter vector. Recompute per-object
entry counters in drain_pending_change_zone_iteration via resolving_stack_entry,
and add a focused drain regression for Hero vs non-Hero remaining objects.

Co-authored-by: Cursor <cursoragent@cursor.com>

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[MED] Conditional "enters this way" counters are skipped on the single-pick fast paths. Evidence: crates/engine/src/game/effects/change_zone.rs:460 and crates/engine/src/game/effects/change_zone.rs:525 both call execute_zone_move with only &effect_enter_with_counters; unlike the main multi-object loop and the EffectZoneChoice path, neither branch evaluates effect_conditional_enter_with_counters with enter_with_counters_for_object. Why it matters: a ChangeZone effect that resolves to one eligible/random object can enter a matching Hero without the entry-time counter even though the multi-object/choice path is now correct. Suggested fix: compute the per-object counter vector in both single-pick branches before moving the object, or route those branches through process_one_zone_move with ChangeZoneIterationCtx, and add a single-eligible conditional-counter regression.

…phase-rs#4560)

The random-single and single-eligible ChangeZone fast paths called
execute_zone_move with only unconditional enter_with_counters. Merge
conditional riders per object before the move, and add a regression for
the lone eligible Hero graveyard return case.

Co-authored-by: Cursor <cursoragent@cursor.com>
@matthewevans matthewevans self-assigned this Jun 30, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head. The Winter Soldier entry-time counter modeling now carries conditional entry-counter specs through the normal, EffectZoneChoice, pause/resume, and single-pick ChangeZone paths with discriminating engine coverage. Merge when ready.

@matthewevans matthewevans added this pull request to the merge queue Jun 30, 2026
@matthewevans matthewevans removed their assignment Jun 30, 2026
Merged via the queue into phase-rs:main with commit ebaa46d Jun 30, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:engine Core rules engine area:parser Oracle text parser bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Card Bug] Winter Soldier, Reborn Avenger attack trigger does not fire

3 participants