fix(parser): self-scoped damage shields prevent only damage dealt TO the source (Fixes #5652)

[davion-knight]

Summary

A damage-replacement shield whose antecedent is self-scoped — "If [a source would deal] damage to ~, prevent / …instead" — parsed with no recipient scope (valid_card: None, damage_target_filter: None). Per CR 614.1a a None recipient scope applies no restriction, so at runtime the shield also replaced damage the source itself deals:

• Swans of Bryn Argoll — its combat damage to a blocker was prevented (blocker took 0) and it drew off its own damage.
• Phytohydra / Lichenthrope — grew / shrank on damage they dealt.

Fix — reuse valid_card: SelfRef, no new variant

The engine already expresses "damage dealt to this object" as valid_card: Some(TargetFilter::SelfRef) — applied against ProposedEvent::Damage's recipient via affected_object_id() (replacement.rs). So the correct scope already exists; the two self-scoped parser seams simply weren't setting it (per the add-engine-variant gate, an existing mechanism beats a new DamageTargetFilter sibling):

1. parse_damage_to_self_instead_followup — the ~ (self) and you (player) branches resolved identically. Thread which matched and set valid_card: SelfRef for the self case only → Phytohydra, Lichenthrope. The to you player shield (Nefarious Lich) is deliberately left untouched.
2. parse_damage_prevention_replacement — its self-scope scan matched passive dealt to ~ but not the active-voice deal damage to ~ form → Swans. Add that form.

Blast radius

Narrowly ~-anchored, so genuinely-unrestricted replacements ("to a permanent or player" / "any target") are unaffected, and passive Phantom/en-Kor self-shields (already SelfRef via the existing scan) are unchanged. Verified controls: the to you shield stays None; Phantom Nishoba stays SelfRef. This is a 32-card class (Swans, the Phantom/en-Kor/Hydra family, Phytohydra, Lichenthrope, …); the previously-broken members are the instead-followup and active-voice-prevention forms.

Tests

• Parser: self forms bind valid_card: SelfRef; the to you form stays None.
• Runtime (combat): Swans blocks — the attacker's damage to Swans is still prevented, but Swans' combat damage to the attacker now lands (damage_marked == 2; on main it was 0).
• Full engine lib suite green: 16232 passed, 0 failed (incl. the existing self-scoped damage_target_filter guard tests — untouched, since this sets valid_card).

Fixes #5652

[gemini-code-assist]

Code Review

This pull request fixes a bug (#5652) where self-scoped damage shields (such as Swans of Bryn Argoll) incorrectly prevented damage dealt by the source itself instead of only damage dealt to it. It updates the parser to properly identify self-scoped shields and apply TargetFilter::SelfRef, and adds corresponding unit tests. The review feedback points out that using scan_contains to match 'deal damage to ~' violates the repository's strict rule against substring scanning for parser dispatch, recommending a refactor to use modular nom-based combinators instead.

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.

[gemini-code-assist]

Avoid substring scanning (such as scan_contains or .contains()) for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

References

1. Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

The fix is correct and lands at the right seam — blocked only on the bug-pin it was designed to flip, plus two misapplied CR citations. Fix the pin and the annotations and I'll approve on green.

✅ Clean — and one thing you should NOT "fix"

The seam is exactly right. alt((tag("~"), tag("you"))) was discarding which alternative matched, collapsing a self-scoped shield and a player-scoped shield into the same ReplacementDefinition. Capturing it with value(true, tag("~")) / value(false, tag("you")) is the idiomatic nom repair, and gating valid_card(TargetFilter::SelfRef) on it fixes the whole class (Swans, Phytohydra, Lichenthrope), not one card.

Your test is genuinely discriminating. swans_self_shield_does_not_prevent_damage_it_deals carries both a discriminator (Swans' own 2 damage must land on the attacker) and a reach-guard (damage to Swans must still be prevented). Reverting the fix breaks the first; over-scoping breaks the second. That is the right shape.

⚠️ The parse-diff sticky says "✓ No card-parse changes detected." Ignore it — it is not evidence your fix is inert. I verified this rather than taking it at face value: the coverage projection is structurally blind to the field you changed. In crates/engine/src/game/coverage.rs:4327-4353, every ParseCategory::Replacement item is built with details: vec![] hardcoded empty, and repl.valid_card is never projected — while triggers project valid_card (coverage.rs:3597) and statics get static_details(stat). A change to ReplacementDefinition.valid_card cannot appear in the parse-diff signature.

The non-vacuity proof is the CI failure itself, which is worth far more than the sticky. Do not "fix" the zero sticky. That is our tooling gap, not yours — I'm filing it separately.

🔴 Blocker 1 — update the bug-pin your fix is supposed to flip

crates/engine/tests/integration/draw_from_general_post_replacement.rs:221 is failing, and that failure is the strongest evidence on this PR that your fix works. The pin predates your branch, so it cannot have been shaped to fit it, and it says so itself:

// CORRECT: hand_drawn(P0) == 0 and blocker damage_marked == 4.
// These two assertions encode today's wrong values on purpose... Fixing the scoping SHOULD break them.

You changed the behavior it pins, so updating it is part of the fix. Flip both, with the values the pin already states:

• outcome.hand_drawn(P0): 4 → 0
• runner.state().objects[&blocker].damage_marked: 0 → 4

Retire the BUG-PIN (#5652) wording and the test's KNOWN BUG section header at the same time — with the bug fixed, leaving the pin in place would mislead the next reader into thinking the bug is live.

🔴 Blocker 2 — CR 614.1a and CR 615.1a are real rules, but neither governs this

Both numbers exist, so a grep-check passes. Reading them is what fails:

• CR 614.1a — "Effects that use the word 'instead' are replacement effects. Most replacement effects use the word 'instead' to indicate what events will be replaced with other events."
• CR 615.1a — "Effects that use the word 'prevent' are prevention effects. Prevention effects use 'prevent' to indicate what damage will not be dealt."

Both are purely definitional — they classify an effect by its wording. Neither says anything about whom a shield protects, which is the entire claim you attach them to ("a self-scoped damage shield must fire only for damage dealt TO its own object").

The rule that actually governs recipient scope is CR 615.1: prevention effects "watch for a damage event that would happen and completely or partially prevent the damage that would be dealt. They act like 'shields' around whatever they're affecting." — "around whatever they're affecting" is precisely the scoping concept. CR 609.7 / CR 615.2 ("Many prevention effects apply to damage from a source") is the companion for source-side scoping.

Please re-cite in oracle_replacement.rs and replacement.rs. A verified-but-misapplied CR number is worse than none: it reads as though the rule was checked.

🟡 Non-blocking — is_self: bool

CLAUDE.md's no-raw-bool rule targets fields and parameters that distinguish cases in the data model, and you do land on a typed Option<TargetFilter>, so this is not a blocker. But the combinator could return the typed value directly — alt((value(Some(TargetFilter::SelfRef), tag("~")), value(None, tag("you")))) — and drop the bool hop entirely. Your call.

Recommendation

request-changes. Flip the bug-pin (values are in the pin), re-cite to CR 615.1 / CR 609.7. Both are quick. The implementation itself I would take as-is — I'll approve on green.

[matthewevans]

Correction — you beat me to Blocker 1. Ignore it.

You pushed e957df74 at 12:00:33Z; my review posted at 12:02:35Z. I reviewed the previous head (108d4c16) and the review landed on yours, so it asks you to do work you had already done two minutes earlier. My timing, not your oversight.

Blocker 1 (bug-pin) — RESOLVED on the current head. Verified:

• outcome.hand_drawn(P0): 4 → 0 ✓
• blocker.damage_marked: 0 → 4 ✓
• BUG-PIN wording retired and the # This test pins a KNOWN BUG doc section rewritten to # Self-scope (issue #5652, FIXED) ✓

Both values match what the pin itself named as correct, and you replaced the pin's prose instead of just flipping integers under a comment that still said "KNOWN BUG" — which is the part that actually matters for the next reader.

Blocker 2 (CR citations) — still stands. CR 614.1a and CR 615.1a are still on the current head. They are real rules, which is exactly why they slip a grep-check, but neither governs recipient scope:

• CR 614.1a — "Effects that use the word 'instead' are replacement effects."
• CR 615.1a — "Effects that use the word 'prevent' are prevention effects."

Both are definitional: they classify an effect by its wording. Neither says anything about whom a shield protects, which is the whole claim they are attached to.

Re-cite to CR 615.1 — "prevention effects ... act like 'shields' around whatever they're affecting" (that clause is the scoping concept) — with CR 609.7 / CR 615.2 as the companion for source-side scoping.

That is the only thing left. Fix the citations and I'll approve on green.

[davion-knight]

Thanks — all addressed in bd332f9.

Blocker 1 — bug-pin. Flipped in e957df7 (before this review): hand_drawn(P0) 4→0, blocker.damage_marked 0→4, and retired the KNOWN BUG header / BUG-PIN wording so the next reader doesn't think the bug is live.

Blocker 2 — CR citations. You're right; both were definitional and I re-verified against the rules text. Re-cited to the actual scoping rules, matched to each seam's effect type (both carry the identical "shields around whatever they're affecting" language):

• parse_damage_to_self_instead_followup (the "X instead" replacement seam → Phytohydra/Lichenthrope) → CR 614.1.
• parse_damage_prevention_replacement (the active-voice prevention seam → Swans) → CR 615.1.
• Both test doc-comments updated to match.

(My cards are recipient-scoped — the shield is on the object taking damage — so CR 609.7 / 615.2, the source-side companions, don't apply here; I've left those out rather than cite them loosely.)

Non-blocking — is_self: bool. Took your suggestion — the ~/you alternative now yields the typed recipient scope directly and feeds valid_card, dropping the bool hop:

let (i, recipient_scope) = alt((
    value(Some(TargetFilter::SelfRef), tag("~")),
    value(Option::<TargetFilter>::None, tag("you")),
)).parse(i)?;
...
if let Some(scope) = recipient_scope { def = def.valid_card(scope); }

On the zero sticky — understood, thanks for digging into coverage.rs:4327-4353; I won't touch it. Agreed the CI failure (the bug-pin) is the real non-vacuity proof.

Parser gate green; oracle_replacement (344) and game::replacement (195) module tests green; both new tests still pass. On green this should be ready.

[matthewevans]

Approving — the CR citations now land on the rules that actually govern recipient scope, and the typed Option<TargetFilter> is a better fix than the one I asked for.

✅ Clean

The CR re-cite is correct, and for the right reason. CR 614.1 and CR 615.1 both read, verbatim: "They act like 'shields' around whatever they're affecting." "Whatever they're affecting" is the recipient — that is exactly the clause that governs whom a shield protects. The subparts I flagged (614.1a / 615.1a) are purely definitional ("effects that use the word 'instead'/'prevent' are…") and say nothing about scope. You also routed each form to the right rule: the instead form (parse_damage_to_self_instead_followup) to 614.1 (replacement), the active-voice prevent form (parse_damage_prevention_replacement) to 615.1 (prevention). Both verified against docs/MagicCompRules.txt.

The bool → Option<TargetFilter> change is unprompted and is the better design. I asked only for a citation fix; you returned the recipient scope as a typed value from the combinator (value(Some(TargetFilter::SelfRef), tag("~")) / value(None, tag("you"))) instead of an is_self: bool the caller re-interprets. That is CLAUDE.md's rule directly — "never a raw bool… a boolean isn't composable; an existing type is self-documenting" — and it collapses the branch at the call site to if let Some(scope) = recipient_scope. The parse and the meaning now travel together.

The bug-pin flip is the load-bearing evidence. swans_prevented_damage_draws_that_many_for_the_sources_controller was written by an earlier, unrelated effort (Plan 03), and it prophesied this PR: "When the scoping bug is fixed, this test SHOULD go red; flip the two marked assertions then." It went red, and you flipped exactly those two (hand_drawn(P0) 4→0, blocker damage_marked 0→4). A guard planted before your branch existed cannot have been shaped to fit it — that is the strongest evidence available on a PR, and it matters here more than usual, because the parse-diff sticky is structurally blind to this class of fix: game/coverage.rs builds every ParseCategory::Replacement item with details: vec![] and never projects repl.valid_card, so a shield that changes only whom it protects serializes byte-identically and reports "✓ No card-parse changes detected." Filed as #5673 — not your problem, but it is why I did not treat that sticky as evidence either way.

Parser-combinator mandate verified by hand as well (the CI gate cannot see matches! alternation, .rfind(, .split(, or char-literal trim_*_matches): clean — the fix composes alt + value + take_until, no verbatim Oracle-text dispatch.

CI green on bd332f9ac7 — 13 checks, 0 failures.

Approving and enqueuing as bug (parser misparse: a self-scoped shield was silently prevention-scoping the damage the object deals). No quality label — that one is reserved for a one-shot landing with no rework round, and this took a round; the work itself is well above the bar.