fix(engine): stamp the draw total so a chained "discard that many" reads it (#6858)

[matthewevans]

Fixes #6858.

The bug

"Draw N cards, then discard that many" discarded nothing. Varina, Lich Queen drew and gained life on attack but never discarded.

A producer/consumer channel mismatch:

• Consumer: QuantityRef::PreviousEffectAmount { channel: Total } reads only state.last_effect_amount, with no fallback. (Its sibling EventContextAmount does cascade through .or(state.last_effect_count).)
• Producer: Effect::Draw commits its total to state.last_effect_count.
• Neither extractor that populates last_effect_amount — previous_effect_amount_from_events or previous_effect_counts_by_player_from_events — had an Effect::Draw arm. Both fell through to their _ => 0 / _ => return None default.

So last_effect_amount stayed None, the discard count resolved to 0, and discard.rs short-circuited to a no-op before the WaitingFor::DiscardChoice branch was ever reached.

This is a regression, and the contract was already written down

ea9de8764b ("fix(parser): bind discard that many to previous draw count (issue #3296) (#3541)") added an unconditional parser rewrite retargeting Discard's EventContextAmount to PreviousEffectAmount. It was parser-only — the engine-side producer arm was never added, so the rewritten reference has pointed at an unfilled channel ever since.

The obligation was already documented on the PreviousEffectAmount type itself: "Every non-damage producer (life lost, counters removed, cards drawn) stamps only this channel." Draw was a documented producer obligation that was never implemented.

The fix

One arm, reading the committed total rather than re-summing draw events — exactly as the neighbouring RollDie arm defers to die_result_this_resolution. That matters for correctness, not just tidiness: resume_draw_sequence is the single authority for how many cards an instruction actually delivered, so replacement effects are respected. A draw replaced by something else contributes 0; one doubled by a count modifier contributes its post-replacement count.

The arm returns early instead of falling through the > 0 filter below it. A draw that delivered zero cards is a real zero and must stamp Some(0) — otherwise "draw a card for each Island you control, then discard that many" controlling no Islands would inherit whatever amount its preceding chain step left behind.

CR 121.2 + CR 121.2a + CR 608.2c, all grep-verified.

The existing regression test was passing vacuously

issue_3296_hordewing_skaab_discard.rs asserted hand_after == hand_before and passed on main — which superficially contradicts this whole diagnosis. It was vacuous: its helper exited via the Priority arm after advance_until_stack_empty without ever dispatching DecideOptionalEffect, so the draw never happened and "net hand size unchanged" held for the wrong reason. It asserted nothing about library size and nothing about a discard occurring.

Confirmed by observation, not inference: the test's verbatim original body was run against base engine behaviour with the bug fully present, and it passed.

It now keeps advancing until the optional draw is genuinely taken, panics if it never settles rather than silently falling through, and asserts the library actually shrank.

Scope — the class is 6 cards, and 4 more are broken by something else

9 cards carry Discard { count: PreviousEffectAmount }.

Fixed here (6, draw-preceded): Varina, Lich Queen · Hordewing Skaab · Horrid Shadowspinner · Laquatus's Creativity · Last Stand · Transcendent Archaic.

Not fixed, different defect (4): Dreamstealer, Needle Specter, Jagged Poppet, Marina Vendrell's Grimoire. Their Discard is a top-level trigger effect, and last_effect_amount is cleared at depth 0 and only ever written by the sub-ability postlude — so it is always None regardless of producer. Neither this fix nor the alternative reaches them. Filed separately rather than bundled.

Verification

• 4 tests observed red at base, green with the fix — not asserted, run both ways.
• New issue_6858_draw_that_many_discard.rs covers the channel contract as a building block (draw_stamps_the_total_channel_a_chained_that_many_discard_reads), the reported card (varina_attack_trigger_discards_as_many_as_it_drew), and the zero-draw edge case the fix's early return exists for (a_zero_card_draw_stamps_zero_instead_of_inheriting_the_previous_step).
• Full -p phase-engine suite: 22,803 passed / 0 failed. Clippy -D warnings clean.

Note on the alternative fix

Adding .or(state.last_effect_count) at the consumer was considered and rejected: it has a much wider blast radius across every non-damage consumer, whereas the producer arm restores a contract the type documentation already claimed held.

Summary by CodeRabbit

• Bug Fixes

  • Fixed chained draw-and-discard effects so discard amounts consistently match the latest draw, including draws of zero cards.
  • Ensured downstream effects use the committed draw total and do not reuse stale values.
  • Corrected triggered draw effects so the expected discard prompt appears and hand sizes update correctly.

• Tests

  • Added coverage for chained draw/discard effects, optional choices, zero-card draws, and related triggered abilities.

[coderabbitai]

📝 Walkthrough

Walkthrough

Effect::Draw now returns the committed draw count, including zero. Integration tests cover chained draw-discard effects, Varina’s trigger, optional discard handling, and stale amount prevention.

Changes

Draw amount propagation

Layer / File(s)	Summary
Committed draw result crates/engine/src/game/effects/mod.rs	Effect::Draw returns the post-replacement draw count from last_effect_count, including zero.
Optional draw and discard flow crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs	The test repeatedly advances optional effects and verifies the one-card draw and discard prompt.
Chained draw-discard regressions crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs, crates/engine/tests/integration/main.rs	Added coverage for chained amounts, Varina’s attack trigger, zero-card draws, and registration of the integration test module.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• phase-rs/phase#6730: Related effect-count propagation and last_effect_count handling.
• phase-rs/phase#6688: Related Effect::Draw amount propagation semantics.
• phase-rs/phase#5867: Related draw-resolution handling in a different runtime path.

Suggested labels: bug

Suggested reviewers: kiannidev, andriypolanski

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes stamping the draw total for chained discard effects, which is the primary change.
Linked Issues check	✅ Passed	The implementation and regression tests address issue #6858 by making Varina discard the number of cards drawn, including zero draws.
Out of Scope Changes check	✅ Passed	All changes support issue #6858 through the engine fix and focused integration tests; no unrelated code changes are present.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/6858-draw-stamps-total

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/engine/src/game/effects/mod.rs (1)

6791-6884: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Zero-result stale-stamp bug still latent for every non-Draw, non-RollDie effect arm.

This fix correctly special-cases Effect::Draw (and reuses the existing Effect::RollDie pattern) to return the raw Option<i32> early, so a real zero result stamps Some(0) instead of collapsing to None. But every other arm in this same match — DealDamage/DamageAll/DamageEachPlayer (Line 6792), Fight (Line 6803), LoseLife/PayCost (Line 6831), GainLife (Line 6838), RemoveCounter (Line 6845) — still falls through to (amount > 0).then_some(amount) at Line 6884.

Per the new regression test's own reasoning (a_zero_card_draw_stamps_zero_instead_of_inheriting_the_previous_step, which asserts a zero draw "must overwrite the preceding GainLife stamp, not leave it standing"), a None return from this function apparently leaves state.last_effect_amount untouched, not zeroed. That means the exact bug class this PR fixes for Draw — a chained "that many" step reading a stale amount from an earlier chain step — is still present for any card whose chain uses GainLife, LoseLife, RemoveCounter, DealDamage, or Fight with a genuine zero outcome. For example, "Remove all -1/-1 counters from it, then discard a card for each counter removed this way" preceded by a nonzero GainLife step would still leak the stale life-gain amount when zero counters are removed.

Apply the same early-return pattern (stamp Some(sum) unconditionally instead of filtering by > 0) to the other arms, or confirm the caller already treats None as "no data, coerce to 0" rather than "leave stale value standing" — the two are functionally different and only the latter has this defect.

Based on path instructions: CLAUDE.md states "Implement this as a general engine-level effect/quantity-channel fix, not a Varina-specific special case... explicitly handle zero values."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/game/effects/mod.rs` around lines 6791 - 6884, Update the
amount calculation around the match on `ability.effect` so every
quantity-producing arm—damage effects, `Fight`, life changes, and
`RemoveCounter`—returns its computed sum as `Some(amount)` even when zero,
matching the existing early-return behavior of `Effect::Draw` and
`Effect::RollDie`. Prevent the final `(amount > 0).then_some(amount)` path from
converting genuine zero results into `None`; preserve the existing handling for
effects without a quantity result.

Source: Path instructions

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 6791-6884: Update the amount calculation around the match on
`ability.effect` so every quantity-producing arm—damage effects, `Fight`, life
changes, and `RemoveCounter`—returns its computed sum as `Some(amount)` even
when zero, matching the existing early-return behavior of `Effect::Draw` and
`Effect::RollDie`. Prevent the final `(amount > 0).then_some(amount)` path from
converting genuine zero results into `None`; preserve the existing handling for
effects without a quantity result.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 22f28fd5-9164-4d5e-8d30-05f2976507ec

📥 Commits

Reviewing files that changed from the base of the PR and between 37fa488 and 691c4e1.

📒 Files selected for processing (4)

• crates/engine/src/game/effects/mod.rs
• crates/engine/tests/integration/issue_3296_hordewing_skaab_discard.rs
• crates/engine/tests/integration/issue_6858_draw_that_many_discard.rs
• crates/engine/tests/integration/main.rs

[matthewevans]

Good catch on the class, and thank you for naming the discriminator rather than just asserting the bug — "confirm the caller already treats None as 'no data, coerce to 0' rather than 'leave stale value standing' — the two are functionally different and only the latter has this defect." I checked, and you're right about which one it is.

The caller leaves the stale value standing. Both call sites are:

if let Some(amount) = previous_effect_amount_from_events(state, ability, parent_events) {
    state.last_effect_amount = Some(amount);
    …
}

A None return skips the assignment entirely, so whatever an earlier chain step wrote survives. So the stale-stamp class is real for the other arms, as you say.

But the fix you propose would introduce the mirror bug, and that's why it isn't in this PR.

The two cases are not symmetric:

• Effect::Draw returns state.last_effect_count, which is itself an Option and a genuine tri-state maintained by the draw sequence: reset to None at the start of resolution (engine.rs:1064) and explicitly written Some(0) when a producer ran and delivered zero (several sites in engine_resolution_choices.rs). So None means "no draw sequence ran" and Some(0) means "a draw ran and delivered nothing." The early return preserves a distinction that already exists upstream.

• The summing arms (DealDamage/DamageAll/DamageEachPlayer, Fight, LoseLife/PayCost, GainLife, RemoveCounter) compute events.iter().filter_map(…).sum(). A sum of 0 is produced by two indistinguishable situations: no matching events at all, and matching events that sum to zero. There is no upstream tri-state to recover it from.

(amount > 0).then_some(amount) conflates those two by treating both as "no data". Stamping Some(sum) unconditionally would conflate them in the other direction — a chain step whose effect is DealDamage but which 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 would be harder to spot because it silently zeroes rather than silently persists.

A correct fix has to distinguish "no matching events" from "matched events summing to zero" — count the matches rather than filter on the total — and it has to answer a rules question per arm before it can be written: does damage fully prevented count as damage dealt for "that much" purposes (CR 615 vs CR 120.6)? Does RemoveCounter with zero counters present constitute a removal event? Those are per-arm CR determinations with a wide blast radius across existing cards, not a filter flip.

So: real, in the same function, and deliberately not bundled. Filed as a follow-up with this analysis rather than widened into a fix that already has verified red/green evidence for a different, self-contained defect. The Effect::Draw arm here is safe precisely because it inherits a tri-state that already exists; the others need one built.

[github-actions]

Generated for head 691c4e103eda4ee195c13193b64a500fc74091ee.

Parse changes introduced by this PR

✓ No card-parse changes detected.