fix(parser): bind "They" to an earlier mass-effect population, not the trigger's source (Ardbert, Warrior of Darkness)

[galuis116]

Problem

Ardbert, Warrior of Darkness (#5985):

Whenever you cast a white spell, put a +1/+1 counter on each legendary creature you control. They gain vigilance until end of turn.
Whenever you cast a black spell, put a +1/+1 counter on each legendary creature you control. They gain menace until end of turn.

The report -- "his ability only partially fires" -- is exact: the counters landed, the keyword grant did nothing. The parse shows why:

PutCounterAll { P1P1, target: Typed{Creature, You, [Legendary]} }       <- correct
sub_ability: GenericEffect {
  static_abilities: [{ affected: TriggeringSource,                      <- BUG
                       modifications: [AddKeyword(Vigilance)] }],
  duration: UntilEndOfTurn }

resolve_they_pronoun's object-type trigger-subject arm resolved "They" to TargetFilter::TriggeringSource -- the cast spell that triggered the ability. The grant was applied to an object on the stack, so no creature ever gained vigilance/menace. Both halves of the card are affected.

Why ParentTarget is not the answer

My first instinct was to route this through parent_target_available / ParentTarget. That is wrong, and the codebase says so: per chain_has_prior_typed_referent and has_typed_target_widened's single-target whitelist, that axis tracks a CHOSEN referent. A mass ("each ...") effect chooses nothing -- PutCounterAll is deliberately absent from that whitelist -- so ParentTarget has nothing to bind to. The anaphor must inherit the population filter itself.

Fix

Add ParseContext::chain_prior_mass_population, seeded by the effect-chain loop from the most-recent earlier mass clause:

fn clause_ir_mass_population(clause: &ClauseIr) -> Option<TargetFilter> {
    match &clause.parsed.effect {
        Effect::PutCounterAll { target, .. }
        | Effect::PumpAll { target, .. }
        | Effect::DestroyAll { target, .. }
        | Effect::BounceAll { target, .. } => Some(target.clone()),
        _ => None,
    }
}

Those are exactly the variants whose target is a population filter rather than a chosen target -- the same reason they are absent from has_typed_target_widened.

resolve_they_pronoun's object-type arm now prefers that population and falls back to TriggeringSource only when no nearer antecedent exists, so the existing "Whenever one or more creatures attack, they get +1/+0" shape (where the trigger's source genuinely IS the antecedent) is unchanged.

This mirrors the established chain-context pattern (chain_prior_hand_reveal_target, chain_has_prior_exile_producer) rather than inventing a mechanism, and is class-level: any <mass effect over a population>. They <grant> trigger benefits.

Tests

• they_after_mass_effect_binds_that_population_not_trigger_source -- production path over the full card: both halves bind their grant's affected to the population the PutCounterAll actually acted on. The expectation is read from the parsed filter itself rather than a hand-written copy, so the test pins the binding, not a duplicated literal.

Full engine lib suite green (16,754 passed, 0 failed) -- the anaphora change surfaced no regressions. cargo fmt, clippy -p engine --tests, and the parser-combinator gate clean.

Closes #5985

[gemini-code-assist]

Code Review

This pull request introduces chain_prior_mass_population to ParseContext to correctly resolve the pronoun 'they' to a prior mass effect's population instead of defaulting to the triggering source. Feedback on the changes highlights a critical omission where chain_prior_mass_population is never actually populated in the effect-chain loop, rendering the new logic ineffective. Additionally, the review notes a violation of style guide rule R6, pointing out that the comment incorrectly cites CR 608.2c instead of CR 201.5c for pronoun resolution.

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]

[HIGH] Missing population of chain_prior_mass_population in the effect-chain loop. Evidence: crates/engine/src/parser/oracle_ir/context.rs:263.
Why it matters: Without seeding chain_prior_mass_population in the effect-chain parsing loop, the field remains None and the pronoun resolution fallback will always default to TriggeringSource, leaving the bug unfixed.
Suggested fix: Add the population logic to the effect-chain loop (e.g., in oracle_ir/mod.rs or oracle_ir/chain.rs) using the clause_ir_mass_population helper described in the PR description.

[gemini-code-assist]

[MED] Incorrect CR citation for pronoun resolution. Evidence: crates/engine/src/parser/oracle_effect/subject.rs:3034.
Why it matters: Citing CR 608.2c (which governs choices during resolution) for pronoun antecedents is incorrect and violates R6, as the actual rule governing pronoun references is CR 201.5c.
Suggested fix: Change the citation to CR 201.5c.

