Problem
previous_effect_amount_from_events (crates/engine/src/game/effects/mod.rs) ends its summing arms with:
(amount > 0).then_some(amount)
and both call sites consume it as:
if let Some(amount) = previous_effect_amount_from_events(state, ability, parent_events) {
state.last_effect_amount = Some(amount);
…
}
A None return therefore skips the assignment, leaving whatever an earlier chain step wrote still standing. So an effect that genuinely produced zero does not stamp Some(0) — it leaves the previous step's amount in place, and a following "that many" / "that much" clause reads the wrong number.
Affected arms, all of which fall through to that filter:
DealDamage / DamageAll / DamageEachPlayer
Fight
LoseLife / PayCost
GainLife
RemoveCounter
Example shape: "Remove all −1/−1 counters from it, then discard a card for each counter removed this way" preceded by a nonzero GainLife step. With zero counters removed, the discard reads the life-gain amount.
Why this was not fixed alongside #6858
#6858 fixed the same class for Effect::Draw, but that arm is not analogous, and the obvious generalization would introduce the mirror bug.
Effect::Draw returns state.last_effect_count, which is itself an Option and a genuine tri-state maintained upstream: reset to None at the start of resolution (engine.rs:1064), and explicitly written Some(0) when a producer ran and delivered nothing (several sites in engine_resolution_choices.rs). None means "no draw sequence ran"; Some(0) means "a draw ran and delivered zero". The early return simply preserves a distinction that already exists.
The summing arms compute events.iter().filter_map(…).sum(). A sum of 0 arises from two indistinguishable situations:
- no matching events at all — the effect did not produce, and an earlier value should stand; and
- matching events summing to zero — a real zero result that should overwrite.
There is no upstream tri-state to recover the difference from. The current > 0 filter collapses both into (1). Stamping Some(sum) unconditionally would collapse both into (2) — a DealDamage step that emitted no damage events would stamp 0 and clobber a legitimately-standing earlier value. That is the same defect with the sign flipped, and it fails more quietly, since it silently zeroes rather than silently persists.
What a correct fix requires
- Distinguish "no matching events" from "matched events summing to zero" — count the matches rather than filter on the total. Mechanically small.
- Answer a CR question per arm first, because the count alone does not settle semantics:
- Does damage that was fully prevented count as damage dealt for "that much" purposes? (CR 615 prevention vs CR 120.6 "damage dealt")
- Does
RemoveCounter on a permanent with zero counters constitute a removal event at all?
- Does
PayCost / LoseLife of zero constitute a life-loss event? (CR 118.2 — losing 0 life is not a life-loss event, which likely makes (1) the correct answer for that arm specifically.)
Those are per-arm determinations with a wide blast radius across existing cards, which is why this is a separate piece of work rather than a filter flip.
Suggested scoping
Take one arm at a time, each with its own CR determination and its own discriminating test asserting the zero case specifically — a test asserting only the nonzero path cannot distinguish this defect from correct behaviour. Note that previous_effect_counts_by_player_from_events is a sibling with the same shape and should be checked in lockstep.
Found while reviewing #6955 (fixes #6858). Raised by CodeRabbit; the analysis of why the obvious generalization is unsafe is in that PR's discussion.
Problem
previous_effect_amount_from_events(crates/engine/src/game/effects/mod.rs) ends its summing arms with:and both call sites consume it as:
A
Nonereturn therefore skips the assignment, leaving whatever an earlier chain step wrote still standing. So an effect that genuinely produced zero does not stampSome(0)— it leaves the previous step's amount in place, and a following "that many" / "that much" clause reads the wrong number.Affected arms, all of which fall through to that filter:
DealDamage/DamageAll/DamageEachPlayerFightLoseLife/PayCostGainLifeRemoveCounterExample shape: "Remove all −1/−1 counters from it, then discard a card for each counter removed this way" preceded by a nonzero
GainLifestep. With zero counters removed, the discard reads the life-gain amount.Why this was not fixed alongside #6858
#6858 fixed the same class for
Effect::Draw, but that arm is not analogous, and the obvious generalization would introduce the mirror bug.Effect::Drawreturnsstate.last_effect_count, which is itself anOptionand a genuine tri-state maintained upstream: reset toNoneat the start of resolution (engine.rs:1064), and explicitly writtenSome(0)when a producer ran and delivered nothing (several sites inengine_resolution_choices.rs).Nonemeans "no draw sequence ran";Some(0)means "a draw ran and delivered zero". The early return simply preserves a distinction that already exists.The summing arms compute
events.iter().filter_map(…).sum(). A sum of0arises from two indistinguishable situations:There is no upstream tri-state to recover the difference from. The current
> 0filter collapses both into (1). StampingSome(sum)unconditionally would collapse both into (2) — aDealDamagestep that emitted no damage events would stamp0and clobber a legitimately-standing earlier value. That is the same defect with the sign flipped, and it fails more quietly, since it silently zeroes rather than silently persists.What a correct fix requires
RemoveCounteron a permanent with zero counters constitute a removal event at all?PayCost/LoseLifeof zero constitute a life-loss event? (CR 118.2 — losing 0 life is not a life-loss event, which likely makes (1) the correct answer for that arm specifically.)Those are per-arm determinations with a wide blast radius across existing cards, which is why this is a separate piece of work rather than a filter flip.
Suggested scoping
Take one arm at a time, each with its own CR determination and its own discriminating test asserting the zero case specifically — a test asserting only the nonzero path cannot distinguish this defect from correct behaviour. Note that
previous_effect_counts_by_player_from_eventsis a sibling with the same shape and should be checked in lockstep.Found while reviewing #6955 (fixes #6858). Raised by CodeRabbit; the analysis of why the obvious generalization is unsafe is in that PR's discussion.