fix(parser): stop failing open on an unparseable subject, bind targeting compound subjects (#6965)

crates/engine/src/parser/oracle_effect/sequence.rs

@@ -2352,6 +2352,10 @@ fn is_inside_temporal_prefix(lower: &str) -> bool {
 /// - "that creature each" — the object-axis form (CR 115.1 parent-target
 ///   binding; e.g. Gogo, Mysterious Mime's "~ and that creature each get
 ///   +2/+0 and gain haste ... and attack this turn if able").
+/// - "target <filter>'s controller/owner each" — the possessive-actor form
+///   (CR 109.4; Life at Stake's "You and target creature's controller each
+///   secretly choose a number 0 or greater"), delegated to the shared axis
+///   combinator so the two sites cannot drift.
 fn remainder_trimmed_starts_with_compound_subject_each(remainder: &str) -> bool {
     let lower = remainder.to_ascii_lowercase();
     let result: nom::IResult<&str, (), OracleError<'_>> = alt((
@@ -2365,6 +2369,7 @@ fn remainder_trimmed_starts_with_compound_subject_each(remainder: &str) -> bool
         return true;
     }
     controlled_creature_each_subject_starts(&lower)
+        || super::parse_possessive_actor_each_second_subject(&lower).is_some()
 }
 
 fn controlled_creature_each_subject_starts(lower: &str) -> bool {

crates/engine/src/parser/oracle_effect/tests.rs

@@ -413,25 +413,28 @@ fn two_target_fight_pump_keeps_both_slots_and_buffs_slot_zero() {
     }
 }
 
-/// #4751 review (matthewevans): part 1's sentence-bounding drops only a STRAY
-/// `Effect::unimplemented("you")` head from Life at Stake — it does NOT remove a
-/// functional chooser. "You and target creature's controller each secretly
-/// choose a number" is unimplemented on BOTH `main` and this branch: the real
-/// mechanic is a `Choose { NumberRange }` (with the exile + lose-life tail),
-/// byte-identical either way. On `main` the leading "You" was split off its "and"
-/// by a coincidental " loses " in a LATER sentence into a bare
-/// `Unimplemented { name: "you", description: "You" }`; bounding the verb scan to
-/// the first sentence stops that coincidental split, so the chain now heads at
-/// the real `Choose` node instead of the stray "you" fragment. This pins that the
-/// card's actual choose-a-number mechanic survives (no regression) — the
-/// controller was never a *parsed* chooser to lose (that stays a pre-existing gap
-/// on both, orthogonal to this PR).
-#[test]
-fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() {
+/// CR 109.4 + CR 115.1 + CR 608.2c + CR 608.2d: Life at Stake — "You and target
+/// creature's controller each secretly choose a number 0 or greater."
+///
+/// The compound subject's second conjunct names its player THROUGH an announced
+/// object target, so the parse must produce three things, not one:
+///   1. a `TargetOnly { creature }` head declaring the CR 115.1 target slot the
+///      possessive reference (and the later "exile that creature" anaphor) read;
+///   2. a `Choose { NumberRange }` whose chooser is the printed controller
+///      ("you", CR 109.5 — the unscoped resolver default);
+///   3. a SECOND `Choose { NumberRange }` bound to a DISTINCT chooser via
+///      `player_scope: ParentObjectTargetController` (CR 109.4).
+///
+/// The two choosers being distinct is the whole mechanic — one `Choose`, or two
+/// that both prompt the caster, is the bug. Predecessor of this test pinned a
+/// single `Choose` head while the controller was an unparsed chooser; that gap
+/// is now closed.
+#[test]
+fn life_at_stake_binds_both_number_choosers_to_distinct_players() {
     use crate::types::ability::ChoiceType;
 
-    fn collect(a: &AbilityDefinition, out: &mut Vec<Effect>) {
-        out.push((*a.effect).clone());
+    fn collect<'a>(a: &'a AbilityDefinition, out: &mut Vec<&'a AbilityDefinition>) {
+        out.push(a);
         if let Some(s) = a.sub_ability.as_deref() {
             collect(s, out);
         }
@@ -446,22 +449,47 @@ fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() {
     let parsed = parse_oracle_text(text, "Life at Stake", &[], &["Instant".to_string()], &[]);
     let ability = parsed.abilities.first().expect("expected a spell ability");
 
-    // The real mechanic — Choose a number — heads the chain (not a stray
-    // `Unimplemented("you")` fragment).
-    assert!(
-        matches!(
-            &*ability.effect,
-            Effect::Choose {
-                choice_type: ChoiceType::NumberRange { .. },
-                ..
-            }
-        ),
-        "Life at Stake must head at a NumberRange Choose, not a stray you-stub; got {:#?}",
-        ability.effect
+    // CR 115.1: the announced creature is the ability's target — declared by a
+    // slot-only head so the possessive chooser and the "that creature" exile
+    // anaphor both have something to resolve against.
+    let Effect::TargetOnly { target } = &*ability.effect else {
+        panic!(
+            "Life at Stake must head at a TargetOnly declaring the creature target, got {:#?}",
+            ability.effect
+        );
+    };
+    assert_eq!(
+        *target,
+        TargetFilter::Typed(TypedFilter::creature()),
+        "the declared target must be the announced creature"
     );
 
-    let mut effects = Vec::new();
-    collect(ability, &mut effects);
+    let mut links = Vec::new();
+    collect(ability, &mut links);
+
+    // Both halves of the compound subject choose a number, and their choosers
+    // are DIFFERENT players: the printed controller (no scope) and the targeted
+    // creature's controller (CR 109.4).
+    let choosers: Vec<Option<PlayerFilter>> = links
+        .iter()
+        .filter(|link| {
+            matches!(
+                &*link.effect,
+                Effect::Choose {
+                    choice_type: ChoiceType::NumberRange { .. },
+                    ..
+                }
+            )
+        })
+        .map(|link| link.player_scope.clone())
+        .collect();
+    assert_eq!(
+        choosers,
+        vec![None, Some(PlayerFilter::ParentObjectTargetController)],
+        "both conjuncts must choose a number, bound to distinct choosers"
+    );
+
+    let effects: Vec<&Effect> = links.iter().map(|link| &*link.effect).collect();
     assert!(
         effects.iter().any(|e| matches!(
             e,
@@ -479,9 +507,124 @@ fn life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub() {
     assert!(
         !effects
             .iter()
-            .any(|e| matches!(e, Effect::Unimplemented { name, .. } if name == "you")),
-        "the stray Unimplemented(\"you\") head must be gone: {effects:#?}"
+            .any(|e| matches!(e, Effect::Unimplemented { .. })),
+        "no clause of the choose-a-number sentence may fall back to Unimplemented: {effects:#?}"
+    );
+}
+
+/// CR 109.4 + CR 115.1 + CR 608.2c: the possessive-actor second-subject axis is
+/// a CLASS, not Life at Stake's card. The same "you and target &lt;filter&gt;'s
+/// controller each &lt;body&gt;" shape must distribute a body that DOES carry a
+/// recipient slot, binding it through the effect's own field rather than
+/// `player_scope` — and the announced target must be declared exactly once.
+#[test]
+fn possessive_actor_compound_subject_distributes_a_recipient_bearing_body() {
+    let ability = parse_effect_chain(
+        "You and target creature's controller each draw a card.",
+        AbilityKind::Spell,
     );
+
+    let Effect::TargetOnly { target } = &*ability.effect else {
+        panic!(
+            "expected a TargetOnly target declaration, got {:#?}",
+            ability.effect
+        );
+    };
+    assert_eq!(*target, TargetFilter::Typed(TypedFilter::creature()));
+
+    let you = ability
+        .sub_ability
+        .as_deref()
+        .expect("expected the caster half");
+    match &*you.effect {
+        Effect::Draw { target, .. } => assert_eq!(*target, TargetFilter::OriginalController),
+        other => panic!("expected the caster half to Draw, got {other:?}"),
+    }
+    assert_eq!(
+        you.player_scope, None,
+        "a recipient-bearing body binds \"you\" on the effect, not as a fan-out"
+    );
+
+    let them = you
+        .sub_ability
+        .as_deref()
+        .expect("expected the possessive half");
+    match &*them.effect {
+        Effect::Draw { target, .. } => assert_eq!(*target, TargetFilter::ParentTargetController),
+        other => panic!("expected the possessive half to Draw, got {other:?}"),
+    }
+    assert_eq!(
+        them.player_scope, None,
+        "a recipient-bearing body must not ALSO fan out — that would double-apply"
+    );
+}
+
+/// CR 109.4 + 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`.
+#[test]
+fn recipient_less_body_binds_a_chosen_player_conjunct_by_scope() {
+    let parsed = parse_oracle_text(
+        "Choose an opponent. You and that player each sacrifice a creature.",
+        "Infernal Offering",
+        &[],
+        &["Sorcery".to_string()],
+        &[],
+    );
+    let ability = parsed.abilities.first().expect("expected a spell ability");
+
+    let you = ability
+        .sub_ability
+        .as_deref()
+        .expect("expected the caster half after the Choose");
+    assert!(
+        matches!(&*you.effect, Effect::Sacrifice { .. }),
+        "the caster half must sacrifice, got {:#?}",
+        you.effect
+    );
+    assert_eq!(you.player_scope, None, "\"you\" is the unscoped default");
+
+    let them = you
+        .sub_ability
+        .as_deref()
+        .expect("expected the chosen-player half");
+    assert!(
+        matches!(&*them.effect, Effect::Sacrifice { .. }),
+        "the chosen-player half must sacrifice, got {:#?}",
+        them.effect
+    );
+    assert_eq!(
+        them.player_scope,
+        Some(PlayerFilter::ChosenPlayer { index: 0 }),
+        "the chosen opponent must be the acting player of the second half"
+    );
+}
+
+/// 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
+/// flip a coin" (Mana Clash) / "… each secretly choose 1, 2, or 3"
+/// (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() {
+    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
+        );
+        assert_eq!(
+            ability.player_scope, None,
+            "{text:?} must not fabricate a fan-out scope"
+        );
+    }
 }
 
 /// Recursively walk an ability chain (root effect + `sub_ability` + `else_ability`)

crates/engine/src/parser/oracle_ir/ast.rs

@@ -182,7 +182,24 @@ pub(crate) enum ClauseAst {
 
 #[derive(Debug, Clone, PartialEq, Eq, Serialize)]
 pub(crate) struct SubjectPhraseAst {
-    pub(crate) affected: TargetFilter,
+    /// CR 608.2c ("read the whole text and apply the rules of English to the
+    /// text"): the subject the predicate applies to, or `None` when the
+    /// sentence printed a subject the subject grammar could not bind.
+    ///
+    /// **`Option`, not a permissive default (issue #6965).** Both sites that
+    /// re-derive a subject phrase used to substitute `TargetFilter::Any` when
+    /// [`super::SubjectApplication`] could not be produced. `TargetFilter::Any`
+    /// matches unconditionally (`game/filter.rs`), so a parse FAILURE emitted a
+    /// 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
+    /// [`EntersUnderSpec::UnboundAnaphor`].
+    pub(crate) affected: Option<TargetFilter>,
     pub(crate) target: Option<TargetFilter>,
     pub(crate) multi_target: Option<MultiTargetSpec>,
     pub(crate) inherits_parent: bool,

crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snap

@@ -228,8 +228,8 @@ expression: "&ir"
                     "parsed": {
                       "effect": {
                         "type": "Unimplemented",
-                        "name": "can't",
-                        "description": "can't be spent to cast nonartifact spells"
+                        "name": "unbound_subject",
+                        "description": "This mana can't be spent to cast nonartifact spells"
                       },
                       "duration": null,
                       "sub_ability": null,

crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs

@@ -291,19 +291,28 @@ fn intellectual_offering_second_draw_binds_to_chosen_opponent() {
     );
 }
 
-/// Regression guard for the fix above: `inject_subject_target`'s new
-/// `GainLife` arm must NOT rebind the recipient when the detected subject
-/// isn't a genuine player reference. Angel of Destiny's "you and that player
-/// each gain that much life" is a compound subject that `GainLife` doesn't
-/// support in `rewrite_recipient_on_link` (Token/Draw/Discard/Mill/Pump/
-/// GenericEffect only), so it falls through to a non-player-denoting subject
-/// filter; the safe no-subject `Controller` default must survive rather than
-/// being corrupted into an incoherent recipient. This is a pre-existing gap
-/// (the damaged player still doesn't gain life) — not fixed here — but the
-/// `player` field must stay a well-defined `Controller`, not silently swap to
-/// something meaningless.
+/// Regression guard for the fix above, restated by #6965. `inject_subject_target`'s
+/// `GainLife` arm must NOT rebind the recipient when the detected subject isn't a
+/// genuine player reference — Angel of Destiny's "you and that player each gain that
+/// much life" is a compound subject `rewrite_recipient_on_link` has no arm for
+/// (Token/Draw/Discard/Mill/Pump/GenericEffect only).
+///
+/// This used to assert the clause survived as `GainLife { player: Controller }`, and
+/// the comment conceded the gap in the same breath: the damaged player never gained
+/// life. That is a half-applied effect the caster benefits from, and it counted as
+/// SUPPORTED in coverage — the silent-misparse class #6965 exists to remove. It only
+/// reached `GainLife` at all because the unbindable subject fell open. With the
+/// fail-open gone the clause is an honest `unbound_subject` gap: still not playable,
+/// but now visible to coverage instead of masquerading as a working trigger.
+///
+/// The original intent is preserved and strengthened — the point was that a subject
+/// the parser cannot resolve must never be laundered into a concrete recipient. An
+/// `Unimplemented` gap satisfies that more completely than a `Controller` default did.
+///
+/// Forward-red: binding "that player" to the damage-event player will red this test,
+/// which is the intended prompt to assert the real two-recipient shape.
 #[test]
-fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() {
+fn angel_of_destiny_compound_subject_fails_closed_rather_than_half_applying() {
     let mut scenario = GameScenario::new();
     scenario.at_phase(Phase::PreCombatMain);
     let creature = scenario
@@ -327,16 +336,22 @@ fn angel_of_destiny_combat_damage_gain_life_keeps_well_defined_recipient() {
         .execute
         .as_ref()
         .expect("DamageDone trigger must have an execute body");
+    let Effect::Unimplemented { name, description } = execute.effect.as_ref() else {
+        panic!(
+            "`you and that player each gain that much life` must fail closed rather than \
+             bind half the clause to a recipient it cannot name, got {:?}",
+            execute.effect
+        );
+    };
+    assert_eq!(
+        name, "unbound_subject",
+        "the gap must name the SUBJECT as the unbound part — a different name means the \
+         clause failed somewhere else and this test stopped covering the fail-closed path"
+    );
     assert!(
-        matches!(
-            execute.effect.as_ref(),
-            Effect::GainLife {
-                player: TargetFilter::Controller,
-                ..
-            }
-        ),
-        "GainLife.player must stay the well-defined Controller default, not an \
-         unresolved compound-subject filter like Any, got {:?}",
-        execute.effect
+        description
+            .as_deref()
+            .is_some_and(|text| text.contains("that player")),
+        "reach-guard: the gap must quote the conjunct it could not bind, got {description:?}"
     );
 }