From af4a2d9dacc77c73e7561ba5a08452be13aeac7b Mon Sep 17 00:00:00 2001 From: matthewevans Date: Tue, 4 Aug 2026 16:23:04 -0700 Subject: [PATCH] fix(parser): guard an existing player_scope, and pin the reason in the fail-closed tests (#6965) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from the CodeRabbit review of #7003, which merged before these landed. **Correctness.** `bind_recipient_without_recipient_slot` assigned `def.player_scope = Some(scope)` unconditionally while documenting a "Total and FAIL-CLOSED" contract. That contract only covered recipients no `PlayerFilter` can name — not a body that already carries its own iteration scope. Both halves are rewritten from clones of ONE parsed body, so an unguarded stamp would replace a printed per-player fan-out with a single recipient, and with a different one on each half. Refuse when a scope is already present; the caller already turns `false` into `Unimplemented`. **CR citations.** CR 301.5f defines what an ability means by "equipped creature"; it does not define attachment, which is CR 301.5a. Both sites now cite the rule whose body describes the code. CR 109.4 is about which objects have controllers, so it does not describe Infernal Offering's "that player" — a player chosen while applying the effect, CR 608.2d, which the doc already cited. Dropped it there. The targeted-player fail-closed contract now cites CR 115.1, which is the rule that actually makes those conjuncts targets. **Vacuous negative assertions.** Two tests accepted any `Effect::Unimplemented` and so would have passed on a clause that died earlier, or on a dropped effect. Both now assert the gap is named `unbound_subject` and quotes the conjunct that caused it. `recipient_less_body_with_a_targeted_player_conjunct_fails_closed` gains a positive reach guard, since a dead distributor fails closed on everything including what it should bind — the guard carries the full Infernal Offering text, because "that player" without its preceding "Choose an opponent." is itself an unbound subject. The Wand of Orcus test proved only that a trigger reached the stack, which a dropped execute effect would also satisfy while granting no deathtouch; it now inspects the trigger's chain before resolution. **Docs.** `SubjectPhraseAst::affected` claimed one consumer reads it. `sync_subject_into_nested_shuffle_sub` and `inject_subject_target` read it too, via `target.or(affected)`. Narrowed to the invariant actually intended: `ImperativeFallback` is the only consumer treating `None` as a coverage gap; the others treat it as "nothing to rebind". `None` is reachable in all three. Two citations kept deliberately. CR 608.2c stays on the Life at Stake test: it asserts chain order mirroring printed order, which is what that rule governs. CR 109.4 stays on the same test's `ParentObjectTargetController` binding, where the player IS named through a battlefield object's controller. Verified: parser tests 1583 passed; engine lib 18487 passed; integration 4487 passed; clippy -D warnings clean. The new reach guard was watched go red first (it caught a wrong positive example, which is how the antecedent requirement above was found). --- crates/engine/src/parser/oracle_effect/mod.rs | 19 +++++-- .../engine/src/parser/oracle_effect/tests.rs | 52 +++++++++++++++---- crates/engine/src/parser/oracle_ir/ast.rs | 14 +++-- .../wand_of_orcus_compound_subject_6965.rs | 42 +++++++++++++-- 4 files changed, 105 insertions(+), 22 deletions(-) diff --git a/crates/engine/src/parser/oracle_effect/mod.rs b/crates/engine/src/parser/oracle_effect/mod.rs index df5584790f..4c0f1b263f 100644 --- a/crates/engine/src/parser/oracle_effect/mod.rs +++ b/crates/engine/src/parser/oracle_effect/mod.rs @@ -18653,11 +18653,11 @@ fn rewrite_recipient_on_link(def: &mut AbilityDefinition, filter: &TargetFilter) /// `player_scope_from_parent_target_subject`), reused here rather than /// duplicated. /// -/// Total and FAIL-CLOSED: only a recipient an existing `PlayerFilter` can name -/// is bound. A recipient naming a TARGETED player ("target opponent") or an -/// object ("that creature") returns `false`, so the caller falls through to -/// `Effect::Unimplemented` instead of silently letting the printed controller -/// act in someone else's place. +/// Total and FAIL-CLOSED on BOTH sides of the binding. A recipient naming a +/// TARGETED player ("target opponent") or an object ("that creature") returns +/// `false`; so does a body that already carries its own scope. Either way the +/// caller falls through to `Effect::Unimplemented` instead of silently letting +/// the printed controller act in someone else's place. fn bind_recipient_without_recipient_slot( def: &mut AbilityDefinition, filter: &TargetFilter, @@ -18668,6 +18668,15 @@ fn bind_recipient_without_recipient_slot( if matches!(filter, TargetFilter::OriginalController) { return true; } + // The body may already carry a printed iteration scope of its own. Both + // halves are rewritten from clones of ONE parsed body, so an unguarded + // stamp would replace that fan-out with a single recipient — and with a + // DIFFERENT one on each half, since each half binds its own conjunct. The + // printed per-player iteration would be silently lost. Refuse instead: an + // honest gap beats resolving for the wrong set of players. + if def.player_scope.is_some() { + return false; + } let Some(scope) = distribution_recipient_player_scope(filter) else { return false; }; diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index 62ea126b5c..6da5390c94 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -559,11 +559,13 @@ fn possessive_actor_compound_subject_distributes_a_recipient_bearing_body() { ); } -/// CR 109.4 + CR 608.2d: the recipient-less binding channel generalizes past the +/// CR 608.2d: the recipient-less binding channel generalizes past the /// possessive axis. Infernal Offering's "You and that player each sacrifice a /// creature" has no `TargetFilter` recipient slot on `Effect::Sacrifice` either, /// and its second conjunct is the opponent a preceding "Choose an opponent." -/// picked — so the second half binds `player_scope: ChosenPlayer`. +/// picked — a choice announced while applying the effect, not an object whose +/// controller is being read — so the second half binds +/// `player_scope: ChosenPlayer`. #[test] fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() { let parsed = parse_oracle_text( @@ -602,23 +604,55 @@ fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() { ); } -/// CR 109.4: FAIL-CLOSED contract for the recipient-less binding channel. No -/// `PlayerFilter` can name a TARGETED player, so "you and target opponent each +/// CR 115.1: FAIL-CLOSED contract for the recipient-less binding channel. The +/// second conjunct is a TARGETED player — declared as the spell goes on the +/// stack — and no `PlayerFilter` names one, so "you and target opponent each /// flip a coin" (Mana Clash) / "… each secretly choose 1, 2, or 3" -/// (Expert-Level Safe) must stay an honest `Unimplemented` — binding the body to +/// (Expert-Level Safe) must stay an honest `Unimplemented`. Binding the body to /// `PlayerFilter::Opponent` would make EVERY opponent act in a multiplayer game, /// and leaving it unbound would make the caster act twice. #[test] fn recipient_less_body_with_a_targeted_player_conjunct_fails_closed() { + // Positive reach guard: the SAME grammar with a NAMEABLE conjunct must still + // parse. Without it, every assertion below would also hold if the + // compound-subject distributor had stopped running altogether — a dead path + // fails closed on everything, including cases it should bind. + // + // "that player" needs its antecedent: the binding comes from the preceding + // "Choose an opponent.", so the guard has to carry that sentence. A bare + // "You and that player each …" is itself an unbound subject, which is why + // this guard uses the full Infernal Offering text. + let reachable = parse_oracle_text( + "Choose an opponent. You and that player each sacrifice a creature.", + "Infernal Offering", + &[], + &["Sorcery".to_string()], + &[], + ); + let bound_half = reachable + .abilities + .first() + .and_then(|ability| ability.sub_ability.as_deref()) + .expect("reach-guard: the caster half must exist after the Choose"); + assert!( + matches!(&*bound_half.effect, Effect::Sacrifice { .. }), + "reach-guard: the compound-subject distributor must still bind a nameable \ + conjunct, else the fail-closed assertions below are vacuous, got {:#?}", + bound_half.effect + ); + for text in [ "You and target opponent each flip a coin.", "You and target opponent each secretly choose 1, 2, or 3.", ] { let ability = parse_effect_chain(text, AbilityKind::Spell); - assert!( - matches!(&*ability.effect, Effect::Unimplemented { .. }), - "{text:?} must fail closed, got {:#?}", - ability.effect + let Effect::Unimplemented { name, .. } = &*ability.effect else { + panic!("{text:?} must fail closed, got {:#?}", ability.effect); + }; + assert_eq!( + name, "unbound_subject", + "{text:?} must fail closed AT THE SUBJECT — any other gap name means the \ + clause died earlier and this case stopped covering the targeted-player path" ); assert_eq!( ability.player_scope, None, diff --git a/crates/engine/src/parser/oracle_ir/ast.rs b/crates/engine/src/parser/oracle_ir/ast.rs index c95052cbc3..c58de09d2b 100644 --- a/crates/engine/src/parser/oracle_ir/ast.rs +++ b/crates/engine/src/parser/oracle_ir/ast.rs @@ -193,11 +193,15 @@ pub(crate) struct SubjectPhraseAst { /// BOARD-WIDE effect — the grant landed on every permanent, lands and /// artifacts included, while coverage still reported the card as supported. /// Encoding the unbound state in the type makes that fail-open - /// unrepresentable: every consumer must say what it does with `None`, and - /// the one consumer that actually reads this field - /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm, the only - /// predicate kind that applies the subject filter) fails closed to - /// `Effect::unimplemented`. Same shape, same reason, as + /// unrepresentable: every consumer must say what it does with `None`. + /// `lower_subject_predicate_ast`'s `ImperativeFallback` arm — the only + /// predicate kind that applies the subject filter — is the only consumer + /// that treats `None` as a coverage GAP, failing closed to + /// `Effect::unimplemented`. The other readers + /// (`sync_subject_into_nested_shuffle_sub`, `inject_subject_target`) reach + /// it through `target.or(affected)` and treat `None` as "nothing to + /// rebind", returning early. `None` is therefore reachable in all three — + /// do not assume otherwise when editing them. Same shape, same reason, as /// [`EntersUnderSpec::UnboundAnaphor`]. pub(crate) affected: Option, pub(crate) target: Option, diff --git a/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs b/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs index a507c58787..186c1121de 100644 --- a/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs +++ b/crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs @@ -21,13 +21,16 @@ //! //! CR 611.2c: one continuous effect naming several subjects determines the set //! each part applies to independently — i.e. the UNION of the named subjects. -//! CR 301.5f: an Equipment attaches to a creature. +//! CR 301.5a: an Equipment is attached to a creature, which is then the +//! "equipped creature". CR 301.5f: an ability referring to the "equipped +//! creature" means whatever creature the permanent is attached to. //! CR 702.2b: deathtouch. CR 702.11b: hexproof. use engine::game::combat::AttackTarget; use engine::game::game_object::AttachTarget; use engine::game::layers::evaluate_layers; use engine::game::scenario::{GameRunner, GameScenario, P0, P1}; +use engine::types::ability::Effect; use engine::types::card_type::CoreType; use engine::types::identifiers::ObjectId; use engine::types::keywords::Keyword; @@ -76,8 +79,9 @@ fn wand_of_orcus_unbindable_subject_grants_nothing_board_wide() { let mut runner = scenario.build(); - // CR 301.5f: make the Wand a real Equipment attached to `host`, so its - // "equipped creature attacks" trigger has a subject to fire on. + // CR 301.5a: attach the Wand to `host` so it is a real Equipment with an + // equipped creature. CR 301.5f: that is what its "equipped creature + // attacks" trigger resolves against, so the trigger has a subject to fire on. { let obj = runner.state_mut().objects.get_mut(&wand).unwrap(); obj.card_types.core_types = vec![CoreType::Artifact]; @@ -104,6 +108,38 @@ fn wand_of_orcus_unbindable_subject_grants_nothing_board_wide() { "the attack trigger must be on the stack, or nothing below is exercised" ); + // ...and the trigger must carry the SPECIFIC gap this test is about. Stack + // presence alone proves only that a trigger was created: a regression that + // dropped the execute effect entirely would also grant no deathtouch and + // leave every assertion below green. Pin the reason, not just the silence. + { + let wand_obj = runner.state().objects.get(&wand).unwrap(); + let gap = wand_obj + .trigger_definitions + .iter_unchecked() + .filter_map(|entry| entry.definition.execute.as_ref()) + .find_map(|exec| match exec.effect.as_ref() { + Effect::Unimplemented { name, description } => Some((name, description)), + _ => None, + }) + .expect( + "the attack trigger's execute chain must be an Unimplemented gap — if it \ + parsed, or vanished, the deathtouch assertions below prove nothing", + ); + assert_eq!( + gap.0, "unbound_subject", + "the gap must name the SUBJECT as the unbound part; another name means the \ + clause failed elsewhere and this test stopped covering the fail-closed path" + ); + assert!( + gap.1 + .as_deref() + .is_some_and(|text| text.contains("Zombies you control")), + "reach-guard: the gap must quote the unbindable conjunct, got {:?}", + gap.1 + ); + } + runner.advance_until_stack_empty(); runner.state_mut().layers_dirty.mark_full(); evaluate_layers(runner.state_mut());