fix(parser): bind PutCounterAll "untap them" to countered set (#5286)

[andriypolanski]

Summary

Lulu, Loyal Hollyphant was listed as fully supported, but at the beginning of your end step (with revolt satisfied) it put +1/+1 counters on tapped creatures you control and then failed to untap them — only Lulu herself would untap. This fix rewrites the chained plural untap anaphor after a mass counter head to TrackedSet(0) so runtime resolution binds the creatures that received counters.

Fixes #5286.

Root cause

Lulu's end-step trigger parses correctly through PutCounterAll (each tapped creature you control), but the "then untap them" sub lowered to SetTapState { target: SelfRef, scope: Single }.

Phase-trigger bodies carry ctx.subject = Any, so resolve_it_pronoun binds bare "them" to the trigger source (Lulu) instead of the countered population. ParentTarget would also no-op at runtime because PutCounterAll does not populate ability.targets.

Changes

Parser — lowering (oracle_effect/lower.rs)

• Added patch_population_head_tap_anaphor (sibling of patch_self_ref_head_tap_anaphor): when a broadcast PutCounterAll head chains a single-scope tap/untap anaphor (SelfRef or ParentTarget), rewrite to TrackedSet(0) so tap_untap.rs consumes the set published by CounterAdded events (CR 608.2c + CR 122.1).

Tests

• Parser: lulu_loyal_hollyphant_counters_then_untap_them_lowers_correctly, put_counter_all_head_plural_untap_rewrites_to_tracked_set
• Integration: issue_5286_lulu_loyal_hollyphant.rs — revolt + tapped creatures get counters and untap; no revolt → no effect

Out of scope

• Card-data regeneration (cargo card-data); runtime from_oracle_text path is fixed and integration tests use live Oracle text.
• Generalizing to other mass-effect heads (e.g. PumpAll + untap them); patch is scoped to PutCounterAll publishers.

Test plan

• cargo fmt --all
• cargo test -p engine --lib lulu_loyal_hollyphant_counters_then_untap_them
• cargo test -p engine --lib put_counter_all_head_plural_untap_rewrites_to_tracked_set
• cargo test -p engine --test integration issue_5286 (2/2)

Risk notes

• Gate is narrow: only PutCounterAll with a broadcast population filter, immediate chained SetTapState with scope: Single and SelfRef/ParentTarget. patch_self_ref_head_tap_anaphor and targeted-counter chains are unchanged.
• Covers the class of "put a counter on each …, then untap them" (Lulu and future cards with the same shape).

[gemini-code-assist]

Code Review

This pull request implements support for resolving plural anaphors (e.g., "then untap them") following mass counter placement effects (PutCounterAll), specifically addressing the behavior of Lulu, Loyal Hollyphant (Issue #5286). It introduces patch_population_head_tap_anaphor to rewrite the target of the chained tap/untap effect to a tracked set, and adds corresponding integration tests. The feedback suggests avoiding swallowing errors with .ok() in the test driver and instead using .expect(...) to ensure unexpected failures are surfaced.

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.

[gemini-code-assist]

Avoid swallowing all errors (e.g., using .ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs. Since OrderTriggers and PassPriority are expected to succeed in these states, use .expect(...) to ensure any unexpected failures are surfaced immediately.

Suggested change

	WaitingFor::OrderTriggers { .. } => {
	runner
	.act(GameAction::OrderTriggers { order: vec![0] })
	.ok();
	}
	WaitingFor::Priority { .. } if runner.state().phase == Phase::End => {
	if runner.state().stack.is_empty() {
	return;
	}
	runner.act(GameAction::PassPriority).ok();
	}
	WaitingFor::OrderTriggers { .. } => {
	runner
	.act(GameAction::OrderTriggers { order: vec![0] })
	.expect("order triggers");
	}
	WaitingFor::Priority { .. } if runner.state().phase == Phase::End => {
	if runner.state().stack.is_empty() {
	return;
	}
	runner.act(GameAction::PassPriority).expect("pass priority");
	}

References

1. Avoid swallowing all errors (e.g., using .ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.

[github-actions]

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

1 card(s) · ability/Untap · field target: self → tracked set #0

Examples: Lulu, Loyal Hollyphant

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

[matthewevans]

APPROVE (maintainer sign-off) — fix(parser): bind PutCounterAll "untap them" to the countered set (#5286).

Seam. Direct sibling of the existing patch_self_ref_head_tap_anaphor, sharing the lower_effect_chain_ir chokepoint (call site lower.rs:2328, immediately after the SelfRef-head sibling). It must run at lowering time: by resolution the SelfRef-vs-countered-set discriminator is erased. Correct location, most idiomatic implementation (typed matches! on Effect variants, no string dispatch — nom-irrelevant, this is an AST pass).

Blast radius — confronted, not assumed. Parse-diff sticky (baseline main 7b5b324d5751, updated 2026-07-10T07:19:52Z) reports exactly 1 card, 1 signature: ability/Untap field target: self → tracked set #0, Lulu, Loyal Hollyphant. Freshness chain holds: head push 07:08:34Z → Card-data check 07:19:36Z → sticky 07:19:52Z. Because this is a parser-surface PR, a clean no_changes sticky would have signalled an inert fix (the #5480 failure mode). This one fires on the real card; the test's LULU_ORACLE is character-identical to the printed Oracle text (no hand-normalization).

Gating verified by code + data.

• is_population_counter_publisher returns true only for Effect::PutCounterAll whose target is NOT SelfRef|ParentTarget|TriggeringSource|CostPaidObject — so a targeted counter effect never trips it.
• The inner rewrite touches only SetTapState { scope: Single, target: SelfRef | ParentTarget }; an existing TrackedSet target is left alone.
• Sibling trace (demonstrative "untap those creatures"): Virtue of Loyalty and The Fifth Doctor already lower their untap sub to SetTapState { target: TrackedSet #0 } — untouched by the patch (confirmed both in card-data and by the 1-card sticky).
• Negative trace (targeted counter): Leo's Guidance's head is PutCounter (single, targeted), not PutCounterAll, and its head filter is creature-scoped → active_population stays false → the branch never fires → its SetTapState { target: ParentTarget } binding survives.

Test discrimination. issue_5286_lulu_... drives the real end-step trigger through the pipeline. If the fix were reverted, "them" would lower to SelfRef and bind Lulu (untapped, not in the countered set), so the !obj.tapped assertions on Tapped A / Tapped B would fail. The paired +1/+1 == Some(1) assertion is a positive reach-guard proving the trigger fired (guards against a vacuous pass). The no-revolt test pins the intervening-if.

One LOW (noted, not blocking). The drive_end_step_stack helper swallows OrderTriggers/PassPriority with .ok() rather than .expect(...), so an unexpected Err (e.g. a priority-loop freeze) is masked into a less precise failure signal. Impact is bounded: the 0..64 loop plus the downstream counter/untap assertions still fail the test. This confirms Gemini's single comment, downgraded to LOW.

All 13 required checks green.