Suggested change

	// Object-type trigger subject: the trigger's SOURCE is "they" only when no
	// nearer antecedent exists.
	//
	// CR 608.2c (issue #5985): a mass ("each …") effect in an earlier clause of
	// the same chain establishes an object population, and that population is
	// the nearer antecedent — Ardbert, Warrior of Darkness: "Whenever you cast a
	// white spell, put a +1/+1 counter on each legendary creature you control.
	// They gain vigilance until end of turn." "They" is those creatures, not the
	// cast spell. Binding to the spell granted the keyword to an object on the
	// stack, so the grant half silently did nothing while the counters landed.
	//
	// A mass effect chooses no target, so `ParentTarget` cannot express this
	// (see `has_typed_target_widened`'s single-target whitelist); the anaphor
	// inherits the population FILTER itself.
	// Object-type trigger subject: the trigger's SOURCE is "they" only when no
	// nearer antecedent exists.
	//
	// CR 201.5c (issue #5985): a mass ("each …") effect in an earlier clause of
	// the same chain establishes an object population, and that population is
	// the nearer antecedent — Ardbert, Warrior of Darkness: "Whenever you cast a
	// white spell, put a +1/+1 counter on each legendary creature you control.
	// They gain vigilance until end of turn." "They" is those creatures, not the
	// cast spell. Binding to the spell granted the keyword to an object on the
	// stack, so the grant half silently did nothing while the counters landed.
	//
	// A mass effect chooses no target, so ParentTarget cannot express this
	// (see has_typed_target_widened's single-target whitelist); the anaphor
	// inherits the population FILTER itself.

References

1. R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . A CR annotation where the cited rule's body does not describe what the code is doing is a violation. ^(link)

[galuis116]

Thanks for the pass. Both findings are inaccurate -- evidence below so a human reviewer isn't misled.

[HIGH] "chain_prior_mass_population is never populated" -- it is

It is seeded by the effect-chain loop at crates/engine/src/parser/oracle_effect/mod.rs:27333, in the same chunk-context construction as its siblings (chain_prior_hand_reveal_target, chain_has_prior_exile_producer):

chain_prior_mass_population: builder
    .clauses()
    .iter()
    .rev()
    .find_map(clause_ir_mass_population),

The cited evidence (context.rs:263) is the field's declaration, not its population site -- the seeding lives in the chain loop, which is exactly where the PR describes it.

This is also self-evident from the test: they_after_mass_effect_binds_that_population_not_trigger_source asserts the grant's affected equals the population the PutCounterAll acted on. If the field were always None, the code would fall back to TriggeringSource and that assertion would fail. It passes.

[MED] "CR 201.5c governs pronoun antecedents, not CR 608.2c" -- inverted

Verified against docs/MagicCompRules.txt:

• CR 201.5c (line 1332): "Text printed on some cards refers to that card by a shortened version of its name. Instances of a card's shortened name used in this manner are treated as though they used the card's full name." -- this governs shortened card names (e.g. "Kroxa" for "Kroxa, Titan of Death's Hunger"). It says nothing about pronouns.
• CR 608.2c (line 2793): "...In some cases, later text on the card may modify the meaning of earlier text (for example, "Destroy target creature. It can't be regenerated")... read the whole text and apply the rules of English to the text."

CR 608.2c is the rule about a later clause's pronoun binding to an earlier clause -- its own worked example is literally the pronoun "It" referring back to a previous sentence, which is precisely this card's shape ("...on each legendary creature you control. They gain vigilance..."). CR 201.5c would be the incorrect citation here.

It is also the citation the surrounding authority already uses for this exact question -- resolve_they_pronoun, chain_has_prior_typed_referent, and parent_target_available are all annotated CR 608.2c. Switching this one call site to 201.5c would make it inconsistent and wrong.

No changes made. Happy to revisit if a human reviewer disagrees on either point.

[matthewevans]

Follow-up reviewed. The two Gemini findings do not apply to the current head: parse_effect_chain_ir seeds chain_prior_mass_population from earlier finalized clauses, and the verified CR 608.2c text is the relevant rule for later text changing the meaning of an earlier clause; CR 201.5c concerns shortened card names.

The PR remains held only for the current-head evidence required for an engine/parser change: the coverage parse-diff comment is absent and the Rust lint/test jobs are still running. I will resume review when those artifacts are available.

[github-actions]

Parse changes introduced by this PR · 3 card(s), 4 signature(s) (baseline: main 87aa9f5747a9)

🟡 Modified fields (4 signatures)

• 1 card · 🔄 ability/grant FirstStrike · changed field affects: triggering source → attacking creature
  • Affected (first 3): Karlach, Fury of Avernus
• 1 card · 🔄 ability/grant Haste · changed field affects: triggering source → opponent controls permanent non-land
  • Affected (first 3): My Crushing Masterstroke
