fix(parser): bind "twice its power" damage subject to the target, not the spell (#6208)#6220
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 parser and runtime issues for cards like Inspiring Call and Punishing Punch by correctly rebinding pronouns and source-scoped power references to their intended targets, supported by new integration tests. The review feedback notes an incorrect Magic Comprehensive Rules (CR) citation in the comments, which should be updated to CR 608.2c to align with the repository's style guide.
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 603.7 + issue #6065: "those creatures gain <keyword>" after a | ||
| // "draw a card for each <creature filter>" clause (Inspiring Call). | ||
| // Draw publishes no tracked set (its target is the drawing player), | ||
| // so the branch above skips it and the grant's ParentTarget would | ||
| // resolve to the controller. Bind it directly to the nearest prior | ||
| // Draw's count filter — the counted creatures the pronoun names. |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR rule citation and format. Evidence: crates/engine/src/parser/oracle_effect/assembly.rs:2093.
Why it matters: Citing CR 603.7 (delayed triggers) for a continuous effect on a resolving spell violates the mandatory CR verification rule, and the format must strictly be CR <number>: <description>.
Suggested fix: Change the citation to CR 608.2c and format it with a colon directly after the rule number.
| // CR 603.7 + issue #6065: "those creatures gain <keyword>" after a | |
| // "draw a card for each <creature filter>" clause (Inspiring Call). | |
| // Draw publishes no tracked set (its target is the drawing player), | |
| // so the branch above skips it and the grant's ParentTarget would | |
| // resolve to the controller. Bind it directly to the nearest prior | |
| // Draw's count filter — the counted creatures the pronoun names. | |
| // CR 608.2c: "those creatures gain <keyword>" after a | |
| // "draw a card for each <creature filter>" clause (Inspiring Call). | |
| // Draw publishes no tracked set (its target is the drawing player), | |
| // so the branch above skips it and the grant's ParentTarget would | |
| // resolve to the controller. Bind it directly to the nearest prior | |
| // Draw's count filter — the counted creatures the pronoun names. |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . The cited rule's body must describe what the code is doing. (link)
Parse changes introduced by this PR · 4 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the Punishing Punch fix is promising, but the bundled Inspiring Call change is rules-incorrect and needs a separate, snapshot-aware implementation.
🔴 Blocker
[HIGH] The Inspiring Call grant is bound to a live filter rather than the objects counted when the spell resolved. Evidence: crates/engine/src/parser/oracle_effect/assembly.rs:2099-2106 copies the preceding Draw's ObjectCount filter into the grant, and crates/engine/src/game/layers.rs:3428-3503 preserves that filter on the active continuous effect; layers.rs:2648-2668 explicitly reevaluates affected filters as object population changes. The new runtime test only checks the state immediately after resolution (crates/engine/tests/integration/inspiring_call_indestructible_grant.rs:42-52). Why it matters: a creature initially counted can lose its counter and lose indestructible, while a creature that gains a counter later can gain it; neither is the resolved set denoted by “Those creatures.” Suggested fix: publish/snapshot the counted object IDs at resolution and bind the grant to that exact set, with post-resolution tests for both a counter loss and a later counter gain.
🟡 Non-blocking
[MED] The PR silently combines two independent issue fixes. Evidence: the stated #6208 scope is the target-subject damage change, while the diff also adds #6065's draw_object_count_filter/rewrite_grant_parent_to_filter path and inspiring_call_indestructible_grant.rs. Why it matters: the unrelated behavior change obscures review and makes the current semantic defect harder to isolate. Suggested fix: keep the verified Punishing Punch change in this PR and move a corrected, snapshot-aware Inspiring Call implementation to its own PR.
Gemini's generic CR-citation note was considered, but it gives no location or contradictory rule text; the concrete blocker above is independently confirmed from the current head.
Recommendation: remove/split the Inspiring Call change and resubmit it with a resolved-set implementation; then the Punishing Punch slice can be re-reviewed on its own.
… the spell (phase-rs#6208) Punishing Punch — "Target creature you control deals damage equal to twice its power to target creature an opponent controls." — dealt 0 damage. "Its power" names the first target (the creature dealing the damage). The singular form lowers "its power" to Power{Anaphoric}, which the target-subject path rebinds to Power{Target}. The multiplier form lowers it via the CDA path to Multiply{2, Power{Source}}, and bind_damage_clause_source only rebinds Anaphoric leaves, so Power{Source} survived, read the spell (power 0), and dealt nothing. In the target-subject damage path (wrap_target_subject_damage), after binding damage_source = Target, retarget any residual Source-scoped Power/Toughness leaf in the amount to Target. In that path the subject is the target creature, never the spell, so its self-referential characteristic must follow the subject. Restricted to power/toughness so a legitimate ObjectManaValue{Source} ("this spell's mana value") is left intact. Duggan, Private Detective ("Duggan deals damage equal to twice its power") is self-subject — a bare DealDamage never routed through the target-subject wrapper — so its Power{Source} is untouched (its full-kit test is the regression guard). Class of 4: Punishing Punch, Animist's Might, Polliwallop, Thing Swing. Runtime already resolves Multiply{2, Power{Target}} + damage_source Target (Chandra's Ignition, Duggan's runtime test), so no runtime change. Tests: a parser test (amount is Multiply{2, Power{Target}}, damage_source Target) with the singular and Duggan tests as regression guards, and a runtime test that casts Punishing Punch and asserts 2 x the subject's power is dealt.
3439b9b to
88b7cb0
Compare
|
Thanks for the review. You're right that the Inspiring Call change didn't belong here — I've un-bundled it and rebased onto current To explain how it got mixed in: the Inspiring Call fix already landed separately as #6109, but I branched this work off a stale local On the substance you raised about the Inspiring Call grant binding to a live filter rather than the resolution-time snapshot of counted creatures — that's a fair point about the code now in The Punishing Punch fix itself is unchanged from what you called promising: in the target-subject damage path the "twice its power" multiplier form left |
📝 WalkthroughWalkthroughThe parser now retargets source-scoped power and toughness references in bound damage amounts to the target subject. Parser and integration tests cover Punishing Punch, confirming damage equals twice the subject creature’s power. ChangesDamage amount retargeting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleEffectParser
participant bind_damage_clause_source
participant retarget_source_power_toughness
participant DealDamage
OracleEffectParser->>bind_damage_clause_source: Bind damage clause source
bind_damage_clause_source->>retarget_source_power_toughness: Retarget amount expression
retarget_source_power_toughness-->>bind_damage_clause_source: Return Power{Target}
bind_damage_clause_source->>DealDamage: Store retargeted damage amount
🚥 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: 2
🤖 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/tests.rs`:
- Around line 4901-4913: Extend the tests around the existing amount assertion
to cover the helper’s remaining contract: add focused coverage that retargets
Toughness with Source to the expected target scope, and verify mixed quantities
preserve ObjectManaValue with Source. Keep these as building-block cases
independent of a single card scenario, using the existing QuantityExpr patterns
and assertion style.
In `@crates/engine/tests/integration/punishing_punch_twice_subject_power.rs`:
- Around line 44-55: Add a production-pipeline assertion to the punishing-punch
scenario that applies a source-sensitive protection, infect, or wither rule
after resolving the cast, using the subject creature’s identity so the observed
outcome differs if DamageSource::Target regresses to the spell. Extend the
existing test around runner.cast and the outcome assertions while preserving the
current damage check.
🪄 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: 15df6ac0-61fe-4532-b475-30d82ded5d5e
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/punishing_punch_twice_subject_power.rs
| assert!( | ||
| matches!( | ||
| amount, | ||
| QuantityExpr::Multiply { factor: 2, inner } | ||
| if matches!( | ||
| inner.as_ref(), | ||
| QuantityExpr::Ref { | ||
| qty: QuantityRef::Power { scope: ObjectScope::Target } | ||
| } | ||
| ) | ||
| ), | ||
| "amount must be 2 x TARGET power (not Source, which reads the spell), got {amount:?}" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the helper’s remaining contract.
This covers only nested Power{Source}. Add focused coverage for Toughness{Source} retargeting and for a mixed quantity retaining ObjectManaValue{Source}; otherwise the helper’s stated boundary is unguarded.
As per coding guidelines, “test the building block and its parameter range rather than a single card case.”
🤖 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/parser/oracle_effect/tests.rs` around lines 4901 - 4913,
Extend the tests around the existing amount assertion to cover the helper’s
remaining contract: add focused coverage that retargets Toughness with Source to
the expected target scope, and verify mixed quantities preserve ObjectManaValue
with Source. Keep these as building-block cases independent of a single card
scenario, using the existing QuantityExpr patterns and assertion style.
Source: Coding guidelines
| let outcome = runner | ||
| .cast(punch) | ||
| .target_object(subject) | ||
| .target_object(recipient) | ||
| .resolve(); | ||
|
|
||
| assert_eq!( | ||
| outcome.state().objects[&recipient].damage_marked, | ||
| 6, | ||
| "Punishing Punch deals twice the SUBJECT's power (2 x 3 = 6), not 0 (the \ | ||
| dropped Source referent reads the spell) and not the recipient's own power (4)" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Exercise DamageSource::Target through a source-sensitive runtime rule.
This test proves the amount, but it would still pass if runtime damage source regressed to the spell. Add a production-pipeline case involving protection, infect, or wither so the subject creature’s identity is required for the observed result.
As per path instructions, “a parser AST shape test does NOT prove runtime semantics.”
🤖 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/punishing_punch_twice_subject_power.rs`
around lines 44 - 55, Add a production-pipeline assertion to the punishing-punch
scenario that applies a source-sensitive protection, infect, or wither rule
after resolving the cast, using the subject creature’s identity so the observed
outcome differs if DamageSource::Target regresses to the spell. Extend the
existing test around runner.cast and the outcome assertions while preserving the
current damage check.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The previous scope issue is resolved, but two related merge blockers remain:
retarget_source_power_toughnesspromises to retarget source Power/Toughness while preserving source mana-value/color references, yet its only new test covers nested source Power. Please add focused coverage for source Toughness → target Toughness and a mixed expression that preservesObjectManaValue { Source }.- The new recursive source-scope walker duplicates the existing
rebind_source_amounttraversal with a different leaf set. Please parameterize the existing traversal with an explicit reference-kind selector (all source refs vs. P/T-only) so futureQuantityExprvariants cannot drift between copies; include the boundary tests above.
The existing cast-pipeline Punishing Punch test is discriminating for the claimed production behavior; this request is limited to the new building block’s untested preservation boundary and duplicated recursive implementation.
2 participants
Fixes #6208
Problem
Punishing Punch — "Target creature you control deals damage equal to twice its power to target creature an opponent controls." — dealt 0 damage.
"Its power" names the first target (the creature you control that deals the damage). The singular form ("deals damage equal to its power") lowers "its power" to
Power{Anaphoric}, whichbind_damage_clause_sourcerebinds toPower{Target}in the target-subject path. But the multiplier form ("twice its power") lowers it via the CDA path toMultiply{2, Power{Source}}, and the rebind only rewritesAnaphoricleaves — soPower{Source}survived, read the spell (power 0), and the clause dealt nothing.Fix
In the target-subject damage path (
wrap_target_subject_damage,oracle_effect/mod.rs), after bindingdamage_source = Target, retarget any residualSource-scopedPower/Toughnessleaf in the amount toTarget. In that path the subject is the target creature, never the spell, so its self-referential characteristic must follow the subject. Restricted to power/toughness so a legitimateObjectManaValue{Source}("this spell's mana value") is left intact.Scoping: Duggan, Private Detective ("Duggan deals damage equal to twice its power") is self-subject — a bare
DealDamagethat is never routed through the target-subject wrapper — so itsPower{Source}is untouched (its existing full-kit test is the regression guard). Blast radius is exactly the 4 target-subject cards that share the pattern: Punishing Punch, Animist's Might, Polliwallop, Thing Swing.Runtime is already supported: the singular
Power{Target}+damage_source: Some(Target)form (Rabid Bite, Chandra's Ignition #607) andMultiply{2, Power{…}}damage (Duggan's runtime test) both resolve today — this fix just combines them, no runtime change.Tests
target_subject_damage_twice_its_power_uses_target_source_power): the amount isMultiply{2, Power{Target}}withdamage_source: Some(Target), notPower{Source}. The singular test and Duggan's full-kit test (Power{Source}self-subject) are the regression guards.punishing_punch_twice_subject_power.rs): casts Punishing Punch with a power-3 subject at a toughness-10 opponent creature —damage_marked == 6(2 × 3), not 0 and not the recipient's own power.CR: 208.1, 120.1.
Summary by CodeRabbit
Bug Fixes
Tests