fix(parser, engine): recognize qualified "source of your choice" damage prevention (Circle/Rune of Protection, 13 cards)#5488
Conversation
…ge prevention (Circle/Rune of Protection, 13 cards)
CR 609.7 + CR 609.7a + CR 609.7b: "the next time a <color/type> source of
your choice would deal damage to you this turn, prevent that damage" was
mishandled — parse_oneshot_source_filter only recognized the bare,
unqualified "a source of your choice" form. A qualified form like "a blue
source of your choice" fell through to None entirely, meaning these cards
currently create a fully unconstrained "prevent next damage this turn from
ANY source" shield with no interactive choice ever prompted (strictly
worse than doing nothing). A second, independent gap: prevent_damage.rs's
resolver (which these cards actually route through, not
create_damage_replacement.rs) had zero "prompt if not yet chosen" logic.
Fix: parameterize TargetFilter::ChosenDamageSource from a unit variant
into ChosenDamageSource { filter: Option<Box<TargetFilter>> }, following
the direct precedent of PlayerFilter::OpponentDealtDamage's
Option<Box<TargetFilter>> field and its scanner-recursion in
ability_scan.rs. Add parse_qualified_chosen_damage_source, reusing
parse_type_phrase for the qualifier. Thread the qualifier through both
resolvers' self-prompt blocks via a single prompt_filter binding (feeds
both the candidate-enumeration call and WaitingFor::DamageSourceChoice,
so the interactive prompt itself is qualifier-filtered, not just the
CR 609.7b post-choice recheck). Add the missing self-prompt block to
prevent_damage.rs, mirroring create_damage_replacement.rs's existing one.
Covers the full 13-card class: Circle of Protection (Artifacts, Black,
Blue, Green, Red, White) and Rune of Protection (Artifacts, Black, Blue,
Green, Lands, Red, White) — Circle of Protection: Shadow excluded
(unrelated ChooseAPermanent+Shadow mechanic). Rune of Protection: Lands
exercises the type-qualifier branch distinctly from the color-qualified
cards.
Also updates 3 mtgish-import match sites (replacement.rs, condition.rs)
for the ChosenDamageSource shape change to keep `cargo check --workspace`
green — mechanical compile-compat only, no new converter functionality.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM
…klog All 13 fixed by 6156c152f. Circle of Protection: Shadow (unrelated ChooseAPermanent+Shadow mechanic) and Circle of Protection: Art (a pre-existing, unrelated stray entry) are retained. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM
![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 implements support for qualified damage source choices (e.g., "a red source of your choice" or "a land source of your choice" as seen in the Circle of Protection and Rune of Protection cycles) by parameterizing the TargetFilter::ChosenDamageSource variant with an optional inner filter. It updates the parser to extract these qualifiers, modifies the damage prevention and replacement resolvers to prompt players with the restricted candidate set, and re-evaluates the source's qualifiers live at damage time. A review comment suggests simplifying redundant clone branches and duplicate .clone() calls in the prevent_damage.rs resolver to improve readability.
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.
| let effect_source_filter = match &effect_source_filter { | ||
| Some(TargetFilter::ChosenDamageSource { filter: qualifier }) => { | ||
| if state.last_chosen_damage_source.is_none() { | ||
| let prompt_filter = qualifier.as_deref().cloned().unwrap_or(TargetFilter::Any); | ||
| let options = | ||
| choose_damage_source::damage_source_options(state, ability, &prompt_filter); | ||
| if !options.is_empty() { | ||
| state.pending_continuation = | ||
| Some(PendingContinuation::new(Box::new(ability.clone()))); | ||
| state.waiting_for = WaitingFor::DamageSourceChoice { | ||
| player: ability.controller, | ||
| source_filter: prompt_filter, | ||
| options, | ||
| }; | ||
| events.push(GameEvent::EffectResolved { | ||
| kind: EffectKind::PreventDamage, | ||
| source_id: ability.source_id, | ||
| }); | ||
| return Ok(()); | ||
| } | ||
| // CR 609.7a: no legal candidate — falls through with the record | ||
| // still absent; the post-choice logic below then resolves | ||
| // `resolve_source_filter`'s ChosenDamageSource arm against an empty | ||
| // `last_chosen_damage_source`, producing a `TargetFilter::None` | ||
| // shield that matches nothing (this activation does nothing). | ||
| effect_source_filter.clone() | ||
| } else { | ||
| effect_source_filter.clone() | ||
| } | ||
| } | ||
| other => other.clone(), | ||
| }; |
There was a problem hiding this comment.
[MEDIUM] Simplify redundant clone branches in effect_source_filter match. Evidence: crates/engine/src/game/effects/prevent_damage.rs:298-329.
Why it matters: Removing the redundant else block and duplicate .clone() calls improves code readability and maintainability.
Suggested fix: Consolidate the branches to fall through to a single clone call.
let effect_source_filter = match &effect_source_filter {
Some(TargetFilter::ChosenDamageSource { filter: qualifier }) => {
if state.last_chosen_damage_source.is_none() {
let prompt_filter = qualifier.as_deref().cloned().unwrap_or(TargetFilter::Any);
let options =
choose_damage_source::damage_source_options(state, ability, &prompt_filter);
if !options.is_empty() {
state.pending_continuation =
Some(PendingContinuation::new(Box::new(ability.clone())));
state.waiting_for = WaitingFor::DamageSourceChoice {
player: ability.controller,
source_filter: prompt_filter,
options,
};
events.push(GameEvent::EffectResolved {
kind: EffectKind::PreventDamage,
source_id: ability.source_id,
});
return Ok(());
}
// CR 609.7a: no legal candidate — falls through with the record
// still absent; the post-choice logic below then resolves
// `resolve_source_filter`'s ChosenDamageSource arm against an empty
// `last_chosen_damage_source`, producing a `TargetFilter::None`
// shield that matches nothing (this activation does nothing).
}
effect_source_filter.clone()
}
other => other.clone(),
};
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved
This is strong work, and it lands the mechanic properly rather than patching the symptom.
I verified the qualifier is genuinely threaded end to end, not merely stored. prompt_filter drives both the candidate enumeration and WaitingFor::DamageSourceChoice.source_filter; that gets recorded onto state.last_chosen_damage_source.source_filter, and resolve_source_filter's other arm then builds And { identity, recheck }, which is what actually satisfies the CR 609.7b live property recheck. The sentinel is correctly transient: set, then cleared once drain_pending_continuation runs.
The no-legal-source path is fail-closed. resolve_source_filter(..).unwrap_or(TargetFilter::None) yields a shield that matches nothing, and circle_of_protection_red_no_legal_source_is_noop proves it end to end by putting only a blue source on the battlefield and asserting life stays at 17 after 3 damage.
The tests discriminate, which is the part I care most about. circle_of_protection_red_prompt_options_are_color_filtered seeds both a red and a blue source, so the negative assertion cannot pass vacuously, and it carries a positive reach-guard alongside it. bare_source_of_your_choice_prompt_offers_all_colors is the sibling that proves the two paths actually differ rather than both collapsing to the same behavior. Asserting on WaitingFor::DamageSourceChoice.options directly is the right anchor for this.
Using a typed Option<Box<TargetFilter>> following the PlayerFilter::OpponentDealtDamage precedent, instead of a bool, is exactly the call this repo wants, and ability_scan.rs correctly recurses into the nested filter rather than stopping at a mechanical { .. }.
CR 609.7 quoting Circle of Protection: Red's Oracle text verbatim as its own worked example is a fair characterization; I confirmed it at line 2861 of the rules text.
About the coverage-parse-diff sticky — please read this one
The coverage-parse-diff sticky on this PR says "No card-parse changes detected." On a parser PR that is normally a high-confidence signal that the fix is inert, meaning the parse output did not actually change and the change accomplishes nothing at runtime. I want to be explicit that this is not the case here. I investigated it and it is a blind spot in the sticky's renderer.
coverage.rs builds the PreventDamage effect signature from amount, target and scope only:
Effect::PreventDamage { amount, target, scope, .. } => {
d.push(("amount".into(), format!("{amount:?}")));
d.push(("target".into(), fmt_target(target)));
d.push(("scope".into(), format!("{scope:?}")));
}damage_source_filter is swallowed by the ... So a field going from None to a qualified ChosenDamageSource { filter: Some(..) } produces a byte-identical signature and is invisible to the parse diff. The effect genuinely changed; the renderer simply cannot see the field that changed.
I will file a separate tooling issue for the missing field. Thank you for adding the parse_oracle_text integration test, because that test driving real Oracle text is the only reason this was provable either way. Without it I would have had no evidence to distinguish "renderer blind spot" from "inert fix," and the sticky would have been the deciding signal.
Non-blocking follow-ups
None of these block the merge. Recording them so they are not lost.
1. (Non-blocking) create_damage_replacement.rs empty-options branch attaches no source filter.
When options.is_empty(), that resolver falls through with resolved_source_filter = None, and the shield builder is guarded:
if let Some(filter) = resolved_source_filter {
shield = shield.damage_source_filter(filter);
}With None there is no source filter on the shield, so it replaces damage from any source. Its own comment claims "the replacement does nothing," which is the opposite of what happens. Your prevent_damage.rs handles the identical case correctly by falling through with the ChosenDamageSource filter intact, which then resolves to TargetFilter::None and matches nothing.
I confirmed against main that both the comment and the None fall-through pre-date this PR, and that your change narrows the exposure rather than introducing it, since previously the qualified form never produced a choice at all. It is not a regression and it does not block. The reason I raise it now is that parse_damage_modification_static feeds one source_filter binding into both Effect::CreateDamageReplacement and Effect::PreventDamage, so the empty-source corner becomes reachable with a qualifier attached. Would you prefer to fix it here, mirroring what you already wrote in prevent_damage.rs, or in a follow-up? Either is fine by me.
2. (Non-blocking) The self-prompt block now lives in two resolvers, and they already disagree.
Item 1 is concrete evidence that the duplicated block has drifted on the empty-options path. That is the usual argument for extracting a single prompt_for_damage_source(..) authority. Two flows sits under this repo's "3+ = generalize" bar, so I am recording this as a recommendation rather than a request.
3. (Non-blocking) One CR annotation is misapplied, and one PR-body citation is off.
The CR 615.3 comment in the new prevent_damage.rs test justifies hosting the shield on the source object's replacement_definitions. CR 615.3 reads:
There are no special restrictions on casting a spell or activating an ability that generates a prevention effect. Such effects last until they're used up or their duration has expired.
It says nothing about where a shield is stored. Its second sentence is a good anchor for one-shot consumption, together with CR 609.7b's "if the shield prevents no damage, the shield isn't used up." The hosting claim needs a different citation or none.
Separately, in the PR body only, CR 614.5 is described as "one-shot replacement consumption." The actual text is that a replacement effect does not invoke itself repeatedly and gets only one opportunity to affect an event. Related, though not the same claim. No code change needed for that one.
4. (Non-blocking) No test covers the an <type> article shape.
The new integration test covers Circle of Protection: Red (color qualifier) and Rune of Protection: Lands (type qualifier, a land source). Nothing exercises an artifact source, which is what Circle of Protection: Artifacts and Rune of Protection: Artifacts use. parse_article = alt((tag("an "), tag("a "))) handles it, so I read this as a coverage gap rather than a bug. A third case in the existing test would close it.
One correction in your favor
Your root-cause narrative says the qualified form "returned None entirely" on main. I went in expecting to flag this as an overstatement, on the theory that parse_oneshot_source_filter falls through to parse_damage_source_filter(body), which might return Some(Typed(red)) and leave main over-preventing from any red source rather than being fully unconstrained.
I traced it and you are right. On main, for "a red source of your choice", split_damage_source_noun yields qualifier = "red" and tail = "of your choice". That tail is non-empty and matches neither "you control" nor "of the chosen type", so parse_damage_source_subject bails to None. parse_type_phrase then leaves "source of your choice" as a non-empty remainder, so that path returns None too. parse_oneshot_source_filter really does return None, the shield gets no damage_source_filter, and it replaces damage from any source with no choice ever prompted. Your description of the bug is accurate as written.
Approving. Nice work.
…-diff signature (phase-rs#5493) `effect_details` built the `PreventDamage` signature from a hand-picked field list that dropped `damage_source_filter` (swallowed by `..`), so any parser change to it was invisible to the `coverage-parse-diff` sticky — the required review evidence for parser-surface PRs. That produced a false `No card-parse changes detected` on phase-rs#5488, whose 13 Circle/Rune of Protection cards genuinely moved from an unqualified `ChosenDamageSource` to `ChosenDamageSource { filter: Some(..) }`. Emit `damage_source_filter` in the signature (via `fmt_target`, which already renders the qualifier). Per the issue's "audit the renderers" note, also emit `CreateDamageReplacement`'s `source_filter` (fed by the same `parse_oneshot_source_filter` binding) and its `target_filter`, which had the same blind spot. Test: `prevent_damage_signature_exposes_damage_source_filter` asserts the field appears when set and is omitted when `None` — reverting the fix fails it. Fixes phase-rs#5492.
2 participants
Summary
Fixes a real 13-card class where "the next time a
<color/type>source of your choice would deal damage to you this turn, prevent that damage" (Circle of Protection cycle + Rune of Protection cycle) was mishandled by the parser and resolver.Affected cards (confirmed via Scryfall for all 13, verbatim grammar identical modulo cost and the color/type qualifier):
Circle of Protection: Shadow is explicitly excluded — it uses an unrelated
ChooseAPermanent+ Shadow-matching mechanic, notChooseADamageSource.CR 609.7 (
docs/MagicCompRules.txt) literally quotes Circle of Protection: Red's Oracle text as its own worked example — this is a foundational rules mechanic, not a narrow card-specific fix.Root cause
Two independent, compounding gaps:
parse_oneshot_source_filter(oracle_replacement.rs) only recognized a bare, unqualified"a source of your choice"/"that source". A qualified form like"a blue source of your choice"fell through the whole chain and returnedNoneentirely. Net effect: these 13 cards currently create a fully unconstrained "prevent the next damage this turn from any source" shield, with no interactive choice ever prompted — strictly worse than doing nothing, since it silently over-delivers value with zero player agency.prevent_damage.rs::resolve(the resolver these cards actually route through, notcreate_damage_replacement.rs) had zero "prompt if not yet chosen" logic. Its siblingcreate_damage_replacement.rs::resolvealready had this exact self-prompt pattern for the bare form (hardcoded toTargetFilter::Any) —prevent_damage.rsnever got the equivalent.Fix
TargetFilter::ChosenDamageSourcefrom a bare unit variant intoChosenDamageSource { filter: Option<Box<TargetFilter>> }, following the direct existing precedent ofPlayerFilter::OpponentDealtDamage { source: Option<Box<TargetFilter>> }(types/ability.rs:5751-5757) and its scanner-recursion pattern (ability_scan.rs:3039-3049) — not a new pattern.parse_qualified_chosen_damage_source(oracle_replacement.rs), reusingparse_type_phrasefor the qualifier — the same building blockparse_damage_source_subject_filteralready composes elsewhere in this file for non-anaphoric forms.prompt_filterbinding, consumed by both the candidate-enumeration call andWaitingFor::DamageSourceChoice.source_filter— so the interactive prompt itself is qualifier-filtered (e.g. Circle of Protection: Red only ever offers red sources as choices), not just the CR 609.7b post-choice recheck. A dedicated test (circle_of_protection_red_prompt_options_are_color_filtered) assertsWaitingFor::DamageSourceChoice.optionsdirectly, specifically to catch a regression where only the recheck stays qualifier-aware but the prompt itself silently reverts toAny.prevent_damage.rs, mirroringcreate_damage_replacement.rs's existing one.ability_rw.rs,ability_scan.rs,cost_payability.rs,coverage.rs,filter.rs,trigger_matchers.rs,oracle_effect/lower.rs,oracle_static/restriction.rs) to compile against the new struct-variant shape.ability_scan.rs's scanner arm is the one site needing real compositional logic (recursing into the nested filter forAxescomputation), not just a mechanical{ .. }— it mirrors theOpponentDealtDamageprecedent exactly.crates/mtgish-import(a separate workspace member) pattern-matched the old unit-variant shape in 3 places (convert/replacement.rs,convert/condition.rs), which would have brokencargo check --workspace. Updated mechanically ({ filter: None }/{ .. }) — no new converter functionality, purely keeping it compiling against a type it already references.No
ControllerRef/boolshortcuts anywhere;Option<Box<TargetFilter>>is the established idiom for this exact "optional nested filter on a fixed-shape variant" pattern.Gate A — parser combinator compliance
Gate B — anchored citations (≥2)
crates/engine/src/types/ability.rs:5751-5757—PlayerFilter::OpponentDealtDamage { source: Option<Box<TargetFilter>> }, the direct precedent forChosenDamageSource's new field shape.crates/engine/src/game/ability_scan.rs:3039-3049— that precedent's scanner-recursion pattern, mirrored exactly forChosenDamageSource's ownscan_target_filterarm.crates/engine/src/game/effects/create_damage_replacement.rs(existing self-prompt block, lines ~79-131 pre-diff) — the pattern mirrored intoprevent_damage.rs's new block.CR references
replacement_definitions, not a global pending-registry — this distinction surfaced and was correctly resolved during a test-authoring fix mid-implementation (see Validation below).Tier
Tier: Frontier
Justification: this is a 13-card foundational-mechanic fix that parameterizes a shared
TargetFilterenum variant across ~24 production match sites in the engine plus 3 sites in a separate workspace crate, adds a new resolver code path (prevent_damage.rspreviously had no self-prompt logic at all), and went through 3 rounds of plan review and 3 rounds of implementation review before landing — substantially beyond a single-arm parser fix.Model: claude-sonnet-5
Thinking: high
Validation performed
cargo fmt --all— clean (pre- and post-rebase)./scripts/check-parser-combinators.sh— exit 0docs/MagicCompRules.txt, zeroUNVERIFIEDcargo clippy -p engine -p mtgish-import --all-targets -- -D warnings— clean, zero warnings (confirmed pre-rebase)cargo check --workspace --all-targets— clean, confirms every workspace crate (engine, mtgish-import, engine-wasm, phase-ai, phase-server, draft-core, draft-wasm, lobby-broker, seat-reducer, manabrew-compat, feed-scraper, engine-inventory-gen) compiles against the newChosenDamageSourceshapecargo test -p engine --lib— 15962 passed, 0 failed, 6 ignored (confirmed pre-rebase)cargo test -p engine --test integration— 2563 passed, 0 failed, 2 ignored (confirmed pre-rebase) — includes a dedicated new test file covering both the color-qualifier branch (Circle of Protection: Red) and the type-qualifier branch (Rune of Protection: Lands, which also confirms its independentCyclingline decomposes correctly and doesn't interact with the prevention clause)/review-impl— 3 rounds. Round 1 found 2 issues (a test asserting the wrong shield-storage location due to a genuine CR 615.3 nuance — permanent-sourced shields host on the object, not a global registry — and a broken integration-test assertion expectingCyclingto appear as a secondabilityrather than inextracted_keywords), both fixed. Round 2 (a deliberate holistic fresh-eyes pass) found themtgish-importcross-crate compile break described above, fixed and independently re-verified via directcargo check --workspace. Round 3 confirmed clean end-to-end.Validation NOT performed (disclosed honestly)
Post-rebase, this branch could not get a post-rebase re-confirmation of
cargo clippyorcargo test -p engine --lib— 5 consecutive attempts of each were killed mid-compile at the identical "Compiling engine v0.20.0" stage, with the system independently confirmed idle (no resource contention) each time via direct process inspection. This matches an unrelated, intermittent background-task environment issue observed repeatedly elsewhere in this session (resolved by simple retries in most other cases; this branch happened to hit the retry ceiling on both commands). The rebase itself introduced zero code conflicts — onlydocs/parser-misparse-backlog.md's top-summary totals needed manual arithmetic reconciliation against a sibling PR merged in the interim (Nettling Imp, #5463) — so the regression risk from the rebase is low, but I'm not claiming a post-rebase clippy/test re-run happened when it didn't.Backlog hygiene
Removed all 13 cards from
docs/parser-misparse-backlog.md's category 11 list, decrementing that category's count (170→157) and the top-summary totals accordingly (rebased onto the post-#5463 baseline: 4776→4763 distinct, 4810→4797 total appearances).Circle of Protection: Shadowand the pre-existing unrelatedCircle of Protection: Artstray entry are correctly retained.🤖 Generated with Claude Code