• 1 card · 🔄 ability/grant Menace · changed field affects: triggering source → legendary you control creature
  • Affected (first 3): Ardbert, Warrior of Darkness
• 1 card · 🔄 ability/grant Vigilance · changed field affects: triggering source → legendary you control creature
  • Affected (first 3): Ardbert, Warrior of Darkness

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

[matthewevans]

Changes requested — the new anaphora context covers only a subset of the mass-population grammar, so it is not safe to merge as the general fix.

🔴 Blocker

crates/engine/src/parser/oracle_effect/mod.rs:22435 defines clause_ir_mass_population as the prior population for a later They <grant> clause, but it recognizes only PutCounterAll, PumpAll, DestroyAll, and BounceAll. The same engine surface has other object-population effects with the same target semantics: CounterAll, GainControlAll, DamageAll, DoublePTAll, ExploreAll, and mass SetTapState; the existing mass-filter walker at crates/engine/src/parser/oracle_trigger.rs:3939-3951 already shows that wider family. A trigger whose first clause lowers through one of the omitted object-population effects still reaches resolve_they_pronoun at subject.rs:3045 with no chain_prior_mass_population, and silently falls back to TriggeringSource.

Please make the prior-population extraction a single, shared authoritative mapping for all object-population effect variants (rather than another partial list), then add sibling regression coverage for at least an omitted GainControlAll/DoublePTAll-shaped producer plus the existing counter case. The test needs to assert the following grant uses the producer population, not merely that parsing succeeds.

✅ Clean

The current parse-diff evidence is fresh for 1131e373 and scoped to Ardbert's two grants. Gemini's two comments do not apply to this head: the chain builder seeds the context before each later chunk, and CR 608.2c is the relevant verified instruction-order rule for the earlier-clause antecedent.

Recommendation: request changes for the shared mass-population extractor and sibling tests; then re-review the new head.

[galuis116]

Addressed in 4e1b4f84c -- you're right, that was a partial list, not the class.

Single shared authority

The variant list is now hoisted into one place and generates both accessors, so there is no second list to drift:

macro_rules! mass_population_filter_arms {
    ($effect:expr) => {
        match $effect {
            Effect::CounterAll { target } | Effect::GainControlAll { target } => Some(target),
            Effect::PumpAll { target, .. }
            | Effect::DamageAll { target, .. }
            | Effect::DestroyAll { target, .. }
            | Effect::BounceAll { target, .. }
            | Effect::PutCounterAll { target, .. }
            | Effect::DoublePTAll { target, .. } => Some(target),
            Effect::ExploreAll { filter } => Some(filter),
            Effect::SetTapState { target, scope: EffectScope::All, .. } => Some(target),
            _ => None,
        }
    };
}

pub(crate) fn mass_population_filter(effect: &Effect) -> Option<&TargetFilter>;
pub(crate) fn mass_population_filter_mut(effect: &mut Effect) -> Option<&mut TargetFilter>;

I used a macro deliberately: two hand-written matches (one &, one &mut) would be exactly the drift this review is about.

All the producers you named are now covered -- CounterAll, GainControlAll, DamageAll, DoublePTAll, ExploreAll, mass SetTapState. clause_ir_mass_population is now a one-liner over the authority.

The walker you pointed at now shares it

Thanks for the pointer to oracle_trigger.rs:3939-3951 -- that was the right precedent. Rather than mirror its list, I made it delegate to the same mass_population_filter_mut, so the attached-host "each other" retarget and the anaphora seeding provably read one list instead of two copies.

Sibling coverage

they_binds_producer_population_across_mass_effect_family is table-driven over PutCounterAll (Ardbert's own shape) plus the previously-omitted GainControlAll, DoublePTAll, and mass SetTapState. Each case:

• reads the population back out of the parsed producer via the shared authority (so it pins the binding rather than duplicating a literal filter),
• asserts the following grant's affected equals that population, and
• asserts it is not TriggeringSource.

I verified each omitted producer genuinely regressed before the widening -- e.g. gain control of all creatures target opponent controls. They gain haste bound the grant to Typed{Creature, TargetOpponent} only after the fix; previously it fell through to the trigger's source.

Full engine lib suite green (16,770 passed, 0 failed); cargo fmt, clippy -p engine --tests, and the parser gate clean.

Also noted your confirmation on Gemini's two comments -- no changes were made for either.

[matthewevans]

Current head reviewed; held for fresh CI. The shared mass-population extractor now covers the previously omitted sibling producers and the parser tests exercise counter, control, P/T, and mass-tap shapes. I also removed the new CR 611.2c citations: that rule governs the locked affected set of a resolving continuous effect, whereas the shared accessor also classifies one-shot effects. No additional implementation finding on 89c49b6.