From 290d6de1d0302e05451705ce2ebb07bd32eba433 Mon Sep 17 00:00:00 2001 From: Rajah James Date: Thu, 9 Jul 2026 17:15:46 -0400 Subject: [PATCH 1/3] fix(parser): recognize "the active player has controlled continuously since the beginning of the turn" target clause (Nettling Imp, Norritt, Arcum's Whistle) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR 102.1 + CR 302.6 + CR 508.1a: the target-selection relative clause was silently dropped, over-broadening the target filter to any non-Wall creature instead of one continuously controlled by the active player. Extends parse_ownership_or_controller_suffix with a new arm reusing ControllerRef::ActivePlayer and FilterProp::ControlledContinuouslySinceTurnBegan verbatim — both pre-existing and already wired through runtime evaluation (game/filter.rs). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --- crates/engine/src/game/ability_utils.rs | 72 +++++++++++++++++++++++ crates/engine/src/parser/oracle_target.rs | 47 +++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/crates/engine/src/game/ability_utils.rs b/crates/engine/src/game/ability_utils.rs index 2e975479f3..523e37243c 100644 --- a/crates/engine/src/game/ability_utils.rs +++ b/crates/engine/src/game/ability_utils.rs @@ -9127,6 +9127,78 @@ mod tests { assert!(matches!(err, EngineError::ActionNotAllowed(_))); } + // Nettling Imp / Norritt / Arcum's Whistle class: "target creature the active + // player has controlled continuously since the beginning of the turn". Proves + // BOTH predicates gate legality independently — the active-player controller + // scope AND the continuity flag — via three fixtures, only one of which is a + // legal target. Positive full-set assertion (not a bare negative) so neither + // hostile fixture can be excluded vacuously by an upstream short-circuit. + #[test] + fn build_target_slots_active_player_controlled_continuously_since_turn_began() { + use crate::types::ability::{ControllerRef, FilterProp}; + use crate::types::zones::Zone; + + let mut state = GameState::new_two_player(42); + // Active player (CR 102.1) is PlayerId(0) at game start. + assert_eq!(state.active_player, PlayerId(0)); + + // Legal: active player's control, no summoning sickness (create_object + // does not set the flag — a "pre-existing" battlefield creature). + let continuous_active = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Continuous Active".to_string(), + Zone::Battlefield, + ); + // Illegal via continuity: active player's control, but summoning-sick. + let fresh_active = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Fresh Active".to_string(), + Zone::Battlefield, + ); + // Illegal via controller: continuous control, but by the non-active player. + let continuous_nonactive = create_object( + &mut state, + CardId(3), + PlayerId(1), + "Continuous Nonactive".to_string(), + Zone::Battlefield, + ); + for creature in [continuous_active, fresh_active, continuous_nonactive] { + state + .objects + .get_mut(&creature) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); + } + state.objects.get_mut(&fresh_active).unwrap().summoning_sick = true; + + let ability = ResolvedAbility::new( + Effect::TargetOnly { + target: TargetFilter::Typed( + TypedFilter::creature() + .controller(ControllerRef::ActivePlayer) + .properties(vec![FilterProp::ControlledContinuouslySinceTurnBegan]), + ), + }, + vec![], + ObjectId(900), + PlayerId(0), + ); + + let slots = build_target_slots(&state, &ability).expect("target slots should build"); + assert_eq!(slots.len(), 1); + assert_eq!( + slots[0].legal_targets, + vec![TargetRef::Object(continuous_active)] + ); + } + #[test] fn build_target_slots_uses_prior_player_targets_for_relative_controller_filters() { use crate::types::ability::ControllerRef; diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index 87985af567..84d3a26c23 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -5434,6 +5434,33 @@ fn parse_ownership_or_controller_suffix( } return own_ctrl_offset + (own_ctrl.len() - rest.len()); } + // CR 102.1 + CR 302.6 + CR 508.1a: "the active player has controlled + // continuously since the beginning of the turn" is a target-selection + // relative clause (Nettling Imp / Norritt / Arcum's Whistle) — distinct + // from `parse_controller_suffix`'s past-tense LOOK-BACK arm ("the active + // player controlled", CR 608.2i, used by look-back aggregates over + // objects that may have since left the battlefield). This clause instead + // restricts a LIVE battlefield target: it must both (a) currently be + // controlled by the active player and (b) have been so controlled + // without interruption since that player's turn began — the same + // continuity test CR 508.1a uses to gate which creatures may attack. + // Both facts are pushed together: `*controller` pins the live + // ActivePlayer scope, and `FilterProp::ControlledContinuouslySinceTurnBegan` + // pins the continuity predicate (runtime-evaluated in game/filter.rs as + // `!obj.summoning_sick`, CR 302.6's summoning-sickness flag). Only the + // ActivePlayer subject is recognized — no card in the corpus was found + // using a "you've controlled/you have controlled continuously..." form + // for this clause, so that variant is not built ahead of a card that + // needs it. + if let Ok((rest, _)) = tag::<_, _, OracleError<'_>>( + "the active player has controlled continuously since the beginning of the turn", + ) + .parse(own_ctrl) + { + *controller = Some(ControllerRef::ActivePlayer); + properties.push(FilterProp::ControlledContinuouslySinceTurnBegan); + return own_ctrl_offset + (own_ctrl.len() - rest.len()); + } let (ctrl, ctrl_len) = parse_controller_suffix(text, ctx).map_or((None, 0), |(ctrl, len)| (Some(ctrl), len)); @@ -7991,6 +8018,26 @@ mod tests { assert_eq!(rest, ""); } + // Nettling Imp / Norritt / Arcum's Whistle class: verbatim clause from + // Nettling Imp's real Oracle text. Building-block test — isolates the new + // controller+continuity arm from the pre-existing "non-Wall" type-filter + // handling (already covered elsewhere). + #[test] + fn parse_target_active_player_controlled_continuously_since_turn_began() { + let (f, rest) = parse_target( + "target creature the active player has controlled continuously since the beginning of the turn", + ); + assert_eq!( + f, + TargetFilter::Typed( + TypedFilter::creature() + .controller(ControllerRef::ActivePlayer) + .properties(vec![FilterProp::ControlledContinuouslySinceTurnBegan]) + ) + ); + assert_eq!(rest.trim(), ""); + } + #[test] fn saddled_type_phrase_parses_as_target() { // CR 702.171b: a "saddled " selector must carry FilterProp::IsSaddled From 2d7ecccf12055a2dbd536556fffcd08628284ba6 Mon Sep 17 00:00:00 2001 From: Rajah James Date: Thu, 9 Jul 2026 17:15:52 -0400 Subject: [PATCH 2/3] docs: remove Nettling Imp and Norritt from parser-misparse-backlog Both cards are fixed by 97943a468. Arcum's Whistle was never listed in this file (it has an unrelated pay-X-or-else gap on the effect side, separate from the target-filter fix). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --- docs/parser-misparse-backlog.md | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/docs/parser-misparse-backlog.md b/docs/parser-misparse-backlog.md index f2e742e514..56e3e5c07c 100644 --- a/docs/parser-misparse-backlog.md +++ b/docs/parser-misparse-backlog.md @@ -3,8 +3,8 @@ Consolidated from 50 per-batch clustering passes over the whole card database. Synonymous per-batch clusters were merged into canonical root causes, their card lists unioned and deduped, and ranked by total card appearances (largest first). - **Canonical root causes:** 30 -- **Distinct cards implicated:** 4778 -- **Total card appearances across root causes:** 4812 (a card may appear under more than one root cause when it exhibits multiple distinct misparses) +- **Distinct cards implicated:** 4776 +- **Total card appearances across root causes:** 4810 (a card may appear under more than one root cause when it exhibits multiple distinct misparses) This is the prioritized "fix N root causes → unlock M cards" backlog: the top handful of root causes account for the majority of broken cards. @@ -12,7 +12,7 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top | # | Root cause | # cards | Fix hint (where it likely lives) | |---|------------|--------:|----------------------------------| -| 1 | Relative-clause / filter restriction on target dropped | 752 | oracle_target.rs / game/filter.rs — extend TargetFilter property extraction for trailing relative clauses | +| 1 | Relative-clause / filter restriction on target dropped | 750 | oracle_target.rs / game/filter.rs — extend TargetFilter property extraction for trailing relative clauses | | 2 | Dropped intervening-if / gating condition (condition: null) | 606 | oracle_nom/condition.rs parse_inner_condition — trigger/static parsers must delegate condition extraction here | | 3 | Anaphor bound to wrong referent | 404 | oracle_quantity.rs context-ref resolution + game/ability_utils.rs forward_result wiring | | 4 | Conjoined / chained second effect clause dropped | 387 | oracle.rs effect-chain composition — split on 'and'/'then'/sentence boundaries and build sub_ability chain | @@ -47,7 +47,7 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top ## Full card lists per root cause -### 1. Relative-clause / filter restriction on target dropped (752 cards) +### 1. Relative-clause / filter restriction on target dropped (750 cards) **Signature.** TargetFilter/affected emitted with empty or missing properties; a trailing restrictive clause (type, subtype, color, mana value, zone, combat/temporal/control predicate, exclusion) is silently dropped, over-broadening the filter. @@ -496,7 +496,6 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top - Necrotic Plague - Needle Drop - Netcaster Spider -- Nettling Imp - Neural Network - Niambi, Esteemed Speaker - Nick Fury, Agent of S.H.I.E.L.D. @@ -504,7 +503,6 @@ This is the prioritized "fix N root causes → unlock M cards" backlog: the top - Nightshade Seer - Nimble Obstructionist - Noctis, Heir Apparent -- Norritt - Not of This World - Oath of Druids - Oath of Ghouls From d2b998be5f80cc83be5d10b134e734c0c3ed4431 Mon Sep 17 00:00:00 2001 From: Rajah James Date: Thu, 9 Jul 2026 23:23:56 -0400 Subject: [PATCH 3/3] fix(parser): decompose Nettling Imp continuity-clause tag into composable atoms, add end-to-end regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses two MED findings from maintainer review of PR #5463. Finding 1 (parser architecture): the new arm in parse_ownership_or_controller_suffix matched the entire target-selection relative clause as one verbatim tag, violating the prohibition on verbatim Oracle-text string matching. Decompose it into three composable nom atoms — subject ("the active player"), verb ("has controlled"), and continuity tail ("continuously since the beginning of the turn") — sequenced with the same tuple-`.map().parse()` idiom the "owned by" arm directly above uses, mirroring parse_continuity_exemption_clause (oracle.rs). Behavior is unchanged; the ActivePlayer-only scope stays the sole recognized subject (no "you"-variant alt, decided against in review). Finding 2 (test doesn't cover the regression): the isolated parse_target AST test and the hand-constructed build_target_slots runtime test would both stay green if the parser arm were reverted — the runtime test bypasses the parser entirely. Add a third, complementary end-to-end integration test that parses Nettling Imp's real Oracle text through parse_oracle_text and drives the parser-produced AbilityDefinition through build_resolved_from_def -> build_target_slots against the same three fixtures. If the continuity arm regresses, the parsed filter degrades to non-Wall-creature-only and the summoning-sick / non-active-player fixtures would wrongly become legal, failing the legal_targets assertion. The two pre-existing tests are retained as complementary layers. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --- crates/engine/src/parser/oracle_target.rs | 17 ++- crates/engine/tests/integration/main.rs | 1 + .../nettling_imp_continuity_target.rs | 141 ++++++++++++++++++ 3 files changed, 154 insertions(+), 5 deletions(-) create mode 100644 crates/engine/tests/integration/nettling_imp_continuity_target.rs diff --git a/crates/engine/src/parser/oracle_target.rs b/crates/engine/src/parser/oracle_target.rs index 84d3a26c23..08b2bf1959 100644 --- a/crates/engine/src/parser/oracle_target.rs +++ b/crates/engine/src/parser/oracle_target.rs @@ -5451,12 +5451,19 @@ fn parse_ownership_or_controller_suffix( // ActivePlayer subject is recognized — no card in the corpus was found // using a "you've controlled/you have controlled continuously..." form // for this clause, so that variant is not built ahead of a card that - // needs it. - if let Ok((rest, _)) = tag::<_, _, OracleError<'_>>( - "the active player has controlled continuously since the beginning of the turn", + // needs it. The clause is sequenced from three composable atoms — subject + // ("the active player"), verb ("has controlled"), and continuity tail + // ("continuously since the beginning of the turn") — rather than one + // verbatim tag, mirroring `parse_continuity_exemption_clause` (oracle.rs) + // and the "owned by" tuple idiom immediately above. + let active_player_continuity: nom::IResult<&str, (), OracleError<'_>> = ( + tag("the active player"), + tag(" has controlled"), + tag(" continuously since the beginning of the turn"), ) - .parse(own_ctrl) - { + .map(|_| ()) + .parse(own_ctrl); + if let Ok((rest, ())) = active_player_continuity { *controller = Some(ControllerRef::ActivePlayer); properties.push(FilterProp::ControlledContinuouslySinceTurnBegan); return own_ctrl_offset + (own_ctrl.len() - rest.len()); diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 0e02241b28..63c4c23356 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -531,6 +531,7 @@ mod mycoloth_upkeep_trigger; mod mystic_forge_regression; mod narset_jeskai_waymaster_draw_spells_cast; mod necrodominance_pay_any_life_draw; +mod nettling_imp_continuity_target; mod ninjutsu_cluster; mod nix_counter_no_mana_spent; mod ob_nixilis_captive_kingpin_life_loss; diff --git a/crates/engine/tests/integration/nettling_imp_continuity_target.rs b/crates/engine/tests/integration/nettling_imp_continuity_target.rs new file mode 100644 index 0000000000..44894c6954 --- /dev/null +++ b/crates/engine/tests/integration/nettling_imp_continuity_target.rs @@ -0,0 +1,141 @@ +//! Nettling Imp / Norritt / Arcum's Whistle continuity target clause — PR #5463 +//! fix round, addressing the maintainer's "test doesn't cover the actual +//! regression" finding. +//! +//! The two pre-existing tests are each insufficient alone: +//! * `parse_target_active_player_controlled_continuously_since_turn_began` +//! (oracle_target.rs) only checks the isolated `parse_target` AST shape — it +//! never reaches target selection. +//! * `build_target_slots_active_player_controlled_continuously_since_turn_began` +//! (ability_utils.rs) hand-constructs the `FilterProp`, bypassing the parser +//! entirely — reverting the new parser arm leaves it green. +//! +//! This end-to-end test closes the gap. It parses Nettling Imp's REAL Oracle +//! text through the top-level `parse_oracle_text` entry point and drives the +//! resulting PARSER-PRODUCED `AbilityDefinition` through the production +//! `build_resolved_from_def` -> `build_target_slots` path (the same path used by +//! the live activation flow). If the decomposed continuity arm in +//! `parse_ownership_or_controller_suffix` is reverted or broken, the parsed +//! filter degrades to non-Wall-creature-only (no `ActivePlayer` controller, no +//! `ControlledContinuouslySinceTurnBegan` property), so `build_target_slots` +//! would then wrongly admit the summoning-sick and non-active-player fixtures — +//! failing the exact `legal_targets` assertion below. + +use engine::game::ability_utils::{build_resolved_from_def, build_target_slots}; +use engine::game::zones::create_object; +use engine::parser::oracle::parse_oracle_text; +use engine::types::ability::{ControllerRef, FilterProp, TargetFilter, TargetRef}; +use engine::types::card_type::CoreType; +use engine::types::game_state::GameState; +use engine::types::identifiers::CardId; +use engine::types::zones::Zone; +use engine::types::PlayerId; + +/// Verbatim Scryfall Oracle text for Nettling Imp. +const NETTLING_IMP_ORACLE: &str = "{T}: Choose target non-Wall creature the active player has controlled continuously since the beginning of the turn. That creature attacks this turn if able. Destroy it at the beginning of the next end step if it didn't attack this turn. Activate only during an opponent's turn, before attackers are declared."; + +#[test] +fn nettling_imp_parsed_continuity_filter_restricts_target_slots_end_to_end() { + // --- Parse the real card through the real top-level entry point. --- + let parsed = parse_oracle_text( + NETTLING_IMP_ORACLE, + "Nettling Imp", + &[], + &["Creature".to_string()], + &["Imp".to_string()], + ); + assert_eq!( + parsed.abilities.len(), + 1, + "expected the single {{T}} activated ability" + ); + let ability_def = &parsed.abilities[0]; + + // Positive reach-guard: prove the parse produced the FULL continuity filter + // (active-player controller + continuity property), not the degraded + // non-Wall-creature-only fallback. This is the exact seam the Finding 1 + // decomposition parses; if that arm regresses, both facts vanish here and + // the slot assertion below would no longer be a meaningful discriminator. + let parsed_filter = ability_def + .effect + .target_filter() + .expect("Nettling Imp surfaces a target-selection filter"); + let TargetFilter::Typed(typed) = parsed_filter else { + panic!("expected a Typed target filter, got {parsed_filter:?}"); + }; + assert_eq!( + typed.controller, + Some(ControllerRef::ActivePlayer), + "continuity clause must pin the ActivePlayer controller scope" + ); + assert!( + typed + .properties + .contains(&FilterProp::ControlledContinuouslySinceTurnBegan), + "continuity clause must carry ControlledContinuouslySinceTurnBegan, got {:?}", + typed.properties + ); + + // --- Three fixtures; only one is a legal target. --- + let mut state = GameState::new_two_player(42); + // CR 102.1: the active player is PlayerId(0) at game start. + assert_eq!(state.active_player, PlayerId(0)); + + // Legal: active player's control, no summoning sickness (create_object does + // not set the flag — a "pre-existing" battlefield creature). + let continuous_active = create_object( + &mut state, + CardId(1), + PlayerId(0), + "Continuous Active".to_string(), + Zone::Battlefield, + ); + // Illegal via continuity: active player's control, but summoning-sick. + let fresh_active = create_object( + &mut state, + CardId(2), + PlayerId(0), + "Fresh Active".to_string(), + Zone::Battlefield, + ); + // Illegal via controller: continuous control, but by the non-active player. + let continuous_nonactive = create_object( + &mut state, + CardId(3), + PlayerId(1), + "Continuous Nonactive".to_string(), + Zone::Battlefield, + ); + for creature in [continuous_active, fresh_active, continuous_nonactive] { + state + .objects + .get_mut(&creature) + .unwrap() + .card_types + .core_types + .push(CoreType::Creature); + } + state.objects.get_mut(&fresh_active).unwrap().summoning_sick = true; + + // The Nettling Imp source itself, controlled by the active player. + let imp = create_object( + &mut state, + CardId(4), + PlayerId(0), + "Nettling Imp".to_string(), + Zone::Battlefield, + ); + + // --- Drive the PARSED ability through the production target-slot builder. --- + let resolved = build_resolved_from_def(ability_def, imp, PlayerId(0)); + let slots = build_target_slots(&state, &resolved).expect("target slots should build"); + assert_eq!(slots.len(), 1, "single target selection slot"); + // Revert-failing assertion: with the continuity arm reverted the parsed + // filter is non-Wall-creature-only, so this set would also contain + // `fresh_active` and `continuous_nonactive`. + assert_eq!( + slots[0].legal_targets, + vec![TargetRef::Object(continuous_active)], + "only the continuously-active-player-controlled creature is a legal target" + ); +}