From d4f971d4164d0922b6e837960896e066565bb73e Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 07:04:46 -0700 Subject: [PATCH 1/2] test(engine): positively discriminate the assimilate target filter Tests 3a and 3b asserted only that an illegal candidate stayed in its graveyard, which is equally consistent with "the filter rejected it" and "the ETB production never fired at all". Seed a legal AND an illegal candidate in one fixture and declare the illegal one FIRST, so the filter is observable across all three reachable states: correct the legal set is a singleton, so auto_select_targets_for_ability assigns it and no prompt is raised; the legal card enters, the illegal one stays over-match two legal candidates, auto-selection declines, a required slot IS created, and pick_slot_target fills it from the DECLARED order -- taking the bait, flipping both legs under-match empty legal set, so CR 603.3d removes the trigger before any slot exists and the POSITIVE leg fails Declaration order is a regression-only instrument: it converts the over-match case from pick_slot_target's no-declared-target panic into a clean assertion failure. Each test holds two filter legs constant and varies one, so 3a isolates the Creature type leg and 3b the ownership leg (CR 109.4: a graveyard card has no controller, so "an opponent's graveyard" rides as ownership). InZone { Graveyard } is isolated by neither and keeps only its parser-level coverage. Both tests are now revert-failing. 3b previously passed with the lowering reverted, because nothing moved at all, and its doc comment claimed only forward-guard status; that claim is updated rather than left stale. Correct Recruit's mis-cited keyword-action rule. CR 701.9 is Discard; Recruit is CR 701.70a. The "standalone keyword action" comment is purely a 701.70a claim, so it is replaced. The discard-result comment genuinely involves both rules -- the discard is 701.9a, the nonland contingency is 701.70a -- so it gains 701.70a instead. Two further Recruit-tagged 701.9a sites are deliberately left alone: they assert snapshot and provenance semantics already covered by 701.9a + 608.2c + 400.7, and assert nothing that 701.70a supplies. Relocate the parser-only fail-closed test beside its sibling shape test in the parser module, and drop the two imports it was the last user of. Replace the ephemeral BASE_SHA run identifier in three doc comments with a stable PR reference. BASE_SHA is an implementation-run artifact whose meaning changes every run and is undefined to a reader of the repository. --- .../src/parser/oracle_effect/imperative.rs | 4 +- .../engine/src/parser/oracle_effect/tests.rs | 32 ++++- .../integration/borg_queen_assimilate.rs | 125 +++++++++++------- 3 files changed, 111 insertions(+), 50 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/imperative.rs b/crates/engine/src/parser/oracle_effect/imperative.rs index 2eeeca2649..9a202b49c3 100644 --- a/crates/engine/src/parser/oracle_effect/imperative.rs +++ b/crates/engine/src/parser/oracle_effect/imperative.rs @@ -9813,7 +9813,7 @@ pub(super) fn parse_imperative_family_ast( return Some(ast); } - // CR 701.9a + CR 608.2c: Recruit is a standalone keyword action. Keep this + // CR 701.70a + CR 608.2c: Recruit is a standalone keyword action. Keep this // an anchored nom production so `recruiter` and compound text do not become // an accidental Recruit instruction. if all_consuming(terminated( @@ -11942,7 +11942,7 @@ pub(crate) fn try_parse_reflexive_coin_flip_branch<'a>( pub(super) fn lower_imperative_family_ast(ast: ImperativeFamilyAst) -> ParsedEffectClause { match ast { - // CR 701.9a + CR 608.2c: Recruit's contingent token sees exactly the + // CR 701.9a + CR 701.70a + CR 608.2c: Recruit's contingent token sees exactly the // immediately preceding discard result. The typed condition is carried // by the direct child, so no later chain step can consume stale discard // provenance. diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 4148e9b0ee..cc79e977cc 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -52451,8 +52451,9 @@ fn borg_queen_assimilate_lowers_to_reanimate_then_retype_chain() { // (1) POSITIVE REACH-GUARD (mandatory): zero `Effect::Unimplemented` in the // whole chain. Every negative assertion below is non-vacuous only because - // this passes — at BASE_SHA the execute IS an `Unimplemented { name: - // "assimilate" }`, so this is also the revert-failing assertion. + // this passes — with the assimilate lowering (PR #7096) reverted the execute + // IS an `Unimplemented { name: "assimilate" }`, so this is also the + // revert-failing assertion. assert!( !ability_chain_has_unimplemented(execute), "the assimilate trigger must lower with no residual Unimplemented node: {execute:#?}" @@ -52615,3 +52616,30 @@ fn borg_queen_assimilate_lowers_to_reanimate_then_retype_chain() { retype.modifications ); } + +/// 3c. Parser fail-closed: an `assimilate` phrasing whose target is NOT a +/// graveyard card is a shape this production does not model, so it must keep +/// producing `Effect::Unimplemented` and coverage must stay honestly RED rather +/// than be silently lowered into a reanimation. `name` is `"assimilate"` because +/// the imperative fallback derives it from the clause's first word. +/// +/// Paired positive: the real card's phrasing in the same test produces a +/// `ChangeZone`, so the negative is about the graveyard guard and not about a +/// production that never fires. +#[test] +fn assimilate_without_a_graveyard_target_stays_unimplemented() { + let non_graveyard = parse_effect("assimilate target creature you control"); + assert!( + matches!( + &non_graveyard, + Effect::Unimplemented { name, .. } if name == "assimilate" + ), + "a non-graveyard assimilate phrasing must stay honestly unsupported, got {non_graveyard:?}" + ); + + let real = parse_effect("assimilate target creature card from an opponent's graveyard"); + assert!( + matches!(real, Effect::ChangeZone { .. }), + "reach-guard: the printed phrasing must lower to a ChangeZone, got {real:?}" + ); +} diff --git a/crates/engine/tests/integration/borg_queen_assimilate.rs b/crates/engine/tests/integration/borg_queen_assimilate.rs index 538f6ef77e..4131767c1f 100644 --- a/crates/engine/tests/integration/borg_queen_assimilate.rs +++ b/crates/engine/tests/integration/borg_queen_assimilate.rs @@ -40,8 +40,6 @@ use engine::game::layers::evaluate_layers; use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; use engine::game::zones::move_to_zone; -use engine::parser::oracle_effect::parse_effect; -use engine::types::ability::Effect; use engine::types::card_type::{CoreType, Supertype}; use engine::types::counter::CounterType; use engine::types::identifiers::ObjectId; @@ -173,8 +171,9 @@ fn seed_hostile_creature_types(runner: &mut GameRunner) { /// supertype, and non-creature subtype. Plus CR 110.2a's controller override, /// CR 108.3's unchanged ownership, and CR 122.1's entry counter. /// -/// Revert-failing: at BASE_SHA the ETB trigger is `Effect::Unimplemented`, so -/// the victim never leaves the graveyard and reach-guard 2.1 fails first. +/// Revert-failing: with the assimilate lowering (PR #7096) reverted the ETB +/// trigger is `Effect::Unimplemented`, so the victim never leaves the graveyard +/// and reach-guard 2.1 fails first. #[test] fn assimilate_applies_the_full_cr_205_1b_type_change() { let mut scenario = GameScenario::new(); @@ -285,80 +284,114 @@ fn assimilate_applies_the_full_cr_205_1b_type_change() { // Test 3 — fail-closed / negative-sibling coverage // --------------------------------------------------------------------------- -/// 3a. CR 115.2: the filter's `Creature` leg. With only a LAND card in the -/// opponent's graveyard the ETB trigger has no legal target, so nothing enters. -/// Paired positive reach-guard: Borg Queen herself IS on the battlefield, so the -/// cast demonstrably resolved. +/// 3a. CR 115.2: the filter's `Creature` type leg, POSITIVELY discriminated. +/// +/// P1's graveyard holds BOTH an illegal land card and a legal creature card, +/// and the cast declares them IN THAT ORDER — illegal first. The three +/// reachable states are distinct, and the pair below is red in two of them: +/// * CORRECT filter -> the legal set is the singleton {creature}, so +/// `prepare_trigger_targets` (`game/triggers.rs`) auto-assigns it: +/// `ability_utils::auto_select_targets_for_ability` returns the sole +/// assignment and the trigger is `PreparedTriggerTargets::AutoAssigned`. +/// NO prompt is raised, so the declared objects are never consumed here — +/// declaring them is inert in this state, and safe, because nothing checks +/// for unconsumed declarations. The creature enters; the land stays put. +/// * `Creature` leg OVER-matches -> the legal set becomes {creature, land}, +/// auto-selection declines (two assignments), a required slot IS created, +/// and `pick_slot_target` (`game/scenario.rs`) fills it with the FIRST +/// DECLARED legal object — the land. BOTH assertions below flip. +/// * `Creature` leg UNDER-matches to empty -> CR 603.3d removes the trigger +/// before any slot exists (`DroppedNoLegalRequiredTarget`), nothing moves, +/// and the POSITIVE leg below fails. +/// +/// Declaration order is therefore a REGRESSION-ONLY instrument: it makes the +/// over-match case fail cleanly on assertions instead of on `pick_slot_target`'s +/// no-declared-target panic. There is no nondeterminism to design around — the +/// legal candidate is unique in the passing state, and pinned by declaration +/// order in the over-matching one. +/// +/// ISOLATED AXIS: both candidates are cards in the SAME opponent's graveyard, so +/// `Owned { Opponent }` and `InZone { Graveyard }` are held constant and ONLY +/// `type_filters: [Creature]` varies between them. (The ownership leg is +/// isolated by 3b; `InZone { Graveyard }` is isolated by neither.) +/// +/// Revert-failing: with the assimilate lowering (PR #7096) reverted the ETB +/// trigger is `Effect::Unimplemented`, nothing leaves the graveyard, and the +/// battlefield assertion fails first. #[test] -fn assimilate_finds_no_target_when_the_opponent_graveyard_holds_only_a_land() { +fn assimilate_discriminates_a_creature_card_from_a_land_in_the_same_graveyard() { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); + // Declared FIRST so an over-matching `Creature` leg is forced to consume it. let land = scenario.add_land_to_graveyard(P1, "Wastes").id(); + let creature = scenario + .add_creature_to_graveyard(P1, "Graveyard Wizard", 2, 2) + .with_subtypes(vec!["Human", "Wizard"]) + .id(); let borg_queen = borg_queen_in_hand(&mut scenario); let mut runner = scenario.build(); seed_hostile_creature_types(&mut runner); - let outcome = runner.cast(borg_queen).resolve(); + let outcome = runner + .cast(borg_queen) + .target_objects(&[land, creature]) + .resolve(); // Positive reach-guard: the cast resolved. outcome.assert_zone(&[borg_queen], Zone::Battlefield); - // CR 115.2: a land card is not a legal `target creature card`. + // POSITIVE leg (CR 115.2): the creature card IS a legal `target creature + // card` and was taken, so the production demonstrably fired on this fixture. + outcome.assert_zone(&[creature], Zone::Battlefield); + // NEGATIVE leg, paired with the above: a land card is NOT a legal + // `target creature card`, so it was skipped despite being declared first. outcome.assert_zone(&[land], Zone::Graveyard); } -/// 3b. CR 108.3: the `Owned { controller: Opponent }` leg. A creature card in -/// P0's OWN graveyard is not a legal target. +/// 3b. CR 108.3: the `Owned { controller: Opponent }` leg, POSITIVELY +/// discriminated, by the same three-state instrument as 3a (see 3a's comment +/// for the auto-assign / over-match / under-match breakdown). +/// +/// ISOLATED AXIS: both candidates are CREATURE cards in a GRAVEYARD, so +/// `type_filters: [Creature]` and `InZone { Graveyard }` are held constant and +/// ONLY the owner varies (P0's own graveyard vs P1's). CR 109.4: a graveyard +/// card has no controller, so "an opponent's graveyard" rides as OWNERSHIP. /// -/// This case also passes at BASE_SHA (where nothing moves at all), so it is a -/// GUARD against a future filter regression, not a revert-failing test. The -/// paired positive reach-guard keeps it from being vacuous about the cast. +/// Revert-failing (CHANGED by this strengthening): the previous +/// single-candidate form also passed with the assimilate lowering (PR #7096) +/// reverted, because nothing moved at all, so it was only a forward guard. This +/// form asserts that the OPPONENT-owned card reaches the battlefield, so a +/// revert now fails it. #[test] -fn assimilate_cannot_take_a_card_from_its_own_controllers_graveyard() { +fn assimilate_discriminates_an_opponents_graveyard_from_its_own_controllers() { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); + // Declared FIRST so an over-matching ownership leg is forced to consume it. let own_card = scenario .add_creature_to_graveyard(P0, "Own Graveyard Wizard", 2, 2) .with_subtypes(vec!["Human", "Wizard"]) .id(); + let opponent_card = scenario + .add_creature_to_graveyard(P1, "Opponent Graveyard Wizard", 2, 2) + .with_subtypes(vec!["Human", "Wizard"]) + .id(); let borg_queen = borg_queen_in_hand(&mut scenario); let mut runner = scenario.build(); seed_hostile_creature_types(&mut runner); - let outcome = runner.cast(borg_queen).resolve(); + let outcome = runner + .cast(borg_queen) + .target_objects(&[own_card, opponent_card]) + .resolve(); // Positive reach-guard: the cast resolved. outcome.assert_zone(&[borg_queen], Zone::Battlefield); - // CR 108.3: "an opponent's graveyard" restricts by OWNERSHIP. + // POSITIVE leg (CR 108.3): the OPPONENT-owned card is legal and was taken. + outcome.assert_zone(&[opponent_card], Zone::Battlefield); + // NEGATIVE leg, paired with the above: P0's OWN card is not legal and was + // skipped despite being declared first. outcome.assert_zone(&[own_card], Zone::Graveyard); } -/// 3c. Parser fail-closed: an `assimilate` phrasing whose target is NOT a -/// graveyard card is a shape this production does not model, so it must keep -/// producing `Effect::Unimplemented` and coverage must stay honestly RED rather -/// than be silently lowered into a reanimation. `name` is `"assimilate"` because -/// the imperative fallback derives it from the clause's first word. -/// -/// Paired positive: the real card's phrasing in the same test produces a -/// `ChangeZone`, so the negative is about the graveyard guard and not about a -/// production that never fires. -#[test] -fn assimilate_without_a_graveyard_target_stays_unimplemented() { - let non_graveyard = parse_effect("assimilate target creature you control"); - assert!( - matches!( - &non_graveyard, - Effect::Unimplemented { name, .. } if name == "assimilate" - ), - "a non-graveyard assimilate phrasing must stay honestly unsupported, got {non_graveyard:?}" - ); - - let real = parse_effect("assimilate target creature card from an opponent's graveyard"); - assert!( - matches!(real, Effect::ChangeZone { .. }), - "reach-guard: the printed phrasing must lower to a ChangeZone, got {real:?}" - ); -} - // --------------------------------------------------------------------------- // Test 4 — multi-authority identity binding // --------------------------------------------------------------------------- From 442aaf910c79192d1c18d0c68a4e1d833542fa23 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 13:03:04 -0700 Subject: [PATCH 2/2] docs(engine): make the relocated assimilate parser test discoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two comment incoherences introduced by the preceding commit, which moved the parser-only fail-closed test out of the integration suite and into `parser/oracle_effect/tests.rs`. `borg_queen_assimilate.rs`'s module doc pointed at the parser file but named only `borg_queen_assimilate_lowers_to_reanimate_then_retype_chain`, so the relocated `assimilate_without_a_graveyard_target_stays_unimplemented` was referenced from nowhere in the tree — the one thing the relocation was supposed to preserve was the ability to find it. Both are now named, and labelled by what they assert (positive lowering / fail-closed negative) rather than listed. The relocated test also kept its `3c.` ordinal, which indexed the integration file's 3a/3b/3c scheme. In its new home that prefix points at nothing. Dropped; the doc comment already states what the test discriminates. Comment-only: 0 non-comment lines change. Verified via `git diff -U0` with comment lines filtered out, so this cannot alter parse output, and `crates/engine/src/parser/oracle_effect/tests.rs` is gated behind `#[cfg(test)] mod tests;` and is never compiled into the `oracle-gen` binary that generates card data. --- crates/engine/src/parser/oracle_effect/tests.rs | 2 +- crates/engine/tests/integration/borg_queen_assimilate.rs | 6 ++++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index cc79e977cc..0e60cfdf64 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -52617,7 +52617,7 @@ fn borg_queen_assimilate_lowers_to_reanimate_then_retype_chain() { ); } -/// 3c. Parser fail-closed: an `assimilate` phrasing whose target is NOT a +/// Parser fail-closed: an `assimilate` phrasing whose target is NOT a /// graveyard card is a shape this production does not model, so it must keep /// producing `Effect::Unimplemented` and coverage must stay honestly RED rather /// than be silently lowered into a reanimation. `name` is `"assimilate"` because diff --git a/crates/engine/tests/integration/borg_queen_assimilate.rs b/crates/engine/tests/integration/borg_queen_assimilate.rs index 4131767c1f..18060906d9 100644 --- a/crates/engine/tests/integration/borg_queen_assimilate.rs +++ b/crates/engine/tests/integration/borg_queen_assimilate.rs @@ -17,8 +17,10 @@ //! //! These are RUNTIME tests: they cast through `GameRunner::cast(..).resolve()` //! and read back EFFECTIVE post-`evaluate_layers` characteristics. The AST-shape -//! coverage lives in `parser/oracle_effect/tests.rs` -//! (`borg_queen_assimilate_lowers_to_reanimate_then_retype_chain`). +//! coverage lives in `parser/oracle_effect/tests.rs` — both the positive +//! lowering (`borg_queen_assimilate_lowers_to_reanimate_then_retype_chain`) and +//! the fail-closed negative +//! (`assimilate_without_a_graveyard_target_stays_unimplemented`). //! //! FOOT-GUN, load-bearing in every test here: `layers.rs`'s //! `RemoveAllSubtypes { SubtypeSet::Creature }` arm retains any subtype NOT in