fix(engine): resolve per-source power in each-X-deals-damage clauses - #7322
Conversation
Each-source damage clauses with an own-characteristic amount ("each other
Bird you control deals damage equal to its power to target creature an
opponent controls" — Bartz and Boko) fell through try_parse_each_source_
deals_damage's Fixed-only amount guard and mis-parsed to a single
ability-sourced DealDamage: the "each <filter> you control" subject was
dropped and Power{Anaphoric} resolved against the trigger source, not each
batch member.
Introduce ObjectScope::BatchSource (CR 120.1 per-iteration batch source) so
QuantityRef::Power { scope: BatchSource } types the per-source amount, thread
a QuantityContext.damage_source field + a per-source resolve wrapper
(resolve_quantity_with_targets_and_damage_source), and have
resolve_each_source_deals_damage resolve the amount per source id (live +
LKI fallback, CR 113.7a) inside the existing simultaneous-batch entries loop.
The parser guard now rebinds the clause's deferred "its" pronoun to
BatchSource via the structural rebind helper instead of rejecting it.
10-card class: Bartz and Boko, Judgment of Alexander, Kamahl's Will, Master
of the Wild Hunt, Moonlight Hunt, Nissa's Judgment, Sarkhan the Mad,
Season's Beatings, Signature Slam, The Bears of Littjara. Sources whose
riders the filter model cannot express fail closed to Unimplemented
(Season's Beatings' "random" recipient; Master of the Wild Hunt's "tapped
this way" source restriction) rather than silently degrading. Adds a
discriminating runtime cast-pipeline test (0/9 dies to sum 9, uniform/
ability-source 8 survives, Bartz excluded from attribution) plus parser and
resolver building-block tests pinning the class.
|
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 (4)
📝 WalkthroughWalkthroughThe PR adds ChangesBatchSource damage flow
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This PR corrects per-source damage parsing and resolution, with the supplied checks and runtime tests passing; no actionable merge-blocking risk remains after normal review. Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant EachSourceDealsDamage
participant QuantityResolver
participant GameState
OracleParser->>EachSourceDealsDamage: bind anaphoric amount to BatchSource
EachSourceDealsDamage->>QuantityResolver: resolve amount for each source
QuantityResolver->>GameState: read live object or LKI characteristics
GameState-->>QuantityResolver: source quantity
QuantityResolver-->>EachSourceDealsDamage: per-source amount
EachSourceDealsDamage->>EachSourceDealsDamage: create non-zero damage entries
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 3
🧹 Nitpick comments (1)
crates/engine/src/game/quantity.rs (1)
425-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnify the scope-carrying
QuantityRefenumeration to prevent drift.
ref_contains_scopelistsPower,Toughness,ObjectManaValue,ObjectColorCount,ObjectNameWordCount,ObjectTypelineComponentCount,ManaSymbolsInManaCost, andCountersOnas the scope-carryingQuantityRefvariants, then falls back to_ => false.Two sibling functions enumerate almost the same set but omit
CountersOn:
scope_is_resolution_only's inner match (insidequantity_expr_uses_resolution_only_object_scope, around line 395).leaf_scope_missing's inner match (insidequantity_expr_missing_resolution_only_referent, around line 580).If a future card uses
CountersOnwith a resolution-only scope (Anaphoric,Demonstrative, orBatchSource),quantity_expr_uses_resolution_only_object_scopeandquantity_expr_missing_resolution_only_referentwill not detect it, even thoughquantity_expr_contains_scopeproves the detection is straightforward to add. Extract one shared list (or a small helper) of scope-carryingQuantityRefvariants and reuse it across all three functions.🤖 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/quantity.rs` around lines 425 - 464, Unify scope-carrying QuantityRef classification across quantity_expr_contains_scope, quantity_expr_uses_resolution_only_object_scope’s scope_is_resolution_only, and quantity_expr_missing_resolution_only_referent’s leaf_scope_missing. Extract a shared helper or exhaustive match that includes CountersOn and all existing scoped variants, then reuse it so future QuantityRef additions cannot drift between these checks.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/src/parser/oracle_effect/subject.rs`:
- Around line 7548-7576: Update both tests,
each_seasons_beatings_random_recipient_fails_closed and
each_master_of_the_wild_hunt_tapped_this_way_fails_closed, to assert the
specific guard-owned Unimplemented key in addition to matching
Effect::Unimplemented. Use the key emitted by the new fail-closed guards so the
tests prove these inputs reach the intended guards rather than passing through
the unrelated non-Fixed damage-amount fallback.
In `@crates/engine/tests/integration/bartz_and_boko_each_source_damage.rs`:
- Around line 80-82: Update both Bartz fixtures in the positive and negative
test setups to include Bartz’s printed Human and Bird subtypes after creating
the card, using the existing subtype/card-data configuration mechanism. Keep the
assertions unchanged so the tests exercise exclusion of the Bird subtype
candidate.
- Around line 33-35: Update the rules citation in the test module’s opening
comments to reference CR 608.2h for the last-known-information fallback,
retaining CR 113.7a only if needed to describe ability independence from its
source. Ensure the cited rule directly supports the source lookup behavior
implemented by the test.
---
Nitpick comments:
In `@crates/engine/src/game/quantity.rs`:
- Around line 425-464: Unify scope-carrying QuantityRef classification across
quantity_expr_contains_scope, quantity_expr_uses_resolution_only_object_scope’s
scope_is_resolution_only, and quantity_expr_missing_resolution_only_referent’s
leaf_scope_missing. Extract a shared helper or exhaustive match that includes
CountersOn and all existing scoped variants, then reuse it so future QuantityRef
additions cannot drift between these checks.
🪄 Autofix
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: d15bd8c3-ae45-4fa5-8db8-d1666fe331b7
📒 Files selected for processing (13)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/replacement.rscrates/engine/src/game/restrictions.rscrates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/bartz_and_boko_each_source_damage.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 10 card(s), 15 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — 2 test-discrimination blockers on 5fddcdcee12ad6cd838b20fb373945eb9bd0b170.
🔴 Blocker
-
subject.rs:7548— the Season's Beatings and Master of the Wild Hunt tests only matchEffect::Unimplemented. Before this PR, their own-power amounts already fell through the unrelated non-Fixedrejection, so both tests still pass if the newrandom/tapped this wayguards are removed. Assert the guard-owned key,name == "each_source_unrepresentable_rider", in both tests. This makes the fail-closed path discriminating. -
bartz_and_boko_each_source_damage.rs:80and:135— the two Bartz fixtures do not give Bartz its printedHumanandBirdsubtypes. It therefore never enters the candidate set, so the positive and zero-member tests can both pass ifFilterProp::Anotherstops excluding it. Add.with_subtypes(vec!["Human", "Bird"])to both fixtures; retain the existing assertions so they exercise the intended exclusion.
🟡 Correction
bartz_and_boko_each_source_damage.rs:33 attributes LKI lookup to CR 113.7a. The current official CR 608.2h is the direct authority: when information is required from a specific object no longer in its expected public zone, use its last known information. Cite CR 608.2h for the fallback (and retain CR 113.7a only for ability/source independence if that is the claim being made).
These current-head findings are independently reported by CodeRabbit and confirmed against the diff. The implementation’s parser/resolver seam otherwise looks appropriate; after the fixes, please let CI and the required current-head parse-diff artifact complete for a renewed review.
Recommendation: update this PR with the two discriminating fixtures and citation correction, then request re-review.
…ng (review) Follow-up to the each-source-damage fix (5fddcdc) addressing code review: - subject.rs: the Season's Beatings ("random" recipient) and Master of the Wild Hunt ("tapped this way") fail-closed tests previously matched only `Effect::Unimplemented { .. }` — the same outcome as the pre-guard fallthrough, so both passed without the guards. Pin the guard-owned key `each_source_unrepresentable_rider` (via a const comparison) so the fail-closed path discriminates; neutralized-guard revert probe fails both. - bartz integration tests: give Bartz its printed Human/Bird subtypes in both fixtures so he genuinely matches the "Bird" filter, making the `FilterProp::Another` "other" exclusion load-bearing (probes: dropping "other" with subtypes present fails both tests; without subtypes both pass regardless). - CR annotations: the batch-member LKI fallback is CR 608.2h (effect reads a specific object no longer in its expected public zone -> last known information), not CR 113.7a (ability-source independence). Corrected the 7 commit-added comment sites and the integration test module doc.
There was a problem hiding this comment.
Reviewed current head 4ab57b9. The prior blockers are resolved: strict-failure parser fixtures now assert the exact unimplemented key, the Bartz runtime fixture makes Bartz a Human Bird so the “other” exclusion is real, and CR 608.2h correctly supports the LKI fallback. Manual trace confirms ObjectScope::BatchSource is rebound from the per-source anaphor, resolved per batch member, and preserved through replacement continuations. Required CI and parse-diff are current and clean; CodeRabbit’s current-head delta sweep found no new actionable finding. Its older classifier-drift nit remains nonblocking because this PR does not produce a CountersOn BatchSource card path.
One conflict, in the import list of oracle_effect/subject.rs: this branch added EffectScope for the broadcast ForceAttack arm while phase-rs#7326 / phase-rs#7322 / phase-rs#7333 added ObjectScope. Both are needed; kept both.
2 participants
Summary
Fixes the mis-parse of the filter-source own-power damage class — "each you control deals damage equal to its power to " (Bartz and Boko's ETB trigger). The clause fell through
try_parse_each_source_deals_damage's Fixed-only amount guard, dropping the "each you control" subject and mis-parsing to a single ability-sourcedDealDamagewhosePower { Anaphoric }amount resolved against the trigger source instead of each batch member. The fix extendsEffect::EachSourceDealsDamage.amountto per-source resolution via a newObjectScope::BatchSource(CR 120.1), resolves the amount per source id (live + LKI, CR 113.7a), and rebinds the clause's deferred "its" pronoun to the per-batch-source scope at the each-source intercept.Files changed
crates/engine/src/types/ability.rs—ObjectScope::BatchSourcevariant;EachSourceDealsDamage.amountdoccrates/engine/src/game/quantity.rs—QuantityContext.damage_sourcefield;quantity_expr_contains_scope;resolve_quantity_with_targets_and_damage_source; BatchSource arms at all ObjectScope match sites;damage_source: Nonethreading (15 construction sites)crates/engine/src/game/effects/deal_damage.rs— per-source amount resolution inresolve_each_source_deals_damage(simultaneous-batch/stash paths unchanged)crates/engine/src/parser/oracle_effect/subject.rs— guard replaced with structural anaphor detection + rebind; fail-closed rider gate (randomrecipient,tapped this waysources); flips the old negative test; new parser tests; 10-card class commentcrates/engine/src/game/ability_rw.rs,ability_scan.rs,coverage.rs,effects/mod.rs,layers.rs,replacement.rs,restrictions.rs— additive BatchSource classification armscrates/engine/tests/integration/bartz_and_boko_each_source_damage.rs(new) — discriminating runtime testcrates/engine/tests/integration/main.rs— mod lineTrack
Developer
LLM
Model: deepseek-v4-flash-0731
Tier: Frontier
Thinking: max
Implementation method (required)
Method: /engine-implementer
CR references
120.1, 120.3, 120.4a, 120.4b, 120.6, 120.10, 704.5g, 113.7a, 208.1, 208.3, 608.2, 608.2c, 202.3, 109.4, 616.1; 303.4 (deferred attached-host fail-closed precedent). All verified in
docs/MagicCompRules.txt.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— clean, no diff.cargo clippy --all-targets -- -D warnings— clean (exit 0).cargo test -p phase-engine— lib 18906 passed / 0 failed / 6 ignored; integration 4839 passed / 0 failed / 2 ignored; 21 bin + 9 doc tests passed / 0 failed../scripts/gen-card-data.sh— exit 0;bartz and bokotrigger effect =EachSourceDealsDamage, amount scope =BatchSource, noparse_warnings.scripts/check-parser-combinators.sh c162191c9— Gate G PASS; Gate A PASS (see below).cargo coverage— exit 0; Bartz and Boko / Judgment of Alexander / Signature Slamsupported=true, gap=0.cargo semantic-audit— exit 0.cargo test -p phase-engine --test integration bartz— 2 passed (discriminating runtime tests).Gate A
Gate A PASS head=5fddcdcee12ad6cd838b20fb373945eb9bd0b170 base=c162191c91ff7aa2434c42baeb3fc9d60ce510a1
Anchored on
resolve_each_target_power_damage) — theEachDealsDamageEqualToPowersibling resolver: each chosen source's own power to a shared recipient (CR 120.1); the per-source amount resolution added here mirrors its per-source semantics.resolve_quantity_with_targets_and_recipient) — wrapper-shape precedent thatresolve_quantity_with_targets_and_damage_sourcemirrors.Final review-impl
Final review-impl PASS head=5fddcdcee12ad6cd838b20fb373945eb9bd0b170
Claimed parse impact
Bartz and Boko, Judgment of Alexander, Kamahl's Will, Master of the Wild Hunt, Moonlight Hunt, Nissa's Judgment, Sarkhan the Mad, Season's Beatings, Signature Slam, The Bears of Littjara. (8 parse to
EachSourceDealsDamagewith aBatchSourceper-source amount; Season's Beatings and Master of the Wild Hunt's each-source clauses fail closed toEffect::Unimplementedbecause their riders are unrepresentable.)Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes