From 95fe53d0f4c2e281d5b1ac09703dc700a9a0a965 Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Fri, 3 Jul 2026 22:49:42 +0800 Subject: [PATCH 1/2] fix(parser): count "for each you control with " (Skycat Sovereign, Aven Gagglemaster) The "for each" quantity grammar had a combinator for the any-controller " on the battlefield with " form (parse_for_each_battlefield_type_with_keyword, backing Radiant, Archangel and Pride of the Clouds) but none for the controller-scoped " you control with " form. The bare parse_for_each_controlled_type arm matched "creature you control" and stranded " with flying" as an unconsumed remainder, so the whole "for each" quantity failed to fully consume and the dependent effect (dynamic pump / life gain / mana) was silently dropped. Add parse_for_each_controlled_type_with_keyword as the "you control" sibling of the existing battlefield-wide combinator, generalized over the full KEYWORDS table (not just flying) and the optional "other"/"another" exclusion prefix, and register it ahead of the bare controlled-type arm so the longer keyword-qualified form is tried first. The population is scoped to the source's controller (ControllerRef::You, CR 109.4). Covers the whole class of " you control with " for-each clauses: Skycat Sovereign ("gets +1/+1 for each other creature you control with flying"), Aven Gagglemaster, Aerial Assault, Alert Heedbonder, and Overgrown Battlement. Model: claude-opus-4-8[1m] (via Claude Code) Co-Authored-By: Claude Opus 4.8 (1M context) --- .../engine/src/parser/oracle_nom/quantity.rs | 129 ++++++++++++++++++ .../engine/src/parser/oracle_static/tests.rs | 46 +++++++ 2 files changed, 175 insertions(+) diff --git a/crates/engine/src/parser/oracle_nom/quantity.rs b/crates/engine/src/parser/oracle_nom/quantity.rs index 0ef3a7beed..69aff45df1 100644 --- a/crates/engine/src/parser/oracle_nom/quantity.rs +++ b/crates/engine/src/parser/oracle_nom/quantity.rs @@ -2965,6 +2965,13 @@ fn parse_for_each_clause_ref_with_they_controller( // "basic land type" is not mis-consumed as a creature/permanent type. // Anaphoric "they control" binds to the iterating/scoped player here. |i| parse_basic_land_types_among_lands_controlled_by_ref(i, they_controller.clone()), + // CR 109.4: "[other] you control with " — the + // controller-scoped sibling of `parse_for_each_battlefield_type_with_keyword`. + // Must precede `parse_for_each_controlled_type`, whose shorter + // " you control" match would otherwise strand " with " as an + // unconsumed remainder (Skycat Sovereign, Aven Gagglemaster, Overgrown + // Battlement). + parse_for_each_controlled_type_with_keyword, parse_for_each_controlled_type, // CR 201.2: "for each [other] named you control" // (Seven Dwarves). The `named X` qualifier sits between the type word @@ -3992,6 +3999,55 @@ fn parse_for_each_battlefield_type_with_keyword(input: &str) -> OracleResult<'_, )) } +/// CR 109.4: Parse "[other] you [already] control with " in a +/// "for each" clause -> a controller-scoped (`ControllerRef::You`) population +/// count of permanents of the given type that have the named keyword, with an +/// optional "other"/"another" exclusion of the source object. +/// +/// Controller-scoped sibling of `parse_for_each_battlefield_type_with_keyword` +/// (the any-controller "on the battlefield with " form): the same +/// trailing `with ` predicate, but the population is restricted to the +/// source's controller via " you control" rather than " on the battlefield" +/// (CR 109.4 — only objects with a controller are counted). Must precede +/// `parse_for_each_controlled_type`, whose bare " you control" arm would +/// otherwise match first and strand " with " as an unconsumed +/// remainder, dropping the quantity. Backs dynamic P/T anthems and per-count +/// effects such as Skycat Sovereign ("~ gets +1/+1 for each other creature you +/// control with flying"), Aven Gagglemaster ("2 life for each creature you +/// control with flying"), and Overgrown Battlement ("{G} for each creature you +/// control with defender"). Generalized over every evergreen keyword via +/// `parse_keyword_name` + `FilterProp::WithKeyword`, so it covers the whole +/// class, not one card. +fn parse_for_each_controlled_type_with_keyword(input: &str) -> OracleResult<'_, QuantityRef> { + let (rest, has_other) = + opt(alt((value((), tag("other ")), value((), tag("another "))))).parse(input)?; + let (rest, tf) = parse_type_filter_word(rest)?; + // Mirror the bare `parse_for_each_controlled_type` controller phrase, + // tolerating the "already" adverb (" you already control with …"). + let (rest, _) = tag(" you").parse(rest)?; + let (rest, _) = opt(tag(" already")).parse(rest)?; + let (rest, _) = tag(" control with ").parse(rest)?; + let (rest, keyword_name) = parse_keyword_name(rest)?; + let keyword: Keyword = keyword_name.parse().unwrap(); + + let mut properties = Vec::new(); + if has_other.is_some() { + properties.push(FilterProp::Another); + } + properties.push(FilterProp::WithKeyword { value: keyword }); + + Ok(( + rest, + QuantityRef::ObjectCount { + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![tf], + controller: Some(ControllerRef::You), + properties, + }), + }, + )) +} + fn parse_for_each_controlled_type(input: &str) -> OracleResult<'_, QuantityRef> { // CR 109.4: Only objects on the stack or on the battlefield have a // controller, so a "you control" count is over battlefield permanents @@ -4427,6 +4483,79 @@ mod tests { } } + /// CR 109.4: controller-scoped "for each [other] you control with + /// " count — the "you control" sibling of + /// `parse_for_each_battlefield_type_with_keyword`, generalized over the + /// KEYWORDS table and the optional "other"/"another" exclusion. Backs Skycat + /// Sovereign, Aven Gagglemaster, and Overgrown Battlement. + #[test] + fn parse_for_each_controlled_type_with_keyword_scoped_count() { + for (clause, other, kw) in [ + ( + "other creature you control with flying", + true, + Keyword::Flying, + ), + ( + "creature you control with vigilance", + false, + Keyword::Vigilance, + ), + ( + "creature you control with defender", + false, + Keyword::Defender, + ), + ] { + let (rest, q) = parse_for_each_clause_ref(clause).unwrap(); + assert_eq!(rest, "", "{clause:?} should fully consume"); + match q { + QuantityRef::ObjectCount { + filter: TargetFilter::Typed(tf), + } => { + assert_eq!( + tf.controller, + Some(ControllerRef::You), + "{clause:?}: scoped to the source's controller" + ); + assert_eq!( + tf.properties.contains(&FilterProp::Another), + other, + "{clause:?}: Another presence must match the other/another prefix" + ); + assert!( + tf.properties + .contains(&FilterProp::WithKeyword { value: kw }), + "{clause:?}: must gate on the named keyword" + ); + } + other => panic!("{clause:?}: expected ObjectCount, got {other:?}"), + } + } + } + + /// CR 109.4: the bare "for each you control" arm still parses without + /// a keyword predicate — the new keyword arm must not shadow it. + #[test] + fn parse_for_each_controlled_type_bare_still_parses() { + let (rest, q) = parse_for_each_clause_ref("creature you control").unwrap(); + assert_eq!(rest, ""); + match q { + QuantityRef::ObjectCount { + filter: TargetFilter::Typed(tf), + } => { + assert_eq!(tf.controller, Some(ControllerRef::You)); + assert!( + !tf.properties + .iter() + .any(|p| matches!(p, FilterProp::WithKeyword { .. })), + "bare arm must not gate on a keyword" + ); + } + other => panic!("expected ObjectCount, got {other:?}"), + } + } + /// CR 604.3 + CR 109.4: opponent-controlled and chosen-player CDA counts. #[test] fn parse_number_of_controlled_type_opponent_and_chosen_player_cda() { diff --git a/crates/engine/src/parser/oracle_static/tests.rs b/crates/engine/src/parser/oracle_static/tests.rs index 777234e9a5..3be49d1a49 100644 --- a/crates/engine/src/parser/oracle_static/tests.rs +++ b/crates/engine/src/parser/oracle_static/tests.rs @@ -23060,3 +23060,49 @@ fn static_self_dynamic_pump_for_each_other_creature_on_battlefield_with_keyword( def.modifications ); } + +/// CR 604.1 + CR 611.3a + CR 613.4c + CR 109.4: Skycat Sovereign — "~ gets +/// +1/+1 for each other creature you control with flying." The controller +/// scope ("you control") and the keyword qualifier ("with flying") both trail +/// the type word — the controller-scoped counterpart of Radiant, Archangel's +/// battlefield-wide pump. The "for each" grammar had a combinator for the +/// any-controller "on the battlefield with " form but none for the +/// "you control with " form, so this dynamic pump previously failed to +/// parse and the whole modification was dropped. +#[test] +fn static_self_dynamic_pump_for_each_other_creature_you_control_with_keyword() { + let def = parse_static_line("~ gets +1/+1 for each other creature you control with flying.") + .expect("Skycat Sovereign's dynamic pump must parse"); + assert_eq!(def.mode, StaticMode::Continuous); + assert_eq!(def.affected, Some(TargetFilter::SelfRef)); + + let expected = QuantityExpr::Ref { + qty: QuantityRef::ObjectCount { + filter: TargetFilter::Typed(TypedFilter { + type_filters: vec![TypeFilter::Creature], + controller: Some(ControllerRef::You), + properties: vec![ + FilterProp::Another, + FilterProp::WithKeyword { + value: Keyword::Flying, + }, + ], + }), + }, + }; + + assert!(def.modifications.iter().any( + |m| matches!(m, ContinuousModification::AddDynamicPower { value } if value == &expected) + )); + assert!(def.modifications.iter().any( + |m| matches!(m, ContinuousModification::AddDynamicToughness { value } if value == &expected) + )); + assert!( + !def.modifications.iter().any(|m| matches!( + m, + ContinuousModification::AddPower { .. } | ContinuousModification::AddToughness { .. } + )), + "must not emit flat P/T modifications alongside dynamic ones: {:?}", + def.modifications + ); +} From 96f65749b8d61e7b82c0244aa5aba4c969e0ed2f Mon Sep 17 00:00:00 2001 From: luciferlive112116 <291889058+luciferlive112116@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:44:24 +0800 Subject: [PATCH 2/2] fix(parser): map keyword name via map_res instead of unwrap (review) Addresses review on #5019 (gemini-code-assist, HIGH): parse_for_each_controlled_type_with_keyword converted the parsed keyword name to `Keyword` with `.parse().unwrap()`, which would panic if a name failed to convert. Route it through `map_res(parse_keyword_name, |s| s.parse::())` so an unconvertible name yields a graceful nom parse error instead. Every `KEYWORDS` entry has a valid `FromStr`, so this is a defensive guard, not an expected failure path. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/engine/src/parser/oracle_nom/quantity.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/parser/oracle_nom/quantity.rs b/crates/engine/src/parser/oracle_nom/quantity.rs index 69aff45df1..6cd6231e52 100644 --- a/crates/engine/src/parser/oracle_nom/quantity.rs +++ b/crates/engine/src/parser/oracle_nom/quantity.rs @@ -7,7 +7,7 @@ use crate::parser::oracle_nom::error::OracleError; use nom::branch::alt; use nom::bytes::complete::{tag, take_until, take_while1}; -use nom::combinator::{all_consuming, map, opt, value}; +use nom::combinator::{all_consuming, map, map_res, opt, value}; use nom::multi::separated_list1; use nom::sequence::{pair, preceded, terminated}; use nom::Parser; @@ -4027,8 +4027,12 @@ fn parse_for_each_controlled_type_with_keyword(input: &str) -> OracleResult<'_, let (rest, _) = tag(" you").parse(rest)?; let (rest, _) = opt(tag(" already")).parse(rest)?; let (rest, _) = tag(" control with ").parse(rest)?; - let (rest, keyword_name) = parse_keyword_name(rest)?; - let keyword: Keyword = keyword_name.parse().unwrap(); + // Map the parsed keyword name through `Keyword`'s `FromStr` with `map_res` + // so an unconvertible name fails the parse (a nom error) rather than + // panicking — every `KEYWORDS` entry converts, so this is a graceful guard, + // not an expected failure path. + let (rest, keyword) = + map_res(parse_keyword_name, |s: &str| s.parse::()).parse(rest)?; let mut properties = Vec::new(); if has_other.is_some() {