Fix Jagged Lightning - #6692
Conversation
…gged Lightning)
"<source> deals N damage to each of <cardinality> <noun>" is a 3x2 grammar
matrix -- cardinality {bounded, optional, exact} x noun {bare-plural "targets"
(CR 115.4), typed "target <type>" (CR 601.2c)} -- of which only two cells were
implemented, one each by `strip_bounded_targets_placeholder` and
`strip_optional_target_prefix`. Every other cell fell through to
`Effect::DamageAll`, silently turning a two-target spell into a board wipe:
Jagged Lightning dealt 3 damage to EVERY creature instead of to each of two
chosen ones.
Parameterize the seam across both axes with one authority combinator
(`parse_each_of_target_distribution`) rather than adding sibling strippers, and
route both damage-seam consumers through it so the target filter and the
announced count come from a single parse and cannot disagree. The count travels
on `ParseContext` with an explicit set/reset lifecycle, so
`extract_deal_damage_multi_target` is demoted to a fallback and left unchanged.
CR 601.2c fixes the announced target count ("In some cases, the number of
targets will be defined by the spell's text"); CR 115.4 gives the bare plural
its damage target class (creature, player, planeswalker, or battle).
17 cards move from DamageAll to DealDamage with a correct MultiTargetSpec:
Jagged Lightning, Swelter, Twinstrike, Furious Reprisal, Pinnacle of Rage,
Firestorm, Meteor Blast, Fall of the Titans, Jaya's Immolating Inferno,
Shower of Coals, Myojin of Roaring Blades, Chandra the Firebrand,
Chandra Hope's Beacon, Jeska Thrice Reborn, Batroc the Leaper,
Betrayal at the Vault, Spinning Wheel Kick.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TTbBQugiEc8PYTxUvE56w
…eculative and nested parses Addresses review findings on the "each of <N> target(s)" damage head. - `try_parse_radiance_color_fanout_damage` parsed against the real `ParseContext` and abandoned on three branches with the announced count still recorded, so a discarded attempt could hand its count to whatever clause committed next. It now clones and commits on success only, matching the shape `try_parse_multi_target_damage_chain` already uses. - `try_split_damage_compound` re-enters `lower_imperative_clause` for the continuation, and that nested frame clears the side channel on entry. It now saves the primary head's count across the sub-parse and attaches it to the clause it returns -- this function returns ahead of the `take()` + DealDamage fixup, so the count had to be applied here or it was dropped. - Correct the `ParseContext` doc comment, which claimed the compound continuation sites parse into a clone. Only the chain recognizer did. - Name the one cardinality x noun cell the combinator deliberately does not cover: `other`/`another` on the bare-plural arm, where there is nowhere for CR 115.3 cross-slot distinctness to land. The bare-damage CHAIN recognizer still drops the count for a chained each-of head. No printed card has that shape, and the test now documents it as a known gap rather than asserting it as correct. No parse output changes: the regenerated export still differs from the pre-change baseline in exactly the same 17 cards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019TTbBQugiEc8PYTxUvE56w
…-of remainder
Second review round.
`parse_each_of_target_distribution` returned a left-trimmed remainder, but a
leading space is the compound-boundary marker every consumer of
`try_parse_damage_with_remainder` keys on -- `try_split_damage_compound` matches
`tag(" and ")` and explicitly does not trim. So a bare-plural head followed by a
second clause ("... to each of two targets and you gain 2 life") handed the
splitter "and you gain 2 life", the match failed, and the continuation was
silently dropped, while the same sentence with a typed noun split correctly.
Return the remainder untrimmed and trim only at the typed call site, which is
the arm that actually needs a phrase starting at "target ".
Also correct the Batroc pin test's comment: it claimed the card's backlog bullet
stays, but the regenerated export shows its root-cause-phase-rs#15 defect is genuinely
fixed and the bullet was retired with the rest. What remains untested there is
the runtime path, not the parse.
No parse output changes: still exactly the same 17 cards versus the pre-change
baseline.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019TTbBQugiEc8PYTxUvE56w
github-actions
Bot
added
the
needs-maintainer
|
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 Oracle parser now recognizes “each of” damage target cardinalities, preserves announced counts through clause lowering, prevents speculative or cross-clause state leakage, and adds parser and integration coverage for exact, variable, heterogeneous, and optional targeting. ChangesEach-of damage target parsing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant OracleParser
participant ParseContext
participant EffectLowerer
participant GameRunner
OracleText->>OracleParser: parse each-of cardinality and noun
OracleParser->>ParseContext: store pending target count
EffectLowerer->>ParseContext: consume pending target count
EffectLowerer->>GameRunner: produce DealDamage with multi_target
GameRunner->>GameRunner: resolve declared targets and damage
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)crates/engine/tests/fixtures/integration_cards.jsonCheckov skipped this file: it is too large to scan (10838464 bytes) 🔧 ast-grep (0.44.1)crates/engine/tests/fixtures/integration_cards.jsonast-grep skipped this file: it is too large to scan (10838464 bytes) crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/parser/oracle_effect/mod.rs (2)
14848-14867: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
pending_damage_multi_targetis not actually the primary authority — the text-scanning block at Line 14852 runs first.For a
DealDamageclause with a target filter, the generic fixup at Lines 14852-14857 (extract_exact_target_multi_target/ bounded / optional) already populatesclause.multi_target, so by Line 14864clause.multi_target.is_none()is false and the ctx spec is discarded. The comment at Lines 14859-14863 claims the opposite. Today this is invisible because the two authorities agree on the shapes covered by tests (Dire-Strain is exactly the both-fire case), but when they disagree the text scan silently wins — which is the failure mode this PR set out to remove.Simplest fix: consume the ctx spec before the generic block.
🐛 Proposed reordering
+ // CR 601.2c + CR 115.4: the announced target count for a + // "⟨source⟩ deals N damage to each of ⟨count⟩ ⟨noun⟩" head comes from the + // SAME parse that produced the target filter, so it wins over any + // text-scanning extractor below. + if matches!(clause.effect, Effect::DealDamage { .. }) && clause.multi_target.is_none() { + clause.multi_target = pending_damage_multi_target; + } if clause.multi_target.is_none() && triggers::extract_target_filter_from_effect(&clause.effect).is_some() { clause.multi_target = extract_exact_target_multi_target(text) .or_else(|| extract_bounded_target_multi_target(text)) .or_else(|| extract_optional_target_multi_target(text)); } - if matches!(clause.effect, Effect::DealDamage { .. }) && clause.multi_target.is_none() { - clause.multi_target = - pending_damage_multi_target.or_else(|| extract_deal_damage_multi_target(text)); + if matches!(clause.effect, Effect::DealDamage { .. }) && clause.multi_target.is_none() { + clause.multi_target = extract_deal_damage_multi_target(text); }A disagreement fixture (ctx spec vs. text scan producing different counts) would pin the intended precedence.
🤖 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/mod.rs` around lines 14848 - 14867, Reorder the post-parse fixup blocks so the DealDamage branch consumes pending_damage_multi_target before the generic target-filter extraction. Preserve pending_damage_multi_target as the primary value for DealDamage clauses, using extract_deal_damage_multi_target(text) only when the context value is absent; leave the generic exact/bounded/optional extraction for clauses not already assigned a count.
17184-17204: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe primary count is taken before the compound-shape guards and never restored on the bail paths.
take()at Line 17190 empties the side channel, but Lines 17192-17204 can still returnNone(empty remainder, no" and "connector, empty sub-text). On those pathstry_split_damage_compoundbails with the primary head's count already discarded fromctx, and the caller continues intoparse_imperative_effectat Line 14833 relying on a re-parse to re-record it. That recovery is incidental, not guaranteed — any bail after the head has parsed loses the CR 601.2c count.Either move the
take()below the connector guards, or restore on bail:🐛 Proposed fix
- let primary_damage_multi_target = ctx.pending_damage_multi_target.take(); - if remainder.is_empty() { + return None; + } + + // The remainder must start with " and " to be a compound connector. + // Do NOT trim — the leading space is the boundary marker. + let (after_and, _) = tag::<_, _, OracleError<'_>>(" and ") + .parse(remainder) + .ok()?; + let sub_text = after_and.trim(); + if sub_text.is_empty() { return None; } + let primary_damage_multi_target = ctx.pending_damage_multi_target.take();(and drop the duplicated guard block below)
🤖 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/mod.rs` around lines 17184 - 17204, Preserve the primary damage target count when compound parsing exits through its shape guards. In try_split_damage_compound, move the pending_damage_multi_target.take() operation until after the remainder, " and " connector, and non-empty sub_text checks, removing the now-duplicated lower guard block; alternatively restore the captured value before every early None return.
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/lower.rs (1)
12576-12599: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the second bounded cardinality (
one, two, or three) to the matrix.Only
one or twois exercised, so the loop overBOUNDED_TARGET_CARDINALITIESis pinned for a single entry. A"one, two, or three targets"cell would also guard the comma-containing stem against future tokenization changes.♻️ Suggested additional cell
+ // Cell 1b — the second bounded stem, incl. the comma-containing form. + assert_eq!( + parse_each_of_target_distribution("one, two, or three targets"), + Some(( + MultiTargetSpec::fixed(1, 3), + EachOfTargetNoun::AnyTargets, + "" + )) + );🤖 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/lower.rs` around lines 12576 - 12599, Add a regression assertion alongside the existing bounded-cardinality cases for “one, two, or three targets,” verifying it parses to MultiTargetSpec::fixed(1, 3), EachOfTargetNoun::AnyTargets, with an empty remainder; use the existing parse_each_of_target_distribution test matrix and preserve the ordering guard.
🤖 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/parser/oracle_effect/mod.rs`:
- Around line 14848-14867: Reorder the post-parse fixup blocks so the DealDamage
branch consumes pending_damage_multi_target before the generic target-filter
extraction. Preserve pending_damage_multi_target as the primary value for
DealDamage clauses, using extract_deal_damage_multi_target(text) only when the
context value is absent; leave the generic exact/bounded/optional extraction for
clauses not already assigned a count.
- Around line 17184-17204: Preserve the primary damage target count when
compound parsing exits through its shape guards. In try_split_damage_compound,
move the pending_damage_multi_target.take() operation until after the remainder,
" and " connector, and non-empty sub_text checks, removing the now-duplicated
lower guard block; alternatively restore the captured value before every early
None return.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/lower.rs`:
- Around line 12576-12599: Add a regression assertion alongside the existing
bounded-cardinality cases for “one, two, or three targets,” verifying it parses
to MultiTargetSpec::fixed(1, 3), EachOfTargetNoun::AnyTargets, with an empty
remainder; use the existing parse_each_of_target_distribution test matrix and
preserve the ordering guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3692ca02-6183-443d-aa64-3212cfeca77e
📒 Files selected for processing (8)
crates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/jagged_lightning_each_of_two_targets.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
# Conflicts: # crates/engine/tests/fixtures/integration_cards.json
Parse changes introduced by this PR · 17 card(s), 30 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current-head parser review is clean. The parameterized each-of damage target grammar is at the right lowering seam; the published parse diff accounts for the declared 17-card class, and the integration suite exercises exact, bare-target, X-count, and optional-target runtime paths.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The multi-target damage-chain early return drops the primary announced target count. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:16590-16597 records the count through try_parse_damage_with_remainder, but :16679-16686 constructs the returned ParsedEffectClause with multi_target: None; the caller returns before lower_imperative_clause reaches its take()/fallback at :14894-14922. The current regression test deliberately asserts that incorrect None at crates/engine/src/parser/oracle_effect/tests.rs:49715-49737. Why it matters: a spell shaped deals N damage to each of two target creatures and M damage to … gets one mandatory target instead of the text-defined two, contrary to CR 601.2c; the target-slot pipeline expands only when AbilityDefinition.multi_target is present (crates/engine/src/game/ability_utils.rs:4256-4284). Suggested fix: capture the primary pending_damage_multi_target in try_parse_multi_target_damage_chain_inner before parsing continuations and assign it to the returned primary clause; change the chain regression to require Some(MultiTargetSpec::exact(2)) while retaining the no-leak assertion for the following sentence.
[LOW] The new cardinality×noun matrix does not exercise its second shared bounded stem. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:6110-6111 includes "one, two, or three", but the dedicated matrix at :12609-12641 asserts only "one or two". Why it matters: the comma-containing shared branch can regress without a matrix failure. Suggested fix: add a one, two, or three targets assertion expecting MultiTargetSpec::fixed(1, 3) (and, ideally, its typed-noun sibling).
3 participants
Summary
Fixes the dropped-target-count misparse (
docs/parser-misparse-backlog.mdroot cause #15, "Multi-target / 'up to N' optionality or count dropped") for Jagged Lightning and its whole class."<source> deals N damage to each of <cardinality> <noun>"is a cardinality × noun grammar matrix — cardinality {boundedone or two/ optionalup to N/ exactN|X} × noun {bare-pluraltargets(CR 115.4) / typedtarget <type>(CR 115.1a)}. Only two of the six cells were implemented, one each bystrip_bounded_targets_placeholderandstrip_optional_target_prefix. Every other cell fell through toEffect::DamageAll, silently turning a two-target spell into a board wipe:i.e. it dealt 3 damage to every creature on the battlefield, with nothing at runtime flagging it.
Rather than adding a third and fourth sibling stripper, this parameterizes the seam across both axes with one authority combinator (
parse_each_of_target_distribution) and routes both damage-seam consumers through it, so the target filter and the announced count come from a single parse and cannot disagree. The count travels onParseContextwith an explicit set/reset lifecycle;extract_deal_damage_multi_targetis demoted to a fallback and left byte-for-byte unchanged.Backlog hygiene: six cards removed from root cause #15, with the section header, ranked-table row and header totals updated to match.
Files changed
crates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/tests/integration/jagged_lightning_each_of_two_targets.rs(new)crates/engine/tests/integration/main.rscrates/engine/tests/fixtures/integration_cards.jsondocs/parser-misparse-backlog.mdTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
TargetFilter::Any.Effect::DealDamage.Note: the pre-existing
CR 115.1dannotations near this code are a mis-citation (115.1d is about triggered abilities being targeted, not target counts). New code cites CR 601.2c instead; existing occurrences are deliberately left untouched for a separate sweep.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— cleancargo test -p engine --lib—test result: ok. 17713 passed; 0 failed; 6 ignoredcargo test -p engine --test integration—test result: ok. 4009 passed; 0 failed; 2 ignoredcargo clippy-strict— exit 0 (-D warnings)./scripts/gen-card-data.sh— exit 0,Generated client/public/card-data.json (94M, ~35479 cards)Card-data diffed against a pre-change baseline export — exactly 17 cards changed, 0 added, 0 removed. No collateral movement.
Tilt was not running in this environment (
tilt get uiresource clippynon-zero), so the direct-cargo fallback path from CLAUDE.md's risk-scaled cadence was used.Gate A
Base is
upstream/main(the PR target branch), passed explicitly per the script's documented CI usage.Anchored on
crates/engine/src/parser/oracle_effect/counter.rs:307— the existing exact-counteach of <N|X> target <type>→MultiTargetSpec::exact(count)arm in the sibling PutCounter target parser. Same combinator family (parse_multi_target_count_expr+ separator +peek(tag("target"))), same CR 601.2c annotation. This is precisely whyThriveandRot-Curse Rakshasaparse correctly today whileJagged Lightningdid not: the counter seam grew the exact axis and the damage seam never did.crates/engine/src/parser/oracle_effect/lower.rs:6139—strip_bounded_targets_placeholder, the existing bare-plural cardinality stripper that composes its noun off theBOUNDED_TARGET_CARDINALITIESconst and returns(remainder, MultiTargetSpec).parse_bounded_target_cardinalitymirrors itsfor-over-const +tag(stem)shape exactly, as that constant's own doc comment prescribes.crates/engine/src/parser/oracle_effect/lower.rs:6197—strip_optional_target_prefix, the existingup to N→MultiTargetSpec::up_tocardinality stripper; the optional arm is its combinator form.crates/engine/src/parser/oracle_effect/lower.rs:6350— the existingterminated(tag(" tapped"), not(satisfy(|c: char| c.is_alphanumeric())))word-boundary fence, mirrored by the bare-pluraltargetsarm.crates/engine/src/parser/oracle_effect/mod.rs:16517— the existinglet mut tentative_ctx = ctx.clone(); … *ctx = tentative_ctx;clone-and-commit wrapper ontry_parse_multi_target_damage_chain, mirrored fortry_parse_radiance_color_fanout_damage.Final review-impl
Three independent review rounds ran against isolated fresh contexts, each handed only the diff plus
CLAUDE.mdand the skills. Rounds 1 and 2 returned findings; every one was addressed with code (commits50d63c474andd3f3f536a). Round 3 returned zero findings against this head.Claimed parse impact
17 cards, all
DamageAll/Unimplemented→DealDamagewith a correctMultiTargetSpec. Verified by regeneratingcard-data.jsonand diffing against a pre-change baseline:exact(2)exact(2)exact(2)exact(2)exact(2)exact(2)exact(X)exact(X)exact(X)up_to(2)up_to(2)up_to(3)up_to(3)up_to(3)up_to(3)up_to(6)up_to(KickerCount)Two are first-sentence-only fixes: Shower of Coals' Threshold replacement sentence and Twinstrike's Hellbent sentence were already dropped before this change and still are — verified untouched in the diff.
Cards that must not regress and did not (all verified unchanged): Dire-Strain Anarchist, Volatile Arsonist, Wrap in Flames, Slight Malfunction, Summon: Kujata, The Rollercrusher Ride, Smoldering Werewolf, Cast into the Fire, Dual Shot, Fire Shrine Keeper, Prismari Charm, Rowan Fearless Sparkmage, Sparkmage's Gambit, Storm of Steel, Volcanic Salvo.
Deliberately not fixed, and left honestly red: Drakuseth, Maw of Flames (
each of up to two *other* targets— the bare-plural arm has nowhere to land CR 115.3 cross-slot distinctness, and its second damage clause is dropped upstream anyway; root cause #27), Nahiri's Wrath and Crackle with Power (the amount clause is unparsed, not the target count), and The Ultimate Nightmare of Wizards of the Coast® Customer Service.Backlog: six cards removed from §15 (Batroc the Leaper, Chandra Hope's Beacon, Jagged Lightning, Jaya's Immolating Inferno, Pinnacle of Rage, Shower of Coals). §15 header 89→83 (live bullet count is exactly 83), ranked-table row 15 89→83, "Total card appearances" 4767→4761, "Distinct cards implicated" 4733→4727 — each of the six appears zero times elsewhere in the document, so both deltas are −6. Firestorm, Furious Reprisal, Myojin and Betrayal at the Vault appear under other root causes for unrelated defects and were deliberately left alone.
Scope Expansion
None. No new engine enum variant (
MultiTargetSpec::{fixed, up_to, exact}all pre-exist); no runtime, frontend, AI, or multiplayer change;mtgish/untouched.EachOfTargetNounis a file-private two-arm parser classification enum with no serialized surface.Validation Failures
Three process deviations, disclosed rather than papered over:
TEMP-REVERT-PROBE(if true { …old code…; return; }) inlower.rswith the new implementation dead behind#[allow(unreachable_code)], two out-of-scope card-data pipeline artifacts modified (known-tokens.toml,mtgjson-vintage), and the backlog edit undone. All three were caught and corrected before the first commit; the probe removal is why the final verification was re-run from scratch.try_parse_multi_target_damage_chain) still drops the announced count for a chained each-of head. No printed card has that shape — the onlyAtomicCards.jsonentries pairing an "each of ⟨N⟩" head with a following "and" use "and/or" inside the target phrase and all parse with their count intact — so nothing regresses. The test asserts the current behaviour with a comment telling a future maintainer to flip it toexact(2)if the chain recognizer learns to thread the count. Threading it is a separate change against that recognizer, not this seam.CI Failures
None. Two local failures during the run were environmental, not code: a
cr733integration test andgen-card-data.shboth failed under a full disk (No space left on device), and both passed after freeing build artifacts.Summary by CodeRabbit
Bug Fixes
Tests
Documentation