Skip to content

Add Vigor - #1425

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:card/vigor
May 29, 2026
Merged

Add Vigor#1425
matthewevans merged 2 commits into
phase-rs:mainfrom
mike-theDude:card/vigor

Conversation

@mike-theDude

Copy link
Copy Markdown
Collaborator

Summary

Adds engine support for Vigor (#1417). Two parser bugs combined to break Vigor on both sides of its damage-prevention replacement: the recipient valid_card filter was silently dropped at the ", prevent" clause boundary, and the rider's "that creature" anaphor dangled at runtime because passive replacements have no parent target slot. A third latent bug in the put-counter-for-each suffix path ignored the documented inherits_parent subject-application contract.

Files changed

  • crates/engine/src/parser/oracle_replacement.rs
  • crates/engine/src/parser/oracle_effect/mod.rs

CR references

  • CR 614.1a — replacement-effect application
  • CR 615.5 — damage prevention
  • CR 608.2c — object anaphor resolution ("that creature")

Track

Developer

LLM

Model: Claude Opus 4.7
Thinking level: high

Verification

  • cargo fmt --all -- --check clean
  • cargo test -p engine green, including the new vigor_event_recipient_filter_and_counter_target_rewrite unit test
  • Sibling test prevention_counter_followup_uses_prevented_amount_repeat (Test of Faith, spell-driven ParentTarget path) still passes — the rewrite is correctly gated on recipient_is_event_filter = valid_card_filter.is_some()

Scope Expansion

Fix #3 (honoring SubjectApplication::inherits_parent in the put-counter-for-each suffix path) is a class-wide latent fix beyond Vigor. It corrects at least 5 other cards (Dig Deep, Grave Strength, Mask of the Schemer, Raffine / Scheming Seer, Spymaster's Vault) that previously parsed "that creature" as a broad Typed filter. The fix follows the canonical contract already documented at subject.rs:1100-1102, so this is honoring an existing contract rather than introducing new infrastructure.

Follow-up Notes

The recipient-terminator combinator change uses peek(tag(", prevent")) to keep the recipient filter bound through the same-sentence imperative shape. If future replacement clauses introduce a different post-recipient continuation (e.g. ", that damage is dealt to ..."), the terminator alt list will need a corresponding peek arm — flagged here for the next reviewer touching parse_damage_recipient_valid_card_filter.

Two parser bugs that combined to make Vigor's "If damage would be dealt to
another creature you control, prevent that damage. Put a +1/+1 counter on
that creature for each 1 damage prevented this way." misbehave on both
sides of the replacement:

1. `parse_damage_recipient_valid_card_filter` (oracle_replacement.rs) only
   accepted recipient-clause terminators of eof / "." / "this turn" /
   "until end of turn". For the static-shield + same-sentence imperative
   shape ("dealt to <filter>, prevent that damage."), the trailing
   ", prevent" failed the all_consuming check, the recipient filter was
   silently dropped, and the prevention applied to every creature on the
   battlefield (broke both "you control" scoping and "another" exclusion —
   one shared root cause on the typed `valid_card` surface).

2. `parse_damage_prevention_replacement` had no equivalent to
   `rewrite_damage_recipient_to_post_replacement_target` for the object-
   anaphor cohort. The rider's "that creature" correctly parsed to
   `TargetFilter::ParentTarget` via the generic CR 608.2c anaphor path,
   but a passive replacement has no parent target slot, so the binding
   dangled at runtime and the +1/+1 counter rider never fired.

3. `try_parse_for_each_effect` (oracle_effect/mod.rs) suffix-form put-
   counter path ignored `SubjectApplication::inherits_parent`. The
   canonical contract in `parse_subject_application` (subject.rs:1100-1102)
   says the call site must lower an inherited-parent subject to
   `TargetFilter::ParentTarget`. Without this, the typed `Creature`
   filter leaked through and counters landed on every creature.

Fix surface:
- `oracle_replacement.rs`: broaden the recipient terminator combinator
  with `peek(tag(", prevent"))` (CR 614.1a + CR 615.5).
- `oracle_replacement.rs`: new sibling walker
  `rewrite_parent_target_to_post_replacement_damage_target` gated on
  `recipient_is_event_filter = valid_card_filter.is_some()`. Spell-
  driven cohort (Test of Faith) keeps `ParentTarget` for its real
  spell target. Uses the existing `each_target_filter_mut` building
  block. (CR 615.5 + CR 608.2c).
- `oracle_effect/mod.rs`: honor `inherits_parent` in the put-counter-
  for-each suffix path per the documented contract. Class-wide latent
  fix — also corrects at least 5 non-Vigor cards (Dig Deep, Grave
  Strength, Mask of the Schemer, Raffine / Scheming Seer, Spymaster's
  Vault) that previously parsed "that creature" as a broad Typed
  filter.

New unit test `vigor_event_recipient_filter_and_counter_target_rewrite`
asserts (a) recipient filter is Typed Creature + controller=You +
Another, (b) PutCounter target is PostReplacementDamageTarget, (c) count
source is EventContextAmount. Test of Faith sibling test
(`prevention_counter_followup_uses_prevented_amount_repeat`) still passes
— the spell-driven `ParentTarget` path is preserved.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@github-actions github-actions Bot added ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels May 29, 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 updates the damage prevention replacement parser to correctly handle event-driven static prevention replacements (such as Vigor) by capturing whether the recipient filter is event-driven and rewriting dangling parent targets to post-replacement damage targets. Feedback from the review highlights a missing recursion into mode_abilities within the target rewrite helpers to support modal abilities, as well as a missing mandatory CR annotation on rules-touching code.

Comment on lines +4949 to +4961
fn rewrite_parent_target_to_post_replacement_damage_target(def: &mut AbilityDefinition) {
super::oracle_effect::each_target_filter_mut(&mut def.effect, &mut |f| {
if matches!(f, TargetFilter::ParentTarget) {
*f = TargetFilter::PostReplacementDamageTarget;
}
});
if let Some(sub) = def.sub_ability.as_mut() {
rewrite_parent_target_to_post_replacement_damage_target(sub);
}
if let Some(else_branch) = def.else_ability.as_mut() {
rewrite_parent_target_to_post_replacement_damage_target(else_branch);
}
}

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] Missing recursion into mode_abilities in target rewrite helpers. Evidence: crates/engine/src/parser/oracle_replacement.rs:4949-4961.
Why it matters: If a follow-up ability is modal, any ParentTarget inside its mode_abilities will not be rewritten, leaving them dangling in passive replacement contexts.
Suggested fix: Recurse into def.mode_abilities in rewrite_parent_target_to_post_replacement_damage_target (and apply the same fix to its siblings rewrite_parent_target_controller_to_post_replacement_source and rewrite_damage_recipient_to_post_replacement_target).

