Skip to content

feat(engine): count opponents dealt any damage this turn (Furious Spinesplitter, You've Been Caught Stealing)#5144

Open
davion-knight wants to merge 1 commit into
phase-rs:mainfrom
davion-knight:engine/opponent-dealt-damage-this-turn-quantity
Open

feat(engine): count opponents dealt any damage this turn (Furious Spinesplitter, You've Been Caught Stealing)#5144
davion-knight wants to merge 1 commit into
phase-rs:mainfrom
davion-knight:engine/opponent-dealt-damage-this-turn-quantity

Conversation

@davion-knight

Copy link
Copy Markdown
Contributor

Summary

Supports "for each opponent who was dealt damage this turn" (ANY damage — combat OR noncombat) on Furious Spinesplitter and You've Been Caught Stealing. The quantity was swallowed → Fixed 1; only the combat form ("dealt combat damage this turn", Tymna the Weaver) was supported.

Parameterize, don't proliferate (per CLAUDE.md) — reuse the existing typed axis, no new variant/bool:

  • Rename PlayerFilter::OpponentDealtCombatDamage { source }OpponentDealtDamage { kind: DamageKindFilter, source }, where DamageKindFilter is the pre-existing { Any, CombatOnly, NoncombatOnly } enum already threaded through the DamageDealtThisTurn resolver. A #[serde(alias)] + a CombatOnly default preserve legacy data (a bare OpponentDealtCombatDamage meant combat-only, not Any).
  • Parser: "dealt {combat|noncombat|} damage this turn" → the matching kind via a nom alt; "dealt combat damage" still → CombatOnly (Tymna unchanged).
  • Resolver: delegate the is_combat decision to the existing damage_record_matches_kind. Noncombat player damage is already recorded in state.damage_dealt_this_turn (is_combat = false), so no new tracking infrastructure.

Files changed

  • crates/engine/src/types/ability.rs (variant parameterization + serde back-compat)
  • crates/engine/src/parser/oracle_quantity.rs (kind-aware clause combinator)
  • crates/engine/src/parser/oracle_effect/mod.rs (token-for-each subject-strip fallback — see Scope Expansion)
  • crates/engine/src/game/quantity.rs (resolver via damage_record_matches_kind)
  • crates/engine/src/game/{effects/mod.rs, effects/deal_damage.rs, effects/speed_effects.rs, ability_scan.rs, ability_rw.rs, triggers.rs, coverage.rs, replacement.rs} (rename threaded through exhaustive matches / doc)
  • crates/engine/src/parser/{oracle_trigger.rs, oracle_trigger_tests.rs, swallow_check.rs}
  • crates/engine/tests/furious_spinesplitter_any_damage.rs, crates/engine/tests/you_have_been_caught_stealing_any_damage.rs (new)

CR references

  • CR 120.1 — dealing damage
  • CR 120.2a — combat damage; CR 120.2b — noncombat damage (the new kind axis)
  • CR 510.1 — combat damage step; CR 120.9 — "damage dealt by a source" restriction

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high
Tier: Frontier

Verification

  • cargo fmt --all — clean
  • ./scripts/check-parser-combinators.sh — clean (exit 0)
  • cargo check -p engine --all-targets — clean (exhaustive PlayerFilter matches all threaded)
  • cargo clippy -p engine --all-targets -- -D warnings — clean
  • cargo test -p engine — 0 failures (incl. updated + new parser/resolver/serde tests and both new runtime tests)
  • ./scripts/gen-card-data.sh + cargo coverageFurious Spinesplitter & You've Been Caught Stealing: supported: true, gap_count: 0; Tymna the Weaver: unchanged (kind: CombatOnly)
  • cargo semantic-audit — 0 findings for all three cards

Tests:

  • Parser (oracle_quantity.rs) — Any and NoncombatOnly forms → PlayerCount { OpponentDealtDamage { kind } }; combat form still CombatOnly.
  • Resolver (game/quantity.rs) — one combat + one noncombat damaged opponent in the ledger: Any → 2, CombatOnly → 1, NoncombatOnly → 1 (kind-discriminating). Legacy-JSON test: {"type":"OpponentDealtCombatDamage"} (no kind) → CombatOnly; round-trip.
  • Runtime, revert-failing (3 players, both opponents dealt noncombat Shock):
    • you_have_been_caught_stealing_any_damage.rs — Bribe mode (modes(&[1])) → 2 Treasure tokens. Pre-fix Fixed 1 → 1; CombatOnly → 0. Reach-guard: parse = OpponentDealtDamage{Any}, no Unimplemented.
    • furious_spinesplitter_any_damage.rs — end-step trigger → 2 +1/+1 counters (read via runner.state().objects[...].counters). Same revert-failing/reach-guard pattern.

Gate A

./scripts/check-parser-combinators.sh — exit 0, no violations.

Anchored on

  • crates/engine/src/types/ability.rsDamageKindFilter (existing {Any,CombatOnly,NoncombatOnly}) reused as the kind axis, mirroring its use in the DamageDealtThisTurn resolver.
  • crates/engine/src/game/quantity.rs damage_record_matches_kind — the existing per-record kind gate the resolver now delegates to (was a hardcoded r.is_combat).
  • crates/engine/src/parser/oracle_effect/mod.rs — the token-for-each arm's new raw-then-subject-stripped ordering mirrors the pre-existing numeric-for-each arm directly above it.

Scope Expansion

The enum parameterization alone did not flip You've Been Caught Stealing: the token "for each" arm (try_parse_for_each_effecttry_parse_token, oracle_effect/mod.rs) fed the raw subject-bearing base "You create a Treasure token" to try_parse_token, which returned None, stranding the correctly-parsed for-each quantity (this also swallowed the combat token form). Fixed by having the token arm try the raw base first, then fall back to a subject-stripped base — mirroring the numeric arm directly above. Verified additive: swept card-data.json for every create … token for each … card; multi-player subjects (each player/opponent creates …) are handled by player_scope upstream and never reach this fallback, distributive its controller creates … routes elsewhere, and the only newly-captured forms are singular you create … (owner = controller, correct). No card changes owner/scope.

Validation Failures

None.

CI Failures

None.

…nesplitter, You've Been Caught Stealing)

"for each opponent who was dealt damage this turn" (any damage — combat OR
noncombat) was swallowed, defaulting the quantity to Fixed 1. Only the combat
form ("dealt combat damage this turn", Tymna the Weaver) was supported.

Parameterize the existing PlayerFilter variant rather than adding a sibling or a
bool (CLAUDE.md "parameterize, don't proliferate"):

- Rename PlayerFilter::OpponentDealtCombatDamage { source } ->
  OpponentDealtDamage { kind: DamageKindFilter, source }, where DamageKindFilter
  is the pre-existing { Any, CombatOnly, NoncombatOnly } axis already used by the
  DamageDealtThisTurn resolver. serde alias + a CombatOnly default preserve
  legacy data (a bare OpponentDealtCombatDamage means combat-only, not Any).
- Parser: "dealt {combat|noncombat|} damage this turn" -> the matching kind via a
  nom alt; "dealt combat damage" still maps to CombatOnly (Tymna unchanged).
- Resolver: delegate the is_combat decision to the existing
  damage_record_matches_kind; noncombat player damage is already recorded in
  state.damage_dealt_this_turn (is_combat = false), so no new tracking.

Also fixes the token "for each" arm (oracle_effect): "You create a token for each
..." did not strip the "you" subject before try_parse_token, stranding the
for-each quantity (would have swallowed the combat form too). The token arm now
tries the raw base first, then a subject-stripped fallback, mirroring the numeric
arm. Additive — verified against card-data that no other create-token-for-each
card changes owner/scope.

Flips Furious Spinesplitter and You've Been Caught Stealing to supported;
Tymna the Weaver re-emits kind: CombatOnly, unchanged.

CR 120.1 + CR 120.2a + CR 120.2b + CR 510.1 + CR 120.9.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 5, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors PlayerFilter::OpponentDealtCombatDamage to PlayerFilter::OpponentDealtDamage to support filtering by combat, noncombat, or any damage in compliance with CR 120.2a/120.2b. It updates the parser, quantity resolver, replacement effects, and triggers, and adds comprehensive tests for cards like 'Furious Spinesplitter' and 'You've Been Caught Stealing'. A critical compilation issue was identified in crates/engine/src/game/effects/deal_damage.rs where kind is passed as a reference instead of being dereferenced to a value.

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.

state,
p.id,
source_controller,
kind,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

[HIGH] Type mismatch in opponent_dealt_damage_matches call. Evidence: crates/engine/src/game/effects/deal_damage.rs:1670.\n\nWhy it matters: Passing a reference &DamageKindFilter where a value DamageKindFilter is expected will cause a compilation error.\n\nSuggested fix: Dereference kind as *kind.

Suggested change
kind,
*kind,
References
  1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. (link)

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Current head does not compile in the player collection path. Evidence: crates/engine/src/game/effects/deal_damage.rs:1670 passes kind from PlayerFilter::OpponentDealtDamage { kind, ref source } into opponent_dealt_damage_matches, while that helper takes DamageKindFilter by value at crates/engine/src/game/quantity.rs:4525; the sibling resolver path already calls it with *kind. Why it matters: this blocks the Rust lint/build job before the behavior can be accepted. Suggested fix: pass *kind at the deal_damage.rs:1670 call site.

Also holding approval until the current-head parse-diff sticky is available; at review time card-data/coverage was still running and the required <!-- coverage-parse-diff --> evidence was absent.

@matthewevans matthewevans dismissed their stale review July 5, 2026 14:51

Dismissed by maintainer automation: the compile finding was incorrect because this match arm owns PlayerFilter, so kind is a DamageKindFilter value, not a borrowed value.

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 6 card(s), 8 signature(s) (baseline: main b820a65a2d0e)

1 card(s) · ability/PutCounter · field counter: 1 P1P1PlayerCount { filter: OpponentDealtDamage { kind: Any, source: None } } P1P1

Examples: Furious Spinesplitter

1 card(s) · ability/PutCounter · field duration: until end of turn

Examples: Furious Spinesplitter

1 card(s) · ability/Token · field duration: until end of turn

Examples: You've Been Caught Stealing

1 card(s) · ability/Token · field token: +1/+1 White Soldier (Creature Soldier) tappedquest counters on self× +1/+1 White Soldier (Creature Soldier) tapped

Examples: Yes Man, Personal Securitron

1 card(s) · ability/Token · field token: Powerstone (Artifact Powerstone) tapped# of in graveyard zone changed this turn from Battlefield to Graveyard scoped player controls card non-land× Powerstone…

Examples: Wreck Hunter

1 card(s) · ability/Token · field token: Treasure (Artifact Treasure)# of each opponent who was dealt damage this turn× Treasure (Artifact Treasure)

Examples: You've Been Caught Stealing

1 card(s) · ability/Token · field token: Treasure (Artifact Treasure)# of target player controls artifact× Treasure (Artifact Treasure)

Examples: Cavern-Hoard Dragon

1 card(s) · ability/Token · field token: Treasure (Artifact Treasure) tapped# of opponent controls creature× Treasure (Artifact Treasure) tapped

Examples: Covetous Elegy

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@matthewevans matthewevans self-assigned this Jul 5, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I re-checked the current head after the parse-diff landed: the damage-kind parameterization is at the right quantity/player-filter seam, the runtime tests discriminate Any damage from both Fixed 1 and CombatOnly, and the parse-diff matches the intended token/counter quantity changes without an unexplained scope loss.

@matthewevans matthewevans added the enhancement New feature or request label Jul 5, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 5, 2026
@matthewevans matthewevans removed their assignment Jul 5, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to pull back the approval after an adversarial pass found a real parser-scope risk in the token-for-each fallback.

[MED] Subject-bearing token-for-each clauses can be accepted with the wrong token creator. Evidence: crates/engine/src/parser/oracle_effect/mod.rs now falls back from raw token parsing to subject::strip_subject_clause(base_no_duration) before calling token::try_parse_token. try_parse_token defaults Effect::Token.owner to TargetFilter::Controller, and the early return from try_parse_for_each_effect only reapplies optional/condition slots before returning. It does not run the later subject-target/recipient rebinding machinery that normally lifts a subject like each player or target opponent into the token owner.

Why this matters: the fallback needed for You create a Treasure token for each ... is good, but as written it can also make broader subject forms parse as supported while creating the tokens for the source controller. Please either limit this fallback to the you create ... form needed here, or route the parsed token through the existing subject/player-scope rebinding path and add a regression for a non-controller token creator.

@matthewevans matthewevans removed this pull request from the merge queue due to a manual request Jul 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants