From 7384e94001e9bed70946342f35a7d494e6260e6a Mon Sep 17 00:00:00 2001 From: Jacob Woodson Date: Fri, 14 Aug 2026 14:15:04 -0500 Subject: [PATCH 1/6] Fix Heart-Shaped Herb --- .../engine/src/parser/oracle_effect/lower.rs | 107 +++++++-- .../src/parser/oracle_effect/sequence.rs | 98 ++++++++ crates/engine/src/parser/oracle_tests.rs | 214 ++++++++++++++++++ .../integration/heart_shaped_herb_monarch.rs | 208 +++++++++++++++++ crates/engine/tests/integration/main.rs | 1 + 5 files changed, 610 insertions(+), 18 deletions(-) create mode 100644 crates/engine/tests/integration/heart_shaped_herb_monarch.rs diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 0fd8850fb9..cefdfe2067 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -10813,14 +10813,65 @@ pub(crate) fn parse_with_counters_suffix(lower: &str) -> Vec<(CounterType, Quant /// Like [`parse_with_counters_suffix`], but also reports the byte offset in /// `lower` at which the matched `"with N counter(s) [on it]"` clause -/// begins (the start of the `"with "` token). Callers that need to excise the +/// BEGINS (the start of the `"with "` token). Callers that need to excise the /// consumed counter clause from a larger remainder — e.g. -/// `strip_return_destination_ext_with_remainder`, so "return it to the -/// battlefield tapped and with two stun counters under its owner's control" -/// does not leave a dangling "and with two stun counters …" clause once the -/// counters are lifted onto `enter_with_counters` (Unstoppable Slasher) — use -/// this offset to truncate. Returns `None` for the offset when no counter -/// clause matched. +/// `strip_return_destination_ext_with_remainder` — use this offset to +/// truncate. Returns `None` for the offset when no counter clause matched. +/// +/// CR 122.1: counters are the marker this clause places. +/// +/// CONTRACT LIMITATION — read before relying on this. The offset marks only +/// the clause's START, not its end, so a caller that truncates at it also +/// discards everything AFTER the clause. `strip_return_destination_ext_with_remainder` +/// does exactly that: it keeps `text[entry_offset..entry_offset + off]`. +/// +/// That is NOT sound "because counter suffixes are clause-final" — they +/// frequently are not. Two printed counterexamples, both verbatim from +/// `data/mtgjson/AtomicCards.json`: +/// * Heart-Shaped Herb — "… return that card to the battlefield under its +/// owner's control with three +1/+1 counters on it AND YOU BECOME THE +/// MONARCH." +/// * Cosima, God of the Voyage — "… return Cosima to the battlefield with X +/// +1/+1 counters on it AND DRAW X CARDS, where X is the number of voyage +/// counters on it." +/// +/// What makes the truncation safe is UPSTREAM, not the corpus: the chunk-level +/// bare-and splitter `starts_bare_and_clause_lower` (sequence.rs) peels +/// recognized verb-headed tails into their own clause chunks before this +/// function ever sees the remainder. Heart-Shaped Herb's tail is peeled by the +/// `"you become "` arm — pinned end-to-end by +/// `you_become_monarch_conjunct_splits_without_trailing_period` (sequence.rs), +/// which asserts the counter clause and the monarch conjunct land in separate +/// chunks. Cosima's tail heads with `"draw "`, which has its own arm in the +/// same list. A tail whose head verb has NO arm there is still dropped +/// SILENTLY. The fix for the next such card is therefore to add the splitter +/// arm (or widen this to a real start..end span) — never to assume +/// clause-finality. +/// +/// The one corpus invariant that does hold is narrower: no printed Oracle text +/// puts a control/attach/tap clause after the counters. Reproduce it with jq +/// over Oracle text — NOT with a raw `grep -c` on the file, which cannot work: +/// `AtomicCards.json` is a single JSON line (`wc -l` reports 0), so `grep -c` +/// can only ever return 0 or 1, and it matches rulings/flavor/metadata rather +/// than `.text` (the previously cited +/// `grep -cE "counters? (under|attached|tapped)"` returns 1, and its sole hit +/// is ruling text, not a card). +/// +/// ```text +/// jq -r '[.data[][].text // empty +/// | select(test("counters? (under|attached|tapped)"))] | length' \ +/// data/mtgjson/AtomicCards.json # => 0 +/// ``` +/// +/// An earlier version of this comment cited Unstoppable Slasher as a +/// mid-clause example, with the text "return it to the battlefield tapped and +/// with two stun counters under its owner's control". THAT TEXT IS NOT REAL. +/// The printed card reads "…return it to the battlefield tapped under its +/// owner's control with two stun counters on it." The fabricated example +/// propagated into a downstream implementation plan before it was caught; if +/// this contract is ever widened to a full start..end span, verify the +/// motivating card against `data/mtgjson/AtomicCards.json` first (CLAUDE.md, +/// "Verify the card, not just the rule"). pub(crate) fn parse_with_counters_suffix_spanned( lower: &str, ) -> (Vec<(CounterType, QuantityExpr)>, Option) { @@ -10905,11 +10956,25 @@ pub(crate) fn parse_counter_suffix_body_combinator( let (rest, _) = tag(" counter").parse(rest)?; // Optional plural "s". let (rest, _) = nom::combinator::opt(tag::<_, _, OracleError<'_>>("s")).parse(rest)?; - // CR 614.1c: "on it" is grammatical filler — present in "return it to the - // battlefield with two +1/+1 counters on it" but absent when a controller - // clause follows ("return it to the battlefield tapped and with two stun - // counters under its owner's control", Unstoppable Slasher). Optional so - // both shapes lift the counters onto `enter_with_counters`. + // CR 614.1c: "on it" is grammatical filler and BOTH spellings are printed, + // so the terminator must be optional. Filler PRESENT: "…with two stun + // counters on it." (Unstoppable Slasher), "…with a +1/+1 counter on it." + // Filler ABSENT: "…with six +1/+1 counters.", "…with a first strike + // counter.", "…with three dread counters." Optional so both shapes lift + // the counters onto `enter_with_counters`. + // + // NOTE: this comment previously justified the optional filler with + // "absent when a controller clause follows", attributed to Unstoppable + // Slasher. That attribution was fabricated — Unstoppable Slasher reads + // "…return it to the battlefield tapped under its owner's control with two + // stun counters on it.", i.e. filler PRESENT. Presence or absence of the + // filler does NOT correlate with what follows the clause; it is free + // variation in the printed wording, which is why the terminator is + // `opt` rather than conditioned on a lookahead. (No printed card does put + // a control/attach/tap clause after the counters — see the CONTRACT + // LIMITATION note on `parse_with_counters_suffix_spanned` for the jq that + // reproduces that, and for why the truncation's real safety argument is + // upstream rather than corpus-based.) let (rest, _) = nom::combinator::opt(tag::<_, _, OracleError<'_>>(" on it")).parse(rest)?; Ok(( @@ -11721,12 +11786,18 @@ mod tests { )); } - /// CR 614.1c + issue #1498: "return it to the battlefield tapped and with - /// two stun counters under its owner's control" (Unstoppable Slasher) must - /// lift the stun counters onto `enter_with_counters` and excise the counter - /// clause from the returned remainder so no dangling follow-up clause is - /// re-parsed. The `" on it"` filler is absent here (a controller clause - /// follows the counters), which the optional terminator now tolerates. + /// CR 614.1c + issue #1498: a counter clause with no `" on it"` filler must + /// still lift its counters onto `enter_with_counters` and be excised from + /// the returned remainder, so no dangling follow-up clause is re-parsed. + /// + /// SYNTHETIC INPUT — not a printed card. This text was previously + /// attributed to Unstoppable Slasher; that attribution was fabricated. The + /// real card reads "…return it to the battlefield tapped under its owner's + /// control with two stun counters on it." (filler present, clause-final). + /// The filler-less shape this test pins IS real, but only clause-finally + /// ("… with six +1/+1 counters."); the trailing controller clause here is + /// an artificial stress case for the excision path. See the CONTRACT + /// LIMITATION note on `parse_with_counters_suffix_spanned`. #[test] fn return_to_battlefield_lifts_stun_counters_without_on_it_filler() { let (target, dest, remainder) = strip_return_destination_ext_with_remainder( diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 7437fa9d2e..174369e633 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -2765,6 +2765,39 @@ fn starts_bare_and_clause_lower(s: &str) -> bool { value((), tag("you search ")), value((), tag("you surveil ")), value((), tag("you get ")), + // CR 725.1: "There is no monarch in a game until an effect instructs a + // player to become the monarch" — becoming the monarch is its own + // instruction, never a noun-phrase continuation of the conjunct before + // it. CR 608.2c: the controller follows the printed instructions "in + // the order written", so a trailing "… and you become the monarch" is + // the NEXT instruction and must reach the clause dispatcher on its own. + // + // Exactly two lines in the whole corpus contain " and you become " — + // both were broken, in different ways: + // Heart-Shaped Herb: "…with three +1/+1 counters on it AND YOU BECOME + // THE MONARCH" — dropped SILENTLY (reported supported, zero gaps), + // because the return-destination counter-suffix truncation in + // `strip_return_destination_ext_with_remainder` (lower.rs) cuts the + // remainder at the counter clause's START offset, discarding the + // tail before any guard could see it. + // Fall from Favor: "tap enchanted creature AND YOU BECOME THE + // MONARCH" — isolated by `try_split_targeted_compound` (mod.rs) but + // dispatched through `parse_imperative_effect`, which never tries + // the subject-predicate path for a bare "you" subject, so it + // surfaced as `Unimplemented { name: "you" }`. + // Splitting at the chunk level runs BEFORE both of those seams, so this + // one arm recovers both cards. + // + // The tag is the SUBJECT+VERB boundary, not the designation: "become" + // is a registered `PREDICATE_VERBS` member (subject.rs) and + // `build_become_clause` (subject.rs) is the single authority that + // adjudicates WHICH become (monarch / life total / day-night / color / + // animation). Matching "you become the monarch" here would put + // card-specific knowledge in the sentence-chunking layer. Mirrors the + // `it becomes ` arm below — same predicate, different subject. A + // blanket "you " rule is NOT safe: " and you control" + // (22 corpus lines) is a relative clause, never a clause start. + value((), tag("you become ")), value((), tag("you may ")), // CR 614.1b + CR 603.7a: "Effects that use the word 'skip' are // replacement effects" — a skip is its own instruction, never a @@ -12407,6 +12440,71 @@ mod tests { )); } + /// CR 725.1 + CR 608.2c: "… and you become " is a subject + + /// predicate clause, never a noun-phrase continuation. The splitter arm is + /// deliberately VERB-level (`"you become "`), not designation-level, so + /// `build_become_clause` (subject.rs) stays the single authority over which + /// become is meant. + #[test] + fn bare_and_clause_starts_on_you_become_subject_predicate() { + // The two real corpus lines this fixes (Heart-Shaped Herb, Fall from + // Favor) both use the monarch designation. + assert!(starts_bare_and_clause("you become the monarch")); + assert!(starts_bare_and_clause("You become the Monarch")); + + // Generalization guard: the arm is designation-agnostic, so a future + // non-monarch `become` conjunct is carried too. These are SYNTHETIC + // inputs to the predicate — both phrases are real Oracle text, but + // neither currently occurs after a bare " and ", so this is not an + // end-to-end reachability claim. Downstream, `build_become_clause` + // declines the monarch arm and falls through to the animation path, + // which is the honest-defer route. + assert!(starts_bare_and_clause("you become the starting player")); + assert!(starts_bare_and_clause( + "you become a card until you leave your library or that library is shuffled" + )); + + // Negative: " and you control …" is a RELATIVE clause (22 corpus + // lines), never a clause start. A blanket "you " rule + // would have split these and changed unrelated cards. + assert!(!starts_bare_and_clause("you control")); + assert!(!starts_bare_and_clause("you control a Swamp")); + assert!(!starts_bare_and_clause( + "you control a legendary creature or planeswalker" + )); + } + + /// CR 725.1 + CR 608.2c: the bare-and split must peel the monarch conjunct + /// off Heart-Shaped Herb's activated ability into its OWN chunk, and + /// `push_clause_chunk`'s `trim_end_matches(['.', ','])` must strip the + /// sentence-final period. The period is load-bearing: `build_become_clause` + /// gates on the exact match `become_text.eq_ignore_ascii_case("the + /// monarch")` (subject.rs), which a surviving "." would defeat, silently + /// falling through to the animation path. + #[test] + fn you_become_monarch_conjunct_splits_without_trailing_period() { + // Verbatim Oracle text (data/mtgjson/AtomicCards.json), effect body of + // the "{2}, {T}, Sacrifice this artifact:" ability. + let chunks = clause_texts( + "return that card to the battlefield under its owner's control with three +1/+1 counters on it and you become the monarch.", + ); + assert_eq!( + chunks.last().map(String::as_str), + Some("you become the monarch"), + "monarch conjunct must be its own chunk with no trailing period, got {chunks:?}" + ); + // Paired positive reach-guard: the leading return clause must survive + // intact, so a passing split assertion cannot be an artifact of the + // sentence being mangled. + assert_eq!( + chunks.first().map(String::as_str), + Some( + "return that card to the battlefield under its owner's control with three +1/+1 counters on it" + ), + "return clause must remain whole, got {chunks:?}" + ); + } + /// CR 608.2c: Anaphoric back-reference conjuncts. Nalia de'Arnise's third /// ability is the canonical exemplar — "put a +1/+1 counter on each /// creature you control and those creatures gain deathtouch until end of diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index e83c4c068c..9a0a320914 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -13073,6 +13073,220 @@ fn you_become_the_monarch_subject() { ); } +/// Walks an ability chain looking for any clause that failed closed to +/// [`Effect::Unimplemented`]. Used by the monarch conjunct tests as a +/// non-vacuous guard: recovering a clause is only a fix if it produces a real +/// typed effect rather than a differently-shaped gap. +/// +/// Traverses every nested-definition field on `AbilityDefinition` — +/// `sub_ability`, `else_ability` (CR 608.2c "Otherwise, …" branch) and +/// `mode_abilities` (CR 700.2 modal) — mirroring +/// `AbilityDefinition::normalize_parsed_replacement_flags` (types/ability.rs), +/// the existing authority for "walk this definition's nested chain". Partial +/// traversal would reintroduce the exact vacuous-negative class this guard +/// exists to prevent: a `you become ` conjunct that landed in an +/// unvisited branch still carrying `Effect::Unimplemented` would pass +/// silently. Neither Heart-Shaped Herb nor Fall from Favor produces an +/// else-branch or modes today, so this is forward protection, not a live fix. +fn monarch_chain_has_unimplemented(def: &AbilityDefinition) -> bool { + if matches!(*def.effect, Effect::Unimplemented { .. }) { + return true; + } + def.sub_ability + .as_deref() + .is_some_and(monarch_chain_has_unimplemented) + || def + .else_ability + .as_deref() + .is_some_and(monarch_chain_has_unimplemented) + || def + .mode_abilities + .iter() + .any(monarch_chain_has_unimplemented) +} + +/// CR 725.1 + CR 608.2c: Heart-Shaped Herb's activated ability ends with +/// "… with three +1/+1 counters on it and you become the monarch". Before the +/// `"you become "` bare-and splitter arm, the trailing conjunct was dropped +/// SILENTLY — the return-destination counter-suffix truncation in +/// `strip_return_destination_ext_with_remainder` (lower.rs) cut the remainder +/// at the counter clause's start offset, so the card reported as fully +/// supported with zero gaps while discarding a printed instruction. +/// +/// The monarch grant must land NESTED under the `EffectOutcome`-gated +/// `ChangeZone`, not as a sibling of the `Sacrifice`: CR 608.2c means +/// declining "You may sacrifice a creature" must skip the monarch grant too. +/// A `SequentialSibling` placement would wrongly hand out the monarch on +/// decline, so the link is the load-bearing assertion here. +#[test] +fn heart_shaped_herb_activated_ability_grants_monarch_as_continuation() { + use crate::parser::oracle_effect::parse_effect_chain; + use crate::types::ability::{AbilityCondition, AbilityKind, SubAbilityLink}; + + // Verbatim Oracle text (data/mtgjson/AtomicCards.json), effect body of the + // "{2}, {T}, Sacrifice this artifact:" ability. + let def = parse_effect_chain( + "You may sacrifice a creature. If you do, return that card to the battlefield under its owner's control with three +1/+1 counters on it and you become the monarch.", + AbilityKind::Activated, + ); + + // Paired positive reach-guard: the leading sacrifice and the gated return + // must both still be present, so a passing monarch assertion cannot be an + // artifact of the sentence being re-parsed into something else. + assert!( + matches!(*def.effect, Effect::Sacrifice { .. }), + "head effect must remain Sacrifice, got {:?}", + def.effect, + ); + let change_zone = def + .sub_ability + .as_ref() + .expect("sacrifice must carry the gated return as its sub-ability"); + assert!( + matches!( + *change_zone.effect, + Effect::ChangeZone { + destination: Zone::Battlefield, + .. + } + ), + "gated sub must remain the battlefield return, got {:?}", + change_zone.effect, + ); + assert_eq!( + change_zone.condition, + Some(AbilityCondition::EffectOutcome { + signal: crate::types::ability::EffectOutcomeSignal::OptionalEffectPerformed, + }), + "the return must stay gated on the optional sacrifice being performed" + ); + + // The fix: the monarch conjunct is recovered as the return's continuation. + let monarch = change_zone + .sub_ability + .as_ref() + .expect("the 'and you become the monarch' conjunct must be recovered"); + assert!( + matches!(*monarch.effect, Effect::BecomeMonarch), + "expected BecomeMonarch, got {:?}", + monarch.effect, + ); + // CR 608.2c: a ContinuationStep under the gated return is skipped when the + // optional sacrifice is declined. This is the assertion that flips if the + // splitter arm is reverted (the node disappears entirely). + assert_eq!( + monarch.sub_link, + SubAbilityLink::ContinuationStep, + "monarch grant must be a continuation of the gated return, not an \ + independent sibling — a sibling would grant the monarch even when the \ + optional sacrifice is declined" + ); + assert!( + !monarch_chain_has_unimplemented(&def), + "no clause may fail closed to Unimplemented" + ); +} + +/// CR 725.1 + CR 608.2c: Fall from Favor — "When this Aura enters, tap +/// enchanted creature and you become the monarch." Before the splitter arm the +/// conjunct was isolated by `try_split_targeted_compound` (mod.rs) but +/// dispatched through `parse_imperative_effect`, which never tries the +/// subject-predicate path for a bare "you" subject, so it surfaced as +/// `Effect::Unimplemented { name: "you" }` and the card was reported as +/// unsupported. Splitting at the chunk level runs first, so the conjunct +/// reaches `try_parse_subject_become_clause` → `build_become_clause`. +#[test] +fn fall_from_favor_trigger_body_grants_monarch_not_unimplemented() { + use crate::parser::oracle_effect::parse_effect_chain; + use crate::types::ability::AbilityKind; + + // Verbatim Oracle text (data/mtgjson/AtomicCards.json), trigger body. + let def = parse_effect_chain( + "tap enchanted creature and you become the monarch", + AbilityKind::Spell, + ); + + // Paired positive reach-guard: the tap clause must survive. A chain that + // lost the tap half must not pass this test. + assert!( + matches!( + *def.effect, + Effect::SetTapState { + state: TapStateChange::Tap, + .. + } + ), + "tap clause must remain intact, got {:?}", + def.effect, + ); + let monarch = def + .sub_ability + .as_ref() + .expect("the 'and you become the monarch' conjunct must be recovered"); + assert!( + matches!(*monarch.effect, Effect::BecomeMonarch), + "expected BecomeMonarch, got {:?}", + monarch.effect, + ); + assert!( + !monarch_chain_has_unimplemented(&def), + "the bare 'you' subject must no longer fail closed to Unimplemented" + ); +} + +/// CR 608.2c: the `sub_link` on a recovered `you become …` conjunct comes from +/// the printed BOUNDARY, not from the verb. A sentence boundary must yield +/// `SequentialSibling` (the monarch grant is then independent of the preceding +/// instruction), while the bare-and conjunct above yields `ContinuationStep`. +#[test] +fn you_become_monarch_sub_link_tracks_boundary_not_verb() { + use crate::parser::oracle_effect::parse_effect_chain; + use crate::types::ability::{AbilityKind, SubAbilityLink}; + + let sentence = parse_effect_chain( + "Tap enchanted creature. You become the monarch.", + AbilityKind::Spell, + ); + let monarch = sentence + .sub_ability + .as_ref() + .expect("sentence-boundary monarch clause must be present"); + assert!( + matches!(*monarch.effect, Effect::BecomeMonarch), + "expected BecomeMonarch, got {:?}", + monarch.effect, + ); + assert_eq!( + monarch.sub_link, + SubAbilityLink::SequentialSibling, + "a sentence boundary must produce an independent sibling" + ); + + // Hostile fixture: swap the become-verb conjunct for an already-supported + // `you gain ` conjunct at the SAME bare-and boundary. The link must be + // identical, proving it is derived from the boundary rather than the verb. + let gain = parse_effect_chain( + "tap enchanted creature and you gain 2 life", + AbilityKind::Spell, + ); + let gain_sub = gain + .sub_ability + .as_ref() + .expect("bare-and 'you gain' conjunct must be present"); + let become_chain = parse_effect_chain( + "tap enchanted creature and you become the monarch", + AbilityKind::Spell, + ); + let become_sub = become_chain + .sub_ability + .as_ref() + .expect("bare-and 'you become' conjunct must be present"); + assert_eq!( + become_sub.sub_link, gain_sub.sub_link, + "the bare-and boundary must produce the same link for both verbs" + ); +} + // ── Coverage batch: prevent damage ──────────────────────────────── #[test] diff --git a/crates/engine/tests/integration/heart_shaped_herb_monarch.rs b/crates/engine/tests/integration/heart_shaped_herb_monarch.rs new file mode 100644 index 0000000000..28dbdf026d --- /dev/null +++ b/crates/engine/tests/integration/heart_shaped_herb_monarch.rs @@ -0,0 +1,208 @@ +//! Heart-Shaped Herb — the activated ability's trailing "and you become the +//! monarch" conjunct (CR 725.1) must actually grant the monarch at runtime. +//! +//! Oracle (verbatim, data/mtgjson/AtomicCards.json): +//! If a source an opponent controls would deal damage to you, prevent 1 of +//! that damage. +//! {2}, {T}, Sacrifice this artifact: You may sacrifice a creature. If you +//! do, return that card to the battlefield under its owner's control with +//! three +1/+1 counters on it and you become the monarch. +//! +//! Before the `"you become "` bare-and splitter arm (parser/oracle_effect/ +//! sequence.rs), the final conjunct was dropped SILENTLY: the return-destination +//! counter-suffix truncation in `strip_return_destination_ext_with_remainder` +//! (lower.rs) cut the remainder at the counter clause's START offset, discarding +//! the tail. The card reported as fully supported with zero coverage gaps while +//! never granting the monarch. +//! +//! Every test here drives the real `apply()` pipeline (GameScenario + +//! GameRunner::activate + CR 602 announce/pay/resolve) and asserts measured +//! state deltas, never AST shape. + +use engine::game::scenario::{GameRunner, GameScenario, P0}; +use engine::types::counter::CounterType; +use engine::types::mana::{ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; +use engine::types::zones::Zone; +use engine::types::ObjectId; + +const P1: PlayerId = PlayerId(1); + +const ORACLE: &str = "If a source an opponent controls would deal damage to you, prevent 1 of that damage.\n{2}, {T}, Sacrifice this artifact: You may sacrifice a creature. If you do, return that card to the battlefield under its owner's control with three +1/+1 counters on it and you become the monarch."; + +/// Build P0's turn at PreCombatMain with the Herb on the battlefield as an +/// artifact and EXACTLY ONE creature P0 controls. +/// +/// The single-creature constraint is load-bearing, not incidental: `Effect:: +/// Sacrifice` only raises `WaitingFor::EffectZoneChoice` when the eligible pool +/// EXCEEDS the count. With one eligible creature the CR 701.21a mandatory-all +/// fast path fires and no prompt opens — which matters because +/// `AbilityActivation` has no `.effect_zone(..)` setter (its `ResolutionPolicy` +/// hardcodes an empty `effect_zone_cards`), so a second eligible creature would +/// stall `drive_resolution` and surface as a confusing "monarch is None" +/// failure that mimics the bug under test. If a future variant needs a larger +/// pool, add `.effect_zone(&[ObjectId])` to `AbilityActivation` mirroring +/// `SpellCast::effect_zone` first. +fn setup() -> (GameRunner, ObjectId, ObjectId) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + // {2} generic — auto-tap is NOT modeled by the driver, so fund the pool. + scenario.with_mana_pool( + P0, + (0..4) + .map(|_| ManaUnit::new(ManaType::Green, ObjectId(0), false, vec![])) + .collect(), + ); + let herb = { + let mut b = scenario.add_creature(P0, "Heart-Shaped Herb", 0, 0); + b.from_oracle_text(ORACLE).as_artifact(); + b.id() + }; + let creature = scenario.add_vanilla(P0, 2, 2); + let runner = scenario.build(); + (runner, herb, creature) +} + +fn counters(runner: &GameRunner, id: ObjectId) -> u32 { + runner + .state() + .objects + .get(&id) + .and_then(|o| o.counters.get(&CounterType::Plus1Plus1).copied()) + .unwrap_or(0) +} + +/// CR 725.1 + CR 109.5: accepting the optional sacrifice must make the player +/// who ACTIVATED the ability the monarch. +/// +/// Revert-failing assertion: `state().monarch == Some(P0)`. With the splitter +/// arm reverted the `BecomeMonarch` node does not exist in the chain at all, so +/// `monarch` stays `None`. +#[test] +fn accepting_the_sacrifice_makes_the_activating_player_the_monarch() { + let (mut runner, herb, creature) = setup(); + assert_eq!( + runner.state().monarch, + None, + "precondition: no monarch before activation (CR 725.1)" + ); + + let outcome = runner + .activate(herb, 0) + .pay_with(&[herb]) + .accept_optional() + .resolve(); + + // THE fix assertion. + assert_eq!( + outcome.state().monarch, + Some(P0), + "CR 725.1: the activating player must become the monarch" + ); + + // Paired positive reach-guards — prove the chain was actually REACHED and + // the gated return ran, so the monarch assertion cannot be an artifact of + // the ability short-circuiting somewhere else. + assert_eq!( + outcome.zone_of(creature), + Zone::Battlefield, + "the sacrificed creature must be returned to the battlefield" + ); + assert_eq!( + counters(&runner, creature), + 3, + "CR 122.1: the returned creature must enter with three +1/+1 counters" + ); +} + +/// CR 608.2c: the monarch grant is a ContinuationStep under the +/// `EffectOutcome`-gated return, so DECLINING "You may sacrifice a creature" +/// must skip it. A `SequentialSibling` placement would wrongly hand out the +/// monarch on decline. +/// +/// Non-vacuous by construction: the negative monarch assertion is paired with +/// two positive guards proving the ability really was activated and its costs +/// really were paid — otherwise "monarch is None" would pass trivially on a +/// failed activation. +#[test] +fn declining_the_sacrifice_skips_the_monarch_grant() { + let (mut runner, herb, creature) = setup(); + + let outcome = runner + .activate(herb, 0) + .pay_with(&[herb]) + .decline_optional() + .resolve(); + + assert_eq!( + outcome.state().monarch, + None, + "CR 608.2c: declining the optional sacrifice must skip the gated \ + continuation, so no monarch is designated" + ); + + // Reach-guard (a): the ability WAS activated and its sacrifice cost paid — + // the Herb itself is gone from the battlefield (CR 701.21a). + assert_ne!( + outcome.zone_of(herb), + Zone::Battlefield, + "the Herb must have been sacrificed as an activation cost, proving the \ + ability was actually activated" + ); + // Reach-guard (b): the gated ChangeZone did NOT run. A bare "creature is + // not on the battlefield" check would be vacuous here (it never left), so + // assert on the counters the return would have added. + assert_eq!( + counters(&runner, creature), + 0, + "the gated return must not have run, so no +1/+1 counters were placed" + ); +} + +/// Hostile multi-authority fixture. The sentence names TWO different players: +/// the returned card's OWNER (who gains control of the creature, CR 110.2) and +/// the ability's CONTROLLER (who becomes the monarch, CR 109.5). They are +/// normally the same player, which is exactly why a wrong binding would hide. +/// Here they are forced apart, and the opponent is already the monarch, so a +/// no-op would be indistinguishable from success without this fixture. +#[test] +fn monarch_binds_to_the_controller_while_the_creature_returns_to_its_owner() { + let (mut runner, herb, creature) = setup(); + // The opponent is already the monarch — so "monarch == Some(P0)" can only + // be produced by the grant actually running, never by the initial state. + runner.state_mut().monarch = Some(P1); + // P0 controls the creature (so P0 may sacrifice it, CR 701.21a) but P1 + // OWNS it (so "under its owner's control" returns it under P1's control). + // `GameScenario` exposes no owner setter; `state_mut()` is the documented + // escape hatch. + runner.state_mut().objects.get_mut(&creature).unwrap().owner = P1; + + let outcome = runner + .activate(herb, 0) + .pay_with(&[herb]) + .accept_optional() + .resolve(); + + // CR 109.5: "you" on an activated ability is the player who ACTIVATED it — + // not the sacrificed card's owner. + assert_eq!( + outcome.state().monarch, + Some(P0), + "CR 109.5: the monarch must be the ability's controller (P0), taking \ + the designation away from P1" + ); + // CR 110.2 / CR 110.2a: "under its owner's control" is a separate + // authority and must still bind to P1. + assert_eq!( + outcome.zone_of(creature), + Zone::Battlefield, + "the creature must be returned to the battlefield" + ); + assert_eq!( + outcome.state().objects[&creature].controller, + P1, + "CR 110.2: the returned creature enters under its OWNER's control (P1), \ + independently of who becomes the monarch" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 40c770b545..085622cef5 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -286,6 +286,7 @@ mod hag_noxious_nightmares_menace_grant; mod halana_alena_partners_where_x; mod harrow_regression; mod hawkeye_avenging_archer_dealt_damage_draw; +mod heart_shaped_herb_monarch; mod heist_production_path_handoff; mod hellkite_tyrant_steal_artifacts_2906; mod heroic_defiance_recipient_color_4590; From aca23c6a9f1d97228a4982bb9a01ea6780e5c86d Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:20:02 -0500 Subject: [PATCH 2/6] fix(parser): consume the enter-with-counters rider instead of truncating at it Addresses the review blocker on #7402. strip_return_destination_ext_with_remainder lifted the "with N counter(s)" clause onto enter_with_counters and then returned text[entry_offset..entry_offset + off], where off was the clause's START offset -- so every instruction printed AFTER the counter clause was discarded. Heart-Shaped Herb's "...with three +1/+1 counters on it and you become the monarch" lost the monarch instruction at this seam before the bare-and splitter could ever see it, and a trailing control clause vanished with it. Riders, the control clause and the counter clause are independent battlefield-entry conditions printed in any order (CR 614.1c, CR 508.4, CR 708.3, CR 110.2a, CR 122.1), so consume them as one order-independent run to a fixpoint via a new leading-anchored parse_leading_enter_counters_clause (connector handling mirrors parse_one_battlefield_rider). Consuming rather than excising keeps the remainder a genuine suffix, which makes the whole class of mid-clause truncation structurally impossible rather than corpus-dependent. Two supporting changes fall out of the same grammar: * parse_with_counters_suffix_spanned now returns the clause's full Range instead of a bare start offset. The one remaining call site that still truncates at start (split_counterless_enter_counters) documents in place why its tail is empty by construction; the exile path's assert_no_compound_remainder now checks BOTH sides of the excised span instead of only the head it used to keep. * The rider body accepts counter clauses conjoined by " and " inside one "with" -- "with a hexproof counter and an indestructible counter on it" (Perennation), "...with a vigilance counter and a lifelink counter on it" (Gilraen, Dunedain Protector), "...with two +1/+1 counters and a lifelink counter on it" (Dust Animus), Voidpouncer. Only the first conjunct was lifted before, so the rest were silently dropped; without this, preserving the tail would merely have converted that silent drop into a dangling remainder. separated_list1 is the right combinator because nom backtracks the separator when the element fails, so a non-counter conjunct ("and you become the monarch", "and draw two cards", "and with haste") is never swallowed -- every element must open with a count or article. Also drops the CR 614.1c citation from the "on it" optional-filler comment: the rule defines "enters with" effects as replacement effects and says nothing about Oracle grammar, so the citation did not describe the code it annotated. Tests: the synthetic excision regression now asserts the trailing "under its owner's control" survives onto dest.control (it came back None under the old truncation); new cases pin the surviving instruction for Heart-Shaped Herb's verbatim text, Perennation's conjoined rider, and the non-counter conjunct boundary. Verified: cargo fmt --all; cargo clippy -p phase-engine --all-targets -D warnings (exit 0); cargo test -p phase-engine (19099 + 21 + 9 + 4985 passed, 0 failed); scripts/check-parser-combinators.sh (Gate G PASS + Gate A PASS). Co-Authored-By: Claude Opus 5 --- .../src/parser/oracle_effect/imperative.rs | 17 +- .../engine/src/parser/oracle_effect/lower.rs | 458 +++++++++++------- crates/engine/src/parser/oracle_effect/mod.rs | 12 +- .../src/parser/oracle_effect/sequence.rs | 13 +- crates/engine/src/parser/oracle_tests.rs | 13 +- .../integration/heart_shaped_herb_monarch.rs | 19 +- 6 files changed, 344 insertions(+), 188 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index c288c87d94..4710ab8014 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -8985,7 +8985,7 @@ pub(super) fn parse_exile_ast( // Excise the consumed clause so the debug-only compound-remainder assert // below does not flag it. let rem_lower = rem.to_ascii_lowercase(); - let (mut enter_with_counters, counters_offset) = + let (mut enter_with_counters, counters_span) = super::parse_with_counters_suffix_spanned(&rem_lower); // CR 122.2 + CR 702.62a: Adopt the counters lifted off a counterless-origin // descriptive target above (Doom's Time Platform) when the post-target @@ -9014,12 +9014,19 @@ pub(super) fn parse_exile_ast( let rest_lower_full = rest_text.to_ascii_lowercase(); enter_with_counters = super::parse_with_counters_suffix(&rest_lower_full); } - let _rem = match counters_offset { - Some(off) => &rem[..off], - None => rem, + // Excise ONLY the counter clause's span and check BOTH sides. Truncating at + // the span's start (as this did before) hid any compound instruction printed + // after the rider from the very assert whose job is to catch silent + // remainder drops. + let (_rem_head, _rem_tail) = match &counters_span { + Some(span) => (&rem[..span.start], &rem[span.end..]), + None => (rem, ""), }; #[cfg(debug_assertions)] - assert_no_compound_remainder(_rem, text); + { + assert_no_compound_remainder(_rem_head, text); + assert_no_compound_remainder(_rem_tail, text); + } // CR 701.5a: "exile target spell" must constrain targeting to the stack, // mirroring parse_counter_ast at line 1218-1219. let target = if nom_primitives::scan_contains(rest_lower, "spell") { diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index cefdfe2067..d1c133b88d 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -2,6 +2,7 @@ use nom::branch::alt; use nom::bytes::complete::{tag, take_till1, take_until}; use nom::character::complete::{multispace0, multispace1, satisfy}; use nom::combinator::{all_consuming, eof, map, not, opt, peek, rest, value, verify}; +use nom::multi::separated_list1; use nom::sequence::{preceded, terminated}; use nom::Parser; @@ -7122,64 +7123,68 @@ pub(super) fn strip_return_destination_ext_with_remainder( // rows, whose `control` is always `None` per CR 110.1 @ :614). // `*row_control` is a `Copy` read out of the `&'static` table row. let mut control: Option = *row_control; - if *zone == Zone::Battlefield { - let (rider_rest, riders) = - strip_trailing_battlefield_riders(&lower[entry_offset..]); - face_down |= riders.face_down; - transformed |= riders.transformed; - enter_tapped |= riders.enter_tapped; - enters_attacking |= riders.enters_attacking; - entry_offset = lower.len() - rider_rest.len(); - // CR 110.2a: the table enumerates only the first- and - // owner-person spellings, so a third-person clause ("under - // their control", "under that player's control") survives on - // the tail. Consume it HERE, BEFORE `after_destination`, the - // counters-suffix scan and `original_after_destination` all - // read `entry_offset` — otherwise the clause is both dropped - // from the destination AND re-emitted as a dangling remainder. - if control.is_none() { - if let Ok((rest, p)) = parse_leading_control_clause(&lower[entry_offset..]) { - control = Some(p); + // CR 614.1c (:3062) + CR 508.4 (:2312) + CR 708.3 (:5723) + CR + // 110.2a (:618) + CR 122.1 (:1178): battlefield-entry riders, the control + // clause and the "with … counter(s)" clause are INDEPENDENT entry + // conditions printed in any order ("under your control face down and + // tapped", "tapped and with two stun counters on it"). Consume them + // as one order-independent run to a fixpoint rather than as a fixed + // riders → control → riders pass sequence. + // + // CONSUMING (advancing `entry_offset`) rather than excising a span + // out of the middle is what keeps the remainder a true SUFFIX, so an + // instruction printed AFTER the entry clauses stays reachable by + // normal clause processing. The previous code truncated the + // remainder at the counter clause's start offset and so discarded + // everything past it — Heart-Shaped Herb's "…with three +1/+1 + // counters on it and you become the monarch" lost the monarch + // instruction outright, and any trailing control clause vanished + // with it. + let mut enter_with_counters: Vec<(CounterType, QuantityExpr)> = Vec::new(); + loop { + let before = entry_offset; + if *zone == Zone::Battlefield { + let (rider_rest, riders) = + strip_trailing_battlefield_riders(&lower[entry_offset..]); + face_down |= riders.face_down; + transformed |= riders.transformed; + enter_tapped |= riders.enter_tapped; + enters_attacking |= riders.enters_attacking; + entry_offset = lower.len() - rider_rest.len(); + // CR 110.2a: the table enumerates only the first- and + // owner-person spellings, so a third-person clause ("under + // their control", "under that player's control") survives on + // the tail. Consume it here so it is neither dropped from + // the destination NOR re-emitted as a dangling remainder. + if control.is_none() { + if let Ok((rest, p)) = parse_leading_control_clause(&lower[entry_offset..]) + { + control = Some(p); + entry_offset = lower.len() - rest.len(); + } + } + } + // CR 122.1: the counter rider is zone-agnostic here — the + // pre-loop scan it replaces ran for every destination row, not + // just the battlefield ones, so it stays outside the gate above. + if enter_with_counters.is_empty() { + if let Ok((rest, counters)) = + parse_leading_enter_counters_clause(&lower[entry_offset..]) + { + enter_with_counters = counters; entry_offset = lower.len() - rest.len(); - // CR 614.1c (:3060) + CR 508.4 (:2309) + CR 708.3 - // (:5707): entry riders are order-independent and may - // ALSO trail the control clause ("under your control - // face down and tapped"). - let (rest2, riders2) = - strip_trailing_battlefield_riders(&lower[entry_offset..]); - face_down |= riders2.face_down; - transformed |= riders2.transformed; - enter_tapped |= riders2.enter_tapped; - enters_attacking |= riders2.enters_attacking; - entry_offset = lower.len() - rest2.len(); } } - } - let after_destination = &lower[entry_offset..]; - let (enter_with_counters, counters_offset) = - parse_with_counters_suffix_spanned(after_destination); - // CR 614.1c: when the "with N counter(s)" clause is lifted - // onto `enter_with_counters`, excise it (and any leading " and" - // connector) from the returned remainder so the caller does not - // re-parse "and with two stun counters …" into a dangling - // Unimplemented follow-up clause (Unstoppable Slasher). - let original_after_destination = match counters_offset { - Some(off) => { - // CR 614.1c: strip a trailing " and" connector left after - // excising the consumed counter clause. Space-anchored - // `strip_suffix(" and")` (not `trim_end_matches("and")`, - // which is not word-anchored and would corrupt a remainder - // ending in "brand"/"island"); mirrors the leading - // `strip_leading_sequence_connector` analogue. - let trimmed = text[entry_offset..entry_offset + off].trim_end(); - trimmed - // allow-noncombinator: structural cleanup of a trailing " and" connector on an already-sliced remainder, not parsing dispatch - .strip_suffix(" and") - .map(|s| s.trim_end()) - .unwrap_or(trimmed) + if entry_offset == before { + break; } - None => &text[entry_offset..], - }; + } + // A true suffix of the ORIGINAL-case `text`: everything the entry + // clauses did not consume, in printed order. Riders, control clauses + // and counter types are pure ASCII and case-invariant, so advancing + // the offset on `lower` indexes `text` identically — the same + // invariant the pre-existing `pos + phrase_len` indexing assumes. + let original_after_destination = &text[entry_offset..]; return ( text[..pos].trim(), Some(ReturnDestination { @@ -10797,90 +10802,108 @@ fn parse_signed_pt_component(text: &str) -> Option { /// * "with a/an counter on it" — singular article. /// * Optional "additional " between count and type — purely a synonym in /// this position; the counter is still added once during the move. +/// * Two or more of the above conjoined by `" and "` inside a single "with" +/// ("with a hexproof counter and an indestructible counter on it", +/// Perennation) — the returned `Vec` carries one entry per conjunct. /// /// Returns an empty `Vec` when no clause is present, so the caller can stamp /// it unconditionally. /// -/// Implemented as a `scan_preceded` over the body combinator — the scanner -/// advances at word boundaries, so the suffix can appear anywhere after the -/// destination phrase ("onto the battlefield tapped under your control with -/// two additional +1/+1 counters on it") without the caller having to -/// pre-trim. The body combinator gates on `tag("with ")` then dispatches to -/// `parse_counter_suffix_body`. +/// Implemented as a `scan_preceded` over [`parse_enter_counters_clause_body`] — +/// the scanner advances at word boundaries, so the suffix can appear anywhere +/// after the destination phrase ("onto the battlefield tapped under your control +/// with two additional +1/+1 counters on it") without the caller having to +/// pre-trim. pub(crate) fn parse_with_counters_suffix(lower: &str) -> Vec<(CounterType, QuantityExpr)> { parse_with_counters_suffix_spanned(lower).0 } -/// Like [`parse_with_counters_suffix`], but also reports the byte offset in -/// `lower` at which the matched `"with N counter(s) [on it]"` clause -/// BEGINS (the start of the `"with "` token). Callers that need to excise the -/// consumed counter clause from a larger remainder — e.g. -/// `strip_return_destination_ext_with_remainder` — use this offset to -/// truncate. Returns `None` for the offset when no counter clause matched. +/// Like [`parse_with_counters_suffix`], but also reports the byte range in +/// `lower` that the matched `"with … counter(s) [on it]"` clause occupies — +/// `start` at the `"with "` token, `end` one past the last byte the clause +/// consumed. Returns `None` when no counter clause matched. /// /// CR 122.1: counters are the marker this clause places. /// -/// CONTRACT LIMITATION — read before relying on this. The offset marks only -/// the clause's START, not its end, so a caller that truncates at it also -/// discards everything AFTER the clause. `strip_return_destination_ext_with_remainder` -/// does exactly that: it keeps `text[entry_offset..entry_offset + off]`. -/// -/// That is NOT sound "because counter suffixes are clause-final" — they -/// frequently are not. Two printed counterexamples, both verbatim from -/// `data/mtgjson/AtomicCards.json`: -/// * Heart-Shaped Herb — "… return that card to the battlefield under its -/// owner's control with three +1/+1 counters on it AND YOU BECOME THE -/// MONARCH." -/// * Cosima, God of the Voyage — "… return Cosima to the battlefield with X -/// +1/+1 counters on it AND DRAW X CARDS, where X is the number of voyage -/// counters on it." -/// -/// What makes the truncation safe is UPSTREAM, not the corpus: the chunk-level -/// bare-and splitter `starts_bare_and_clause_lower` (sequence.rs) peels -/// recognized verb-headed tails into their own clause chunks before this -/// function ever sees the remainder. Heart-Shaped Herb's tail is peeled by the -/// `"you become "` arm — pinned end-to-end by -/// `you_become_monarch_conjunct_splits_without_trailing_period` (sequence.rs), -/// which asserts the counter clause and the monarch conjunct land in separate -/// chunks. Cosima's tail heads with `"draw "`, which has its own arm in the -/// same list. A tail whose head verb has NO arm there is still dropped -/// SILENTLY. The fix for the next such card is therefore to add the splitter -/// arm (or widen this to a real start..end span) — never to assume -/// clause-finality. -/// -/// The one corpus invariant that does hold is narrower: no printed Oracle text -/// puts a control/attach/tap clause after the counters. Reproduce it with jq -/// over Oracle text — NOT with a raw `grep -c` on the file, which cannot work: -/// `AtomicCards.json` is a single JSON line (`wc -l` reports 0), so `grep -c` -/// can only ever return 0 or 1, and it matches rulings/flavor/metadata rather -/// than `.text` (the previously cited -/// `grep -cE "counters? (under|attached|tapped)"` returns 1, and its sole hit -/// is ruling text, not a card). -/// -/// ```text -/// jq -r '[.data[][].text // empty -/// | select(test("counters? (under|attached|tapped)"))] | length' \ -/// data/mtgjson/AtomicCards.json # => 0 -/// ``` -/// -/// An earlier version of this comment cited Unstoppable Slasher as a -/// mid-clause example, with the text "return it to the battlefield tapped and -/// with two stun counters under its owner's control". THAT TEXT IS NOT REAL. -/// The printed card reads "…return it to the battlefield tapped under its -/// owner's control with two stun counters on it." The fabricated example -/// propagated into a downstream implementation plan before it was caught; if -/// this contract is ever widened to a full start..end span, verify the -/// motivating card against `data/mtgjson/AtomicCards.json` first (CLAUDE.md, -/// "Verify the card, not just the rule"). +/// The range is a full span, not a bare start offset, precisely so a caller can +/// excise ONLY the clause and keep whatever follows it. Counter clauses are +/// often not clause-final — "…with three +1/+1 counters on it and you become +/// the monarch" (Heart-Shaped Herb), "…with X +1/+1 counters on it and draw X +/// cards" (Cosima, God of the Voyage) — so a caller that truncates at `start` +/// silently discards a printed instruction. Exactly one call site still +/// truncates at `start` (`split_counterless_enter_counters`, mod.rs) and +/// documents in place why its tail is empty by construction. +/// +/// The return-destination path does not use this function at all: it consumes +/// the counter clause as a leading entry rider via +/// [`parse_leading_enter_counters_clause`], which keeps its remainder a true +/// suffix rather than an excised span. pub(crate) fn parse_with_counters_suffix_spanned( lower: &str, -) -> (Vec<(CounterType, QuantityExpr)>, Option) { - nom_primitives::scan_preceded(lower, |i| { - let (i, _) = tag::<_, _, OracleError<'_>>("with ").parse(i)?; - parse_counter_suffix_body_combinator(i) - }) - .map(|(prefix, val, _)| (vec![val], Some(prefix.len()))) - .unwrap_or((Vec::new(), None)) +) -> ( + Vec<(CounterType, QuantityExpr)>, + Option>, +) { + nom_primitives::scan_preceded(lower, parse_enter_counters_clause_body) + .map(|(prefix, counters, rest)| { + let span = prefix.len()..lower.len() - rest.len(); + (counters, Some(span)) + }) + .unwrap_or((Vec::new(), None)) +} + +/// CR 122.1 + CR 614.1c: the body of a `"with …"` enter-with-counters rider — +/// the `"with "` token followed by ONE OR MORE counter clauses conjoined by +/// `" and "`. The conjoined form is printed and load-bearing: +/// * "return target permanent card from your graveyard to the battlefield +/// with a hexproof counter and an indestructible counter on it" +/// (Perennation) +/// * "…return that card to the battlefield under its owner's control with a +/// vigilance counter and a lifelink counter on it" (Gilraen, Dúnedain +/// Protector) +/// * "this creature enters with two +1/+1 counters and a lifelink counter on +/// it" (Dust Animus), "…with two +1/+1 counters and a trample counter on it +/// and with haste" (Voidpouncer) +/// +/// `separated_list1` is the right combinator rather than a hand-rolled loop +/// because nom backtracks the separator when the following element fails, which +/// is what stops the list from swallowing a non-counter conjunct: on "…counters +/// on it and you become the monarch" the `" and "` separator matches but +/// `parse_counter_suffix_body_combinator` rejects "you" at its leading +/// `parse_number`, so the list ends with the separator unconsumed and the +/// monarch instruction stays in the remainder. Same for "and draw X cards" +/// (Cosima) and "and with haste" (Voidpouncer) — every non-counter conjunct is +/// rejected by the element's mandatory leading count/article. +fn parse_enter_counters_clause_body( + input: &str, +) -> OracleResult<'_, Vec<(CounterType, QuantityExpr)>> { + preceded( + tag("with "), + separated_list1(tag(" and "), parse_counter_suffix_body_combinator), + ) + .parse(input) +} + +/// CR 614.1c: the enter-with-counters rider in LEADING position, tolerating the +/// same optional `" and"` / `","` connector that [`parse_one_battlefield_rider`] +/// accepts — the two are sibling entry conditions printed in any order ("to the +/// battlefield tapped and with two stun counters on it"), so they must agree on +/// how conjuncts are joined. +/// +/// Anchoring at the front (rather than scanning, as +/// [`parse_with_counters_suffix_spanned`] does) is what lets +/// `strip_return_destination_ext_with_remainder` CONSUME the clause — advancing +/// its entry offset past it — instead of cutting a span out of the middle of the +/// remainder. A consumed prefix leaves the remainder a genuine suffix slice, so +/// any instruction printed after the entry clauses stays reachable. +fn parse_leading_enter_counters_clause( + input: &str, +) -> OracleResult<'_, Vec<(CounterType, QuantityExpr)>> { + preceded( + (opt(alt((tag(" and"), tag(",")))), tag(" ")), + parse_enter_counters_clause_body, + ) + .parse(input) } /// CR 122.1 + CR 614.1c: Combinator body for "[N|a|an] [additional ] @@ -10956,25 +10979,18 @@ pub(crate) fn parse_counter_suffix_body_combinator( let (rest, _) = tag(" counter").parse(rest)?; // Optional plural "s". let (rest, _) = nom::combinator::opt(tag::<_, _, OracleError<'_>>("s")).parse(rest)?; - // CR 614.1c: "on it" is grammatical filler and BOTH spellings are printed, - // so the terminator must be optional. Filler PRESENT: "…with two stun - // counters on it." (Unstoppable Slasher), "…with a +1/+1 counter on it." - // Filler ABSENT: "…with six +1/+1 counters.", "…with a first strike - // counter.", "…with three dread counters." Optional so both shapes lift - // the counters onto `enter_with_counters`. - // - // NOTE: this comment previously justified the optional filler with - // "absent when a controller clause follows", attributed to Unstoppable - // Slasher. That attribution was fabricated — Unstoppable Slasher reads - // "…return it to the battlefield tapped under its owner's control with two - // stun counters on it.", i.e. filler PRESENT. Presence or absence of the - // filler does NOT correlate with what follows the clause; it is free - // variation in the printed wording, which is why the terminator is - // `opt` rather than conditioned on a lookahead. (No printed card does put - // a control/attach/tap clause after the counters — see the CONTRACT - // LIMITATION note on `parse_with_counters_suffix_spanned` for the jq that - // reproduces that, and for why the truncation's real safety argument is - // upstream rather than corpus-based.) + // "on it" is grammatical filler with no rules content — BOTH spellings are + // printed, so the terminator is `opt` rather than required. Filler PRESENT: + // "…with two stun counters on it." (Unstoppable Slasher), "…with a +1/+1 + // counter on it." Filler ABSENT: "…with a vigilance counter and a lifelink + // counter on it." (Gilraen, Dúnedain Protector — the filler closes the + // conjoined list, so the non-final element carries none), "…with two +1/+1 + // counters and a lifelink counter on it." (Dust Animus). This is an + // observation about printed wording, not a rule; no CR section governs it, + // which is why none is cited here. The rules content of the clause — that + // an "enters with N counters" instruction is a replacement effect applied + // as the object enters — is CR 614.1c, and it is annotated where the + // counters are applied, not on this grammar terminator. let (rest, _) = nom::combinator::opt(tag::<_, _, OracleError<'_>>(" on it")).parse(rest)?; Ok(( @@ -11023,11 +11039,12 @@ pub(crate) fn parse_dynamic_counter_suffix_body( mod tests { use super::{ match_create_of_those_tokens, nest_whenever_this_turn_token_cleanup_delayed_trigger, - parse_where_x_quantity_expression, patch_choose_from_zone_counter_continuation_target, - relink_gated_token_referent_consumers, strip_redundant_flip_win_quantifier, - strip_return_destination_ext_with_remainder, strip_temporal_prefix, strip_temporal_suffix, - strip_trailing_duration, strip_trailing_where_x, - value_quantity_clause_owns_this_turn_suffix, + parse_enter_counters_clause_body, parse_where_x_quantity_expression, + patch_choose_from_zone_counter_continuation_target, relink_gated_token_referent_consumers, + strip_redundant_flip_win_quantifier, strip_return_destination_ext_with_remainder, + strip_temporal_prefix, strip_temporal_suffix, strip_trailing_duration, + strip_trailing_where_x, value_quantity_clause_owns_this_turn_suffix, + ControlClausePossessor, }; use crate::parser::oracle_util::TextPair; use crate::types::ability::{ @@ -11037,6 +11054,7 @@ mod tests { TargetFilter, TriggerDefinition, }; use crate::types::counter::CounterType; + use crate::types::keywords::KeywordKind; use crate::types::phase::Phase; use crate::types::triggers::{PlaneswalkRole, TriggerMode}; use crate::types::zones::Zone; @@ -11786,18 +11804,24 @@ mod tests { )); } - /// CR 614.1c + issue #1498: a counter clause with no `" on it"` filler must - /// still lift its counters onto `enter_with_counters` and be excised from - /// the returned remainder, so no dangling follow-up clause is re-parsed. + /// CR 614.1c (:3062) + CR 110.2a (:618) + issue #1498: a counter clause with + /// no `" on it"` filler must lift its counters onto `enter_with_counters`, + /// and — the discriminating half — whatever is printed AFTER it must survive + /// and reach the normal entry-clause path rather than being truncated away. + /// Here the trailing "under its owner's control" must land on + /// `dest.control`; under the old start-offset truncation it was discarded + /// outright, so `control` came back `None` and this test fails if that + /// behavior returns. /// - /// SYNTHETIC INPUT — not a printed card. This text was previously - /// attributed to Unstoppable Slasher; that attribution was fabricated. The - /// real card reads "…return it to the battlefield tapped under its owner's - /// control with two stun counters on it." (filler present, clause-final). - /// The filler-less shape this test pins IS real, but only clause-finally - /// ("… with six +1/+1 counters."); the trailing controller clause here is - /// an artificial stress case for the excision path. See the CONTRACT - /// LIMITATION note on `parse_with_counters_suffix_spanned`. + /// SYNTHETIC INPUT — not a printed card. This text was once attributed to + /// Unstoppable Slasher; that attribution was fabricated. The real card reads + /// "…return it to the battlefield tapped under its owner's control with two + /// stun counters on it." (filler present, clause-final). The filler-less + /// shape this test pins IS real, but only clause-finally ("…with a vigilance + /// counter and a lifelink counter on it", where the non-final element + /// carries no filler); the trailing controller clause is an artificial + /// stress case for the consumption path, kept because no printed card + /// exercises an entry clause after the counters. #[test] fn return_to_battlefield_lifts_stun_counters_without_on_it_filler() { let (target, dest, remainder) = strip_return_destination_ext_with_remainder( @@ -11811,14 +11835,122 @@ mod tests { dest.enter_with_counters, vec![(CounterType::Stun, QuantityExpr::Fixed { value: 2 })] ); - // The counter clause (and its leading " and" connector) is excised, so - // nothing dangling remains to be re-parsed as a follow-up clause. + // DISCRIMINATING: the clause printed after the counters is consumed as + // an entry clause, not dropped. Truncating at the counter clause's start + // offset (the previous behavior) left this `None`. + assert_eq!( + dest.control, + Some(ControlClausePossessor::Owner), + "the control clause printed after the counters must survive, got {:?}", + dest.control + ); assert_eq!( remainder, "", - "the counter clause must be excised from the remainder, got {remainder:?}" + "every entry clause is consumed, so nothing dangles, got {remainder:?}" ); } + /// CR 725.1 (:6240) + CR 608.2c (:2795) + CR 122.1 (:1178): Heart-Shaped + /// Herb. An instruction printed after the counter clause is NOT part of the + /// destination and must be handed back as the remainder for normal clause + /// processing. This is the unit-level discriminator for the bug the PR + /// fixes: the old start-offset truncation returned "" here, so the monarch + /// instruction never reached a dispatcher. + /// + /// The full-sentence form is split upstream by `starts_bare_and_clause` + /// (sequence.rs) before this function sees it; this test pins the seam + /// itself so the destination parser stops depending on that split for + /// correctness. + #[test] + fn return_to_battlefield_keeps_instruction_printed_after_counters() { + let (target, dest, remainder) = strip_return_destination_ext_with_remainder( + "that card to the battlefield under its owner's control with three +1/+1 counters on it and you become the monarch", + ); + assert_eq!(target, "that card"); + let dest = dest.expect("expected a battlefield return destination"); + assert_eq!(dest.zone, Zone::Battlefield); + assert_eq!(dest.control, Some(ControlClausePossessor::Owner)); + assert_eq!( + dest.enter_with_counters, + vec![(CounterType::Plus1Plus1, QuantityExpr::Fixed { value: 3 })] + ); + // DISCRIMINATING: `" and you become the monarch"` is not a counter, a + // rider or a control clause, so it must come back untouched. + assert_eq!( + remainder, " and you become the monarch", + "the trailing instruction must survive the counter-clause consumption, got {remainder:?}" + ); + } + + /// CR 122.1 (:1178): conjoined counter clauses inside ONE "with …" rider. + /// Verbatim Oracle text of Perennation; Gilraen, Dúnedain Protector prints + /// the same shape after a control clause. Parsing only the first conjunct + /// (the previous behavior) silently dropped the second counter. + #[test] + fn return_to_battlefield_lifts_conjoined_counter_clauses() { + let (_, dest, remainder) = strip_return_destination_ext_with_remainder( + "target permanent card from your graveyard to the battlefield with a hexproof counter and an indestructible counter on it", + ); + let dest = dest.expect("expected a battlefield return destination"); + assert_eq!( + dest.enter_with_counters, + vec![ + ( + CounterType::Keyword(KeywordKind::Hexproof), + QuantityExpr::Fixed { value: 1 } + ), + ( + CounterType::Keyword(KeywordKind::Indestructible), + QuantityExpr::Fixed { value: 1 } + ), + ], + "both conjuncts of the counter rider must be lifted" + ); + assert_eq!(remainder, ""); + } + + /// The conjoined-counter list must NOT swallow a non-counter conjunct: nom + /// backtracks the `" and "` separator when the element parser rejects the + /// text after it, because every element must open with a count or article. + /// Pins the boundary against the conjunct shapes printed after a counter + /// rider — a subject+predicate ("and you become the monarch", + /// Heart-Shaped Herb), an imperative verb ("and draw two cards", the shape + /// Cosima, God of the Voyage prints with an X count) and a second `"with"` + /// rider ("and with haste", Voidpouncer). + /// + /// Cosima's literal `"with X +1/+1 counters"` is deliberately NOT used here: + /// `parse_counter_suffix_body_combinator` opens on + /// `nom_primitives::parse_number`, which accepts digits and English number + /// words but not `"x"`, so an X-counted rider never reaches this list at + /// all. That is a pre-existing gap in the count axis, unrelated to the + /// conjunct boundary this test pins. + #[test] + fn counter_clause_list_stops_at_non_counter_conjunct() { + for (input, expected_rest) in [ + ( + "with three +1/+1 counters on it and you become the monarch", + " and you become the monarch", + ), + ( + "with two +1/+1 counters on it and draw two cards", + " and draw two cards", + ), + // Voidpouncer: a second "with" rider, not a second counter. + ( + "with two +1/+1 counters and a trample counter on it and with haste", + " and with haste", + ), + ] { + let (rest, counters) = + parse_enter_counters_clause_body(input).expect("counter rider must parse"); + assert_eq!(rest, expected_rest, "wrong stop point for {input:?}"); + assert!( + !counters.is_empty(), + "reach guard: the rider itself must have parsed for {input:?}" + ); + } + } + fn variable_x() -> QuantityExpr { QuantityExpr::Ref { qty: QuantityRef::Variable { diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index 5a5c836858..6d3e812348 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -35672,9 +35672,15 @@ pub(super) fn split_counterless_enter_counters( ) { return (clause, Vec::new()); } - let (counters, offset) = parse_with_counters_suffix_spanned(&lower); - match offset { - Some(off) if !counters.is_empty() => (clause[..off].trim_end(), counters), + let (counters, span) = parse_with_counters_suffix_spanned(&lower); + match span { + // Truncating at the span's START (rather than excising `start..end`) is + // deliberate here and loses nothing: `clause` is the TARGET text of an + // exile whose descriptive target was drawn from a counterless origin + // zone, and the counter rider is the last thing printed in it. Anything + // after the rider would belong to the following instruction, which the + // clause splitter has already peeled off before this runs. + Some(span) if !counters.is_empty() => (clause[..span.start].trim_end(), counters), _ => (clause, Vec::new()), } } diff --git a/crates/engine/src/parser/oracle_effect/sequence.rs b/crates/engine/src/parser/oracle_effect/sequence.rs index 174369e633..0814f13bdd 100644 --- a/crates/engine/src/parser/oracle_effect/sequence.rs +++ b/crates/engine/src/parser/oracle_effect/sequence.rs @@ -2775,11 +2775,14 @@ fn starts_bare_and_clause_lower(s: &str) -> bool { // Exactly two lines in the whole corpus contain " and you become " — // both were broken, in different ways: // Heart-Shaped Herb: "…with three +1/+1 counters on it AND YOU BECOME - // THE MONARCH" — dropped SILENTLY (reported supported, zero gaps), - // because the return-destination counter-suffix truncation in - // `strip_return_destination_ext_with_remainder` (lower.rs) cuts the - // remainder at the counter clause's START offset, discarding the - // tail before any guard could see it. + // THE MONARCH" — dropped SILENTLY (reported supported, zero gaps). + // It failed at TWO seams: the return-destination counter suffix in + // `strip_return_destination_ext_with_remainder` (lower.rs) used to + // truncate its remainder at the counter clause's START offset, + // discarding the tail before any guard could see it (that seam now + // CONSUMES the clause as a leading entry rider, so the tail + // survives), and this splitter had no arm to peel the tail into its + // own chunk once it did. // Fall from Favor: "tap enchanted creature AND YOU BECOME THE // MONARCH" — isolated by `try_split_targeted_compound` (mod.rs) but // dispatched through `parse_imperative_effect`, which never tries diff --git a/crates/engine/src/parser/oracle_tests.rs b/crates/engine/src/parser/oracle_tests.rs index 9a0a320914..4878f3f6af 100644 --- a/crates/engine/src/parser/oracle_tests.rs +++ b/crates/engine/src/parser/oracle_tests.rs @@ -13106,12 +13106,13 @@ fn monarch_chain_has_unimplemented(def: &AbilityDefinition) -> bool { } /// CR 725.1 + CR 608.2c: Heart-Shaped Herb's activated ability ends with -/// "… with three +1/+1 counters on it and you become the monarch". Before the -/// `"you become "` bare-and splitter arm, the trailing conjunct was dropped -/// SILENTLY — the return-destination counter-suffix truncation in -/// `strip_return_destination_ext_with_remainder` (lower.rs) cut the remainder -/// at the counter clause's start offset, so the card reported as fully -/// supported with zero gaps while discarding a printed instruction. +/// "… with three +1/+1 counters on it and you become the monarch". The trailing +/// conjunct was dropped SILENTLY — the card reported as fully supported with +/// zero gaps while discarding a printed instruction — because both seams it +/// crosses were broken: `strip_return_destination_ext_with_remainder` (lower.rs) +/// truncated its remainder at the counter clause's start offset, and the +/// chunk-level bare-and splitter had no `"you become "` arm to peel the tail +/// into its own clause even once it survived. /// /// The monarch grant must land NESTED under the `EffectOutcome`-gated /// `ChangeZone`, not as a sibling of the `Sacrifice`: CR 608.2c means diff --git a/crates/engine/tests/integration/heart_shaped_herb_monarch.rs b/crates/engine/tests/integration/heart_shaped_herb_monarch.rs index 28dbdf026d..d940196d56 100644 --- a/crates/engine/tests/integration/heart_shaped_herb_monarch.rs +++ b/crates/engine/tests/integration/heart_shaped_herb_monarch.rs @@ -8,12 +8,19 @@ //! do, return that card to the battlefield under its owner's control with //! three +1/+1 counters on it and you become the monarch. //! -//! Before the `"you become "` bare-and splitter arm (parser/oracle_effect/ -//! sequence.rs), the final conjunct was dropped SILENTLY: the return-destination -//! counter-suffix truncation in `strip_return_destination_ext_with_remainder` -//! (lower.rs) cut the remainder at the counter clause's START offset, discarding -//! the tail. The card reported as fully supported with zero coverage gaps while -//! never granting the monarch. +//! The conjunct was dropped SILENTLY — the card reported as fully supported +//! with zero coverage gaps while never granting the monarch — because BOTH +//! seams it had to cross were broken: +//! 1. `strip_return_destination_ext_with_remainder` (lower.rs) truncated its +//! remainder at the counter clause's START offset, discarding everything +//! printed after it. It now CONSUMES the counter clause as a leading entry +//! rider, so the remainder stays a true suffix. +//! 2. The chunk-level bare-and splitter `starts_bare_and_clause_lower` +//! (sequence.rs) had no `"you become "` arm, so even an intact tail was +//! not peeled into its own clause. +//! +//! Either fix alone leaves the card silent; the tests below pin the runtime +//! behavior that requires both. //! //! Every test here drives the real `apply()` pipeline (GameScenario + //! GameRunner::activate + CR 602 announce/pay/resolve) and asserts measured From 31d1ea4b73a0a2030bb523ae591fb82ce67e73af Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 20:53:31 -0500 Subject: [PATCH 3/6] docs(parser): correct the counter-rider card claims against the CI parse diff Comment-only. Two corrections and one review nit, no behavior change. The parse diff for aca23c6a reports 4 cards / 4 signatures: Fall from Favor and Heart-Shaped Herb gain BecomeMonarch, Fall from Favor drops its Effect:you marker, and Perennation + Gilraen, Dunedain Protector gain their second conjoined counter. Dust Animus and Voidpouncer do NOT appear, so the claim that the new list combinator also serves them was wrong: those cards print the same conjoined grammar at the self-referential "enters with" seam, which calls parse_counter_suffix_body_combinator directly rather than through parse_enter_counters_clause_body, and still lifts only the first conjunct. The doc now says so explicitly and names routing that seam as the follow-up, so the next reader does not assume coverage the parse diff does not show. Also renames a stale reference to counters_offset in the exile path, which the Range change in aca23c6a left behind (CodeRabbit nit). Verified: cargo fmt --all; cargo clippy -p phase-engine --all-targets -D warnings (exit 0). Co-Authored-By: Claude Opus 5 --- crates/engine/src/parser/oracle_effect/imperative.rs | 2 +- crates/engine/src/parser/oracle_effect/lower.rs | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 4710ab8014..63ddda1401 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -9004,7 +9004,7 @@ pub(super) fn parse_exile_ast( // carry a "with N counters" FILTER — that reading only applies to // descriptive targets like "exile each creature with a +1/+1 counter on // it"), recover the enter-with-counters suffix from the full clause. The - // `rem` is already empty in this case, so `counters_offset` stays `None`. + // `rem` is already empty in this case, so `counters_span` stays `None`. if enter_with_counters.is_empty() && matches!( parsed_target, diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index d1c133b88d..4f8beb53a4 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -10861,9 +10861,14 @@ pub(crate) fn parse_with_counters_suffix_spanned( /// * "…return that card to the battlefield under its owner's control with a /// vigilance counter and a lifelink counter on it" (Gilraen, Dúnedain /// Protector) -/// * "this creature enters with two +1/+1 counters and a lifelink counter on -/// it" (Dust Animus), "…with two +1/+1 counters and a trample counter on it -/// and with haste" (Voidpouncer) +/// +/// Those two are the cards this combinator actually reaches, confirmed against +/// the CI parse diff. The self-referential "…enters with two +1/+1 counters and +/// a lifelink counter on it" shape (Dust Animus, Voidpouncer) prints the SAME +/// conjoined grammar but is parsed at a different seam that calls +/// `parse_counter_suffix_body_combinator` directly rather than through this +/// list, so it still lifts only the first conjunct. Routing that seam through +/// here is the follow-up; do not assume this function already covers it. /// /// `separated_list1` is the right combinator rather than a hand-rolled loop /// because nom backtracks the separator when the following element fails, which From b09cc528a7952a530d2c7e734e9c2e8ebf268cad Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 14 Aug 2026 19:07:37 -0700 Subject: [PATCH 4/6] fix(PR-7402): correct counter-entry CR annotations Co-authored-by: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> --- crates/engine/src/parser/oracle_effect/lower.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 4f8beb53a4..84d83768fa 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -7123,8 +7123,9 @@ pub(super) fn strip_return_destination_ext_with_remainder( // rows, whose `control` is always `None` per CR 110.1 @ :614). // `*row_control` is a `Copy` read out of the `&'static` table row. let mut control: Option = *row_control; - // CR 614.1c (:3062) + CR 508.4 (:2312) + CR 708.3 (:5723) + CR - // 110.2a (:618) + CR 122.1 (:1178): battlefield-entry riders, the control + // CR 122.6 (:1208): putting counters on an object includes giving + // counters to it as it enters the battlefield. Battlefield-entry + // riders, the control // clause and the "with … counter(s)" clause are INDEPENDENT entry // conditions printed in any order ("under your control face down and // tapped", "tapped and with two stun counters on it"). Consume them @@ -10852,7 +10853,8 @@ pub(crate) fn parse_with_counters_suffix_spanned( .unwrap_or((Vec::new(), None)) } -/// CR 122.1 + CR 614.1c: the body of a `"with …"` enter-with-counters rider — +/// CR 122.6: the body of a `"with …"` rider that gives counters as an object +/// enters the battlefield — /// the `"with "` token followed by ONE OR MORE counter clauses conjoined by /// `" and "`. The conjoined form is printed and load-bearing: /// * "return target permanent card from your graveyard to the battlefield @@ -10889,7 +10891,7 @@ fn parse_enter_counters_clause_body( .parse(input) } -/// CR 614.1c: the enter-with-counters rider in LEADING position, tolerating the +/// CR 122.6: the enter-with-counters rider in LEADING position, tolerating the /// same optional `" and"` / `","` connector that [`parse_one_battlefield_rider`] /// accepts — the two are sibling entry conditions printed in any order ("to the /// battlefield tapped and with two stun counters on it"), so they must agree on @@ -10911,8 +10913,8 @@ fn parse_leading_enter_counters_clause( .parse(input) } -/// CR 122.1 + CR 614.1c: Combinator body for "[N|a|an] [additional ] -/// counter(s) on it". Used by `parse_with_counters_suffix` AND by the exile- +/// Combinator body for "[N|a|an] [additional ] counter(s) on it". Used by +/// `parse_with_counters_suffix` AND by the exile- /// anaphor counter clause in `oracle_replacement.rs` so both paths share the /// same grammar. /// From 9d89ab1d0dbdf69a919f09ecf808b1ad0ef3626b Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 14 Aug 2026 20:04:50 -0700 Subject: [PATCH 5/6] fix(PR-7402): correct remaining counter CR citations Co-authored-by: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> --- crates/engine/src/parser/oracle_effect/lower.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 84d83768fa..0936453edc 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -10794,7 +10794,7 @@ fn parse_signed_pt_component(text: &str) -> Option { Some(PtValue::Fixed(sign * value)) } -/// CR 122.1 + CR 614.1c: Scan a remainder for a "with [N] [type] counter(s) on +/// CR 122.6: Scan a remainder for a "with [N] [type] counter(s) on /// it" suffix and lift the matched counter type + count into a /// `Vec<(CounterType, QuantityExpr)>` slot for `Effect::ChangeZone.enter_with_counters`. /// @@ -11811,7 +11811,7 @@ mod tests { )); } - /// CR 614.1c (:3062) + CR 110.2a (:618) + issue #1498: a counter clause with + /// CR 122.6 (:1208) + CR 110.2a (:618) + issue #1498: a counter clause with /// no `" on it"` filler must lift its counters onto `enter_with_counters`, /// and — the discriminating half — whatever is printed AFTER it must survive /// and reach the normal entry-clause path rather than being truncated away. From 647490b3f4c9928cc37805615b0be55cfbcb02f3 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 14 Aug 2026 21:06:12 -0700 Subject: [PATCH 6/6] fix(PR-7402): scope shared counter grammar annotations Co-authored-by: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> --- .../engine/src/parser/oracle_effect/lower.rs | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/lower.rs b/crates/engine/src/parser/oracle_effect/lower.rs index 0936453edc..4e1bb0ca47 100644 --- a/crates/engine/src/parser/oracle_effect/lower.rs +++ b/crates/engine/src/parser/oracle_effect/lower.rs @@ -10994,10 +10994,9 @@ pub(crate) fn parse_counter_suffix_body_combinator( // conjoined list, so the non-final element carries none), "…with two +1/+1 // counters and a lifelink counter on it." (Dust Animus). This is an // observation about printed wording, not a rule; no CR section governs it, - // which is why none is cited here. The rules content of the clause — that - // an "enters with N counters" instruction is a replacement effect applied - // as the object enters — is CR 614.1c, and it is annotated where the - // counters are applied, not on this grammar terminator. + // which is why none is cited here. When a caller gives counters as an object + // enters the battlefield, CR 122.6 describes that effect; this shared + // grammar does not determine whether its caller is an entry clause. let (rest, _) = nom::combinator::opt(tag::<_, _, OracleError<'_>>(" on it")).parse(rest)?; Ok(( @@ -11011,16 +11010,12 @@ pub(crate) fn parse_counter_suffix_body_combinator( )) } -/// CR 122.1 + CR 614.1c: "a number of counter(s) on it equal to -/// " — dynamic counter count for "enters with counters" clauses -/// (e.g. The Eleventh Doctor: "with a number of time counters on it equal to -/// its mana value") AND for the post-token "create a … token and put[s] a -/// number of counters on it equal to " form (Oversimplify, -/// Fractal Anomaly class). Delegates the quantity to the shared -/// `parse_cda_quantity` building block so any " a number of X -/// counters … equal to …" card parses composed dynamic quantities -/// (twice/half/aggregate/difference), not just bare refs. CR 614.1c is the -/// authorizing rule for "enters with counters" replacement effects. +/// Parses "a number of counter(s) on it equal to " dynamic +/// counts for entry-counter clauses (e.g. The Eleventh Doctor) and post-token +/// counter effects (Oversimplify, Fractal Anomaly class). Delegates the quantity +/// to the shared `parse_cda_quantity` building block so any " a number of +/// X counters … equal to …" card parses composed dynamic quantities +/// (twice/half/aggregate/difference), not just bare refs. pub(crate) fn parse_dynamic_counter_suffix_body( input: &str, ) -> nom::IResult<&str, (CounterType, QuantityExpr), OracleError<'_>> { @@ -11790,7 +11785,7 @@ mod tests { )); } - /// CR 614.1c: dynamic enter-with-counters suffix accepts composed quantities. + /// The shared dynamic counter grammar accepts composed quantities. #[test] fn dynamic_counter_suffix_parses_aggregate_equal_to() { use super::parse_dynamic_counter_suffix_body;