fn rewrite_parent_target_to_post_replacement_damage_target(def: &mut AbilityDefinition) {
    super::oracle_effect::each_target_filter_mut(&mut def.effect, &mut |f| {
        if matches!(f, TargetFilter::ParentTarget) {
            *f = TargetFilter::PostReplacementDamageTarget;
        }
    });
    if let Some(sub) = def.sub_ability.as_mut() {
        rewrite_parent_target_to_post_replacement_damage_target(sub);
    }
    if let Some(else_branch) = def.else_ability.as_mut() {
        rewrite_parent_target_to_post_replacement_damage_target(else_branch);
    }
    for mode in &mut def.mode_abilities {
        rewrite_parent_target_to_post_replacement_damage_target(mode);
    }
}
References
  1. L2. Sibling coverage: If a fix landed in one site of a class, did the siblings need the same fix? Name them in the finding. (link)

Comment on lines +4773 to +4778
// Capture whether the recipient filter was event-driven (typed
// `valid_card`) before moving it onto `def` — the follow-up rewrite
// below uses this signal to distinguish the Vigor cohort (rewrite
// `ParentTarget` → `PostReplacementDamageTarget`) from the spell-driven
// cohort (keep `ParentTarget` for the real spell target).
let recipient_is_event_filter = valid_card_filter.is_some();

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.

medium

[MEDIUM] Missing mandatory CR annotation on rules-touching code. Evidence: crates/engine/src/parser/oracle_replacement.rs:4773-4778.
Why it matters: The style guide requires all rules-touching engine code to carry a verified CR <number>: <description> comment.
Suggested fix: Add CR 615.5 + CR 608.2c: to the comment.

    // CR 615.5 + CR 608.2c: Capture whether the recipient filter was event-driven (typed
    // `valid_card`) before moving it onto `def` — the follow-up rewrite
    // below uses this signal to distinguish the Vigor cohort (rewrite
    // `ParentTarget` → `PostReplacementDamageTarget`) from the spell-driven
    // cohort (keep `ParentTarget` for the real spell target).
    let recipient_is_event_filter = valid_card_filter.is_some();
References
  1. R6. CR annotations are mandatory and verified: Every rules-touching line of engine code must carry a comment of the form CR : . (link)

…ation lens)

Graft phase-rs#1426's runtime integration tests (vigor_regression.rs) onto this PR
so the prevention scope + counter-recipient behavior is driven through the
real apply() pipeline, not just parser-AST shape. The runtime paths
(valid_card gate in replacement.rs, PostReplacementDamageTarget in
targeting.rs:636) were only exercised by sibling cards before.

Tighten vigor_event_recipient_filter_and_counter_target_rewrite to pin
count == EventContextAmount (the field try_parse_for_each_effect sets for
the suffix-form for-each) instead of the loose repeat_for-OR-count
disjunction that did not discriminate which field carries the amount.

@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: Vigor fixed at the correct seam — lowers 'that creature' to ParentTarget in try_parse_for_each_effect (fixes the whole 'counter on that for each X' class) + peek(", prevent") boundary scopes the prevention shield. Grafted #1426's pipeline integration tests. CR-verified.

@matthewevans
matthewevans enabled auto-merge May 29, 2026 18:21
@matthewevans
matthewevans added this pull request to the merge queue May 29, 2026
Merged via the queue into phase-rs:main with commit 7af7e0f May 29, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-contribution PR opened via docs/AI-CONTRIBUTOR.md flow bug Bug fix mechanic:replacement-effects 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.

3 participants