fix(engine): treat a tracked-set-scaled sacrifice count as a set consumer#6191
fix(engine): treat a tracked-set-scaled sacrifice count as a set consumer#6191tryeverything24 wants to merge 2 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #5991 by adding Effect::Sacrifice to effect_references_tracked_set in the engine, ensuring that sacrifice counts scaled by a tracked set (such as Malfegor's ETB ability) resolve correctly. It also introduces comprehensive integration tests to verify this behavior. The reviewer's feedback highlights incorrect MTG Comprehensive Rules (CR) citations in both the engine and test comments, providing corrected rules (such as CR 608.2g and CR 701.21c), and suggests investigating whether other similar effects require sibling coverage to prevent similar bugs.
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.
| // CR 608.2c + CR 701.21a + CR 701.17a: a sacrifice count that reduces | ||
| // the chain tracked set is a CONSUMER of that set (Malfegor: "each | ||
| // opponent sacrifices a creature of their choice for each card | ||
| // discarded this way", issue #5991). Same class as the `ExileTop` | ||
| // arm above (Kylox) and the `repeat_for` check below (Seasoned | ||
| // Pyromancer, #740): without this arm, the preceding discard/ | ||
| // destroy/mill never published its affected ids, so | ||
| // `FilteredTrackedSetSize { caused_by: Discarded }` read an empty or | ||
| // stale set and resolved to 0 — a fully-supported-looking card that | ||
| // forces no sacrifice at all. | ||
| Effect::Sacrifice { count, .. } => quantity_hits_tracked(count), |
There was a problem hiding this comment.
The CR citations in the comment are slightly incorrect or imprecise:
CR 608.2cdescribes choices from non-public zones, whereas sacrifice is a choice from the battlefield (a public zone). The correct rule for determining information from the game on resolution isCR 608.2g.CR 701.21ais the general definition of discard, butCR 701.21cis the specific rule governing "discarded this way" references.
Additionally, per the Sibling coverage (L2) lens, please check if other effects that accept a count or quantity (such as Effect::DrawCard, Effect::Damage, Effect::GainLife, Effect::LoseLife, Effect::Mill, Effect::PutCounters, etc.) also need to be added to effect_references_tracked_set to prevent similar bugs when they are scaled by a tracked set.
| // CR 608.2c + CR 701.21a + CR 701.17a: a sacrifice count that reduces | |
| // the chain tracked set is a CONSUMER of that set (Malfegor: "each | |
| // opponent sacrifices a creature of their choice for each card | |
| // discarded this way", issue #5991). Same class as the `ExileTop` | |
| // arm above (Kylox) and the `repeat_for` check below (Seasoned | |
| // Pyromancer, #740): without this arm, the preceding discard/ | |
| // destroy/mill never published its affected ids, so | |
| // `FilteredTrackedSetSize { caused_by: Discarded }` read an empty or | |
| // stale set and resolved to 0 — a fully-supported-looking card that | |
| // forces no sacrifice at all. | |
| Effect::Sacrifice { count, .. } => quantity_hits_tracked(count), | |
| // CR 608.2g + CR 701.21c + CR 701.17a: a sacrifice count that reduces | |
| // the chain tracked set is a CONSUMER of that set (Malfegor: "each | |
| // opponent sacrifices a creature of their choice for each card | |
| // discarded this way", issue #5991). Same class as the ExileTop | |
| // arm above (Kylox) and the repeat_for check below (Seasoned | |
| // Pyromancer, #740): without this arm, the preceding discard/ | |
| // destroy/mill never published its affected ids, so | |
| // FilteredTrackedSetSize { caused_by: Discarded } read an empty or | |
| // stale set and resolved to 0 — a fully-supported-looking card that | |
| // forces no sacrifice at all. | |
| Effect::Sacrifice { count, .. } => quantity_hits_tracked(count), |
References
- Rule R6 requires every rules-touching line of engine code to carry a verified CR annotation of the form
CR <number>: <description>. (link)
| /// CR 701.9a + CR 701.17a + CR 608.2c: P0 (the caster) discards a 2-card hand | ||
| /// (a mandatory whole-hand discard, no choice needed since hand size == | ||
| /// discard count), then P1 (the lone opponent, controlling 3 creatures) must | ||
| /// sacrifice exactly 2 of them — the discarded-card count, not a fixed 1 and | ||
| /// not 0. This is the exact shape reported in #5991: "forces the opponent to | ||
| /// sacrifice their creatures after I discard my hand." |
There was a problem hiding this comment.
The CR citations in this test comment are incorrect:
CR 701.9ais about drawing a card ("To draw a card, a player moves the top card..."), but the action here is discarding. It should beCR 701.21c(orCR 701.21a).CR 608.2cis about non-public zone choices, but sacrifice is from the battlefield (public zone). It should beCR 608.2g.
| /// CR 701.9a + CR 701.17a + CR 608.2c: P0 (the caster) discards a 2-card hand | |
| /// (a mandatory whole-hand discard, no choice needed since hand size == | |
| /// discard count), then P1 (the lone opponent, controlling 3 creatures) must | |
| /// sacrifice exactly 2 of them — the discarded-card count, not a fixed 1 and | |
| /// not 0. This is the exact shape reported in #5991: "forces the opponent to | |
| /// sacrifice their creatures after I discard my hand." | |
| /// CR 701.21c + CR 701.17a + CR 608.2g: P0 (the caster) discards a 2-card hand | |
| /// (a mandatory whole-hand discard, no choice needed since hand size == | |
| /// discard count), then P1 (the lone opponent, controlling 3 creatures) must | |
| /// sacrifice exactly 2 of them — the discarded-card count, not a fixed 1 and | |
| /// not 0. This is the exact shape reported in #5991: "forces the opponent to | |
| /// sacrifice their creatures after I discard my hand." |
|
Maintainer follow-up: corrected the verified sacrifice citation ( |
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the tracked-set consumer walk is incomplete.
🔴 Blocker
effect_references_tracked_set now treats Effect::count_expr() as though it covers every quantity-bearing field, but count_expr is documented as the primary count/amount accessor (crates/engine/src/types/ability.rs:14399-14403). Effect::Token also evaluates each enter_with_counters quantity during resolution (crates/engine/src/types/ability.rs:9939-9975, crates/engine/src/game/effects/token.rs:2039-2043), while the current special case only examines token power/toughness (crates/engine/src/game/effects/mod.rs:3740-3745).
A producer followed by a token effect with fixed count and P/T but enter_with_counters: [(+1/+1, TrackedSetSize)] is consequently classified as non-consuming; the producer does not publish its set and the counter quantity resolves incorrectly. Other secondary quantity fields have the same structural risk, so adding only this one field would preserve a misleading generic invariant.
Please replace the primary-count shortcut with an exhaustive, authoritative traversal of every QuantityExpr carried by an effect (or an equivalent complete abstraction), and add a runtime regression covering tracked entry counters. The existing Malfegor sacrifice test remains useful but does not exercise this missing path.
✅ Clean
The sacrifice case is correctly identified as a tracked-set consumer, and the existing test drives the real discard/sacrifice pipeline.
Recommendation: rework the quantity traversal, add the entry-counter regression, then request re-review.
2211732 to
33c1226
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head still has the incomplete tracked-set traversal.
🔴 Blocker
effect_references_tracked_set says it walks every quantity position, but its Effect::Token arm only inspects count, power, and toughness at effects/mod.rs:3912-3963. Effect::Token also carries enter_with_counters: Vec<(CounterType, QuantityExpr)> (ability.rs:9996-10000), and the production token resolver evaluates every such quantity (token.rs:2045-2051).
A producer followed by a fixed-count/fixed-P/T token whose entry-counter quantity is TrackedSetSize is therefore classified as non-consuming; the producer does not publish its set and the token receives zero counters. The added Malfegor test only exercises Effect::Sacrifice (issue_5991_malfegor_discard_sacrifice.rs:115-204), so it cannot detect that sibling path.
Please use an exhaustive authority that visits every QuantityExpr carried by an effect (including nested token entry counters), then add a production-chain regression with a tracked-set-scaled entry counter. count_expr() alone is insufficient because it intentionally exposes only the primary count/amount slot.
🟡 Non-blocking
The CR citations at effects/mod.rs:3942 and issue_5991_malfegor_discard_sacrifice.rs:115 cite CR 701.17a, which is the mill rule; sacrifice is CR 701.21a and discard is CR 701.9a (verified in docs/MagicCompRules.txt:3327-3331,3449-3451). Please remove the unrelated citation or replace it with the rule the annotated behavior implements.
Recommendation: complete the quantity traversal and its discriminating runtime test, correct the citations, then request re-review.
|
Addressed both review points: Exhaustive quantity traversal: replaced the enumerated primary-count match in Tests: added a matcher-boundary unit test (a FIXED-count/FIXED-P/T token whose only tracked-set reference is a Citations: removed the mill rule (CR 701.17a); the comments now cite CR 701.9a (discard) and CR 701.21a (sacrifice). Verified locally in an isolated target dir: fmt clean, workspace clippy |
33c1226 to
7539181
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe effect quantity visitor now covers directly carried quantity expressions, and tracked-set detection uses it. Integration tests validate Malfegor’s discard-scaled sacrifice, empty-hand behavior, and tracked-set-driven token counters. ChangesTracked-set quantity flow
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/tests/integration/issue_5991_malfegor_discard_sacrifice.rs (1)
209-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo reach-guard that the discard step actually executed with zero cards discarded.
malfegor_empty_hand_discard_causes_no_sacrificeonly asserts the absence of a sacrifice prompt. Since the hand starts empty, there's no independent evidence (e.g. inspectingeventsfor a discard-count-zero event, or asserting on the tracked-set size directly) that the discard effect was reached and correctly computed a0, as opposed to the whole chain having short-circuited before even evaluating the discard/tracked-set-publish step for an unrelated reason. Theeventsvector is captured but never inspected.As per path instructions, "For every negative assertion ... require a paired positive reach-guard proving the input actually reached the code under test ... an upstream short-circuit makes a negative pass for the wrong reason."
🤖 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/tests/integration/issue_5991_malfegor_discard_sacrifice.rs` around lines 209 - 235, Strengthen malfegor_empty_hand_discard_causes_no_sacrifice with a positive reach-guard proving the empty discard step executed and computed zero discarded cards. Inspect the captured events or the relevant tracked-set state after resolve_ability_chain, using the existing event/state symbols, while preserving the current assertions that no creatures are sacrificed and no sacrifice prompt appears.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.
Inline comments:
In `@crates/engine/tests/integration/issue_5991_malfegor_discard_sacrifice.rs`:
- Around line 205-207: Replace the incorrect CR 107.3a citation in the
negative/no-regression guard comment with a rule citation whose verified text in
docs/MagicCompRules.txt supports choosing zero, such as CR 107.1c if confirmed.
Keep the explanation that an empty hand produces zero sacrifices and preserves
the legal no-op behavior.
---
Nitpick comments:
In `@crates/engine/tests/integration/issue_5991_malfegor_discard_sacrifice.rs`:
- Around line 209-235: Strengthen
malfegor_empty_hand_discard_causes_no_sacrifice with a positive reach-guard
proving the empty discard step executed and computed zero discarded cards.
Inspect the captured events or the relevant tracked-set state after
resolve_ability_chain, using the existing event/state symbols, while preserving
the current assertions that no creatures are sacrificed and no sacrifice prompt
appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0f4d864-a4eb-44b9-b46c-e3b69f6af5bb
📒 Files selected for processing (4)
crates/engine/src/game/effects/mod.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_5991_malfegor_discard_sacrifice.rscrates/engine/tests/integration/main.rs
| /// Negative/no-regression guard: an empty hand discards nothing, so the | ||
| /// tracked set is empty and the opponent must sacrifice 0 creatures — a | ||
| /// legal no-op (CR 107.3a), not an erroneous fixed-1 sacrifice. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
CR 107.3a citation doesn't describe this behavior.
CR 107.3a governs how a player chooses/announces the value of X in a mana/activation cost — it has nothing to do with a zero-count sacrifice being a legal no-op. This citation doesn't support the claim it's attached to; it looks like a nearby/adjacent rule number was picked without verifying the rule body (the failure mode CLAUDE.md explicitly warns about with CR 119/120/121 and 701.x mix-ups). CR 107.1c ("may choose any number, including zero") is closer to what's actually intended here, but should be verified against docs/MagicCompRules.txt before use.
As per path instructions, "a CR citation whose rule body does not describe the code" must be flagged, and CR numbers must be verified via docs/MagicCompRules.txt before being added.
Suggested correction
-/// Negative/no-regression guard: an empty hand discards nothing, so the
-/// tracked set is empty and the opponent must sacrifice 0 creatures — a
-/// legal no-op (CR 107.3a), not an erroneous fixed-1 sacrifice.
+/// Negative/no-regression guard: an empty hand discards nothing, so the
+/// tracked set is empty and the opponent must sacrifice 0 creatures — a
+/// legal no-op (CR 107.1c: a chosen/derived number may be zero), not an
+/// erroneous fixed-1 sacrifice.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Negative/no-regression guard: an empty hand discards nothing, so the | |
| /// tracked set is empty and the opponent must sacrifice 0 creatures — a | |
| /// legal no-op (CR 107.3a), not an erroneous fixed-1 sacrifice. | |
| /// Negative/no-regression guard: an empty hand discards nothing, so the | |
| /// tracked set is empty and the opponent must sacrifice 0 creatures — a | |
| /// legal no-op (CR 107.1c: a chosen/derived number may be zero), not an | |
| /// erroneous fixed-1 sacrifice. |
🤖 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/tests/integration/issue_5991_malfegor_discard_sacrifice.rs`
around lines 205 - 207, Replace the incorrect CR 107.3a citation in the
negative/no-regression guard comment with a rule citation whose verified text in
docs/MagicCompRules.txt supports choosing zero, such as CR 107.1c if confirmed.
Keep the explanation that an empty hand produces zero sacrifices and preserves
the legal no-op behavior.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — for_each_quantity_expr is still incomplete on 7539181.
🔴 Blocker
The new authority says it visits every QuantityExpr, but it classifies Effect::Mana as quantity-free at crates/engine/src/types/ability.rs:14961. ManaProduction carries live count: QuantityExpr slots (for example Colorless at :1877-1880, AnyOneColor at :1888-1890, and the other dynamic-production variants), and game/effects/mana.rs:454-480 resolves them during this effect's resolution. A producer followed by one of those consumers still fails to publish its tracked set.
Two more live nested carriers have the same hole: Effect::PayCost visits only scale at ability.rs:14772-14776 while its AbilityCost contains dynamic values such as ManaDynamic, PayLife, Discard, PayEnergy, and PaySpeed (:8156-8175, :8251-8257); and Effect::RollDie visits count but not modifier (:14792-14793), though DieRollModifier::{Add,Subtract} holds a QuantityExpr (:1099-1103) resolved in game/effects/roll_die.rs:73-79.
Please make the traversal genuinely compositional and exhaustive across these nested domain types (including recursive cost forms such as Composite, OneOf, EffectCost, and PerCounter where their quantities resolve in the current chain), then add discriminating production-chain coverage for the newly traversed live seams. This is the same tracked-set-publication defect class the PR is intended to remove, so the current "exhaustive" contract is not safe to merge.
🟡 Non-blocking follow-ups
The current CodeRabbit review correctly flags that the empty-hand regression at issue_5991_malfegor_discard_sacrifice.rs:205-234 has no positive reach guard and carries an unrelated CR 107.3a citation. Please either add a direct event/state assertion proving the empty discard step ran, and replace the citation only after verifying it locally, or remove the unsupported citation.
✅ Clean
The Malfegor discard-to-sacrifice test and the token entry-counter regression both drive the relevant resolution chain and would catch their respective prior shortcuts. The current parse-diff sticky is no_changes for this head, matching the non-parser scope.
Recommendation: complete the nested quantity traversal and its regressions, address the small test follow-up, then request re-review.
|
Correction to the blocker line reference: |
7539181 to
cf6844c
Compare
|
Rebased onto current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new exhaustive visitor still omits direct QuantityExpr carriers.
🔴 Blocker
[HIGH] Effect::for_each_quantity_expr is incomplete despite its exhaustive-contract documentation. Evidence: crates/engine/src/types/ability.rs:14961 groups Effect::Mana { .. } with no quantities, while ManaProduction directly carries count: QuantityExpr (for example Colorless at :1878-1881, AnyOneColor at :1888-1891, and AnyOneColorAmongPermanents at :2005-2008). Effect::PayCost at :14772-14775 walks only scale, dropping dynamic AbilityCost fields (ManaDynamic :8156-8158, PayLife :8170-8172, Discard :8173-8175, PayEnergy/PaySpeed :8260-8266). Effect::RollDie at :14792-14794 walks only count, although modifier contains DieRollModifier::{Add,Subtract} values at :1099-1103 and RollDie carries that modifier at :11861-11868. Why it matters: a tracked-set producer can fail to publish before any of these downstream consumers, yielding a zero or incorrect resolution; the visitor's claimed exhaustive contract would then silently regress as these variants evolve. Suggested fix: compose dedicated exhaustive visitors for ManaProduction, AbilityCost, and DieRollModifier, and add focused regressions proving the tracked-set classifier reaches each missing carrier.
Recommendation: request changes — complete the visitor and its regression matrix before re-review.
…umer Fixes phase-rs#5991. Malfegor: "Flying. When Malfegor enters the battlefield, discard your hand, then each opponent sacrifices a creature of their choice for each card discarded this way." The compound trigger parses correctly into a DiscardCard producer chained to a Sacrifice sub-ability whose count reads FilteredTrackedSetSize { caused_by: Discarded } -- the standard "for each X discarded/exiled/milled this way" idiom already used elsewhere in the engine (Kylox's ExileTop, Seasoned Pyromancer's repeat_for). effect_references_tracked_set(), which decides whether a preceding producer effect needs to publish its affected object ids into the chain-scoped tracked set before a downstream consumer reads it, had no arm for Effect::Sacrifice. So the discard step's affected ids were never published, and the sacrifice count's FilteredTrackedSetSize read an empty or stale set and resolved to 0 -- Malfegor's opponent was never asked to sacrifice anything, a fully-parsed card that silently does nothing. Fixed by adding a Sacrifice { count, .. } => quantity_hits_tracked(count) arm, mirroring the existing ExileTop arm immediately above it. New end-to-end test driving the real DiscardCard -> Sacrifice resolution chain via resolve_ability_chain (the same production entry point every triggered ability resolves through), using the parser's own parse_effect_chain output, not a hand-built ResolvedAbility. Discriminating scenario: controller discards a 2-card hand, opponent controls 3 creatures -- must be asked to sacrifice exactly 2 (not a fixed 1, not 0, the two most plausible pre-fix symptoms). Verified locally: fmt clean, clippy clean (-D warnings), 3545 integration tests pass (3544 pre-existing + 1 new), 0 regressions.
…lassification Review follow-up (phase-rs#5991): effect_references_tracked_set's enumerated quantity match only saw a handful of primary count/amount slots — in particular Effect::Token's enter_with_counters entry-counter quantities were invisible, so a producer chained into a fixed-count/fixed-P/T token entering "with a +1/+1 counter for each card <verbed> this way" was classified non-consuming, the producer never published its set, and the token entered with zero counters. Replaced the shortcut with Effect::for_each_quantity_expr, a new exhaustive authority (types/ability.rs) that visits EVERY QuantityExpr an effect directly carries — all secondary slots (enter_with_counters, keep_count_expr, max_ticket_cost, dynamic PtValue power/toughness, conditional entry counters, ForEachCategory per-member counts, ...) — and recurses through variants embedding another Effect/ResolvedAbility. The match is exhaustive with no wildcard arm (same convention as count_expr()/target_filter()), so a newly added variant must be explicitly classified. Deferred definition payloads (AbilityDefinition, ContinuousModification, ...) are documented as out of scope: their quantities evaluate in their own later resolution context, with current- chain consumption signalled by explicit flags (CreateDelayedTrigger's uses_tracked_set). Also per review: corrected the CR citations (sacrifice is CR 701.21a, discard CR 701.9a — CR 701.17a is the mill rule and was removed), and added both a matcher-boundary unit test (tracked entry counter on an otherwise-fixed token marks the ability as consuming) and a production- chain regression (discard 2 -> token enters with 2 +1/+1 counters via resolve_ability_chain). Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17516 passed; engine integration 3751 passed.
cf6844c to
610d2ab
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new exhaustive quantity visitor is still incomplete on 610d2abe839c6134bd93c37859b9e28155edd14f.
🔴 Blocker
[HIGH] The new Effect::for_each_quantity_expr still drops live nested quantity carriers. Evidence: crates/engine/src/types/ability.rs:14961 groups Effect::Mana as quantity-free although its ManaProduction owns dynamic count values; :14772-14776 visits only PayCost::scale and not its AbilityCost; :14792-14794 visits only RollDie::count and not dynamic DieRollModifier::{Add,Subtract} quantities. Why it matters: a preceding producer will still fail to publish its tracked set for those current-chain consumers, so their tracked quantity resolves as zero or wrong despite the visitor’s exhaustive contract. Suggested fix: compose exhaustive visitors for ManaProduction, recursive AbilityCost (including nested forms), and DieRollModifier, invoke them from Effect, and add production-chain regressions for each seam.
🟡 Non-blocking
[MED] The empty-hand regression is a vacuous negative check and retains an unsupported CR citation. Evidence: crates/engine/tests/integration/issue_5991_malfegor_discard_sacrifice.rs:205-234 only asserts no prompt/no battlefield change and never checks the captured resolution events; :207 cites CR 107.3a for the zero-count no-op. Why it matters: an upstream short-circuit that skips the discard/tracked-set path still passes. Suggested fix: add a discard-resolution reach guard, then verify a replacement citation against the local Comprehensive Rules file or remove it.
✅ Clean
The current parse-diff sticky comment reports no card-parse changes, which matches this engine-only change. I also rechecked the current CodeRabbit findings; they identify the same remaining incomplete-traversal and test/citation issues.
Recommendation: request changes — complete the nested visitor composition and add discriminating production-path tests before re-review.
2 participants
Fixes #5991.
Malfegor: "Flying. When Malfegor enters the battlefield, discard your hand, then each opponent sacrifices a creature of their choice for each card discarded this way."
Root cause & fix
The compound trigger parses correctly into a
DiscardCardproducer chained to aSacrificesub-ability whose count readsFilteredTrackedSetSize { caused_by: Discarded }— the standard "for each X discarded/exiled/milled this way" idiom already used elsewhere in the engine (Kylox'sExileTop, Seasoned Pyromancer'srepeat_for).effect_references_tracked_set(), which decides whether a preceding producer effect needs to publish its affected object ids into the chain-scoped tracked set before a downstream consumer reads it, had no arm forEffect::Sacrifice. So the discard step's affected ids were never published, and the sacrifice count'sFilteredTrackedSetSizeread an empty or stale set and resolved to 0 — Malfegor's opponent was never asked to sacrifice anything, a fully-parsed card that silently does nothing.Fixed by adding a
Sacrifice { count, .. } => quantity_hits_tracked(count)arm, mirroring the existingExileToparm immediately above it.Testing
New end-to-end test driving the real
DiscardCard→Sacrificeresolution chain viaresolve_ability_chain(the same production entry point every triggered ability resolves through), using the parser's ownparse_effect_chainoutput, not a hand-builtResolvedAbility. Discriminating scenario: controller discards a 2-card hand, opponent controls 3 creatures — must be asked to sacrifice exactly 2 (not a fixed 1, not 0, the two most plausible pre-fix symptoms).Verified locally: fmt clean, clippy clean (
-D warnings), 3545 integration tests pass (3544 pre-existing + 1 new), 0 regressions.Summary by CodeRabbit
Bug Fixes
Tests