From be3049f43598588799f4fc7f9534170b6ca971b3 Mon Sep 17 00:00:00 2001 From: Jacob Woodson Date: Thu, 13 Aug 2026 22:21:37 -0500 Subject: [PATCH 1/5] Partial: Fight for the Throne --- crates/engine/src/game/ability_rw.rs | 62 +- crates/engine/src/game/ability_scan.rs | 31 +- crates/engine/src/game/coverage.rs | 175 +- crates/engine/src/game/effects/mod.rs | 221 +- crates/engine/src/game/engine.rs | 123 +- crates/engine/src/game/scenario.rs | 51 + crates/engine/src/game/triggers.rs | 1851 ++++++++++++++++- .../game/triggers_ordering_parity_tests.rs | 166 +- .../src/parser/oracle_effect/conditions.rs | 123 +- .../engine/src/parser/oracle_effect/tests.rs | 85 +- crates/engine/src/parser/oracle_trigger.rs | 15 + crates/engine/src/types/ability.rs | 60 + ...r_the_throne_monarch_gated_on_commander.rs | 241 +++ crates/engine/tests/integration/main.rs | 1 + 14 files changed, 3143 insertions(+), 62 deletions(-) create mode 100644 crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index 4bc7653a5d..5771d4c6cf 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -1909,6 +1909,14 @@ fn legacy_ability_condition(x: &AbilityCondition) -> bool { | AbilityCondition::ControllerControlledMatchingAsCast { .. } | AbilityCondition::SourceLacksKeyword { .. } | AbilityCondition::WasStartingPlayer { .. } + // CR 903.3d: `false` here is about the D5 LEGACY-BATCH-PROMPT axis only — + // "does this leaf carry one of the 12 retained event-context refs" — not + // about reads. It carries none, exactly like the board-reading + // `ControllerControlsMatching` above and the `TriggerCondition` / + // `StaticCondition` mirrors below. The commander gate's actual read + // (a live battlefield census, CR 903.3d) is classified in + // `rw_ability_condition` via `commander_control_read`. + | AbilityCondition::ControlsCommander { .. } | AbilityCondition::AdditionalCostPaidInstead | AbilityCondition::AlternativeManaCostPaid | AbilityCondition::EffectOutcome { .. } @@ -3602,6 +3610,46 @@ fn board_membership_read(filter: &TargetFilter) -> RwProfile { p } +/// CR 903.3d + CR 603.3b: the single read profile for a commander-control gate, +/// shared by ALL THREE condition-vocabulary mirrors (`AbilityCondition`, +/// `TriggerCondition`, `StaticCondition`). +/// +/// CR 903.3d: "If an effect refers to controlling a commander, it refers to a +/// permanent on the battlefield that is a commander." That is a LIVE BOARD +/// CENSUS, not a static card attribute: `game::commander::controls_own_commander` +/// / `controls_any_commander` scan `state.battlefield` for +/// `is_commander && controller == you [&& owner == you] && is_phased_in`. Only +/// the `is_commander` and `owner` conjuncts are frozen card attributes (CR 903.3); +/// battlefield MEMBERSHIP, the CONTROLLER field and CR 702.26b phased-in status +/// are all sibling-mutable, and `StateKind::SetMembership` is exactly "which +/// objects are where / whose" plus the kind a `PhaseOut` write records. +/// +/// So this must NOT be `RwProfile::empty()`: a sibling copy whose effect moves a +/// commander onto or off the battlefield, steals it, or phases it out FEEDS this +/// gate's truth, and `group_is_order_independent` would otherwise auto-order the +/// pair against a gate that write flips (CR 603.3b). Modeled exactly like the +/// direct analogue `AbilityCondition::ControllerControlsMatching` — +/// `board_membership_read` over the population CR 903.3d names, expressed in the +/// engine's own filter vocabulary so the census/zone/controller spans are derived +/// by the one authority rather than hand-assembled. +/// +/// `ControllerRef::You` for both ownership arms: CR 109.5 "you" is the evaluating +/// player, and `Own` merely adds the frozen owner conjunct on top of the same +/// controller-keyed census (a strictly narrower population, so the `You` span +/// stays sound). +fn commander_control_read() -> RwProfile { + board_membership_read(&TargetFilter::Typed( + TypedFilter::permanent() + .controller(ControllerRef::You) + .properties(vec![ + FilterProp::IsCommander, + FilterProp::InZone { + zone: Zone::Battlefield, + }, + ]), + )) +} + /// §L2 (CR 400.7 + CR 400.1): a zone-change per-turn journal read, keyed to its /// DESTINATION zone when known. A `None` destination stays fail-closed (`Any` /// zones, via `board_membership_read`). Never overrides a self-scoped read (whose @@ -6157,6 +6205,10 @@ fn rw_ability_condition(x: &AbilityCondition) -> RwProfile { } p } + // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. + // The three condition-vocabulary mirrors of this ONE printed clause share + // that helper, so none of them can drift from the others. + AbilityCondition::ControlsCommander { .. } => commander_control_read(), AbilityCondition::AdditionalCostPaidInstead | AbilityCondition::AlternativeManaCostPaid | AbilityCondition::EffectOutcome { .. } @@ -6297,10 +6349,13 @@ fn rw_trigger_condition(x: &TriggerCondition) -> RwProfile { | TriggerCondition::TributeNotPaid | TriggerCondition::CastDuringPhase { .. } | TriggerCondition::CastTimingPermission { .. } - | TriggerCondition::ControlsCommander { .. } | TriggerCondition::ChosenLabelIs { .. } | TriggerCondition::ExceptFirstDrawInDrawStep | TriggerCondition::PlacedByAbilitySource => RwProfile::empty(), + // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. + // Shared with the `AbilityCondition` / `StaticCondition` mirrors of the + // same printed clause. + TriggerCondition::ControlsCommander { .. } => commander_control_read(), } } @@ -6397,12 +6452,15 @@ fn rw_static_condition(x: &StaticCondition) -> RwProfile { | StaticCondition::WasCast { .. } | StaticCondition::IsRingBearer | StaticCondition::RingLevelAtLeast { .. } - | StaticCondition::ControlsCommander { .. } | StaticCondition::SourceControllerEquals { .. } | StaticCondition::EnchantedIsFaceDown | StaticCondition::AdditionalCostPaid | StaticCondition::CastingAsVariant { .. } | StaticCondition::None => RwProfile::empty(), + // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. + // Shared with the `AbilityCondition` / `TriggerCondition` mirrors of the + // same printed clause. + StaticCondition::ControlsCommander { .. } => commander_control_read(), } } diff --git a/crates/engine/src/game/ability_scan.rs b/crates/engine/src/game/ability_scan.rs index 3a32bab3d8..2e3fa48c51 100644 --- a/crates/engine/src/game/ability_scan.rs +++ b/crates/engine/src/game/ability_scan.rs @@ -2681,6 +2681,21 @@ fn scan_ability_condition(x: &AbilityCondition, mode: ScanMode) -> Axes { } AbilityCondition::HasMaxSpeed => Axes::NONE, AbilityCondition::IsMonarch => Axes::NONE, + // CR 903.3d: "controlling a commander" is a permanent ON THE BATTLEFIELD + // that is a commander — a live board census (`game::commander` scans + // `state.battlefield` for `is_commander && controller == you [&& owner == + // you] && is_phased_in`), so a sibling copy that moves, steals or phases a + // commander can flip this gate (CR 603.3b ordering-relevance). Self-asserts + // its own `sibling: true` literal, as the ⛔ INVARIANT on + // `scan_target_filter`'s `Typed` arm requires of every board-aggregate + // caller. `event` stays false: the census reads no triggering-event + // characteristic. `ownership: _` is destructured explicitly (as the + // `TriggerCondition` mirror does) so a future field forces a re-audit here. + AbilityCondition::ControlsCommander { ownership: _ } => Axes { + event: false, + sibling: true, + projected: false, + }, // CR 309.7: controller-state predicate — touches no scan axis. AbilityCondition::CompletedDungeon { .. } => Axes::NONE, AbilityCondition::IsInitiative => Axes::NONE, @@ -3368,7 +3383,13 @@ fn scan_trigger_condition(x: &TriggerCondition, mode: ScanMode) -> Axes { sibling: true, projected: false, }, - TriggerCondition::ControlsCommander { ownership: _ } => Axes::NONE, + // CR 903.3d: live battlefield census — same self-asserted board read as the + // `AbilityCondition` / `StaticCondition` mirrors of this printed clause. + TriggerCondition::ControlsCommander { ownership: _ } => Axes { + event: false, + sibling: true, + projected: false, + }, TriggerCondition::IsRenowned { subject: _ } => Axes::NONE, TriggerCondition::HasCounters { .. } => Axes { event: false, @@ -3642,7 +3663,13 @@ fn scan_static_condition(x: &StaticCondition, mode: ScanMode) -> Axes { StaticCondition::WasCast { zone: _ } => Axes::NONE, StaticCondition::IsRingBearer => Axes::NONE, StaticCondition::RingLevelAtLeast { level: _ } => Axes::NONE, - StaticCondition::ControlsCommander { ownership: _ } => Axes::NONE, + // CR 903.3d: live battlefield census — same self-asserted board read as the + // `AbilityCondition` / `TriggerCondition` mirrors of this printed clause. + StaticCondition::ControlsCommander { ownership: _ } => Axes { + event: false, + sibling: true, + projected: false, + }, StaticCondition::SourceIsTapped => Axes::NONE, StaticCondition::IsTapped { scope, .. } => { let mut acc = Axes::NONE; diff --git a/crates/engine/src/game/coverage.rs b/crates/engine/src/game/coverage.rs index fce3ba8abd..e3686ed1cd 100644 --- a/crates/engine/src/game/coverage.rs +++ b/crates/engine/src/game/coverage.rs @@ -18,9 +18,9 @@ use crate::parser::oracle_util::SELF_REF_TYPE_PHRASES; use crate::types::ability::{ AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, ActivationRestriction, AdditionalCost, AggregateFunction, AttackScope, AttackSubject, CardTypeSetSource, ChoiceType, - CoinFlipResult, Comparator, ContinuousModification, ControllerRef, CountScope, - CounterSourceRider, DelayedTriggerCondition, DieRollModifier, DoublePTMode, Duration, - EachDamageRecipient, Effect, EffectOutcomeSignal, EffectScope, FilterProp, + CoinFlipResult, CommanderOwnership, Comparator, ContinuousModification, ControllerRef, + CountScope, CounterSourceRider, DelayedTriggerCondition, DieRollModifier, DoublePTMode, + Duration, EachDamageRecipient, Effect, EffectOutcomeSignal, EffectScope, FilterProp, ForEachCategoryAction, GameRestriction, LibraryPosition, ManaProduction, ObjectProperty, ObjectScope, PerpetualModification, PlayerFilter, PlayerScope, PtStat, PtValue, PtValueScope, QuantityExpr, QuantityRef, ReplacementCondition, ReplacementDefinition, ReplacementMode, @@ -3911,6 +3911,23 @@ fn fmt_comparator(c: &Comparator) -> &'static str { } } +/// CR 903.3 vs CR 903.3d: the single label authority for a commander-control +/// gate, shared by ALL FOUR condition-vocabulary formatters +/// (`AbilityCondition`, `TriggerCondition`, `StaticCondition`, and any future +/// mirror). +/// +/// The two arms are DIFFERENT predicates — CR 903.3 + CR 109.5 "your commander" +/// is owned AND controlled, CR 903.3d "a commander" is controlled by any owner — +/// so they must never share a label. Collapsing them prints a strictly weaker +/// predicate than the card, and the parse-details / Alt-hover overlay is what +/// bug triage reads. Centralized here so one mirror cannot drift from another. +fn fmt_commander_ownership(ownership: &CommanderOwnership) -> &'static str { + match ownership { + CommanderOwnership::Own => "you control your commander", + CommanderOwnership::Any => "you control a commander", + } +} + /// Format an `AbilityCondition` as a human-readable string for the parse-details overlay. fn fmt_ability_condition(cond: &AbilityCondition) -> String { match cond { @@ -3975,6 +3992,9 @@ fn fmt_ability_condition(cond: &AbilityCondition) -> String { ), AbilityCondition::HasMaxSpeed => "has max speed".into(), AbilityCondition::IsMonarch => "is monarch".into(), + AbilityCondition::ControlsCommander { ownership } => { + fmt_commander_ownership(ownership).into() + } AbilityCondition::CompletedDungeon { specific } => match specific { None => "you've completed a dungeon".into(), Some(dungeon) => format!("you've completed {dungeon}"), @@ -4154,7 +4174,7 @@ fn fmt_trigger_condition(cond: &crate::types::ability::TriggerCondition) -> Stri } TC::ManaSpentCondition { .. } => "mana spent condition".into(), TC::HadCounters { .. } => "had counters".into(), - TC::ControlsCommander { .. } => "you control a commander".into(), + TC::ControlsCommander { ownership } => fmt_commander_ownership(ownership).into(), TC::IsRenowned { .. } => "is renowned".into(), TC::HasCounters { minimum, maximum, .. @@ -4341,7 +4361,7 @@ fn fmt_static_condition(cond: &StaticCondition) -> String { }, SC::IsRingBearer => "is the ring-bearer".into(), SC::RingLevelAtLeast { level } => format!("ring level ≥ {level}"), - SC::ControlsCommander { .. } => "you control a commander".into(), + SC::ControlsCommander { ownership } => fmt_commander_ownership(ownership).into(), SC::SourceIsTapped => "source is tapped".into(), SC::IsTapped { .. } => "is tapped".into(), SC::SourceIsSaddled => "source is saddled".into(), @@ -7850,6 +7870,8 @@ fn condition_feature(cond: &AbilityCondition) -> (&'static str, FeatureSupport) AbilityCondition::ManaColorSpent { .. } => ("ManaColorSpent", Handled), AbilityCondition::HasMaxSpeed => ("HasMaxSpeed", Handled), AbilityCondition::IsMonarch => ("IsMonarch", Handled), + // CR 903.3d: evaluated at resolution via `game::commander`. + AbilityCondition::ControlsCommander { .. } => ("ControlsCommander", Handled), // CR 309.7: evaluated at resolution via `dungeon::has_completed_dungeon`. AbilityCondition::CompletedDungeon { .. } => ("CompletedDungeon", Handled), AbilityCondition::IsInitiative => ("IsInitiative", Handled), @@ -8285,6 +8307,27 @@ fn static_condition_feature(cond: &StaticCondition) -> (&'static str, FeatureSup StaticCondition::AnyPlayerAttackedYouLastTurn => ("AnyPlayerAttackedYouLastTurn", Handled), StaticCondition::OpponentPoisonAtLeast { .. } => ("OpponentPoisonAtLeast", Unhandled), StaticCondition::UnlessPay { .. } => ("UnlessPay", Handled), + // CR 903.3d: the RUNTIME does evaluate this static + // (`layers::evaluate_static_condition`, layers.rs:1875, delegating both + // ownership arms to the single `game::commander` authority), so the + // `Unhandled` tag below understates the resolver. + // + // It stays `Unhandled` DELIBERATELY, and must not be flipped as a rider on + // an unrelated change: the tag is currently the only thing holding two + // demonstrably MISPARSED Lieutenant cards out of the supported set. Of the + // seven `ControlsCommander` statics in the pool, Convergence of Dominion + // parses to a static with `modifications: []` (a no-op continuous effect) + // and Thunderfoot Baloth collapses "this creature gets +2/+2 and other + // creatures you control get +2/+2 and have trample" into ONE `SelfRef` + // static, dropping the "other creatures you control" clause and granting + // trample to the Baloth itself. Flipping the tag alone would advertise both + // as `supported: true, gap_count: 0`. + // + // Land the flip in its own change, AFTER an empty-`modifications` static + // and a dropped continuous-modification clause each register as real gaps. + // Nothing in the commander-gate work depends on this tag — Fight for the + // Throne's intervening-`if` is an `AbilityCondition`, classified `Handled` + // in `condition_feature` above. StaticCondition::ControlsCommander { .. } => ("ControlsCommander", Unhandled), // SourceIsEquipped resolved by layers::evaluate_condition (layers.rs:1057) StaticCondition::SourceIsEquipped => ("SourceIsEquipped", Handled), @@ -12229,6 +12272,128 @@ mod tests { assert!(missing.is_empty()); } + /// CR 903.3d: the Lieutenant STATIC's `Unhandled` coverage tag is a + /// deliberate mask, not an oversight — this pins both halves so the flip + /// cannot be smuggled in as a rider on an unrelated change. + /// + /// The runtime DOES evaluate `StaticCondition::ControlsCommander` + /// (`layers::evaluate_static_condition`, layers.rs:1875), so on the + /// resolver axis alone the tag understates the engine. But the tag is also + /// the only thing keeping two demonstrably misparsed Lieutenant cards out + /// of the supported set, so it must stay until those misparses register as + /// real gaps. + /// + /// The second assertion is the evidence, on verbatim Oracle text: Thunderfoot + /// Baloth's "…this creature gets +2/+2 AND OTHER CREATURES YOU CONTROL get + /// +2/+2 and have trample" collapses into a single `SelfRef` static, so the + /// other-creatures clause is silently dropped and trample lands on the Baloth + /// itself — with no gap recorded anywhere. + /// + /// FLIP PROTOCOL: when the dropped-clause misparse (and Convergence of + /// Dominion's empty-`modifications` no-op static) each register as a gap, the + /// second assertion here goes red. THAT is the signal to flip the tag to + /// `Handled` and delete this test — not before. + #[test] + fn lieutenant_commander_static_tag_is_a_deliberate_mask() { + let (label, support) = static_condition_feature(&StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + assert_eq!(label, "ControlsCommander"); + assert!( + matches!(support, FeatureSupport::Unhandled), + "the mask must stay until the two misparses register as gaps; see the \ + FLIP PROTOCOL on this test" + ); + + let parsed = crate::parser::parse_oracle_text( + "Trample\nLieutenant — As long as you control your commander, this creature gets \ + +2/+2 and other creatures you control get +2/+2 and have trample.", + "Thunderfoot Baloth", + &[], + &["Creature".to_string()], + &["Beast".to_string()], + ); + // Reach-guard: the fixture only exercises the misparse if the parser + // really produced the OWNER-scoped commander gate. + let commander_statics: Vec<_> = parsed + .statics + .iter() + .filter(|s| { + matches!( + &s.condition, + Some(StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Own + }) + ) + }) + .collect(); + assert!( + !commander_statics.is_empty(), + "the Lieutenant line must parse to an Own-scoped ControlsCommander static: {:#?}", + parsed.statics + ); + assert!( + commander_statics + .iter() + .all(|s| matches!(s.affected, Some(TargetFilter::SelfRef))), + "MISPARSE STILL PRESENT (expected): the \"other creatures you control\" clause \ + is dropped and the whole Lieutenant grant lands on SelfRef. When this goes \ + red the parser was fixed — flip `static_condition_feature` to Handled and \ + delete this test. Got {commander_statics:#?}" + ); + } + + /// CR 903.3 vs CR 903.3d: the parse-details label is what bug triage reads, + /// so the two ownership arms must never print the same string — in ANY of + /// the condition-vocabulary formatters. + #[test] + fn commander_ownership_labels_differ_in_every_formatter() { + for (ability_label, trigger_label, static_label) in [ + ( + fmt_ability_condition(&AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }), + fmt_trigger_condition( + &crate::types::ability::TriggerCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }, + ), + fmt_static_condition(&StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }), + ), + ( + fmt_ability_condition(&AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Any, + }), + fmt_trigger_condition( + &crate::types::ability::TriggerCondition::ControlsCommander { + ownership: CommanderOwnership::Any, + }, + ), + fmt_static_condition(&StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Any, + }), + ), + ] { + assert_eq!( + ability_label, trigger_label, + "the three mirrors of ONE printed clause must render identically" + ); + assert_eq!(ability_label, static_label); + } + assert_ne!( + fmt_static_condition(&StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }), + fmt_static_condition(&StaticCondition::ControlsCommander { + ownership: CommanderOwnership::Any, + }), + "CR 903.3 \"your commander\" is strictly narrower than CR 903.3d \"a \ + commander\"; collapsing them prints a weaker predicate than the card" + ); + } + #[test] fn vanilla_object_has_no_unimplemented_mechanics() { let obj = make_obj(); diff --git a/crates/engine/src/game/effects/mod.rs b/crates/engine/src/game/effects/mod.rs index e76a16c5a8..7140a02aa0 100644 --- a/crates/engine/src/game/effects/mod.rs +++ b/crates/engine/src/game/effects/mod.rs @@ -9,13 +9,13 @@ use crate::game::conditions::{ use crate::game::filter; use crate::game::speed::has_max_speed; use crate::types::ability::{ - AbilityCondition, AbilityCost, AbilityKind, CardPlayMode, CardTypeSetSource, ControllerRef, - CopyRetargetPermission, CostPaidObjectSnapshot, EachDamageRecipient, Effect, EffectError, - EffectKind, EffectOutcomeSignal, EffectResolutionResult, EffectScope, FilterProp, - OpponentMayScope, PlayerFilter, PlayerScope, QuantityExpr, QuantityRef, RepeatContinuation, - ResolvedAbility, RevealUntilDisposition, SacrificeCost, SacrificeRequirement, SharedQuality, - SharedQualityRelation, SiblingCondition, SubAbilityLink, TapStateChange, TargetChoiceTiming, - TargetFilter, TargetRef, ThisWayCause, + AbilityCondition, AbilityCost, AbilityKind, CardPlayMode, CardTypeSetSource, + CommanderOwnership, ControllerRef, CopyRetargetPermission, CostPaidObjectSnapshot, + EachDamageRecipient, Effect, EffectError, EffectKind, EffectOutcomeSignal, + EffectResolutionResult, EffectScope, FilterProp, OpponentMayScope, PlayerFilter, PlayerScope, + QuantityExpr, QuantityRef, RepeatContinuation, ResolvedAbility, RevealUntilDisposition, + SacrificeCost, SacrificeRequirement, SharedQuality, SharedQualityRelation, SiblingCondition, + SubAbilityLink, TapStateChange, TargetChoiceTiming, TargetFilter, TargetRef, ThisWayCause, }; #[cfg(test)] use crate::types::ability::{AttackScope, AttackSubject}; @@ -2933,6 +2933,81 @@ fn condition_depends_on_effect_performed(condition: &AbilityCondition) -> bool { } } +/// CR 603.12 + CR 615.5: Whether a SUB-ability's own gate makes it resolve even +/// though its parent's condition was FALSE — the single authority for that +/// question, with two consumers. +/// +/// * `resolve_ability_chain` (below) uses it on the condition-false path to +/// decide which sub-abilities still run. +/// * `triggers::delayed_body_outlives_a_false_gate` uses it so the CR 603.4 +/// fire-time hoist of a DELAYED body's intervening-`if` declines on exactly the +/// sub shapes this resolution path would still have run. If the two predicates +/// drifted, the hoist would either delete printed work (hoisting a body whose +/// sub survives) or leave CR 603.4 unenforced (declining a body whose sub does +/// not). +/// +/// The two classes: a CR 603.12 performed/reflexive gate, whose truth is only +/// knowable at resolution and is re-evaluated on its own; and a CR 615.5 +/// INDEPENDENT per-event gate (Comeuppance's mutually-exclusive +/// creature/noncreature reflection riders), which references the event rather +/// than the parent's effect. +/// +/// NOT included, deliberately: an unconditional `SequentialSibling` (it also +/// runs on the false path, but as the next clause of the SAME gated sentence — +/// CR 603.4 says it must not happen either, which is precisely why the delayed +/// hoist must still apply to it) and `SiblingCondition::ReplicatedOrBranch` +/// (a structural marker on the sub, not a condition, so it is tested against +/// the sub itself by [`sub_outlives_false_parent_gate`], which wraps this). +pub(crate) fn condition_survives_false_parent_gate(condition: &AbilityCondition) -> bool { + condition_depends_on_effect_performed(condition) + || matches!( + condition, + AbilityCondition::PostReplacementDamageSourceMatchesFilter { .. } + ) +} + +/// CR 603.12 + CR 615.5 + CR 702.1c: whether a SUB-ability still resolves even +/// though its parent's condition was FALSE, for a reason INDEPENDENT of the +/// parent's gate — the single authority for that whole question. +/// +/// Two components, and both must stay together: +/// +/// * the sub's own gate is one of the classes +/// [`condition_survives_false_parent_gate`] names (a CR 603.12 performed / +/// reflexive gate, or a CR 615.5 independent per-event gate); +/// * the sub is a per-item keyword-list REPLICATION branch (CR 702.1c "the same +/// is true" — Kathril / Mutable Pupa). That is a structural marker on the sub +/// (`SiblingCondition::ReplicatedOrBranch`) rather than a condition, and it +/// only means "independent OR-branch" on a `SequentialSibling` link, so BOTH +/// conjuncts are part of the test. +/// +/// Two consumers, which is why this is factored out rather than spelled twice: +/// `resolve_ability_chain` (below) on its condition-false path, and +/// `triggers::delayed_body_outlives_a_false_gate`, so the CR 603.4 fire-time +/// hoist of a DELAYED body's intervening-`if` declines on EXACTLY the sub shapes +/// this resolution path still runs. When the two spellings drifted, the hoist +/// could either delete printed work (hoisting a body whose sub survives) or +/// leave CR 603.4 unenforced (declining a body whose sub does not). +/// +/// Both consumers ask the question of the DIRECT sub only. This call site never +/// looks deeper: when the direct sub does not qualify, `resolve_chain_body` +/// returns and nothing further down the chain resolves, so a grandchild's +/// `else_ability` or reflexive gate is reachable only THROUGH a qualifying direct +/// sub. `delayed_body_outlives_a_false_gate` matches that exactly (it used to +/// recurse, which declined the hoist for chains whose resolution does nothing). +/// +/// NOT included: an UNCONDITIONAL `SequentialSibling`. It also runs on the false +/// path, but as the next clause of the SAME gated sentence, so CR 603.4 says it +/// must not happen either — the delayed hoist must still apply to it. That +/// disjunct therefore stays at the `resolve_ability_chain` call site alone. +pub(crate) fn sub_outlives_false_parent_gate(sub: &ResolvedAbility) -> bool { + sub.condition + .as_ref() + .is_some_and(condition_survives_false_parent_gate) + || (sub.sibling_condition == SiblingCondition::ReplicatedOrBranch + && sub.sub_link == SubAbilityLink::SequentialSibling) +} + /// CR 603.12 + CR 608.2c: Whether a reflexive condition reads the per-resolution /// `last_zone_changed_ids` ledger ("if a [noun] was [verb]ed this way"). Unlike /// `condition_depends_on_effect_performed` (which gates on the @@ -3236,6 +3311,9 @@ fn condition_reads_filter_population( | AbilityCondition::IsInitiative | AbilityCondition::HasCityBlessing | AbilityCondition::HasEnduringStory + // CR 903.3d: a commander-designation predicate — carries neither a + // `TargetFilter` nor a `QuantityExpr`, so it reads no filter population. + | AbilityCondition::ControlsCommander { .. } | AbilityCondition::IsRingBearer | AbilityCondition::CompletedDungeon { .. } | AbilityCondition::TargetHasKeywordInstead { .. } @@ -3637,6 +3715,10 @@ fn should_resolve_subability_on_optional_decline(ability: &ResolvedAbility) -> b | AbilityCondition::IsInitiative | AbilityCondition::HasCityBlessing | AbilityCondition::HasEnduringStory + // CR 903.3d: a live game-state gate, not a decline-alternative + // selector — declining the optional effect does not pick a + // commander-control branch. Same reading as `IsMonarch` above. + | AbilityCondition::ControlsCommander { .. } | AbilityCondition::DiscardedCardMatchesFilter { .. } | AbilityCondition::IsRingBearer | AbilityCondition::TargetHasKeywordInstead { .. } @@ -9676,10 +9758,6 @@ fn resolve_chain_body( // must be evaluated on its own regardless of whether this node's // gate held. Without this, the noncreature rider never fires when // the creature rider's gate is false (and vice-versa). - let sub_has_independent_event_gate = matches!( - sub.condition.as_ref(), - Some(AbilityCondition::PostReplacementDamageSourceMatchesFilter { .. }) - ); // CR 702.1c ("the same is true") + CR 608.2c (written order): A // sub produced by per-item keyword-list replication // (`SiblingCondition::ReplicatedOrBranch`) is an INDEPENDENT @@ -9695,15 +9773,16 @@ fn resolve_chain_body( // above, keyed on the replication marker rather than the condition // variant (the gate here is a plain `ZoneChangeObjectMatchesFilter` // / `QuantityCheck` that would otherwise look dependent). - let sub_is_replicated_or_branch = sub.sibling_condition - == SiblingCondition::ReplicatedOrBranch - && sub.sub_link == SubAbilityLink::SequentialSibling; - if sub - .condition - .as_ref() - .is_some_and(condition_depends_on_effect_performed) - || sub_has_independent_event_gate - || sub_is_replicated_or_branch + // + // All THREE of the above independent-sub classes are the single + // authority `sub_outlives_false_parent_gate`, which + // `triggers::delayed_body_outlives_a_false_gate` also consults so + // the CR 603.4 fire-time hoist declines on EXACTLY the sub shapes + // this resolution path still runs. Only the unconditional + // `SequentialSibling` escape below is local to this call site — + // CR 603.4 says that clause must NOT survive a false gate on a + // delayed body, so the hoist deliberately does not mirror it. + if sub_outlives_false_parent_gate(sub) || (sub.sub_link == SubAbilityLink::SequentialSibling && sub.condition.is_none()) { @@ -12472,6 +12551,29 @@ pub(crate) fn evaluate_condition( crate::game::restrictions::spell_cast_with_variant_this_turn(state, variant) } AbilityCondition::IsMonarch => eval_is_monarch(state, ability.controller), + // CR 903.3d: "If an effect refers to controlling a commander, it refers to + // a permanent on the battlefield that is a commander." CR 903.3 + CR 109.5 + // narrow the Lieutenant reading ("your commander") to a commander the + // player also OWNS, because the designation is an attribute of the card + // itself — a stolen commander is still its owner's. + // + // Delegates to the single `game::commander` authority — the same helpers + // `layers::evaluate_static_condition`, `triggers` and `restrictions` use + // for the three sibling mirrors — so `Own` cannot be silently widened to + // "any commander you control", and the CR 702.26b phased-out exclusion is + // applied once, in one place. + // + // CR 109.5 + CR 603.7d: "you" is the resolving ability's controller; for a + // delayed triggered ability that is the player who controlled the creating + // spell as it resolved, which is already stamped onto the delayed ability. + AbilityCondition::ControlsCommander { ownership } => match ownership { + CommanderOwnership::Own => { + crate::game::commander::controls_own_commander(state, ability.controller) + } + CommanderOwnership::Any => { + crate::game::commander::controls_any_commander(state, ability.controller) + } + }, // CR 309.7: dungeon completion is a controller-state predicate, evaluated // as the ability resolves. Delegates to the single truth function shared // with `TriggerCondition::CompletedDungeon` so the intervening-if reading @@ -13329,6 +13431,85 @@ mod tests { // that much damage. Before the fix the returned card was not bound as the // earlier-instruction referent (only PUBLIC-zone moves were), so the damage // resolved to 0. + /// CR 903.3 vs CR 903.3d: the `evaluate_condition` commander arm's TWO + /// ownership readings must stay distinct, and BOTH must be driven. + /// + /// `Own` (CR 903.3 + CR 109.5, the Lieutenant reading) is owned AND + /// controlled; `Any` (CR 903.3d) is controlled by any owner. The single + /// discriminating fixture is a STOLEN commander: P1 owns it, P0 controls it. + /// `Any` must be true and `Own` must be false against the same state — an + /// `Any` arm that collapsed to the `Own` helper (or vice versa) fails here + /// even though every card shipping today produces only `Own`. + /// + /// This is deliberately a unit assertion on the production resolver arm + /// rather than a card fixture: a card-data census shows Fight for the Throne + /// is the sole producer of `AbilityCondition::ControlsCommander` and it is + /// `Own`, so the `Any` arm has no reachable card to drive it yet. + #[test] + fn evaluate_condition_commander_ownership_arms_are_distinct() { + let mut state = GameState::new_two_player(42); + // CR 903.3: the designation is an attribute of the CARD, so this + // commander stays P1's however control moves. + let stolen = create_object( + &mut state, + CardId(1), + PlayerId(1), + "Opposing Commander".to_string(), + Zone::Battlefield, + ); + { + let obj = state.objects.get_mut(&stolen).expect("staged commander"); + obj.is_commander = true; + // CR 613.1b (Layer 2): control moved to P0, ownership unchanged. + obj.base_controller = Some(PlayerId(0)); + obj.controller = PlayerId(0); + } + // Non-vacuity guard: the owner/controller divergence must really exist, + // or neither assertion below discriminates anything. + let staged = state.objects.get(&stolen).expect("staged commander"); + assert_eq!( + staged.owner, + PlayerId(1), + "the fixture only isolates the owner conjunct if P1 still OWNS it" + ); + assert_eq!( + staged.controller, + PlayerId(0), + "the fixture only isolates the controller conjunct if P0 CONTROLS it" + ); + + let ability = ResolvedAbility::new( + Effect::BecomeMonarch, + Vec::new(), + ObjectId(999), + PlayerId(0), + ); + + assert!( + evaluate_condition( + &AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Any, + }, + &state, + &ability, + ), + "CR 903.3d: \"a commander\" is controller-only, so a stolen opponent's \ + commander satisfies it" + ); + assert!( + !evaluate_condition( + &AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }, + &state, + &ability, + ), + "CR 903.3 + CR 109.5: \"your commander\" also requires ownership, so the \ + stolen commander must NOT satisfy it. A true here means the Own arm was \ + widened to controls_any_commander" + ); + } + #[test] fn continuation_resume_preserves_live_delayed_trigger_firing() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/game/engine.rs b/crates/engine/src/game/engine.rs index 01f19f68d3..ad7ca50d0f 100644 --- a/crates/engine/src/game/engine.rs +++ b/crates/engine/src/game/engine.rs @@ -16108,9 +16108,126 @@ mod stage2_injector_tests { // Current-main port: #7221's typed player-action completion seam and the // contemporaneous upstream changes moved these three producers. Re-derived // in the merged source, still in their named production functions. - "game/effects/mod.rs:6640".to_string(), - "game/effects/mod.rs:6717".to_string(), - "game/effects/mod.rs:9922".to_string(), + // + // Fight for the Throne commander-gate unit (base 8035813e6): + // `:6640/:6717/:9922 ⇒ :6647/:6724/:9929`, uniform +7. LOCAL, not upstream, + // so the CI-vs-local diagnosis in the header does not apply. `git diff -U0` + // on effects/mod.rs has exactly four hunks: `@@ -12,7 +12,7 @@` (net 0 — the + // `CommanderOwnership` import reflow), `@@ -3238,0 +3239,3 @@` (+3, the + // `AbilityCondition::ControlsCommander` leaf arm in + // `condition_reads_filter_population`), `@@ -3639,0 +3643,4 @@` (+4, the same + // variant joining `should_resolve_subability_on_optional_decline`'s live-gate + // list), and `@@ -12474,0 +12482,23 @@` (+23, the `evaluate_condition` + // delegation to `game::commander`) which sits BELOW all three producers and + // therefore moves none of them. 3 + 4 = the whole +7, and predicted + // `6640+7`/`6717+7`/`9922+7` equal the observed coordinates exactly. None of + // the three arms mints a prompt — they are condition CLASSIFICATION arms plus + // one boolean predicate delegation. Identity re-established, not assumed: each + // producer at its new coordinate is sha256-identical to + // `8035813e6:effects/mod.rs` at its old one (`a8512b402f8675b7`, + // `82c6c569182ae4ed`, `c9d8e7ba3b9e29e2`) and still sits inside the enclosing + // function this row NAMES (`drive_sequential_repeated_optional_payment` ×2, + // `resolve_chain_body`). The diff instrument discriminates: the three OLD + // coordinates now hold a `PendingRepeatedOptionalPayment` field init, a + // `payment_unit` argument, and a `trigger_events` clone — none of which mints + // anything. Set preservation: the two asserts above this one ran FIRST and + // both fired GREEN on the run that caught this (total still 37, partition + // still 5/7/25), and the other two entries did not move. + // + // Fight for the Throne review-fix round (same base 8035813e6): + // `:6647/:6724/:9929 ⇒ :6680/:6757/:9964`, i.e. `+40/+40/+42` measured + // from BASE (`:6640/:6717/:9922`), not from the row above. LOCAL, not + // upstream, so the CI-vs-local diagnosis in the header does not apply. + // `git diff -U0 8035813e6` on effects/mod.rs now has eight hunks; the + // ones that move a producer are, in order: `@@ -2935,0 +2936,33 @@` + // (+33, `condition_survives_false_parent_gate` — the single authority + // the CR 603.4 delayed-hoist carve-out now shares with + // `resolve_chain_body`), `@@ -3238,0 +3272,3 @@` (+3) and + // `@@ -3639,0 +3676,4 @@` (+4) — the two condition-classification arms + // already logged above. 33 + 3 + 4 = the `+40` on the first two. The + // third takes a further `+2` from the two hunks INSIDE + // `resolve_chain_body` and above its own gate: + // `@@ -9679,4 +9719,10 @@` (+6, the twin sub-gate clauses collapsed + // into the shared `condition_survives_false_parent_gate` call) and + // `@@ -9701,5 +9747 @@` (−4, the corresponding conjunct removal); the + // remaining hunks (`@@ -12,7 +12,7 @@` net 0, the `evaluate_condition` + // delegation, and `mod tests`) are net-zero or BELOW all three. + // Predicted `6640+40`/`6717+40`/`9922+42` equal the observed + // coordinates exactly. None of the moved code mints a prompt: it is one + // boolean condition classifier plus the call sites that consume it. + // Identity re-established, not assumed: each producer at its new + // coordinate is sha256-identical to `8035813e6:effects/mod.rs` at its + // old one (`9869a19f28c791ee`, `2bc316e3aa0297f8`, `8df98486627bfe15`) + // and still sits inside the enclosing production function it always + // did — `drive_sequential_repeated_optional_payment` (6653-6687), + // `resolve_repeated_optional_payment_choice` (6695-6779) and + // `resolve_chain_body`. (The row above names the first two as + // `drive_sequential_repeated_optional_payment` ×2; re-derived here, the + // second is the `resolve_repeated_optional_payment_choice` resume arm, + // which is what the older entries called it.) The diff instrument + // discriminates: the three OLD coordinates now hold a blank line, an + // `.active_repeated_optional_payment_frame_mut()` call and a `//` + // comment, none of which mints anything. Set preservation: the two + // asserts above this one ran FIRST and both fired GREEN on the run that + // caught this (total still 37, partition still 5/7/25), and the other + // two entries did not move. + // + // Fight for the Throne review-fix round 2 (same base 8035813e6): + // `:6715/:6792/:9994`, i.e. `+75/+75/+72` measured from BASE + // (`:6640/:6717/:9922`), not from the row above. LOCAL, not upstream, so + // the CI-vs-local diagnosis in the header does not apply. `git diff -U0 + // 8035813e6` on effects/mod.rs has eight hunks; above the first two + // producers are `@@ -2935,0 +2936,68 @@` (+68 — the condition classifier + // of the row above, now also carrying the shared + // `sub_outlives_false_parent_gate` authority the CR 603.4 delayed-hoist + // carve-out and `resolve_chain_body` both call), `@@ -3238,0 +3307,3 @@` + // (+3) and `@@ -3639,0 +3711,4 @@` (+4): 68 + 3 + 4 = the `+75`. The third + // producer nets `−3` more from the two hunks INSIDE `resolve_chain_body` + // and above its own gate — `@@ -9679,4 +9753,0 @@` (−4, the twin + // `sub_survives_false_parent_gate` / `sub_is_replicated_or_branch` locals + // collapsed into the one shared call) and `@@ -9698,9 +9769,10 @@` (+1) — + // giving `+72`; the remaining hunks (`@@ -12,7 +12,7 @@` net 0, the + // `evaluate_condition` delegation, and `mod tests`) are net-zero or BELOW + // all three. Predicted `6640+75`/`6717+75`/`9922+72` equal the observed + // coordinates exactly. None of the moved code mints a prompt: it is one + // boolean sub-classification predicate plus the call site that consumes it. + // Identity re-established, not assumed: each producer at its new coordinate + // is sha256-identical to `8035813e6:effects/mod.rs` at its old one + // (`9869a19f28c791ee`, `2bc316e3aa0297f8`, `8df98486627bfe15`) and still + // sits inside the enclosing production function this row NAMES — + // `drive_sequential_repeated_optional_payment` (opens 6702), + // `resolve_repeated_optional_payment_choice` (opens 6730) and + // `resolve_chain_body`. The diff instrument discriminates: the three OLD + // coordinates now hold a `return Ok(());`, an + // `optional_cost_payments_this_resolution` binding and a + // `resolve_optional_effect_decision(` call, none of which mints anything. + // Set preservation: the two asserts above this one ran FIRST and both + // fired GREEN on the run that caught this (total still 37, partition still + // 5/7/25), and the other two entries did not move. + // Fight for the Throne, same branch, same base `8035813e6`: + // `:6715/:6792/:9994 ⇒ :6722/:6799/:10001`, a UNIFORM `+7` on all three, + // i.e. `+82/+82/+79` measured from the base. The row above measured + // `+75/+75/+72` from that same base, so the delta is `+7` and its sole + // cause is that the FIRST hunk GREW: `@@ -2935,0 +2936,68 @@ ⇒ + // @@ -2935,0 +2936,75 @@`, the `condition_survives_false_parent_gate` + // doc/authority block picking up seven more lines. No hunk was added or + // removed and none changed size: the other four are byte-for-byte the + // sizes the row above names (`+3` at `:3307 ⇒ :3314`, `+4` at + // `:3711 ⇒ :3718`, `−4` at `:9753 ⇒ :9760`, `+1` at `:9769 ⇒ :9776`) and + // merely shifted by the same `+7`, which is what makes the shift uniform + // even for the third producer (its `−4/+1` pair still nets the same `−3` + // below the other two). Predicted `6640+82`/`6717+82`/`9922+79` against + // `8035813e6` equal the observed coordinates exactly. Identity + // re-established, not assumed: each producer at its new coordinate is + // sha256-identical to `8035813e6:effects/mod.rs` at its old one + // (`7067db50922da31f`, `975791a569b1f587`, `967e35eb66a5780b` over the + // 15-line mint expression at each site). This card's own effects/mod.rs + // edits — the two `AbilityCondition::ControlsCommander` registrations at + // `:3314`/`:3718` and the `evaluate_condition` arm below all three — mint + // nothing: they are classifier list entries and one condition evaluator. + "game/effects/mod.rs:6722".to_string(), + "game/effects/mod.rs:6799".to_string(), + "game/effects/mod.rs:10001".to_string(), // UNMOVED across the rebase, and that is itself evidence the SET did not // move: a census that had gained or lost a producer would not leave this // entry both byte-identical AND at the same coordinate. diff --git a/crates/engine/src/game/scenario.rs b/crates/engine/src/game/scenario.rs index c2b40851ad..e8ba7f6020 100644 --- a/crates/engine/src/game/scenario.rs +++ b/crates/engine/src/game/scenario.rs @@ -1156,6 +1156,57 @@ impl<'a> CardBuilder<'a> { // --- Special modifiers --- + /// CR 903.3: Mark this object as its owner's commander IN PLACE, without + /// moving it to the command zone (unlike `GameScenario::with_commander`, + /// which forces `Zone::Command`). + /// + /// CR 903.3d resolves "controlling a commander" against a permanent ON THE + /// BATTLEFIELD, so every "you control your/a commander" gate — Lieutenant + /// statics, Lieutenant triggers, activation restrictions, and + /// resolution-time gates alike — needs a battlefield commander to be + /// reachable at all. This is that building block. + pub fn commander(&mut self) -> &mut Self { + self.obj().is_commander = true; + self + } + + /// CR 613.1b (Layer 2: control-changing effects) + CR 109.5: put this object + /// under `player`'s control while leaving `owner` unchanged — the + /// owner/controller divergence a stolen permanent has. + /// + /// This is the only way to exercise the two conjuncts of + /// `game::commander::controls_own_commander` (owner, then controller) + /// independently, which CR 903.3 makes observable: the commander designation + /// is an attribute of the card, so a stolen commander is still its owner's. + /// + /// Sets BOTH fields on purpose: Layer 2 recomputes `obj.controller` from + /// `base_controller.unwrap_or(owner)` on every `evaluate_layers` pass, so + /// setting `controller` alone is silently reverted by the first layer pass a + /// cast pipeline runs. `controller` is set too so the state is coherent + /// before any layer pass. + /// + /// Battlefield-only: `state.battlefield` is a flat list with no per-player + /// split, so no zone-list bookkeeping is needed. Do NOT use this for + /// hand/library/graveyard objects, whose zone lists are keyed by owner — + /// the `debug_assert_eq!` below ENFORCES that precondition rather than + /// merely documenting it, so a misapplied call fails loudly at the fixture + /// that wrote it instead of desynchronizing an owner-keyed zone list and + /// surfacing somewhere unrelated. + pub fn controlled_by(&mut self, player: PlayerId) -> &mut Self { + let obj = self.obj(); + debug_assert_eq!( + obj.zone, + Zone::Battlefield, + "CardBuilder::controlled_by is battlefield-only: object {:?} is in {:?}, whose \ + zone list is keyed by OWNER, so diverging the controller would corrupt the fixture", + obj.id, + obj.zone, + ); + obj.base_controller = Some(player); + obj.controller = player; + self + } + /// Mark this creature as having summoning sickness (entered this turn). pub fn with_summoning_sickness(&mut self) -> &mut Self { let turn = self.state.turn_number; diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 33869320d2..2418be81a1 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -7,10 +7,11 @@ use crate::types::ability::{ AbilityCondition, AbilityCost, AbilityDefinition, AbilityKind, AdditionalCostOrigin, BounceSelection, CardTypeSetSource, CastManaSpentMetric, ChosenAttribute, CommanderOwnership, ControllerRef, CopyRetargetPermission, DamageKindFilter, DelayedTriggerCondition, Effect, - ModalChoice, ObjectScope, OriginConstraint, PlayerFilter, PtValue, QuantityExpr, QuantityRef, - RenownSubject, ResolvedAbility, SacrificeCost, TargetFilter, TargetRef, TributeOutcome, - TriggerCondition, TriggerConstraint, TriggerDefinition, TriggerDefinitionOccurrenceRef, - TriggerDefinitionRef, TriggerEntry, TriggerGrantProducerKey, TypeFilter, TypedFilter, + FilterProp, ModalChoice, ObjectScope, OriginConstraint, PlayerFilter, PlayerScope, PtValue, + QuantityExpr, QuantityRef, RenownSubject, ResolvedAbility, SacrificeCost, StaticCondition, + TargetFilter, TargetRef, TributeOutcome, TriggerCondition, TriggerConstraint, + TriggerDefinition, TriggerDefinitionOccurrenceRef, TriggerDefinitionRef, TriggerEntry, + TriggerGrantProducerKey, TypeFilter, TypedFilter, }; #[cfg(test)] use crate::types::ability::{EffectScope, TapStateChange}; @@ -8866,16 +8867,572 @@ fn expand_multi_fire_damage_occurrences( } } +/// CR 603.4 + CR 608.2c: does this delayed body still carry work that the +/// resolution-time reading performs when the gate is FALSE — work the fire-time +/// hoist would silently DELETE? +/// +/// This is the carve-out for [`delayed_intervening_if`], and it is deliberately +/// narrower than the "any `sub_ability`" test it replaces. The two cases the old +/// blanket conflated are NOT the same: +/// +/// * `else_ability` ("…, if X, A. Otherwise, B.") is not an intervening-`if` in +/// the CR 603.4 sense at all — the ability has printed work on the false path, +/// so it MUST still trigger and resolve its else branch. Always declines. +/// * An UNCONDITIONAL `SequentialSibling` sub is the second clause of the ONE +/// gated sentence ("…, if you control your commander, draw a card and create a +/// token"). CR 603.4 gates the whole ability, so that clause must not happen +/// either — yet `resolve_ability_chain` (`effects/mod.rs`, the +/// `sub_link == SequentialSibling && condition.is_none()` escape) resolves it +/// even when the parent gate is false. Declining the hoist here is what LEAVES +/// that violation in place, so this class must hoist. +/// +/// The three sub shapes that genuinely survive a false parent gate mirror +/// `resolve_ability_chain`'s own escape hatches one-for-one — literally, by +/// calling the SAME predicate (`effects::sub_outlives_false_parent_gate`) rather +/// than restating it, so the pair cannot drift: +/// +/// 1. a reflexive / performed gate (`condition_depends_on_effect_performed`, +/// CR 603.12 — Council's Deliberation's `OptionalEffectPerformed` rider, the +/// only conditioned delayed body with a sub in the card pool today). Its truth +/// is only knowable at resolution; hoisting the parent gate is CONSERVATIVELY +/// declined so that in-pool shape keeps byte-identical behaviour; +/// 2. an INDEPENDENT per-event gate (CR 615.5 +/// `PostReplacementDamageSourceMatchesFilter`, Comeuppance's mutually-exclusive +/// riders), which resolves on its own predicate regardless of the parent's; +/// 3. a `SiblingCondition::ReplicatedOrBranch` on a `SequentialSibling` link +/// (CR 702.1c keyword-list replication, Kathril / Mutable Pupa), an +/// independent OR-branch gated on its own keyword. +/// +/// Inspects the DIRECT sub only, because that is the whole of what +/// `resolve_chain_body`'s condition-false path inspects: it resolves +/// `else_ability` if present, else the direct `sub_ability` when +/// `sub_outlives_false_parent_gate` (or the unconditional-`SequentialSibling` +/// escape) accepts it, and otherwise RETURNS — nothing deeper in the chain runs. +/// A grandchild's `else_ability` or CR 603.12 reflexive gate is therefore reached +/// only THROUGH a direct sub that already qualified, so recursing past the direct +/// sub would decline the hoist for chains whose resolution does nothing at all, +/// leaving CR 603.4's fire-time half unenforced for that shape. Pinned by +/// `two_deep_chain_mirrors_the_resolvers_direct_sub_test`. +fn delayed_body_outlives_a_false_gate(ability: &ResolvedAbility) -> bool { + if ability.else_ability.is_some() { + return true; + } + let Some(sub) = ability.sub_ability.as_deref() else { + return false; + }; + crate::game::effects::sub_outlives_false_parent_gate(sub) +} + +/// CR 603.4: does this gate read a binding that the FIRE-TIME leg of the hoist +/// resolves DIFFERENTLY from the resolution-time leg it is supposed to mirror? +/// +/// The two legs of the CR 603.4 pair must be the same predicate over the same +/// values. They are not the same *evaluator*: the fire-time leg runs +/// `check_trigger_condition_with_source`, whose `QuantityContext` +/// (`quantity::resolve_quantity_for_trigger_check`) is built from the delayed +/// ability's controller, its CR 400.7 source context and the matched event — it +/// has NO access to the delayed ability's snapshotted `targets` and no +/// resolution-scoped player. So a gate that reads one of those resolves against +/// the wrong object or player at fire time and would gate the ability off the +/// stack (and, for a consumed one-shot, delete it) on a value the resolution-time +/// reader would never have computed. +/// +/// Rather than let the two legs disagree, such a gate DECLINES the hoist and +/// keeps today's resolution-only reading — the same conservative treatment the +/// two bridges already give every other resolution-context predicate. +/// +/// Only `QuantityCheck` carries a `QuantityExpr` (and hence a scope or filter) +/// among the arms `ability_condition_to_static_condition` can bridge — the +/// others (`IsYourTurn`, `CompletedDungeon`, `SourceAttachedToCreature`, +/// `ControlsCommander`) are payload-free. `And`/`Or` do not bridge today; they +/// recurse here anyway so adding them to the bridge cannot silently bypass this. +fn gate_binding_diverges_at_fire_time(condition: &AbilityCondition) -> bool { + match condition { + AbilityCondition::QuantityCheck { lhs, rhs, .. } => { + quantity_expr_binding_diverges(lhs) || quantity_expr_binding_diverges(rhs) + } + AbilityCondition::Not { condition } => gate_binding_diverges_at_fire_time(condition), + AbilityCondition::And { conditions } | AbilityCondition::Or { conditions } => { + conditions.iter().any(gate_binding_diverges_at_fire_time) + } + _ => false, + } +} + +/// CR 603.4: the `QuantityExpr` half of [`gate_binding_diverges_at_fire_time`]. +/// Exhaustive over `QuantityExpr` so a new arithmetic wrapper cannot hide a +/// divergent leaf; the leaf test is [`quantity_ref_binding_diverges`]. +fn quantity_expr_binding_diverges(expr: &QuantityExpr) -> bool { + match expr { + QuantityExpr::Ref { qty } => quantity_ref_binding_diverges(qty), + QuantityExpr::Offset { inner, .. } + | QuantityExpr::ClampMin { inner, .. } + | QuantityExpr::Multiply { inner, .. } + | QuantityExpr::DivideRounded { inner, .. } + | QuantityExpr::UpTo { max: inner } + | QuantityExpr::Power { + exponent: inner, .. + } => quantity_expr_binding_diverges(inner), + QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => { + exprs.iter().any(quantity_expr_binding_diverges) + } + QuantityExpr::Difference { left, right } => { + quantity_expr_binding_diverges(left) || quantity_expr_binding_diverges(right) + } + QuantityExpr::Fixed { .. } => false, + } +} + +/// CR 115.1 + CR 608.2c: an object-axis scope the fire-time `QuantityContext` +/// cannot bind the way the resolving ability does. +/// +/// FAIL-CLOSED, and exhaustive so a new scope must be adjudicated: only the +/// three referents `quantity::resolve_quantity_for_trigger_check` is actually +/// handed — the CR 400.7 source context and the matched event's source/target — +/// are known to read the same on both legs. Everything else is a +/// RESOLUTION-scoped referent: `Target` reads `ability.targets`, `Recipient` is +/// passed as `None`, and the `CostPaidObject` / anaphor / per-resolution-local +/// family resolves through `ResolvedAbility` fields and `effect_context_object`, +/// none of which exist at detection time. Declining costs nothing but the +/// fire-time half of CR 603.4 for shapes no card in the pool has; guessing wrong +/// deletes a real ability. +fn object_scope_unbound_at_fire_time(scope: ObjectScope) -> bool { + match scope { + ObjectScope::Source | ObjectScope::EventSource | ObjectScope::EventTarget => false, + ObjectScope::Target + | ObjectScope::Recipient + | ObjectScope::CostPaidObject + | ObjectScope::Anaphoric + | ObjectScope::Demonstrative + | ObjectScope::OtherRevealedCard + | ObjectScope::AmassedArmy + | ObjectScope::OwnedLinkedExileCard + | ObjectScope::BatchSource => true, + } +} + +/// CR 115.10 + CR 608.2c: the player-axis counterpart, same fail-closed rule. +/// +/// `ScopedPlayer` is the per-iteration player of the RESOLVING ability, but the +/// fire-time context derives its `scoped_player` from the triggering event +/// (`extract_player_from_event`); `Target`, `RecipientController` and +/// `ParentObjectTargetController` all read `ability.targets` or the layer +/// recipient. The rest are derived from the controller (CR 109.5), the attacking +/// source (CR 508.5) or the source's own persisted choice (CR 613.1), all of +/// which the fire-time check has. `AnyTurn` is duration-timing-only and never +/// reaches a quantity, but is adjudicated here rather than wildcarded. +fn player_scope_unbound_at_fire_time(scope: &PlayerScope) -> bool { + match scope { + PlayerScope::ScopedPlayer + | PlayerScope::Target + | PlayerScope::RecipientController + | PlayerScope::ParentObjectTargetController => true, + PlayerScope::AllPlayers { exclude, .. } => exclude + .as_deref() + .is_some_and(player_scope_unbound_at_fire_time), + PlayerScope::Controller + | PlayerScope::Opponent { .. } + | PlayerScope::DefendingPlayer + | PlayerScope::SourceChosenPlayer + | PlayerScope::AnyTurn => false, + } +} + +/// CR 603.4: the leaf test of [`gate_binding_diverges_at_fire_time`]. +/// +/// Two independent reasons a leaf diverges: +/// +/// * it is scoped to an object or player the fire-time context cannot bind +/// (`object_scope_unbound_at_fire_time` / `player_scope_unbound_at_fire_time`); +/// * it counts a POPULATION whose filter the trigger-side bridge REWRITES. +/// `oracle_trigger::static_condition_to_trigger_condition` substitutes +/// `FilterProp::Another` → `FilterProp::OtherThanTriggerObject` on the +/// fire-time leg only (CR 603.4, Valakut's ruling), while +/// `effects::evaluate_condition` keeps the source-exclusion reading — so the +/// same printed "two or more OTHER creatures" counts a different population on +/// each leg. Declining is the conservative half of that pair; the alternative +/// (substituting on both legs) would change the resolution-time reading of +/// every non-delayed consumer of the same condition. +/// +/// * it reads a RESOLUTION-SCOPED tally that only exists while the ability is +/// resolving. The fire-time leg calls `resolve_quantity_for_trigger_check`, +/// which resolves with `targets = &[]`, `chosen_x = None`, `ability = None` +/// and no resolution-local ledger, so a payload-free leaf can diverge just as +/// badly as a scoped one (CR 608.2c: the chain tallies are established BY the +/// resolution). `TrackedSetSize` and friends read the most recent tracked set +/// at READ time — at fire time that is whatever an unrelated earlier +/// resolution left behind. +/// +/// EXHAUSTIVE and wildcard-free (matching `object_scope_unbound_at_fire_time` / +/// `quantity_expr_binding_diverges`), so a new `QuantityRef` must be adjudicated +/// here rather than silently defaulting to "cannot diverge" — the earlier +/// `_ => false` tail rested on exactly that claim and it was false for the +/// resolution-scoped payload-free family below. When in doubt the answer is +/// `true`: declining costs only the fire-time half of CR 603.4 for that shape, +/// while a wrong `false` deletes a real ability off the stack. +fn quantity_ref_binding_diverges(qty: &QuantityRef) -> bool { + match qty { + QuantityRef::CountersOn { scope, .. } + | QuantityRef::Power { scope } + | QuantityRef::Intensity { scope } + | QuantityRef::Toughness { scope } + | QuantityRef::ObjectManaValue { scope } + | QuantityRef::ObjectColorCount { scope } + | QuantityRef::ObjectNameWordCount { scope } + | QuantityRef::ObjectTypelineComponentCount { scope } + | QuantityRef::ManaSymbolsInManaCost { scope, .. } => { + object_scope_unbound_at_fire_time(*scope) + } + QuantityRef::HandSize { player, .. } + | QuantityRef::LifeTotal { player } + | QuantityRef::GraveyardSize { player, .. } + | QuantityRef::LifeLostThisTurn { player } + | QuantityRef::LifeGainedThisTurn { player } + | QuantityRef::PartySize { player } + | QuantityRef::Speed { player } + | QuantityRef::CardsDrawnThisTurn { player } + | QuantityRef::CardsDiscardedThisTurn { player } + | QuantityRef::LoyaltyAbilitiesActivatedThisTurn { player } => { + player_scope_unbound_at_fire_time(player) + } + QuantityRef::SacrificedThisTurn { player, filter } + | QuantityRef::TokensCreatedThisTurn { player, filter } + | QuantityRef::BattlefieldEntriesThisTurn { player, filter } => { + player_scope_unbound_at_fire_time(player) || filter_binding_diverges(filter) + } + QuantityRef::LandsPlayedThisTurn { player, .. } + | QuantityRef::PlayerActionsThisTurn { player, .. } => { + player_scope_unbound_at_fire_time(player) + } + QuantityRef::ObjectCount { filter } + | QuantityRef::ObjectCountDistinct { filter, .. } + | QuantityRef::ObjectCountBySharedQuality { filter, .. } + | QuantityRef::CountersOnObjects { filter, .. } + | QuantityRef::Aggregate { filter, .. } + | QuantityRef::EnteredThisTurn { filter } + | QuantityRef::DistinctColorsAmongPermanents { filter } + | QuantityRef::DistinctCounterKindsAmong { filter } + | QuantityRef::ControlledByEachPlayer { filter, .. } + | QuantityRef::ZoneChangeCountThisTurn { filter, .. } + | QuantityRef::ZoneChangeAggregateThisTurn { filter, .. } + | QuantityRef::CounterAddedThisTurn { + target: filter, + .. + } => filter_binding_diverges(filter), + QuantityRef::DamageDealtThisTurn { source, target, .. } => { + filter_binding_diverges(source) || filter_binding_diverges(target) + } + // Same filter axis, optional: a `None` filter names no population to + // re-scope, so only the `Some` arm can diverge. + QuantityRef::ZoneCardCount { filter, .. } + | QuantityRef::SpellsCastThisTurn { filter, .. } + | QuantityRef::SpellsCastThisGame { filter, .. } + | QuantityRef::AttackedThisTurn { filter, .. } => { + filter.as_ref().is_some_and(filter_binding_diverges) + } + // CR 205.2a + CR 205.3: the distinct-type family carries its population + // as a `CardTypeSetSource` rather than a bare filter. + QuantityRef::DistinctCardTypes { source } + | QuantityRef::DistinctSubtypes { source, .. } => { + card_type_set_source_binding_diverges(source) + } + // CR 601.2h: `AbilityTarget` is a target-slot read that + // `quantity::resolve_event_scoped_ref` explicitly answers `None` for at + // fire time; `SelfObject` reads `ctx.source` and `TriggeringSpell` is + // resolved from the matched event itself, so both bind identically on + // both legs — except through a `FromSource` metric, which carries its own + // population filter. + QuantityRef::ManaSpentToCast { scope, metric } => { + *scope == crate::types::ability::CastManaObjectScope::AbilityTarget + || match metric { + CastManaSpentMetric::FromSource { source_filter } => { + filter_binding_diverges(source_filter) + } + CastManaSpentMetric::Total + | CastManaSpentMetric::DistinctColors + | CastManaSpentMetric::OfColor { .. } => false, + } + } + // ---- RESOLUTION-SCOPED, payload-free or target-bound: always diverges ---- + // + // CR 115.1: reads the resolving ability's declared targets, which the + // fire-time resolver is handed as `&[]`. + QuantityRef::TargetControllerCounter { .. } + | QuantityRef::TargetObjectManaValue { .. } + | QuantityRef::TargetZoneCardCount { .. } + // CR 107.3a: `X` comes from the resolving ability's `chosen_x`, which is + // `None` at fire time. + | QuantityRef::Variable { .. } + // CR 608.2c: chain-local tracked sets and per-resolution tallies. Each is + // established BY a resolution; at fire time these read whatever an + // unrelated earlier resolution left in the ledger (or nothing at all). + | QuantityRef::TrackedSetSize + | QuantityRef::FilteredTrackedSetSize { .. } + | QuantityRef::TrackedSetAggregate { .. } + | QuantityRef::ExiledFromHandThisResolution + | QuantityRef::PreviousEffectAmount { .. } + | QuantityRef::TimesCostPaidThisResolution + // CR 701.38: the vote tally is published by the resolution that ran the + // vote block. + | QuantityRef::VoteCount { .. } + // CR 106.4: a mana pool the resolution's own costs and mana abilities + // fill and empty; the fire-time reading is a different moment's pool. + | QuantityRef::UnspentMana { .. } + // CR 607.2a + CR 406.6: the source's linked-exile set, read through the + // resolving ability's materialized candidate set + // (`quantity::materialized_linked_exile_candidates`, which reads + // `ability.targets`). + | QuantityRef::CardsExiledBySource + | QuantityRef::ExiledCardPower { .. } + // CR 603.7c + CR 608.2c: the event-context family resolves through + // `state.current_trigger_event(s)` and the resolution-local + // amount/die/substitution cascade (`resolve_ref`'s `EventContextAmount` + // arm), none of which is populated at DETECTION time — the detection-time + // event override is consumed only by `ObjectCount`'s + // `OtherThanTriggerObject` exclusion. + | QuantityRef::EventContextAmount + | QuantityRef::EventContextPlayerCount { .. } + | QuantityRef::EventContextSourceCostX + | QuantityRef::EventContextSourceModesChosen + | QuantityRef::AttachmentsOnLeavingObject { .. } + | QuantityRef::SpellsCastBeforeTriggeringSpell { .. } + | QuantityRef::TriggeringScryLookCount + | QuantityRef::TriggeringScryBottomCount => true, + // ---- Reads that bind IDENTICALLY on both legs ---- + // + // Global or format-level state (CR 103.4, CR 500, CR 117.1), a + // controller-keyed per-turn/per-game accumulator (CR 109.5 — the + // fire-time leg is handed the delayed ability's own controller), or a + // read of the CR 400.7 source object the fire-time context carries + // (`ctx.source`, the same object `ObjectScope::Source` is adjudicated + // non-divergent for above). + QuantityRef::LifeAboveStarting + | QuantityRef::StartingLifeTotal + | QuantityRef::TriggeringDiscoverValue + | QuantityRef::PlayerCount { .. } + | QuantityRef::PlayerCounter { .. } + | QuantityRef::SelfManaValue + | QuantityRef::Devotion { .. } + | QuantityRef::BasicLandTypeCount { .. } + | QuantityRef::CrimesCommittedThisTurn + | QuantityRef::BendTypesThisTurn + | QuantityRef::TurnsTaken + | QuantityRef::ChosenNumber + | QuantityRef::DescendedThisTurn + | QuantityRef::SpellsCastLastTurn + | QuantityRef::DungeonsCompleted + | QuantityRef::CostXPaid + | QuantityRef::KickerCount + | QuantityRef::AdditionalCostPaymentCount + | QuantityRef::AdditionalCostPaymentCountFor { .. } + | QuantityRef::ConvokedCreatureCount + | QuantityRef::ColorsInCommandersColorIdentity + | QuantityRef::CommanderCastFromCommandZoneCount + | QuantityRef::CommanderManaValue { .. } => false, + } +} + +/// CR 603.4: the `CardTypeSetSource` half of [`quantity_ref_binding_diverges`]. +/// Exhaustive for the same reason: each source names a different population, and +/// two of them are resolution-scoped. +fn card_type_set_source_binding_diverges(source: &CardTypeSetSource) -> bool { + match source { + // CR 400.1: a zone census keyed by `CountScope` (controller / opponents / + // all) — the fire-time leg has the controller and reads the same zones. + CardTypeSetSource::Zone { .. } => false, + CardTypeSetSource::Objects { filter } => filter_binding_diverges(filter), + // CR 607.2a + CR 608.2c: the same two resolution-scoped populations + // `CardsExiledBySource` / `TrackedSetSize` are declined for. + CardTypeSetSource::ExiledBySource | CardTypeSetSource::TrackedSet { .. } => true, + } +} + +/// CR 603.4: the filter half of [`quantity_ref_binding_diverges`]. Recurses +/// through exactly the shapes `oracle_trigger::substitute_another_in_filter` +/// rewrites — `Typed` property lists plus the `And`/`Or`/`Not` combinators — so +/// the decline covers precisely the population the fire-time leg would have +/// re-scoped, no more and no less. +fn filter_binding_diverges(filter: &TargetFilter) -> bool { + match filter { + TargetFilter::Typed(tf) => tf + .properties + .iter() + .any(|prop| matches!(prop, FilterProp::Another)), + TargetFilter::Not { filter } => filter_binding_diverges(filter), + TargetFilter::And { filters } | TargetFilter::Or { filters } => { + filters.iter().any(filter_binding_diverges) + } + // CR 115.1 + CR 115.10: resolution-scoped anaphora read `ability.targets` + // / the per-iteration player, neither of which the fire-time context has + // — the population-level counterpart of `ObjectScope::Target`. + TargetFilter::ParentTarget + | TargetFilter::ParentTargetSlot { .. } + | TargetFilter::ParentTargetController + | TargetFilter::ParentTargetOwner + | TargetFilter::ScopedPlayer => true, + _ => false, + } +} + +/// CR 400.1 + CR 603.4: does the STATIC intermediate of the hoist LOSE the zone +/// axis on its way to the fire-time leg? +/// +/// `conditions::ability_condition_to_static_condition` folds the +/// `ObjectCount{filter} >= 1` shape into `StaticCondition::IsPresent`, and +/// `oracle_trigger::static_condition_to_trigger_condition` lowers the AFFIRMATIVE +/// `IsPresent` to `TriggerCondition::ControlsType`, whose evaluator +/// (`check_trigger_condition_with_source`) scans `state.battlefield` and nothing +/// else. The resolution-time leg counts in the FILTER'S OWN zone +/// (`quantity::object_count_matching_ids`, via `TargetFilter::extract_in_zone`). +/// For a filter that names a non-battlefield zone ("if you have a creature card +/// in your graveyard") those are two DIFFERENT predicates — TRUE at resolution, +/// FALSE at fire time — so the hoist would gate the ability off the stack and, +/// for a consumed one-shot, delete it outright. The zone axis is invisible to +/// [`gate_binding_diverges_at_fire_time`], which screens the object, player and +/// `FilterProp::Another` axes of the SAME leaf but not this one, so it is +/// adjudicated here, on the intermediate that actually loses the information. +/// +/// Only the affirmative arm is affected: the NEGATED `IsPresent` bridge already +/// lowers to `QuantityComparison { ObjectCount(f) EQ 0 }`, which resolves through +/// the same zone-aware `QuantityRef::ObjectCount` reader on both legs. +/// `And`/`Or` recurse because the trigger-side bridge maps them member-wise; no +/// `AbilityCondition` reaches them today (`ability_condition_to_static_condition` +/// declines `And`/`Or`), so the recursion is future-proofing, not live behaviour. +/// +/// Declining is the conservative half of the pair, exactly like +/// `gate_binding_diverges_at_fire_time`: the gate keeps today's resolution-only +/// reading rather than being evaluated as a different predicate. +fn static_gate_bridge_loses_zone(condition: &StaticCondition) -> bool { + match condition { + StaticCondition::IsPresent { filter: Some(f) } => f + .extract_in_zone() + .is_some_and(|zone| zone != crate::types::zones::Zone::Battlefield), + StaticCondition::And { conditions } | StaticCondition::Or { conditions } => { + conditions.iter().any(static_gate_bridge_loses_zone) + } + _ => false, + } +} + +/// CR 603.4: recover the intervening-`if` of a DELAYED triggered ability as a +/// trigger-level `TriggerCondition`. +/// +/// A printed "When [event], if [condition], [effect]" trigger carries its gate +/// on `TriggerDefinition.condition`, so both halves of CR 603.4 apply to it: the +/// collection gate refuses to put the ability on the stack, and +/// `stack.rs`'s recheck removes it if the gate flipped. A DELAYED triggered +/// ability parses the very same sentence shape into +/// `Effect::CreateDelayedTrigger`, but the gate lands on the delayed BODY as an +/// `AbilityCondition`, which only the resolution-time reader +/// (`effects::evaluate_condition`) ever consults. That is half of CR 603.4: the +/// ability would still be put onto the stack, be respondable, and count as a +/// triggered ability put onto the stack, when per CR 603.4 it must never have +/// triggered at all. +/// +/// Rather than add a fifth condition-vocabulary mirror, this composes the two +/// EXISTING single-authority bridges — +/// `conditions::ability_condition_to_static_condition` then +/// `oracle_trigger::static_condition_to_trigger_condition`. Both legs decline +/// every resolution-context predicate (`WhenYouDo`, `EffectOutcome`, +/// `HasObjectTarget`, `CoinFlipOutcome`, the casting-context family, …), so only +/// gates that are genuine game-state intervening-`if`s reach the fire-time +/// check; anything else keeps today's resolution-only behaviour unchanged. +/// +/// CR 603.4 governs only an `if` that IMMEDIATELY FOLLOWS the trigger event and +/// gates the WHOLE ability, so a body that still has work to do on the +/// condition-false path is NOT hoisted — see +/// [`delayed_body_outlives_a_false_gate`], which is deliberately NARROWER than +/// "has any sub-ability". +/// +/// The hoisted gate must also be one the two legs read IDENTICALLY, or the pair +/// CR 603.4 requires would be two different predicates — see +/// [`gate_binding_diverges_at_fire_time`] for the object/player/population axes +/// of the leaf, and [`static_gate_bridge_loses_zone`] for the zone axis the +/// `IsPresent` intermediate drops. +fn delayed_intervening_if(ability: &ResolvedAbility) -> Option { + if delayed_body_outlives_a_false_gate(ability) { + return None; + } + let condition = ability.condition.as_ref()?; + if gate_binding_diverges_at_fire_time(condition) { + return None; + } + let static_condition = + crate::parser::oracle_effect::conditions::ability_condition_to_static_condition(condition)?; + if static_gate_bridge_loses_zone(&static_condition) { + return None; + } + crate::parser::oracle_trigger::static_condition_to_trigger_condition(&static_condition) +} + +/// CR 603.4 + CR 603.7b: when the hoisted intervening-`if` was FALSE, is this +/// ONE-SHOT delayed ability CONSUMED by the occurrence it just declined? +/// +/// CR 603.4 says a false gate means the ability "does nothing" — it never +/// triggered. CR 603.7b says a delayed triggered ability "will trigger only +/// once — the next time its trigger event occurs — unless it has a stated +/// duration, such as 'this turn.'" The two only agree on DROPPING the ability +/// when its stated trigger event can occur at most once: +/// +/// * a PHASE-NAMED time ("at the beginning of the next end step"). That named +/// time has now come and gone; a false gate there must not let the ability lie +/// in wait for the FOLLOWING end step; +/// * a zone change of ONE ALREADY-BOUND object ("when THAT creature dies" — +/// `TargetFilter::names_bound_single_object`). The bound object departs once +/// per incarnation, and an object that returns is a NEW object (CR 400.7 / +/// CR 603.7c), so a retained ability could never match it anyway; +/// * a CR 603.12 reflexive, which is checked ONLY against its creation batch and +/// must not be left to fire on a later same-turn event. +/// +/// A BROAD-FILTER event form ("when a creature dies this turn, if X, …") is +/// exactly what CR 603.7b's "unless it has a stated duration" clause keeps +/// watching: the first non-qualifying death must NOT destroy the ability for the +/// rest of the turn. Those stay installed and have their gate re-checked on the +/// next occurrence; their stated duration is enforced by the cleanup purge +/// (`DelayedTriggerLifetime` / `WheneverEventExpiry`), never by this discard. +/// +/// Exhaustive on purpose: a new `DelayedTriggerCondition` must decide whether its +/// stated event is a single occurrence before it can be discarded on a false gate. +fn false_gate_consumes_one_shot(condition: &DelayedTriggerCondition) -> bool { + match condition { + DelayedTriggerCondition::AtNextPhase { .. } + | DelayedTriggerCondition::AtNextPhaseForPlayer { .. } + // A concrete `ObjectId`: the same single-occurrence argument as a + // bound-single-object filter, already resolved to one object. + | DelayedTriggerCondition::WhenLeavesPlay { .. } => true, + DelayedTriggerCondition::WhenDies { filter } + | DelayedTriggerCondition::WhenLeavesPlayFiltered { filter } + | DelayedTriggerCondition::WhenEntersBattlefield { filter } + | DelayedTriggerCondition::WhenDiesOrExiled { filter } => { + filter.names_bound_single_object() + } + // CR 603.12: a reflexive gets exactly one shot on its creation batch — + // the same rule the unmatched-reflexive discard below enforces. Every + // other `WhenNextEvent` watches a broad event predicate. + DelayedTriggerCondition::WhenNextEvent { .. } => is_reflexive_lifetime(condition), + // Never one-shot (`effects::delayed_trigger` computes `one_shot` as + // "not `WheneverEvent`"), so this arm is unreachable from the caller; + // spelled out rather than wildcarded to keep the match exhaustive. + DelayedTriggerCondition::WheneverEvent { .. } => false, + } +} + fn delayed_trigger_to_context( state: &GameState, trigger: DelayedTrigger, trigger_event: GameEvent, ) -> PendingTriggerContext { + // CR 603.4 (second half): carry the hoisted intervening-`if` onto the stack + // entry so `stack.rs`'s resolution recheck applies to a delayed triggered + // ability exactly as it does to a printed one. `delayed_intervening_if` is + // the SAME authority the collection gate below used, so the two halves of + // the CR 603.4 pair cannot read different predicates. + let condition = delayed_intervening_if(&trigger.ability); PendingTriggerContext::delayed( PendingTrigger { source_id: trigger.source_id, controller: trigger.controller, - condition: None, + condition, ability: trigger.ability, timestamp: state.turn_number, target_constraints: Vec::new(), @@ -8919,7 +9476,11 @@ fn collect_matching_delayed_triggers( // `WhenNextEvent` would do). The phase-only path keeps the historical // narrower coin-flip discard gate because it filters non-phase matches out of // the candidate batch. - let mut to_discard: Vec = Vec::new(); + // + // Each entry pairs the index with the terminal disposition to record for it, + // so a discarded reflexive (CR 603.12) and a CR 603.4 intervening-`if` + // failure stay distinguishable in the lifecycle ledger. + let mut to_discard: Vec<(usize, super::lifecycle::DelayedTerminalDisposition)> = Vec::new(); for (idx, delayed) in state.delayed_triggers.iter().enumerate() { if let Some((event_index, trigger_event)) = delayed_trigger_event_with_index( @@ -8933,6 +9494,44 @@ fn collect_matching_delayed_triggers( if !scope.accepts(&trigger_event) { continue; } + // CR 603.4 (first half): "When the trigger event occurs, the ability + // checks whether the stated condition is true. The ability triggers + // only if it is; otherwise it does nothing." The delayed body's + // intervening-`if` was previously consulted ONLY at resolution, so a + // failing gate still put a respondable ability on the stack — and a + // player could then make the gate true in response (getting a + // commander onto the battlefield for Fight for the Throne), which + // CR 603.4 forbids outright. + // + // `check_trigger_condition_with_source` is the same fire-time + // evaluator printed triggers use, given the delayed ability's own + // CR 400.7 source context and the matched event. + if let Some(condition) = delayed_intervening_if(&delayed.ability) { + if !check_trigger_condition_with_source( + state, + &condition, + delayed.controller, + delayed.ability.trigger_source.as_ref(), + Some(&trigger_event), + ) { + // CR 603.4 + CR 603.7b: the ability did not trigger. It is + // removed without firing, tagged `InterveningIfFalse`, ONLY + // when its stated event was a single occurrence that this + // check has now consumed — see + // `false_gate_consumes_one_shot`. Everything else (a + // multi-fire "whenever … this turn", and a one-shot watching + // a BROAD event filter) stays installed and gets its gate + // re-checked on the next occurrence, per CR 603.7b's + // stated-duration clause. + if delayed.one_shot && false_gate_consumes_one_shot(&delayed.condition) { + to_discard.push(( + idx, + super::lifecycle::DelayedTerminalDisposition::InterveningIfFalse, + )); + } + continue; + } + } if delayed.one_shot { to_remove.push((idx, event_index, trigger_event)); } else { @@ -8972,7 +9571,10 @@ fn collect_matching_delayed_triggers( ) } } { - to_discard.push(idx); + to_discard.push(( + idx, + super::lifecycle::DelayedTerminalDisposition::ReflexiveUnmatched, + )); } } @@ -8996,31 +9598,34 @@ fn collect_matching_delayed_triggers( } } - // Remove fired one-shot triggers AND discarded non-matching reflexive triggers + // Remove fired one-shot triggers AND every one-shot dropped without firing // from `delayed_triggers` in a single descending-index pass so every index - // stays valid. Fired one-shots are collected into `to_fire`; discarded - // reflexives (CR 603.12) are dropped without firing. The two index sets are - // disjoint — a trigger either matched (fire) or resolved-without-match - // (discard), never both. + // stays valid. Fired one-shots are collected into `to_fire`; the unfired set + // is dropped with its own recorded disposition — `ReflexiveUnmatched` for a + // CR 603.12 reflexive that never matched, `InterveningIfFalse` for a + // CR 603.4 gate that was false when the trigger event occurred. The two + // index sets are disjoint — a trigger either fired, or was dropped + // unfired, never both. let mut fired_events: std::collections::HashMap = to_remove .iter() .map(|(idx, event_index, event)| (*idx, (*event_index, event.clone()))) .collect(); + let mut unfired_dispositions: std::collections::HashMap< + usize, + super::lifecycle::DelayedTerminalDisposition, + > = to_discard.iter().copied().collect(); let mut combined: Vec = to_remove .iter() .map(|(idx, _, _)| *idx) - .chain(to_discard.iter().copied()) + .chain(to_discard.iter().map(|(idx, _)| *idx)) .collect(); combined.sort_unstable(); for idx in combined.into_iter().rev() { let trigger = state.delayed_triggers.remove(idx); if let Some((event_index, trigger_event)) = fired_events.remove(&idx) { to_fire.push((trigger, event_index, trigger_event, true)); - } else { - super::lifecycle::record_delayed_terminal( - trigger.provenance.firing(), - super::lifecycle::DelayedTerminalDisposition::ReflexiveUnmatched, - ); + } else if let Some(disposition) = unfired_dispositions.remove(&idx) { + super::lifecycle::record_delayed_terminal(trigger.provenance.firing(), disposition); } } @@ -18196,6 +18801,1216 @@ pub mod tests { ); } + /// CR 603.4 (fire-time half) for a DELAYED triggered ability. + /// + /// Stages Fight for the Throne's shape — a one-shot `WhenDies` delayed + /// ability whose body carries the intervening-`if` "if you control your + /// commander" — and drives the production collection entry point + /// (`check_delayed_triggers`) on the death event. + /// + /// REVERT-TO-RED: with the fire-time hoist removed, `state.stack.len()` is + /// `1` in the no-commander half, because the ability was put onto the stack + /// and only skipped later, at resolution. That is precisely the CR 603.4 + /// violation — the ability is respondable, counts as a triggered ability put + /// onto the stack, and a player could make the gate TRUE in response. + /// + /// The commander half is the reachability proof: the very same fixture with + /// a commander staged DOES reach the stack, so the empty-stack assertion + /// below cannot be passing because the delayed trigger never matched. + /// + /// The two `DelayedSubject` halves pin the CR 603.7b survival split that + /// `false_gate_consumes_one_shot` decides — see the assertions below. + #[test] + fn delayed_intervening_if_gates_the_ability_off_the_stack_at_fire_time() { + /// Which shape of "when [it] dies" the fixture installs. + #[derive(Clone, Copy)] + enum DelayedSubject { + /// "when A creature dies this turn" — a CLASS filter that can match + /// again later in the turn. + BroadFilter, + /// "when THAT creature dies" — one already-bound object. + BoundObject, + } + + fn run(stage_commander: bool, subject: DelayedSubject) -> (usize, usize) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0401), + controller, + "Fight for the Throne".to_string(), + Zone::Battlefield, + ); + if stage_commander { + // CR 903.3 + CR 903.3d: owned AND controlled, on the battlefield. + let commander = make_creature(&mut state, controller, "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + // The intervening-`if` as the parser lowers it onto the delayed BODY. + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + let filter = match subject { + DelayedSubject::BroadFilter => TargetFilter::Any, + DelayedSubject::BoundObject => TargetFilter::SpecificObject { id: victim }, + }; + assert_eq!( + filter.names_bound_single_object(), + matches!(subject, DelayedSubject::BoundObject), + "reach-guard: the fixture's filter must land on the side of \ + `names_bound_single_object` the case under test intends, or both halves \ + take the same retention branch" + ); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { filter }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + (state.stack.len(), state.delayed_triggers.len()) + } + + let (gated_stack, gated_remaining) = run(false, DelayedSubject::BroadFilter); + assert_eq!( + gated_stack, 0, + "CR 603.4: with the intervening-if FALSE when the trigger event occurs, the \ + delayed ability must never be put onto the stack. A 1 here is the shipped \ + defect — a respondable ability that CR 603.4 says never triggered" + ); + assert_eq!( + gated_remaining, 1, + "CR 603.4 + CR 603.7b: the ability DID NOT TRIGGER, and a broad-filter form \ + has a stated duration, so it must keep watching for a later qualifying \ + death. A 0 here is the defect where the first non-qualifying death silently \ + destroys the ability for the rest of the turn" + ); + + let (fired_stack, fired_remaining) = run(true, DelayedSubject::BroadFilter); + assert_eq!( + fired_stack, 1, + "reachability proof: with an owned-and-controlled commander the SAME fixture \ + puts the delayed ability on the stack, so the gated assertion above is not \ + passing because the trigger never matched" + ); + assert_eq!( + fired_remaining, 0, + "the fired one-shot is removed from delayed_triggers as before" + ); + + let (bound_gated_stack, bound_gated_remaining) = run(false, DelayedSubject::BoundObject); + assert_eq!( + bound_gated_stack, 0, + "CR 603.4: the bound-object form is gated off the stack on a false gate too" + ); + assert_eq!( + bound_gated_remaining, 0, + "CR 603.7b + CR 400.7: the ONE bound object has now died, and an object that \ + returns is a new object, so nothing is left for this ability to watch — it \ + is consumed. A 1 here would leak a permanently inert delayed trigger" + ); + + let (bound_fired_stack, bound_fired_remaining) = run(true, DelayedSubject::BoundObject); + assert_eq!( + bound_fired_stack, 1, + "reachability proof for the bound-object half: the same fixture with the gate \ + TRUE reaches the stack, so the retention assertion above is about the gate" + ); + assert_eq!( + bound_fired_remaining, 0, + "the fired one-shot is removed from delayed_triggers as before" + ); + } + + /// CR 603.7b for the PHASE-NAMED one-shot class — the case the discard was + /// always sound for. "At the beginning of the next end step, if [gate], …" + /// names ONE specific later time; when that time arrives with the gate false + /// the ability did nothing (CR 603.4) AND has nothing left to wait for, so it + /// must not lie in wait for the FOLLOWING end step. + /// + /// REVERT-TO-RED: drop the `AtNextPhase` arm from + /// `false_gate_consumes_one_shot` and `remaining` below is `1` — a delayed + /// ability that outlives its own named deadline. + /// + /// The gate-true half is the reachability proof that this fixture's delayed + /// trigger genuinely matches the end-step event. + #[test] + fn phase_named_one_shot_is_consumed_by_a_false_intervening_if() { + fn run(stage_commander: bool) -> (usize, usize) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0406), + controller, + "End Step Rider".to_string(), + Zone::Battlefield, + ); + if stage_commander { + let commander = make_creature(&mut state, controller, "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::AtNextPhase { phase: Phase::End }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + check_delayed_triggers(&mut state, &[GameEvent::PhaseChanged { phase: Phase::End }]); + (state.stack.len(), state.delayed_triggers.len()) + } + + let (gated_stack, gated_remaining) = run(false); + assert_eq!( + gated_stack, 0, + "CR 603.4: the ability must never be put onto the stack with the gate false" + ); + assert_eq!( + gated_remaining, 0, + "CR 603.7b: \"at the beginning of the NEXT end step\" names one specific later \ + time, which has now passed — the ability is consumed, not left waiting" + ); + + let (fired_stack, fired_remaining) = run(true); + assert_eq!( + fired_stack, 1, + "reachability proof: with the gate TRUE the same fixture reaches the stack, so \ + the consumption above is not a never-matched trigger" + ); + assert_eq!( + fired_remaining, 0, + "the fired one-shot is removed as before" + ); + } + + /// HOSTILE fixture for the CR 603.4 hoist's carve-out. + /// + /// "…, if X, A. Otherwise, B." is NOT an intervening-`if` in the CR 603.4 + /// sense — the ability has work to do when the gate is false, so it MUST + /// still be put onto the stack and resolve its `else_ability`. + /// `delayed_body_outlives_a_false_gate` declines every body carrying an + /// `else_ability` for exactly this reason. + /// + /// REVERT-TO-RED: drop the `else_ability` arm from + /// `delayed_body_outlives_a_false_gate` and the gate hoists, the ability + /// never reaches the stack, and the Otherwise branch is silently deleted — + /// life stays 20 and the stack stays empty. + #[test] + fn delayed_body_with_an_otherwise_branch_is_never_gated_off_the_stack() { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + state.players[0].life = 20; + + let source = create_object( + &mut state, + CardId(0x0603_0402), + controller, + "Otherwise Rider".to_string(), + Zone::Battlefield, + ); + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + // Gate is FALSE (no commander anywhere), so the Otherwise branch is the + // one that must run. + let mut ability = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + ability.else_ability = Some(Box::new(ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 1 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ))); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + assert_eq!( + state.stack.len(), + 1, + "a body with an Otherwise branch must still reach the stack even though its \ + own condition is false" + ); + + let mut events = Vec::new(); + crate::game::stack::resolve_top(&mut state, &mut events); + assert_eq!( + state.players[0].life, 21, + "CR 608.2c: the Otherwise branch must resolve. 20 here means the CR 603.4 \ + hoist swallowed a non-intervening-if gate and deleted the else branch" + ); + assert_eq!( + state.monarch, None, + "the gated primary effect must still not happen" + ); + } + + /// CR 603.4 for a CHAINED delayed body — the case the old blanket + /// `sub_ability.is_some()` guard silently got wrong. + /// + /// "When [event], if [gate], A and B" lowers B as an UNCONDITIONAL + /// `SequentialSibling` sub of the gated body. CR 603.4's `if` gates the WHOLE + /// ability, so with the gate false NEITHER clause may happen. Under the old + /// guard the hoist was skipped AND `resolve_ability_chain`'s + /// `SequentialSibling && condition.is_none()` escape resolved B anyway — the + /// ability went on the stack and B happened with the gate false. + /// + /// REVERT-TO-RED: restore `|| ability.sub_ability.is_some()` in + /// `delayed_body_outlives_a_false_gate` and the gated half below reports + /// `stack == 1` and life 21 — B resolved through a false CR 603.4 gate. + /// + /// The commander half is the reachability proof AND the non-deletion proof: + /// the same fixture with the gate TRUE runs both clauses. + #[test] + fn delayed_body_with_an_unconditional_sequential_sub_is_gated_as_one_ability() { + fn run(stage_commander: bool) -> (usize, i32, Option) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + state.players[0].life = 20; + + let source = create_object( + &mut state, + CardId(0x0603_0403), + controller, + "Chained Rider".to_string(), + Zone::Battlefield, + ); + if stage_commander { + let commander = make_creature(&mut state, controller, "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + // "…and gain 1 life" — the second clause of the ONE gated sentence. + let mut sub = ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 1 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + sub.sub_link = crate::types::ability::SubAbilityLink::SequentialSibling; + assert!( + sub.condition.is_none(), + "reach-guard: the sub must be UNCONDITIONAL, or it takes a different \ + resolve_ability_chain branch than the one under test" + ); + ability.sub_ability = Some(Box::new(sub)); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + let stack_len = state.stack.len(); + if stack_len == 1 { + let mut events = Vec::new(); + crate::game::stack::resolve_top(&mut state, &mut events); + } + (stack_len, state.players[0].life, state.monarch) + } + + let (gated_stack, gated_life, gated_monarch) = run(false); + assert_eq!( + gated_stack, 0, + "CR 603.4: the intervening-if gates the WHOLE ability, chain and all" + ); + assert_eq!( + gated_life, 20, + "CR 603.4: the chained second clause must NOT happen through a false gate. \ + 21 means the unconditional SequentialSibling escaped the gate" + ); + assert_eq!( + gated_monarch, None, + "the gated primary effect must not happen" + ); + + let (fired_stack, fired_life, fired_monarch) = run(true); + assert_eq!( + fired_stack, 1, + "reachability proof: with the gate TRUE the same fixture reaches the stack" + ); + assert_eq!( + fired_monarch, + Some(PlayerId(0)), + "the gated primary effect resolves when the gate is true" + ); + assert_eq!( + fired_life, 21, + "non-deletion proof: the chained clause still resolves when the gate is true, \ + so the gated assertion above is not passing because the chain was dropped" + ); + } + + /// CR 603.4: the two legs of the hoist must be the SAME predicate over the + /// SAME values. A gate whose quantity reads a binding only the + /// resolution-time leg has — the delayed ability's snapshotted `targets` + /// (`ObjectScope::Target`), the per-iteration player + /// (`PlayerScope::ScopedPlayer`), or a population the trigger bridge + /// re-scopes (`FilterProp::Another` → `OtherThanTriggerObject`) — must + /// DECLINE the hoist rather than gate the ability off the stack on a value + /// the resolver would never have computed. + /// + /// Each case is a MINIMAL PAIR: the two halves differ only in the scope or + /// the one filter property. The `hoisted` half proves a `QuantityCheck` gate + /// really does bridge and really is evaluated at fire time (so the declined + /// half's `stack == 1` is the decline, not a non-bridging condition); the + /// `declined` half proves the divergent reading never reaches the gate. + /// + /// REVERT-TO-RED: drop the `gate_binding_diverges_at_fire_time` call from + /// `delayed_intervening_if` and every `declined` half below reports + /// `stack == 0` — the ability deleted off the stack on a fire-time reading + /// (empty targets / event-derived player / trigger-object exclusion) that the + /// resolution-time reader does not share. + #[test] + fn divergent_gate_bindings_decline_the_fire_time_hoist() { + fn run(condition: AbilityCondition) -> usize { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0407), + controller, + "Divergent Rider".to_string(), + Zone::Battlefield, + ); + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(condition); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + state.stack.len() + } + + let counters_on = |scope| AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::CountersOn { + scope, + counter_type: Some(crate::types::counter::CounterType::Plus1Plus1), + }, + }, + comparator: crate::types::ability::Comparator::GE, + rhs: QuantityExpr::Fixed { value: 3 }, + }; + // No object in the fixture carries a +1/+1 counter, so BOTH readings are + // false — the halves differ only in whether the gate is consulted at all. + assert_eq!( + run(counters_on(ObjectScope::Source)), + 0, + "CR 603.4: a Source-scoped counter gate binds identically on both legs, so it \ + hoists and gates the ability off the stack" + ); + assert_eq!( + run(counters_on(ObjectScope::Target)), + 1, + "CR 115.1: `ObjectScope::Target` reads the delayed ability's snapshotted \ + targets, which the fire-time context does not carry. 0 here means the hoist \ + gated the ability off the stack on an empty-targets reading" + ); + + let life_total = |player| AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::LifeTotal { player }, + }, + comparator: crate::types::ability::Comparator::GE, + rhs: QuantityExpr::Fixed { value: 100 }, + }; + assert_eq!( + run(life_total(PlayerScope::Controller)), + 0, + "reach-guard: the same comparison scoped to the controller DOES hoist (nobody \ + has 100 life), so the declined half below is the scope and nothing else" + ); + assert_eq!( + run(life_total(PlayerScope::ScopedPlayer)), + 1, + "CR 115.10: `ScopedPlayer` is the RESOLVING ability's per-iteration player; \ + the fire-time context derives its scoped player from the event instead" + ); + + let controls_creature = |props: Vec| AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::ObjectCount { + filter: TargetFilter::Typed( + TypedFilter::creature() + .controller(ControllerRef::You) + .properties(props), + ), + }, + }, + comparator: crate::types::ability::Comparator::GE, + rhs: QuantityExpr::Fixed { value: 1 }, + }; + assert_eq!( + run(controls_creature(vec![])), + 0, + "reach-guard: the unqualified population gate hoists (the controller controls \ + no creature), so the declined half below is the `Another` prop alone" + ); + assert_eq!( + run(controls_creature(vec![FilterProp::Another])), + 1, + "CR 603.4: `oracle_trigger` rewrites `Another` to `OtherThanTriggerObject` on \ + the fire-time leg only, so the two legs would count DIFFERENT populations" + ); + } + + /// CR 400.1 + CR 603.4: the ZONE axis of the same two-legs-must-agree rule. + /// + /// `ObjectCount{filter} >= 1` folds into `StaticCondition::IsPresent`, whose + /// affirmative trigger-side bridge is `TriggerCondition::ControlsType` — a + /// BATTLEFIELD-ONLY scan. The resolution leg counts in the FILTER'S OWN zone. + /// A gate on a graveyard population therefore reads TRUE at resolution and + /// FALSE at fire time, and hoisting it would gate the ability off the stack + /// (deleting a consumed one-shot) on a predicate the resolver never computes. + /// + /// MINIMAL PAIR: the two halves differ only in the `InZone { Graveyard }` + /// property. The battlefield half is the reach-guard — it proves an + /// `ObjectCount >= 1` gate really does bridge and really is evaluated at fire + /// time, so the graveyard half's `stack == 1` is the decline and nothing else. + /// + /// REVERT-TO-RED: drop the `static_gate_bridge_loses_zone` call from + /// `delayed_intervening_if` and the graveyard half reports `stack == 0` (and + /// `monarch == None`) — the ability deleted at fire time on a battlefield scan + /// of a graveyard population. + #[test] + fn non_battlefield_presence_gate_declines_the_fire_time_hoist() { + fn run(props: Vec) -> (usize, Option) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0409), + controller, + "Graveyard Rider".to_string(), + Zone::Battlefield, + ); + // The counted population lives in the GRAVEYARD, never on the + // battlefield: the controller controls no creature there, so a + // battlefield scan of this filter is false either way. + let buried = create_object( + &mut state, + CardId(0x0603_040A), + controller, + "Buried Squire".to_string(), + Zone::Graveyard, + ); + { + let obj = state.objects.get_mut(&buried).expect("staged card"); + obj.card_types.core_types.push(CoreType::Creature); + obj.base_card_types = obj.card_types.clone(); + } + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { + qty: QuantityRef::ObjectCount { + filter: TargetFilter::Typed( + TypedFilter::creature() + .controller(ControllerRef::You) + .properties(props), + ), + }, + }, + comparator: crate::types::ability::Comparator::GE, + rhs: QuantityExpr::Fixed { value: 1 }, + }); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::SpecificObject { id: victim }, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + let stack_len = state.stack.len(); + if stack_len == 1 { + let mut events = Vec::new(); + crate::game::stack::resolve_top(&mut state, &mut events); + } + (stack_len, state.monarch) + } + + let (battlefield_stack, battlefield_monarch) = run(vec![]); + assert_eq!( + battlefield_stack, 0, + "reach-guard: a battlefield-scoped `ObjectCount >= 1` gate bridges and IS \ + evaluated at fire time — the controller controls no creature, so CR 603.4 \ + keeps the ability off the stack" + ); + assert_eq!( + battlefield_monarch, None, + "nothing resolved on the battlefield-scoped half" + ); + + let (graveyard_stack, graveyard_monarch) = run(vec![FilterProp::InZone { + zone: Zone::Graveyard, + }]); + assert_eq!( + graveyard_stack, 1, + "CR 400.1 + CR 603.4: the affirmative `IsPresent` bridge is battlefield-only, \ + so a graveyard population would read FALSE at fire time while the resolver \ + reads TRUE. The hoist must decline and the ability must reach the stack" + ); + assert_eq!( + graveyard_monarch, + Some(PlayerId(0)), + "divergence proof: the RESOLUTION-time leg reads the same gate as TRUE (one \ + creature card in the controller's graveyard), so a fire-time deletion would \ + have destroyed an ability that was supposed to resolve" + ); + } + + /// CR 608.2c + CR 603.4: a RESOLUTION-SCOPED quantity leaf carries no scope, + /// player or filter payload, yet the fire-time leg cannot reproduce it — the + /// resolver is called with no resolving ability, no targets and no chain + /// ledger, so `TrackedSetSize` reads whatever an unrelated earlier resolution + /// left behind (here: nothing). + /// + /// MINIMAL PAIR against another PAYLOAD-FREE leaf whose reading is identical + /// on both legs (`TurnsTaken`), so the decline below cannot be explained by + /// "payload-free gates never hoist". + /// + /// REVERT-TO-RED: restore the `_ => false` tail of + /// `quantity_ref_binding_diverges` and the tracked-set half reports + /// `stack == 0` with `delayed_triggers` emptied — the one-shot deleted by + /// `false_gate_consumes_one_shot` on a value the resolver never computed. + #[test] + fn resolution_scoped_quantity_gate_declines_the_fire_time_hoist() { + // Unit pins for the two adjudications the production pair below drives. + assert!( + quantity_ref_binding_diverges(&QuantityRef::TrackedSetSize), + "CR 608.2c: the chain tracked set exists only during a resolution" + ); + assert!( + !quantity_ref_binding_diverges(&QuantityRef::TurnsTaken), + "CR 500: turns taken is global state both legs read identically" + ); + + fn run(qty: QuantityRef, threshold: i32) -> (usize, usize) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_040B), + controller, + "Tracked Rider".to_string(), + Zone::Battlefield, + ); + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::QuantityCheck { + lhs: QuantityExpr::Ref { qty }, + comparator: crate::types::ability::Comparator::GE, + rhs: QuantityExpr::Fixed { value: threshold }, + }); + state.delayed_triggers.push(DelayedTrigger { + // A bound single object, so a false gate would CONSUME the + // one-shot (`false_gate_consumes_one_shot`) rather than leave it + // installed — the deletion half of the defect. + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::SpecificObject { id: victim }, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + (state.stack.len(), state.delayed_triggers.len()) + } + + // `TurnsTaken >= 100` is false on both legs — the reach-guard that a + // payload-free `QuantityCheck` gate does hoist and does gate at fire time. + let (turns_stack, turns_remaining) = run(QuantityRef::TurnsTaken, 100); + assert_eq!( + turns_stack, 0, + "reach-guard: a payload-free gate whose reading is leg-independent hoists, so \ + the declined half below is the RESOLUTION scope and nothing else" + ); + assert_eq!( + turns_remaining, 0, + "CR 603.7b: the bound-object one-shot is consumed by its own event" + ); + + let (tracked_stack, tracked_remaining) = run(QuantityRef::TrackedSetSize, 1); + assert_eq!( + tracked_stack, 1, + "CR 608.2c: `TrackedSetSize` is published BY a resolution; the fire-time leg \ + reads a foreign (here empty) ledger, so the gate must keep its \ + resolution-only reading and the ability must reach the stack" + ); + assert_eq!( + tracked_remaining, 0, + "the ability FIRED (it is off the delayed list because it went on the stack), \ + not because a false gate deleted it — see the stack assertion above" + ); + } + + /// CR 603.4 + CR 608.2c: `delayed_body_outlives_a_false_gate` inspects the + /// DIRECT sub only, because that is all `resolve_chain_body`'s + /// condition-false path inspects. A 2-deep chain whose direct sub does NOT + /// qualify resolves nothing at all, so the hoist must still apply even though + /// a GRANDCHILD carries an `else_ability`. + /// + /// REVERT-TO-RED: restore the `|| delayed_body_outlives_a_false_gate(sub)` + /// recursion and the gated half below reports `stack == 1` — an ability put + /// onto the stack (respondable, and CR 603.4 says it never triggered) to + /// resolve exactly nothing. + /// + /// The resolver-mirror assertion drives `effects::resolve_ability_chain` + /// directly on the SAME body with the SAME false gate and proves it performs + /// no work, so the hoist deletes nothing. + #[test] + fn two_deep_chain_mirrors_the_resolvers_direct_sub_test() { + /// "…, if [gate], become the monarch" whose direct sub is an ORDINARY + /// conditional continuation and whose GRANDCHILD carries an + /// `else_ability` (life gain) that only runs if the chain gets that far. + fn body(source: ObjectId, controller: PlayerId) -> ResolvedAbility { + let mut grandchild = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + grandchild.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + grandchild.else_ability = Some(Box::new(ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 1 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ))); + + let mut sub = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + // NOT one of the surviving classes: an ordinary game-state gate on a + // continuation link, so `resolve_chain_body` stops here when the + // parent gate is false. + sub.condition = Some(AbilityCondition::IsYourTurn); + sub.sub_link = crate::types::ability::SubAbilityLink::ContinuationStep; + sub.sub_ability = Some(Box::new(grandchild)); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + ability.sub_ability = Some(Box::new(sub)); + ability + } + + fn run(stage_commander: bool) -> (usize, i32, Option) { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + state.players[0].life = 20; + + let source = create_object( + &mut state, + CardId(0x0603_040C), + controller, + "Two Deep Rider".to_string(), + Zone::Battlefield, + ); + if stage_commander { + let commander = make_creature(&mut state, controller, "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let ability = body(source, controller); + // Reach-guard: the shape under test is a 2-deep chain whose DIRECT sub + // is not one of the surviving classes but whose grandchild is. + let direct = ability.sub_ability.as_deref().expect("staged direct sub"); + assert!( + !crate::game::effects::sub_outlives_false_parent_gate(direct), + "the direct sub must NOT qualify, or the hoist declines for the shallow \ + reason and the 2-deep case is never exercised" + ); + assert!( + direct + .sub_ability + .as_deref() + .is_some_and(|grandchild| grandchild.else_ability.is_some()), + "the GRANDCHILD must carry the surviving shape the old recursion tripped on" + ); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + let stack_len = state.stack.len(); + if stack_len == 1 { + let mut events = Vec::new(); + crate::game::stack::resolve_top(&mut state, &mut events); + } + (stack_len, state.players[0].life, state.monarch) + } + + let (gated_stack, gated_life, gated_monarch) = run(false); + assert_eq!( + gated_stack, 0, + "CR 603.4: with the gate false the resolver would run NOTHING (the direct sub \ + does not survive), so the fire-time half must gate the ability off the stack" + ); + assert_eq!(gated_life, 20, "no chain work happened"); + assert_eq!(gated_monarch, None, "no chain work happened"); + + // Resolver mirror: the same body, the same false gate, resolved directly. + // Nothing runs — which is exactly why hoisting deletes nothing. + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + state.players[0].life = 20; + let source = create_object( + &mut state, + CardId(0x0603_040D), + controller, + "Two Deep Rider".to_string(), + Zone::Battlefield, + ); + let mut events = Vec::new(); + crate::game::effects::resolve_ability_chain( + &mut state, + &body(source, controller), + &mut events, + 0, + ) + .expect("chain resolution"); + assert_eq!( + state.players[0].life, 20, + "CR 608.2c: `resolve_chain_body` stops at the non-surviving DIRECT sub, so the \ + grandchild's else branch never runs — 21 here would mean the hoist above \ + deleted printed work" + ); + assert_eq!( + state.monarch, None, + "the gated primary effect and the gated grandchild both stay unperformed" + ); + + let (fired_stack, _fired_life, fired_monarch) = run(true); + assert_eq!( + fired_stack, 1, + "reachability proof: the SAME fixture with the gate TRUE reaches the stack" + ); + assert_eq!( + fired_monarch, + Some(PlayerId(0)), + "and resolves its gated effect" + ); + } + + /// CR 603.4 + CR 702.1c: `delayed_body_outlives_a_false_gate` and + /// `resolve_chain_body` must decline / run on EXACTLY the same sub shapes. + /// The `ReplicatedOrBranch` marker only means "independent OR-branch" on a + /// `SequentialSibling` link — that is the conjunct + /// `effects::sub_outlives_false_parent_gate` now carries for BOTH consumers. + /// + /// REVERT-TO-RED: drop the `sub_link == SequentialSibling` conjunct from + /// `sub_outlives_false_parent_gate` and the `ContinuationStep` half below + /// reports `stack == 1` — the hoist declined for a sub the resolver would + /// never have run through a false gate, leaving CR 603.4 unenforced. + #[test] + fn replicated_or_branch_carve_out_requires_a_sequential_sibling_link() { + fn run(sub_link: crate::types::ability::SubAbilityLink) -> usize { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0408), + controller, + "Replicated Rider".to_string(), + Zone::Battlefield, + ); + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + // Gate FALSE: no commander is staged anywhere. + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + let mut sub = ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 1 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + sub.sibling_condition = crate::types::ability::SiblingCondition::ReplicatedOrBranch; + sub.sub_link = sub_link; + // Reach-guard: the sub must carry the replication marker, or both + // halves take the plain no-sub branch and the pair proves nothing. + assert_eq!( + sub.sibling_condition, + crate::types::ability::SiblingCondition::ReplicatedOrBranch + ); + ability.sub_ability = Some(Box::new(sub)); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + state.stack.len() + } + + assert_eq!( + run(crate::types::ability::SubAbilityLink::SequentialSibling), + 1, + "CR 702.1c: a replicated OR-branch on a SequentialSibling link is exactly what \ + `resolve_chain_body` still runs through a false parent gate, so the hoist \ + must decline and the ability must still reach the stack" + ); + assert_eq!( + run(crate::types::ability::SubAbilityLink::ContinuationStep), + 0, + "CR 603.4: on a ContinuationStep link the resolver would NOT have run the sub \ + either, so nothing survives the false gate and the whole ability must be \ + gated off the stack" + ); + } + + /// CR 603.4 + CR 903.3d for the NEGATED commander gate ("if you don't + /// control your commander"). + /// + /// The hoist composes two bridges, and only the FIRST had a negated + /// commander arm: `ability_condition_to_static_condition` produced + /// `Not{ControlsCommander}` while + /// `static_condition_to_trigger_condition`'s `Not` sub-match fell through to + /// `_ => None`, so `delayed_intervening_if` returned `None` and the negated + /// gate kept only the resolution-time half of CR 603.4. + /// + /// REVERT-TO-RED: remove the `StaticCondition::ControlsCommander` arm from + /// that `Not` sub-match (`oracle_trigger.rs`) and the no-commander half below + /// reports `stack == 1` — the ability was put onto the stack instead of + /// having never triggered. + #[test] + fn negated_delayed_intervening_if_gates_at_fire_time_too() { + fn run(stage_commander: bool) -> usize { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0405), + controller, + "Negated Rider".to_string(), + Zone::Battlefield, + ); + if stage_commander { + let commander = make_creature(&mut state, controller, "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::Not { + condition: Box::new(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }), + }); + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + state.stack.len() + } + + assert_eq!( + run(true), + 0, + "CR 603.4: with a commander on the battlefield the NEGATED gate is false at \ + fire time, so the ability must never be put onto the stack" + ); + assert_eq!( + run(false), + 1, + "reachability proof: with no commander the negated gate is TRUE and the same \ + fixture reaches the stack" + ); + } + + /// HOSTILE fixture for the narrowed carve-out's OTHER arm (CR 603.12). + /// + /// A sub whose own gate is a performed/reflexive one ("…, if you do, …" — + /// Council's Deliberation's shape, the only conditioned delayed body with a + /// sub in the pool today) is re-evaluated on its own at resolution, so + /// `delayed_body_outlives_a_false_gate` conservatively declines the CR 603.4 + /// hoist and this class keeps byte-identical behaviour: the ability still + /// reaches the stack even with the parent gate false. + /// + /// REVERT-TO-RED: drop the `condition_survives_false_parent_gate` arm and the + /// hoist fires, the stack stays empty, and the in-pool reflexive shape changes + /// behaviour as a side effect of a commander-gate fix. + #[test] + fn delayed_body_with_a_reflexive_sub_is_never_gated_off_the_stack() { + let mut state = setup(); + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + &mut state, + CardId(0x0603_0404), + controller, + "Reflexive Rider".to_string(), + Zone::Battlefield, + ); + let victim = make_creature(&mut state, PlayerId(1), "Doomed Squire", 1, 1); + + let mut ability = ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + let mut sub = ResolvedAbility::new( + Effect::GainLife { + amount: QuantityExpr::Fixed { value: 1 }, + player: TargetFilter::Controller, + }, + vec![], + source, + controller, + ); + sub.condition = Some(AbilityCondition::effect_performed()); + // Reach-guard: the carve-out is keyed on this classification, not on the + // mere presence of a sub. + assert!( + crate::game::effects::condition_survives_false_parent_gate( + sub.condition.as_ref().expect("staged reflexive gate") + ), + "the fixture must carry a gate the shared authority classifies as \ + surviving a false parent gate" + ); + ability.sub_ability = Some(Box::new(sub)); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenDies { + filter: TargetFilter::Any, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + let death = zone_changed_event( + victim, + Zone::Battlefield, + Zone::Graveyard, + vec![CoreType::Creature], + Vec::new(), + ); + check_delayed_triggers(&mut state, &[death]); + assert_eq!( + state.stack.len(), + 1, + "a body whose sub carries a CR 603.12 performed gate keeps its resolution-time \ + reading and must still reach the stack" + ); + } + #[test] fn delayed_phase_trigger_batches_with_normal_phase_trigger_before_priority() { let mut state = setup(); diff --git a/crates/engine/src/game/triggers_ordering_parity_tests.rs b/crates/engine/src/game/triggers_ordering_parity_tests.rs index 9a0a1c2923..86bdabf98d 100644 --- a/crates/engine/src/game/triggers_ordering_parity_tests.rs +++ b/crates/engine/src/game/triggers_ordering_parity_tests.rs @@ -232,6 +232,48 @@ const DOCUMENTED_OVER_PROMPT: &[&str] = &[ // Exploration misparse-fix fixture regen surfaced this card in the sweep // corpus; same adjudication as the #7031 branch). "arashin sovereign", + // ---- commander-census read × token-creation write (CR 903.3d) ---- + // "At the beginning of your upkeep, if you control a commander, create a token + // that's a copy of this creature." The CR 603.4 intervening-if is a LIVE + // battlefield census (`TriggerCondition::ControlsCommander` ⇒ + // `commander_control_read`, ability_rw.rs), and each member WRITES battlefield + // membership by minting a token — a structural `SetMembership` read × creation + // feed, so `profiles_conflict` prompts. + // + // CONSERVATIVE: the created object is a token copy of Biowaste Blob, which is + // never a commander — CR 903.3's own example is exactly this ("A permanent + // that's copying a commander … is not a commander"), because the designation + // is an attribute of the CARD rather than a characteristic, and CR 707.2's + // copiable values are only the printed characteristics. So no member's write + // can flip another member's gate, and the two creations commute up to + // relabeling. Proving + // that needs "this creation cannot produce a commander", which no context-free + // recognizer in scope can see: the read's census is `Census::Any` (a commander + // may be any permanent type, CR 903.3), so the census/zone/controller + // disjointness rows all overlap. Fail-closed prompt, never an under-prompt. + // + // The alternative — declaring the commander gate read-free — is exactly the + // unsound classification this ledger row exists to avoid: a sibling that + // MOVES, steals or phases out a real commander genuinely flips the gate. + // + // CONSUMPTION: this row is gated like every other one — the exact-set + // `DOCUMENTED_OVER_PROMPT` assertion runs BEFORE the STRICT PROOF-GATE (see + // `ordering_parity_sweep`), so an unrelated red gate can no longer leave a + // newly-added row unverified. That ordering was introduced with this row for + // exactly that reason: at the time it landed the sweep was red on 18 + // unexplained same-event over-prompts (`hidden *` / `opal *` / `veiled *` / + // `lurking skirge` / `impending disaster` / `veil of birds`) and the + // completeness assertion downstream of the gate never executed. Those 18 are + // CORPUS DRIFT, not this change: every one carries + // `TriggerCondition::SourceMatchesFilter` or no trigger condition, ZERO carry + // `ControlsCommander` (checked against the full export), and this change's + // classification edits are confined to the `ControlsCommander` arms of + // `rw_trigger_condition` / `rw_static_condition` / `rw_ability_condition` and + // `scan_*_condition`. They must land as their own corpus-drift rows, NOT + // folded in here. The `over_prompt_hit members` line printed above the + // assertion names the consumed set directly, so the next maintainer need not + // re-derive it. + "biowaste blob", ]; /// Batch-depth GENUINE order-dependence (kept SEPARATE from the same-event @@ -785,6 +827,46 @@ fn ordering_parity_sweep() { se_event_object_class.len(), se_event_object_class ); + // CONSUMED `DOCUMENTED_OVER_PROMPT` membership, emitted as evidence for the same + // reason the two class memberships above are: it names exactly which ledger rows + // the corpus hit on this run. `over_prompt_hit` is a subset of + // `DOCUMENTED_OVER_PROMPT` by construction (both insertion sites are inside a + // `DOCUMENTED_OVER_PROMPT.contains(..)` arm), so this line plus the const is a + // complete consumption proof on its own. Evidence only — the exact-set assertion + // immediately below is the gate. + eprintln!( + "over_prompt_hit members ({}/{}): {:?}", + over_prompt_hit.len(), + DOCUMENTED_OVER_PROMPT.len(), + over_prompt_hit + ); + + // LEDGER COMPLETENESS, adjudicated BEFORE the STRICT PROOF-GATE below (full-DB + // only — the committed fixture lacks these cards). Every allowlist entry MUST be + // consumed: an unhit entry is stale bookkeeping (the card cleared — remove it — + // or its name drifted). + // + // Ordering is load-bearing, not cosmetic. The strict gate panics on ANY + // unexplained decision diff, including pure corpus drift that has nothing to do + // with the ledger; while it is red, a `DOCUMENTED_OVER_PROMPT` row added by + // ordinary card work would never be checked at all, so a stale or mis-keyed row + // could ride along unverified and only surface later, attributed to whoever + // cleared the unrelated drift. Checking consumption first makes every ledger row + // gated on every full-DB run. The gate's own diff count is carried in this + // message so a simultaneous drift is never hidden by an earlier panic. + if full_db { + let expected_over_prompt: BTreeSet = DOCUMENTED_OVER_PROMPT + .iter() + .map(|s| s.to_string()) + .collect(); + assert_eq!( + over_prompt_hit, + expected_over_prompt, + "DOCUMENTED_OVER_PROMPT stale/unhit entries ({} unexplained decision diff(s) \ + also pending at the STRICT PROOF-GATE below)", + unexplained.len() + ); + } assert!( unexplained.is_empty(), @@ -847,14 +929,9 @@ fn ordering_parity_sweep() { // Ledger completeness (full-DB only — the committed fixture lacks these // cards). Every allowlist entry MUST be consumed: an unhit entry is stale // bookkeeping (the card cleared — remove it — or its name drifted). - let expected_over_prompt: BTreeSet = DOCUMENTED_OVER_PROMPT - .iter() - .map(|s| s.to_string()) - .collect(); - assert_eq!( - over_prompt_hit, expected_over_prompt, - "DOCUMENTED_OVER_PROMPT stale/unhit entries" - ); + // `DOCUMENTED_OVER_PROMPT`'s exact-set assertion is deliberately NOT here: + // it runs ABOVE the STRICT PROOF-GATE (see the comment there) so unrelated + // corpus drift cannot leave a newly-added over-prompt row unverified. let expected_batch_genuine: BTreeSet = BATCH_GENUINE_ROWS.iter().map(|s| s.to_string()).collect(); assert_eq!( @@ -1261,6 +1338,79 @@ fn n_d_intervening_if_case_a_prompts() { ); } +/// N-D2: the commander intervening-`if` is a LIVE BOARD CENSUS (CR 903.3d), so a +/// sibling copy whose resolution writes battlefield membership FEEDS it and the +/// group must PROMPT (CR 603.3b). Biowaste Blob's exact shape ("At the beginning of +/// your upkeep, if you control a commander, create a token that's a copy of this +/// creature") — the card the full-DB sweep's `DOCUMENTED_OVER_PROMPT` row names. +/// +/// `game::commander::controls_any_commander` / `controls_own_commander` scan +/// `state.battlefield` for `is_commander && controller [&& owner] && +/// is_phased_in`. Only the designation and owner conjuncts are frozen card +/// attributes (CR 903.3); membership, control and CR 702.26b phased-in status are +/// all sibling-mutable, so the gate is NOT read-free. +/// +/// BOTH `CommanderOwnership` arms are driven. Biowaste Blob's printed gate is +/// "if you control **a** commander" ⇒ `Any` (verified against +/// `data/mtgjson/AtomicCards.json`), which is the arm the paired +/// `DOCUMENTED_OVER_PROMPT` ledger row actually exercises; `Own` is the arm every +/// card in the pool that says "your commander" (Fight for the Throne) produces. +/// `commander_control_read` is shared by both, so a regression on either arm +/// alone must red here. +/// +/// REVERT-TO-RED: classify `TriggerCondition::ControlsCommander` as +/// `RwProfile::empty()` again (its pre-fix profile) and BOTH gated groups +/// auto-order — CR 603.3b sibling-ordering silently reopened. +/// +/// The second group per arm is the discrimination proof and pins that the prompt +/// is the GATE's read, not the ability: the byte-identical ability with NO +/// condition still auto-orders. (Both groups are source-DEPENDENT — +/// `CopyTokenOf{SelfRef}` — so neither takes the T1 `source_independent` fast +/// path, which is what makes the feed row observable at all.) +#[test] +fn n_d2_commander_intervening_if_is_fed_by_a_sibling_membership_write() { + let state = empty_state(); + let copy_self = || Effect::CopyTokenOf { + target: TargetFilter::SelfRef, + owner: TargetFilter::Controller, + source_filter: None, + enters_attacking: false, + tapped: false, + count: qfix(1), + extra_keywords: vec![], + additional_modifications: vec![], + }; + let ev = shared_etb_event(); + + for ownership in [ + // CR 903.3d "a commander" — Biowaste Blob, the `DOCUMENTED_OVER_PROMPT` row. + crate::types::ability::CommanderOwnership::Any, + // CR 903.3 + CR 109.5 "your commander" — Fight for the Throne. + crate::types::ability::CommanderOwnership::Own, + ] { + let gate = TriggerCondition::ControlsCommander { ownership }; + let gated = vec![ + ctx(10, ra(copy_self()), Some(gate.clone()), ev.clone(), None), + ctx(11, ra(copy_self()), Some(gate), ev.clone(), None), + ]; + assert!( + !group_is_order_independent(&state, &gated), + "CR 903.3d + CR 603.3b: the commander census the CR 603.4 gate reads is fed by \ + a sibling's battlefield-membership write ⇒ prompt (ownership = {ownership:?})" + ); + } + + let ungated = vec![ + ctx(10, ra(copy_self()), None, ev.clone(), None), + ctx(11, ra(copy_self()), None, ev, None), + ]; + assert!( + group_is_order_independent(&state, &ungated), + "discrimination: the SAME ability with no intervening-if auto-orders, so the \ + prompts above are the commander gate's board read and nothing else" + ); +} + /// N-F: die_result conjunct unit pin (CR 706.2 + CR 603.12). Two otherwise- /// identical no-input source-independent triggers off one event with DIFFERENT /// stamped die results are NOT the same state transformation ⇒ not order- diff --git a/crates/engine/src/parser/oracle_effect/conditions.rs b/crates/engine/src/parser/oracle_effect/conditions.rs index 3d16bc3f1c..8d62e92c38 100644 --- a/crates/engine/src/parser/oracle_effect/conditions.rs +++ b/crates/engine/src/parser/oracle_effect/conditions.rs @@ -4570,6 +4570,25 @@ pub(crate) fn static_condition_to_ability_condition( StaticCondition::HasCityBlessing => Some(AbilityCondition::HasCityBlessing), // CR 702.195b: The enduring story designation is available to effects. StaticCondition::HasEnduringStory => Some(AbilityCondition::HasEnduringStory), + // CR 903.3d ("If an effect refers to controlling a commander, it refers to + // a permanent on the battlefield that is a commander") + CR 608.2c: + // commander control is a plain game-state predicate about the resolving + // ability's CONTROLLER, evaluated as the ability resolves — it is not + // source-bound, layer-bound, or cost-bound like the unbridgeable statics + // below. `AbilityCondition::ControlsCommander` is its exact + // effect-resolution equivalent and carries the same `ownership` axis, so + // CR 903.3's owner-scoped "your commander" reading survives lowering. + // + // Listing it among the "no equivalent -> None" arms was a vocabulary + // asymmetry — the same defect the `CompletedADungeon` arm below fixed — + // and it silently dropped Fight for the Throne's intervening-if "if you + // control your commander", making the delayed trigger grant the monarch + // unconditionally. + StaticCondition::ControlsCommander { ownership } => { + Some(AbilityCondition::ControlsCommander { + ownership: *ownership, + }) + } StaticCondition::IsRingBearer => Some(AbilityCondition::IsRingBearer), StaticCondition::OpponentPoisonAtLeast { count } => { Some(opponent_poison_at_least_as_quantity_check(*count)) @@ -4828,7 +4847,6 @@ pub(crate) fn static_condition_to_ability_condition( | StaticCondition::UnlessPay { .. } | StaticCondition::Unrecognized { .. } | StaticCondition::RingLevelAtLeast { .. } - | StaticCondition::ControlsCommander { .. } | StaticCondition::EnchantedIsFaceDown // CR 311.2 / CR 901.7: plane face-up status is a duration-only continuous- // effect condition (evaluated in the layer system), never an @@ -4903,6 +4921,21 @@ pub(crate) fn ability_condition_to_static_condition( AbilityCondition::SourceAttachedToCreature => { Some(StaticCondition::SourceAttachedToCreature) } + // CR 903.3d: round-trips `static_condition_to_ability_condition`'s + // commander arm, carrying the CR 903.3 `ownership` axis unchanged so + // "your commander" can never widen to "a commander" in either direction. + // + // This inverse has TWO consumers, not just the per-`StaticDefinition` + // keyword-grant push-down: `triggers::delayed_intervening_if` composes it + // with `static_condition_to_trigger_condition` to recover the CR 603.4 + // fire-time reading of a delayed triggered ability's intervening-`if` + // (Fight for the Throne). Declining here would leave that half of CR 603.4 + // unenforceable. + AbilityCondition::ControlsCommander { ownership } => { + Some(StaticCondition::ControlsCommander { + ownership: *ownership, + }) + } AbilityCondition::QuantityCheck { lhs, comparator, @@ -7279,9 +7312,95 @@ mod tests { use super::*; use crate::parser::oracle_nom::condition::parse_inner_condition; use crate::parser::parse_oracle_text; - use crate::types::ability::{AggregateFunction, PlayerFilter, SharedQuality}; + use crate::types::ability::{ + AggregateFunction, CommanderOwnership, PlayerFilter, SharedQuality, + }; use crate::types::counter::{CounterMatch, CounterType}; + /// CR 903.3d + CR 608.2c: the `StaticCondition` -> `AbilityCondition` bridge + /// must lower a commander-control gate, and must keep the two `ownership` + /// arms DISTINCT — CR 903.3 + CR 109.5 "your commander" (owned and + /// controlled) is a strictly narrower predicate than CR 903.3d "a commander" + /// (controlled, any owner). A bridge that collapsed the arms would pass a + /// single-arm test and fail this one. + /// + /// The inverse assertions pin the ROUND TRIP. The inverse is not merely the + /// keyword-grant push-down any more: `triggers::delayed_intervening_if` + /// composes `ability_condition_to_static_condition` with + /// `static_condition_to_trigger_condition` to recover the CR 603.4 fire-time + /// reading of a delayed triggered ability's intervening-`if`. A declined + /// inverse silently disables that half of CR 603.4, so the round trip is + /// load-bearing and asserted in both directions, per `ownership` arm. + #[test] + fn commander_control_bridges_round_trip_preserving_ownership() { + for ownership in [CommanderOwnership::Own, CommanderOwnership::Any] { + assert_eq!( + static_condition_to_ability_condition( + &StaticCondition::ControlsCommander { ownership }, + &mut ParseContext::default(), + ), + Some(AbilityCondition::ControlsCommander { ownership }), + "CR 903.3d: the {ownership:?} commander gate must lower to its exact \ + effect-resolution equivalent, preserving the ownership axis" + ); + assert_eq!( + ability_condition_to_static_condition(&AbilityCondition::ControlsCommander { + ownership + }), + Some(StaticCondition::ControlsCommander { ownership }), + "CR 903.3: the {ownership:?} gate must invert back to its exact static \ + counterpart — the CR 603.4 fire-time hoist composes this inverse with \ + static_condition_to_trigger_condition" + ); + assert_eq!( + crate::parser::oracle_trigger::static_condition_to_trigger_condition( + &StaticCondition::ControlsCommander { ownership } + ), + Some(crate::types::ability::TriggerCondition::ControlsCommander { ownership }), + "CR 603.4: the second leg of the hoist must also preserve {ownership:?}" + ); + + // CR 603.4 + CR 903.3d, NEGATED form ("if you don't control your + // commander"). Both legs must carry the negation, or a negated gate + // round-trips through the first leg and dies at the second — leaving + // `delayed_intervening_if` with only the RESOLUTION-time half of + // CR 603.4 for that card, silently. + let negated_static = StaticCondition::Not { + condition: Box::new(StaticCondition::ControlsCommander { ownership }), + }; + let negated_ability = AbilityCondition::Not { + condition: Box::new(AbilityCondition::ControlsCommander { ownership }), + }; + assert_eq!( + static_condition_to_ability_condition( + &negated_static, + &mut ParseContext::default() + ), + Some(negated_ability.clone()), + "the negated {ownership:?} gate must lower with its negation intact" + ); + assert_eq!( + ability_condition_to_static_condition(&negated_ability), + Some(negated_static.clone()), + "the negated {ownership:?} gate must invert back to its exact static \ + counterpart" + ); + assert_eq!( + crate::parser::oracle_trigger::static_condition_to_trigger_condition( + &negated_static + ), + Some(crate::types::ability::TriggerCondition::Not { + condition: Box::new( + crate::types::ability::TriggerCondition::ControlsCommander { ownership } + ), + }), + "CR 603.4: the second leg must bridge the NEGATED {ownership:?} gate too — \ + a `None` here is the asymmetry that makes a negated commander \ + intervening-if resolution-only" + ); + } + } + /// CR 608.2c: Aven Courier's chosen-counter predicate depends on a value /// selected by the immediately preceding instruction. The generic suffix /// condition parser has no such binding and must leave the whole clause for diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index d99a00e2f0..deb74b620a 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -10,8 +10,9 @@ use crate::parser::parse_oracle_text; use crate::types::ability::CardPlayMode::{Cast, Play}; use crate::types::ability::CastFromZoneDriver::{DuringResolution, LingeringPermission}; use crate::types::ability::{ - AttachmentKind, CardSelectionMode, CastManaObjectScope, CastManaSpentMetric, DigRestOrder, - ExcessRecipient, ForEachCategoryAction, ModalChoice, PerpetualModification, SeatDirection, + AttachmentKind, CardSelectionMode, CastManaObjectScope, CastManaSpentMetric, + CommanderOwnership, DigRestOrder, ExcessRecipient, ForEachCategoryAction, ModalChoice, + PerpetualModification, SeatDirection, }; use crate::types::card_type::CoreType; use crate::types::mana::{ManaCost, ManaCostShard}; @@ -51493,6 +51494,86 @@ fn instead_override_lowers_the_typed_completed_dungeon_condition() { ); } +/// CR 603.4 + CR 608.2c + CR 903.3: Fight for the Throne's delayed triggered +/// ability carries an intervening-`if` — "When the creature an opponent +/// controls dies this turn, *if you control your commander*, you become the +/// monarch." The gate must survive lowering as a TYPED `AbilityCondition`. +/// +/// This is the same vocabulary asymmetry the `CompletedADungeon` test above +/// pins, one variant over: `parse_inner_condition` produced +/// `StaticCondition::ControlsCommander` all along, but the StaticCondition -> +/// AbilityCondition bridge declared it had "no effect-resolution equivalent", +/// so `strip_leading_general_conditional` dropped it on the floor and the +/// delayed trigger made you the monarch UNCONDITIONALLY. +/// +/// SHAPE test — it pins the parsed AST only. The runtime semantics, including +/// the `Own`-vs-`Any` stolen-commander discrimination, are covered by +/// `tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs`, +/// which drives the real cast pipeline. +#[test] +fn delayed_trigger_intervening_if_retains_the_commander_control_gate() { + let parsed = parse_oracle_text( + "Put a +1/+1 counter on target creature you control. Then it fights target \ + creature an opponent controls. When the creature an opponent controls dies \ + this turn, if you control your commander, you become the monarch.", + "Fight for the Throne", + &[], + &["Instant".to_string()], + &[], + ); + let ability = parsed.abilities.first().expect("expected a spell ability"); + + // Reach-guard: every clause must lower for real, so the assertions below are + // read off a fully-parsed chain rather than off an honest-failure stub. + let _ = crate::types::ability_visit::visit_ability_def(ability, &mut |effect| { + assert!( + !matches!(effect, Effect::Unimplemented { .. }), + "no clause of Fight for the Throne may lower to Unimplemented: {effect:#?}" + ); + std::ops::ControlFlow::Continue(()) + }); + + let mut delayed = None; + let _ = crate::types::ability_visit::visit_ability_def(ability, &mut |effect| { + if let Effect::CreateDelayedTrigger { + condition, effect, .. + } = effect + { + delayed = Some((condition.clone(), (**effect).clone())); + return std::ops::ControlFlow::Break(()); + } + std::ops::ControlFlow::Continue(()) + }); + let (delayed_condition, delayed_body) = + delayed.expect("the third sentence must lower to a CreateDelayedTrigger"); + + // CR 603.7c unchanged-behaviour guard: the fought creature stays the trigger's + // referent. Restoring the gate must not disturb the already-correct binding. + assert_eq!( + delayed_condition, + DelayedTriggerCondition::WhenDies { + filter: TargetFilter::ParentTarget + }, + "the delayed trigger must still watch the fought creature" + ); + assert_eq!( + *delayed_body.effect, + Effect::BecomeMonarch, + "the delayed body must still be the monarch effect" + ); + // CR 903.3 + CR 109.5: the regression assertion. Reverting the bridge arm in + // `static_condition_to_ability_condition` makes this `None`, which is exactly + // the shipped misparse — an unconditional monarch grant. + assert_eq!( + delayed_body.condition, + Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }), + "the intervening-if must ride the delayed body as a typed OWNER-scoped \ + commander gate" + ); +} + /// CR 608.2c + CR 614.6: the honest-failure path is CLAUSE-scoped — an "instead" /// override must never absorb the sentence BEFORE it, and must never be emitted /// as an unconditional sequel to the effect it is supposed to REPLACE. diff --git a/crates/engine/src/parser/oracle_trigger.rs b/crates/engine/src/parser/oracle_trigger.rs index 22c4509637..9092dba8c9 100644 --- a/crates/engine/src/parser/oracle_trigger.rs +++ b/crates/engine/src/parser/oracle_trigger.rs @@ -543,6 +543,9 @@ fn rewrite_cost_x_in_condition(cond: &mut crate::types::ability::AbilityConditio | AbilityCondition::IsInitiative | AbilityCondition::HasCityBlessing | AbilityCondition::HasEnduringStory + // CR 903.3d: carries no `QuantityExpr` and nests no condition — nothing + // for the cost-X rewrite to bind. + | AbilityCondition::ControlsCommander { .. } | AbilityCondition::DiscardedCardMatchesFilter { .. } | AbilityCondition::IsRingBearer | AbilityCondition::CompletedDungeon { .. } @@ -4566,6 +4569,18 @@ pub(crate) fn static_condition_to_trigger_condition( variant: *variant, }), }), + // CR 903.3d + CR 603.4: "if you don't control your/a commander". The + // affirmative arm bridges below; without this one the negated form + // round-trips through `ability_condition_to_static_condition` and then + // dies here, so `triggers::delayed_intervening_if` would silently keep + // only the RESOLUTION-time half of CR 603.4 for a negated commander + // gate. `ownership` rides through unchanged — CR 903.3 "your commander" + // must not widen to CR 903.3d "a commander" under negation either. + StaticCondition::ControlsCommander { ownership } => Some(TriggerCondition::Not { + condition: Box::new(TriggerCondition::ControlsCommander { + ownership: *ownership, + }), + }), _ => None, }, diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index caf3fd59cb..4d9c0d037c 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -15071,6 +15071,33 @@ impl TargetFilter { } } + /// CR 608.2c + CR 400.7: Returns true when this filter names ONE object that + /// is already BOUND — a specific permanent or an anaphor to one — rather than + /// a CLASS of objects re-matched against the live board on every check. + /// + /// Used where "how many times can an event matching this filter occur?" is + /// the question, e.g. CR 603.7b's one-shot delayed triggers: a bound object + /// dies, leaves, or enters exactly once per incarnation (a returning object + /// is a NEW object, CR 400.7 / CR 603.7c), whereas a class filter ("a + /// creature") can match an unbounded number of later occurrences. + /// + /// Deliberately NARROWER than the parser's `is_single_object_ref`: that + /// helper also admits `TriggeringSource`, which is re-resolved from whichever + /// event is being examined and is therefore NOT bound in advance, and it has + /// no reason to admit `SpecificObject` / `ParentTargetSlot`. `TrackedSet` + /// and `LastCreated` are excluded for the same reason as a class filter — + /// they can name several objects, so several occurrences. + pub fn names_bound_single_object(&self) -> bool { + matches!( + self, + TargetFilter::SelfRef + | TargetFilter::SpecificObject { .. } + | TargetFilter::AttachedTo + | TargetFilter::ParentTarget + | TargetFilter::ParentTargetSlot { .. } + ) + } + /// CR 115.1: Returns true for filters that are NOT player-chosen targets — /// context references (triggering event participants per CR 603.7c), /// parent target anaphora, and self-references resolve automatically @@ -20079,6 +20106,31 @@ pub enum AbilityCondition { HasCityBlessing, /// CR 702.195b: True when the ability controller has the enduring story designation. HasEnduringStory, + /// CR 903.3d + CR 608.2c: Resolution-time commander-control gate — "if you + /// control your commander" / "if you control a commander". CR 903.3d is the + /// authorizing rule: "If an effect refers to controlling a commander, it + /// refers to a permanent on the battlefield that is a commander." + /// + /// The effect-resolution mirror of `StaticCondition::ControlsCommander` + /// (layers), `TriggerCondition::ControlsCommander` (intervening-if on a + /// printed trigger) and `ParsedCondition::ControlsCommander` + /// (activation/casting restrictions), carrying the same `ownership` axis. + /// + /// CR 903.3 + CR 109.5: `Own` ("your commander") requires the evaluating + /// player to both OWN and control it — CR 903.3 makes the designation an + /// attribute of the *card*, so a stolen commander remains its owner's, not + /// yours. `Any` ("a commander") is controller-only, any owner. + /// + /// CR 109.5: "you" is the resolving ability's controller; for a delayed + /// triggered ability that is the player who controlled the creating spell as + /// it resolved (CR 603.7d), which `Effect::CreateDelayedTrigger` already + /// stamps onto the delayed `ResolvedAbility`. + /// + /// Evaluated live at resolution against the single `game::commander` + /// authority — the same helpers the three sibling mirrors use — so the four + /// readings of one printed clause cannot drift, and the CR 702.26b + /// phased-out exclusion applies uniformly. + ControlsCommander { ownership: CommanderOwnership }, /// CR 701.9a + CR 608.2c: True when the card discarded by the directly /// preceding discard instruction matches `filter`. The resolver reads the /// discard operation's captured hand-time result, never current-zone state @@ -20375,6 +20427,14 @@ impl AbilityCondition { | AbilityCondition::IsInitiative | AbilityCondition::HasCityBlessing | AbilityCondition::HasEnduringStory + // CR 903.3d: a commander-control gate is a plain game-state predicate + // about the resolving ability's controller. It says nothing about + // whether an antecedent optional effect was performed, so a token + // minted under this gate is unconditionally live and the referent + // re-link must NOT fold the gate away as a reflexive "if you do". + // Same classification, for the same reason, as `IsMonarch` and + // `CompletedDungeon` below. + | AbilityCondition::ControlsCommander { .. } | AbilityCondition::DiscardedCardMatchesFilter { .. } | AbilityCondition::IsRingBearer | AbilityCondition::HasObjectTarget diff --git a/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs b/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs new file mode 100644 index 0000000000..4de8850543 --- /dev/null +++ b/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs @@ -0,0 +1,241 @@ +//! Fight for the Throne — the delayed `BecomeMonarch` trigger must honour its +//! intervening-`if` "if you control your commander". +//! +//! Oracle (MSC, verbatim): "Put a +1/+1 counter on target creature you control. +//! Then it fights target creature an opponent controls. When the creature an +//! opponent controls dies this turn, if you control your commander, you become +//! the monarch." +//! +//! The bug: `parse_inner_condition` produced +//! `StaticCondition::ControlsCommander { ownership: Own }` correctly, but the +//! `StaticCondition` -> `AbilityCondition` bridge +//! (`parser::oracle_effect::conditions::static_condition_to_ability_condition`) +//! listed that variant among its "no effect-resolution equivalent -> None" arms, +//! because `AbilityCondition` was the only one of the four condition +//! vocabularies missing a `ControlsCommander` mirror. So +//! `strip_leading_general_conditional` silently discarded the gate and the +//! delayed trigger made you the monarch UNCONDITIONALLY when the fought creature +//! died. +//! +//! These tests parse the verbatim Oracle text through +//! `add_spell_to_hand_from_oracle` (the production parser path the fix modifies) +//! and drive the full cast pipeline, so they exercise parser + runtime +//! end-to-end. They need no `integration_cards.json` regeneration — the card is +//! not in that fixture. +//! +//! Every negative fixture is paired with a positive reach-guard: the fought +//! creature must actually be in the graveyard. Without it a fixture would pass +//! vacuously if the fight failed to kill, the spell fizzled on targeting, or the +//! delayed trigger never fired at all. + +use engine::game::scenario::{GameScenario, P0, P1}; +use engine::types::phase::Phase; +use engine::types::player::PlayerId; +use engine::types::zones::Zone; + +const ORACLE: &str = "Put a +1/+1 counter on target creature you control. \ +Then it fights target creature an opponent controls. When the creature an opponent \ +controls dies this turn, if you control your commander, you become the monarch."; + +/// The post-resolution facts each fixture asserts on. +/// +/// The two commander predicates are non-vacuity guards, not incidental extras: +/// the stolen-commander and owned-but-not-controlled fixtures below are only +/// meaningful if the owner/controller divergence actually materialized. Without +/// them, a `controlled_by` that silently failed (Layer 2 recomputes `controller` +/// from `base_controller` on every pass) would leave P0 controlling no commander +/// at all, and both fixtures would pass for entirely the wrong reason. +struct FightResult { + monarch: Option, + fought_creature_zone: Zone, + /// CR 903.3d: does P0 control a commander on the battlefield, any owner? + p0_controls_any_commander: bool, + /// CR 903.3: does a commander P0 OWNS sit on the battlefield, any controller? + p0_owns_battlefield_commander: bool, +} + +/// Cast and fully resolve Fight for the Throne, with `stage_commander` given the +/// chance to place a commander object first. +fn resolve_fight_for_the_throne(stage_commander: impl FnOnce(&mut GameScenario)) -> FightResult { + let mut scenario = GameScenario::new_n_player(2, 42); + scenario.at_phase(Phase::PreCombatMain); + + let spell = scenario + .add_spell_to_hand_from_oracle(P0, "Fight for the Throne", true, ORACLE) + .id(); + // CR 613.4c + CR 701.14a: the +1/+1 counter makes the 5/5 a 6/6, then each + // creature deals damage equal to its power to the other — 6 kills the 1/1, + // and the 1 dealt back leaves the 6/6 alive. Only the opponent's creature + // dies, which is exactly the event the delayed trigger watches. + let mine = scenario.add_creature(P0, "Throne Claimant", 5, 5).id(); + let theirs = scenario.add_creature(P1, "Doomed Squire", 1, 1).id(); + + // The commander is ALWAYS a distinct object from both fighters. The gate is + // evaluated live as the delayed ability resolves (CR 608.2c), so a commander + // that were itself a fight participant could change zones mid-resolution and + // make the expected value depend on the fight outcome rather than on the gate. + stage_commander(&mut scenario); + + let mut runner = scenario.build(); + // CR 601.2c: slots answered in written order — the PutCounter target (a + // creature you control), then the Fight target (a creature an opponent + // controls). + let outcome = runner.cast(spell).target_objects(&[mine, theirs]).resolve(); + let state = outcome.state(); + FightResult { + monarch: state.monarch, + fought_creature_zone: outcome.zone_of(theirs), + // The sibling authority the `Own` arm must NOT be widened to. + p0_controls_any_commander: engine::game::commander::controls_any_commander(state, P0), + p0_owns_battlefield_commander: state.battlefield.iter().any(|id| { + state + .objects + .get(id) + .is_some_and(|obj| obj.is_commander && obj.owner == P0) + }), + } +} + +/// POSITIVE — CR 903.3 + CR 903.3d: P0 owns and controls a commander on the +/// battlefield, so the intervening-`if` holds and the delayed trigger grants the +/// monarch designation (CR 725.1). +/// +/// This is the reachability proof for the whole file: it shows the delayed +/// trigger fires, its gate is evaluated, and a passing gate still produces the +/// monarch. Without it the three negative fixtures below could all be passing +/// because the trigger never runs. +#[test] +fn own_commander_on_battlefield_grants_the_monarch() { + let result = resolve_fight_for_the_throne(|scenario| { + scenario + .add_creature(P0, "Your Commander", 2, 2) + .commander(); + }); + + assert_eq!( + result.fought_creature_zone, + Zone::Graveyard, + "reach-guard: the fought creature must actually have died for the delayed \ + trigger to fire" + ); + assert_eq!( + result.monarch, + Some(P0), + "CR 903.3d + CR 725.1: with an owned-and-controlled commander on the \ + battlefield the intervening-if holds, so P0 becomes the monarch" + ); +} + +/// NEGATIVE + reach-guard — CR 603.4 + CR 608.2c: with no commander anywhere, +/// the intervening-`if` fails as the delayed ability resolves and nobody becomes +/// the monarch. +/// +/// THE REGRESSION ASSERTION. Before the fix the gate was dropped during lowering +/// and this read `Some(P0)` — an unconditional monarch grant. Reverting the +/// bridge arm in `static_condition_to_ability_condition` flips it back. +#[test] +fn no_commander_means_no_monarch() { + let result = resolve_fight_for_the_throne(|_scenario| {}); + + assert_eq!( + result.fought_creature_zone, + Zone::Graveyard, + "reach-guard: the fought creature must actually have died, so the delayed \ + trigger genuinely fired and was GATED rather than skipped upstream" + ); + assert_eq!( + result.monarch, None, + "CR 608.2c: the intervening-if \"if you control your commander\" fails, so \ + the delayed BecomeMonarch must do nothing. Some(P0) here is the shipped \ + misparse — a dropped gate granting the monarch unconditionally" + ); +} + +/// NEGATIVE + reach-guard — the `Own`-vs-`Any` discriminator. P0 has gained +/// control of P1's commander and owns none of their own. +/// +/// CR 903.3: the commander designation "is not a characteristic of the object +/// represented by the card; rather, it is an attribute of the card itself", so a +/// stolen commander remains its OWNER's commander. Combined with CR 109.5's +/// possessive "your", `controls_own_commander` is false here even though +/// `controls_any_commander` is true. +/// +/// An implementation that delegated the `Own` arm to `controls_any_commander` +/// flips this to `Some(P0)`. +#[test] +fn stolen_opponent_commander_does_not_satisfy_your_commander() { + let result = resolve_fight_for_the_throne(|scenario| { + scenario + .add_creature(P1, "Opposing Commander", 3, 3) + .commander() + .controlled_by(P0); + }); + + assert_eq!( + result.fought_creature_zone, + Zone::Graveyard, + "reach-guard: the fought creature must actually have died for the gate to \ + be reached" + ); + // NON-VACUITY GUARD: the divergence must really exist. If `controlled_by` + // had silently failed, P0 would control no commander at all and the negative + // below would pass without discriminating anything. + assert!( + result.p0_controls_any_commander, + "CR 903.3d: the stolen commander must genuinely be under P0's control, so \ + the Any predicate is TRUE while the Own predicate must be FALSE — that \ + gap is the whole point of this fixture" + ); + assert!( + !result.p0_owns_battlefield_commander, + "P0 must own no commander here, or the fixture would not isolate the \ + owner conjunct" + ); + assert_eq!( + result.monarch, None, + "CR 903.3 + CR 109.5: a commander P0 merely CONTROLS is still its owner's \ + commander, so \"your commander\" is not satisfied. Some(P0) means the Own \ + arm was widened to controls_any_commander" + ); +} + +/// NEGATIVE + reach-guard — the other conjunct. P0 OWNS a commander but P1 +/// controls it, and P0 controls no other commander. +/// +/// CR 903.3d resolves "controlling a commander" against a permanent on the +/// battlefield, and CR 613.1b (Layer 2) is what moved control. Ownership alone +/// must not satisfy the gate. +#[test] +fn owning_a_commander_you_do_not_control_does_not_satisfy_the_gate() { + let result = resolve_fight_for_the_throne(|scenario| { + scenario + .add_creature(P0, "Your Commander", 3, 3) + .commander() + .controlled_by(P1); + }); + + assert_eq!( + result.fought_creature_zone, + Zone::Graveyard, + "reach-guard: the fought creature must actually have died for the gate to \ + be reached" + ); + // NON-VACUITY GUARD: P0's commander must really be on the battlefield (just + // under P1's control). Otherwise this fixture degenerates into "no commander + // exists", which the `no_commander_means_no_monarch` case already covers. + assert!( + result.p0_owns_battlefield_commander, + "CR 903.3: a commander P0 OWNS must be on the battlefield, so only the \ + controller conjunct is doing the work here" + ); + assert!( + !result.p0_controls_any_commander, + "CR 613.1b: control must genuinely have moved to P1, or the controller \ + conjunct is not being isolated" + ); + assert_eq!( + result.monarch, None, + "CR 903.3d: P0 owns but does not CONTROL the commander, so the gate fails. \ + Some(P0) means the Own arm dropped its controller conjunct" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 6155c250ea..3a3c52e9a5 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -223,6 +223,7 @@ mod fevered_visions; mod fewer_than_existential_threshold; mod field_marshal_soldier_anthem_first_strike; mod field_of_ruin_search; +mod fight_for_the_throne_monarch_gated_on_commander; mod finality_counter_death_to_exile; mod fireball_x_cost_surcharge_timing; mod flashback_nonmana_payability; From 0384967b4674517f0b2461ce9ad9a4cadecab5bd Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 01:18:26 -0500 Subject: [PATCH 2/5] fix(engine): consume a one-shot WhenNextEvent on a false intervening-if `false_gate_consumes_one_shot` discarded a one-shot delayed ability whose hoisted intervening-`if` was false only when the `WhenNextEvent` carried the `Reflexive` lifetime. A `ThisTurn` or `Persistent` `WhenNextEvent` therefore stayed installed after the single occurrence it named had already happened, so a later matching event could still fire an ability that CR 603.4 had already resolved as doing nothing. That silently rewrote "when you next X, if C" into "when you next X for which C holds". Every shape the parser builds for this variant names one occurrence: * `ThisTurn` is only ever built from "when you next [event] this turn" (`try_parse_when_next_generic_event`, `build_when_next_delayed_trigger`). "Next" is the ability's own single-occurrence wording; the stated duration bounds only how long it waits for that one occurrence. CR 603.7b's "unless it has a stated duration" clause lifts the once-only cap for "whenever ... this turn" (`WheneverEvent`), not for a "next". * `Persistent` has no stated duration at all (The Pandorica's "when ~ becomes untapped or leaves the battlefield", "when a player planeswalks"), so CR 603.7b's unqualified "will trigger only once - the next time its trigger event occurs" applies directly. * `Reflexive` is CR 603.12, checked only against its creation batch. So the arm returns true for the whole variant. The broad-filter carve-outs (`WhenDies` and friends with a non-bound filter) are unchanged - those are what CR 603.7b's stated-duration clause keeps watching. Adds a two-batch regression through the production `check_delayed_triggers` path: a non-reflexive `ThisTurn` `WhenNextEvent` sees a matching event with the gate false, is asserted consumed, then the gate is made true and a second matching event is asserted unable to reach the stack. A gate-true reachability probe keeps the assertions from passing vacuously. The test fails on the prior implementation. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/triggers.rs | 164 +++++++++++++++++++++++++++-- 1 file changed, 158 insertions(+), 6 deletions(-) diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 2418be81a1..7d29167fc8 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -9381,8 +9381,21 @@ fn delayed_intervening_if(ability: &ResolvedAbility) -> Option /// `TargetFilter::names_bound_single_object`). The bound object departs once /// per incarnation, and an object that returns is a NEW object (CR 400.7 / /// CR 603.7c), so a retained ability could never match it anyway; -/// * a CR 603.12 reflexive, which is checked ONLY against its creation batch and -/// must not be left to fire on a later same-turn event. +/// * EVERY `WhenNextEvent`, whatever its `DelayedTriggerLifetime`. Each shape the +/// parser builds for this variant names ONE occurrence, so the matched event +/// has spent it: +/// - `ThisTurn` is only ever built from "when you **next** [event] this turn" +/// (`try_parse_when_next_generic_event` / `build_when_next_delayed_trigger`). +/// "Next" is the ability's own single-occurrence wording; the stated duration +/// bounds only how long it WAITS for that one occurrence, and CR 603.7b's +/// "unless it has a stated duration" clause lifts the once-only cap for +/// "whenever … this turn" (`WheneverEvent`), not for a "next"; +/// - `Persistent` has NO stated duration (The Pandorica's "when ~ becomes +/// untapped or leaves the battlefield", "when a player planeswalks"), so +/// CR 603.7b's unqualified "will trigger only once — the next time its +/// trigger event occurs" applies directly; +/// - `Reflexive` (CR 603.12) is checked ONLY against its creation batch and must +/// not be left to fire on a later same-turn event. /// /// A BROAD-FILTER event form ("when a creature dies this turn, if X, …") is /// exactly what CR 603.7b's "unless it has a stated duration" clause keeps @@ -9406,10 +9419,13 @@ fn false_gate_consumes_one_shot(condition: &DelayedTriggerCondition) -> bool { | DelayedTriggerCondition::WhenDiesOrExiled { filter } => { filter.names_bound_single_object() } - // CR 603.12: a reflexive gets exactly one shot on its creation batch — - // the same rule the unmatched-reflexive discard below enforces. Every - // other `WhenNextEvent` watches a broad event predicate. - DelayedTriggerCondition::WhenNextEvent { .. } => is_reflexive_lifetime(condition), + // CR 603.4 + CR 603.7b (+ CR 603.12 for `Reflexive`): every lifetime this + // variant carries names ONE occurrence, and the matched event has now + // spent it — see the per-lifetime breakdown above. Retaining it would + // silently rewrite "when you next X, if C" into "when you next X for + // which C holds", letting a later X fire an ability CR 603.4 already + // resolved as doing nothing. + DelayedTriggerCondition::WhenNextEvent { .. } => true, // Never one-shot (`effects::delayed_trigger` computes `one_shot` as // "not `WheneverEvent`"), so this arm is unreachable from the caller; // spelled out rather than wildcarded to keep the match exhaustive. @@ -19021,6 +19037,142 @@ pub mod tests { ); } + /// CR 603.4 + CR 603.7b for the `WhenNextEvent` one-shot class, driven through + /// the production `check_delayed_triggers` path over TWO separate event + /// batches. + /// + /// "When you next [event] this turn, if [gate], …" names ONE occurrence. When + /// that occurrence arrives with the gate false the ability "does nothing" + /// (CR 603.4) and its single shot is spent (CR 603.7b) — making the gate true + /// afterwards must NOT let a second matching event resurrect it. Retaining it + /// would silently rewrite the ability into "when you next [event] for which + /// [gate] holds". + /// + /// REVERT-TO-RED: restore `is_reflexive_lifetime(condition)` on the + /// `WhenNextEvent` arm of `false_gate_consumes_one_shot` and this + /// non-reflexive `ThisTurn` trigger survives batch one, so `after_first` + /// is `1` and the second batch puts the ability on the stack. + #[test] + fn when_next_event_one_shot_is_consumed_by_a_false_intervening_if() { + use crate::types::ability::DelayedTriggerLifetime; + use crate::types::triggers::TriggerMode; + + /// Installs "when you next play a land this turn, if you control your + /// commander, you become the monarch" and returns the land whose play is + /// the matching event. + fn install(state: &mut GameState) -> ObjectId { + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + state, + CardId(0x0603_0407), + controller, + "Next Land Rider".to_string(), + Zone::Battlefield, + ); + let land = create_object( + state, + CardId(0x0603_0408), + controller, + "Played Land".to_string(), + Zone::Battlefield, + ); + + // CR 305.1 + CR 603.2: scope the delayed event to the controller's + // own land drop, exactly as the parser's `WhenNextEvent` builders do. + let mut trigger_def = TriggerDefinition::new(TriggerMode::LandPlayed); + trigger_def.valid_target = Some(TargetFilter::Controller); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + // `WhenNextEvent` matching requires the delayed ability's CR 400.7 + // source context; without it `delayed_trigger_event_with_index` + // returns `None` and the fixture would prove nothing. + let source_object = state.objects.get(&source).expect("installed source"); + ability.trigger_source = Some(trigger_source_context_for_latch(state, source_object)); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WhenNextEvent { + trigger: Box::new(trigger_def), + or_trigger: None, + lifetime: DelayedTriggerLifetime::ThisTurn, + }, + ability: Box::new(ability), + controller, + source_id: source, + one_shot: true, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + land + } + + fn stage_commander(state: &mut GameState) { + // CR 903.3 + CR 903.3d: owned AND controlled, on the battlefield. + let commander = make_creature(state, PlayerId(0), "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + + fn land_played(land: ObjectId) -> GameEvent { + GameEvent::LandPlayed { + object_id: land, + player_id: PlayerId(0), + from_zone: Zone::Hand, + } + } + + // Reachability proof: the same fixture with the gate TRUE on the first + // matching event does reach the stack, so the assertions below are about + // the gate and not about a trigger that never matched `LandPlayed`. + let mut reachable = setup(); + let reachable_land = install(&mut reachable); + stage_commander(&mut reachable); + check_delayed_triggers(&mut reachable, &[land_played(reachable_land)]); + assert_eq!( + reachable.stack.len(), + 1, + "reachability proof: with the gate TRUE the first land drop fires the delayed ability" + ); + + let mut state = setup(); + let land = install(&mut state); + + // Batch one: the ability's single named occurrence, gate FALSE. + check_delayed_triggers(&mut state, &[land_played(land)]); + assert_eq!( + state.stack.len(), + 0, + "CR 603.4: a false intervening-`if` means the ability never triggers, so it must \ + not be put onto the stack" + ); + let after_first = state.delayed_triggers.len(); + assert_eq!( + after_first, 0, + "CR 603.7b: \"when you NEXT play a land this turn\" names ONE occurrence, and that \ + occurrence has now happened — the one-shot is consumed, not left installed" + ); + + // Batch two: the gate is now TRUE and another land is played. A consumed + // one-shot must stay consumed; a retained one would fire here. + stage_commander(&mut state); + check_delayed_triggers(&mut state, &[land_played(land)]); + assert_eq!( + state.stack.len(), + 0, + "CR 603.4 + CR 603.7b: the ability already spent its single occurrence with the \ + gate false; making the gate true afterwards must not let a LATER land drop fire it" + ); + } + /// HOSTILE fixture for the CR 603.4 hoist's carve-out. /// /// "…, if X, A. Otherwise, B." is NOT an intervening-`if` in the CR 603.4 From 91ba9f4689a06ada4947560c0496a3eab83b7f03 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 11:28:51 -0500 Subject: [PATCH 3/5] fix(PR-7389): address review on the CR 603.4 delayed-trigger hoist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking review — `WhenNextEvent` one-shot consumption ------------------------------------------------------ The review held that `false_gate_consumes_one_shot` wrongly consumes EVERY `WhenNextEvent`, because `parse_dealt_damage_this_way_dies_trigger` accepts the broad stated-duration form "a creature dealt damage this way dies this turn" and lowers it to `WhenNextEvent { lifetime: ThisTurn }` — which CR 603.7b's "unless it has a stated duration" clause says must keep watching. The code fact is right; the conclusion is not, and the reason the review reached it is a FALSE claim in this repo's own doc comment. That doc asserted `ThisTurn` "is only ever built from 'when you next [event] this turn'". It is not: Skeletonize's "When a creature dealt damage this way dies this turn" builds it too, with no "next" anywhere. The actual discriminator is WotC's "When" / "Whenever" templating, which the parser already keys on, and CR 603.7b's two outcomes are already modelled as two SIBLING conditions rather than a lifetime: - "Whenever … dealt damage this way dies this turn" -> `WheneverEvent` (multi-fire). Ghired's Belligerence, Reckless Blaze — damage spread over many creatures, so many deaths can qualify. - "When … dealt damage this way dies this turn" -> `WhenNextEvent` (one-shot). Skeletonize — damage to a SINGLE target creature, so at most one death can ever qualify. Those are the only three cards in the pool with that wording and the split is exact for all three. A stated-duration ability that must keep watching is a `WheneverEvent`, and it cannot reach this function at all: `effects::delayed_trigger` computes `one_shot = !matches!(condition, WheneverEvent { .. })` and the only caller gates on `delayed.one_shot`. The protection the review asked for is therefore structural and already present. So the behaviour is unchanged and the justification is rewritten to the one that actually holds. Two tests now pin what the prose claims: - `stated_duration_multi_fire_survives_a_false_intervening_if` — the production-path TWO-EVENT regression the review asked for: gate false on the first matching event, true on the second; the ability must survive the first and fire on the second. One-event fixtures pass under either reading, which is what makes two events the discriminating shape. It also asserts the fired stack entry still carries the gate, so CR 603.4's resolution-time half stays armed (this is what denies the ability if control is lost after it triggers). - `inline_delayed_trigger_whenever_damage_this_way_dies_is_multi_fire` — pins the routing itself, so re-routing the "whenever" form onto `WhenNextEvent` fails loudly instead of silently voiding the guarantee above. An `add-engine-variant` gate run over "add a stated-duration variant to `DelayedTriggerLifetime`" returned REFUSE_WITH_EXISTING_SLOT at Stage 1: the concept already exists as `DelayedTriggerCondition::WheneverEvent`. No new engine surface ships. Review comments --------------- - `delayed_intervening_if`: the review asked for a guard against evaluating a source-relative gate with no creation-time `trigger_source` (which would read nothing, evaluate false for want of a reading rather than on the game state, and via the consumption above DELETE a valid one-shot). The failure mode is real in shape but UNREACHABLE: matching runs before gating and already demands that context — `delayed_trigger_event_with_index` opens its `WhenNextEvent` arm with `let source_context = source_context?;` — so a contextless one-shot never matches an event, never reaches the gate, and is never discarded. A guard was written, and then removed: besides being unreachable it declined the hoist for gates whose fire-time reading is well-defined without a source, reding three existing pins (`divergent_gate_bindings_…`, `non_battlefield_presence_gate_…`, `resolution_scoped_quantity_gate_…`). The reachability argument is documented on `delayed_intervening_if` instead, so the absence of the guard reads as adjudicated rather than overlooked. No machinery ships for a case the control flow already forecloses. - `ability_rw.rs`: the three `ControlsCommander` arms now bind `ownership` explicitly. The module's M3 mandate is review-blocking — `{ .. }` elision is permitted only on maximal-conservative arms, and `commander_control_read()` is a precise profile. - CR citations: CR 608.2c is "follows its instructions in the order written" and was wrong at four sites. `names_bound_single_object` -> CR 603.7c (delayed ability referring to a particular object); the `AbilityCondition` commander gate -> CR 608.2a (resolution-time intervening-`if` recheck); the `StaticCondition` bridge and the Fight for the Throne parse test -> CR 603.4. Each replacement verified against `docs/MagicCompRules.txt`. Merge integration ----------------- The merge with main added two variants that this branch's deliberately exhaustive fail-closed classifiers refused to compile without adjudication — working as designed: - `PlayerScope::SpecificPlayer` -> NOT unbound. A concrete `PlayerId` already snapshotted at resolution is a literal; both legs read the same value. - `QuantityRef::PlayerChosenNumber` -> DIVERGES. The secret-number ledger is populated by, and cleared per, resolution — at fire time it holds an unrelated resolution's numbers or none. Card premise re-verified against Scryfall: Fight for the Throne reads "… When the creature an opponent controls dies this turn, if you control your commander, you become the monarch." Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/ability_rw.rs | 13 +- crates/engine/src/game/triggers.rs | 248 ++++++++++++++++-- .../src/parser/oracle_effect/conditions.rs | 2 +- .../engine/src/parser/oracle_effect/tests.rs | 52 +++- crates/engine/src/types/ability.rs | 7 +- 5 files changed, 288 insertions(+), 34 deletions(-) diff --git a/crates/engine/src/game/ability_rw.rs b/crates/engine/src/game/ability_rw.rs index ee7492c5c7..5ec96bf62d 100644 --- a/crates/engine/src/game/ability_rw.rs +++ b/crates/engine/src/game/ability_rw.rs @@ -6250,7 +6250,12 @@ fn rw_ability_condition(x: &AbilityCondition) -> RwProfile { // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. // The three condition-vocabulary mirrors of this ONE printed clause share // that helper, so none of them can drift from the others. - AbilityCondition::ControlsCommander { .. } => commander_control_read(), + // M3 binding mandate: `commander_control_read()` is a PRECISE profile, not + // `RwProfile::conservative()`, so this arm binds every payload field. + // `ownership` is deliberately not read: CR 903.3d makes both arms the same + // live battlefield census, and the Own/Any distinction narrows WHICH + // commanders qualify, not which state the read touches. + AbilityCondition::ControlsCommander { ownership: _ } => commander_control_read(), AbilityCondition::AdditionalCostPaidInstead | AbilityCondition::AlternativeManaCostPaid | AbilityCondition::EffectOutcome { .. } @@ -6397,7 +6402,8 @@ fn rw_trigger_condition(x: &TriggerCondition) -> RwProfile { // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. // Shared with the `AbilityCondition` / `StaticCondition` mirrors of the // same printed clause. - TriggerCondition::ControlsCommander { .. } => commander_control_read(), + // M3 binding mandate: precise RHS, so bind every payload field. + TriggerCondition::ControlsCommander { ownership: _ } => commander_control_read(), } } @@ -6502,7 +6508,8 @@ fn rw_static_condition(x: &StaticCondition) -> RwProfile { // CR 903.3d: a LIVE battlefield census — see `commander_control_read`. // Shared with the `AbilityCondition` / `TriggerCondition` mirrors of the // same printed clause. - StaticCondition::ControlsCommander { .. } => commander_control_read(), + // M3 binding mandate: precise RHS, so bind every payload field. + StaticCondition::ControlsCommander { ownership: _ } => commander_control_read(), } } diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 29b21bd82f..991c854cdf 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -9034,6 +9034,12 @@ fn player_scope_unbound_at_fire_time(scope: &PlayerScope) -> bool { | PlayerScope::Opponent { .. } | PlayerScope::DefendingPlayer | PlayerScope::SourceChosenPlayer + // CR 109.4: a concrete `PlayerId` already SNAPSHOTTED at resolution — a + // literal, so there is nothing left to bind and both legs read the same + // value. Same argument as `WhenLeavesPlay`'s already-resolved `ObjectId`. + // Like `AnyTurn` it is duration-timing-only and never reaches a quantity, + // but is adjudicated here rather than wildcarded. + | PlayerScope::SpecificPlayer { .. } | PlayerScope::AnyTurn => false, } } @@ -9172,6 +9178,14 @@ fn quantity_ref_binding_diverges(qty: &QuantityRef) -> bool { | QuantityRef::ExiledFromHandThisResolution | QuantityRef::PreviousEffectAmount { .. } | QuantityRef::TimesCostPaidThisResolution + // CR 608.2c: the secret-number ledger is populated BY the + // resolution that ran the choice (Wheel of Misfortune, Menacing Ogre) and + // is cleared per resolution — players who chose no number THIS resolution + // are excluded from the aggregate entirely. At fire time the ledger holds + // an unrelated resolution's numbers or none, so the two legs cannot agree. + // The `player` payload is irrelevant to that verdict: the ledger itself is + // resolution-scoped whichever player the scope selects. + | QuantityRef::PlayerChosenNumber { .. } // CR 701.38: the vote tally is published by the resolution that ran the // vote block. | QuantityRef::VoteCount { .. } @@ -9349,6 +9363,23 @@ fn static_gate_bridge_loses_zone(condition: &StaticCondition) -> bool { /// [`gate_binding_diverges_at_fire_time`] for the object/player/population axes /// of the leaf, and [`static_gate_bridge_loses_zone`] for the zone axis the /// `IsPresent` intermediate drops. +/// +/// CR 400.7: there is deliberately NO guard here for a delayed ability carrying +/// no `trigger_source` context, and the absence is load-bearing rather than an +/// oversight. The worry it would answer is real in shape — a source-relative gate +/// with no context would read nothing, evaluate false for want of a reading +/// rather than on the game state, and (via [`false_gate_consumes_one_shot`]) +/// DELETE a one-shot outright. It cannot happen, because matching runs BEFORE +/// gating and already requires that context: `delayed_trigger_event_with_index` +/// opens its `WhenNextEvent` arm with `let source_context = source_context?;`, so +/// a contextless one-shot never matches an event, never reaches this gate, and is +/// never discarded. The condition is checked at most where a reading exists. +/// +/// A guard here would therefore be unreachable code that also declines the hoist +/// for gates whose fire-time reading is perfectly well-defined without a source +/// (the controller-scoped `QuantityCheck` populations pinned by +/// `non_battlefield_presence_gate_declines_the_fire_time_hoist` and its +/// siblings) — buying nothing and costing CR 603.4's fire-time half. fn delayed_intervening_if(ability: &ResolvedAbility) -> Option { if delayed_body_outlives_a_false_gate(ability) { return None; @@ -9381,28 +9412,37 @@ fn delayed_intervening_if(ability: &ResolvedAbility) -> Option /// `TargetFilter::names_bound_single_object`). The bound object departs once /// per incarnation, and an object that returns is a NEW object (CR 400.7 / /// CR 603.7c), so a retained ability could never match it anyway; -/// * EVERY `WhenNextEvent`, whatever its `DelayedTriggerLifetime`. Each shape the -/// parser builds for this variant names ONE occurrence, so the matched event -/// has spent it: -/// - `ThisTurn` is only ever built from "when you **next** [event] this turn" -/// (`try_parse_when_next_generic_event` / `build_when_next_delayed_trigger`). -/// "Next" is the ability's own single-occurrence wording; the stated duration -/// bounds only how long it WAITS for that one occurrence, and CR 603.7b's -/// "unless it has a stated duration" clause lifts the once-only cap for -/// "whenever … this turn" (`WheneverEvent`), not for a "next"; -/// - `Persistent` has NO stated duration (The Pandorica's "when ~ becomes -/// untapped or leaves the battlefield", "when a player planeswalks"), so -/// CR 603.7b's unqualified "will trigger only once — the next time its -/// trigger event occurs" applies directly; -/// - `Reflexive` (CR 603.12) is checked ONLY against its creation batch and must -/// not be left to fire on a later same-turn event. +/// * EVERY `WhenNextEvent`, whatever its `DelayedTriggerLifetime` — because this +/// variant IS CR 603.7b's once-only half. The rule's two outcomes are modelled +/// as two SIBLING conditions, not as a lifetime: `WheneverEvent` is the +/// "unless it has a stated duration" carve-out (multi-fire, purged by +/// `WheneverEventExpiry`), and `WhenNextEvent` is documented on its own +/// declaration as the "one-shot variant of `WheneverEvent`". A stated-duration +/// ability that must keep watching is therefore a `WheneverEvent` and CANNOT +/// reach this function: `effects::delayed_trigger` computes +/// `one_shot = !matches!(condition, WheneverEvent { .. })` and the only caller +/// gates on `delayed.one_shot`. The lifetime then bounds only how long the +/// single shot WAITS — `ThisTurn` to cleanup, `Persistent` open-ended +/// (The Pandorica), `Reflexive` to its creation batch (CR 603.12). +/// +/// Do NOT re-derive that from the wording "next": the discriminator is WotC's +/// "When" / "Whenever" templating, and the parser already keys on exactly that. +/// The distinction is load-bearing and easy to get backwards, so it is worth +/// naming the case that proves it. `parse_dealt_damage_this_way_dies_trigger` +/// (`oracle_effect/mod.rs`) parses "[subject] dealt damage this way dies +/// [this turn]" — a BROAD filter with no "next" anywhere — and is reached from +/// two call sites that lower it differently, on the templating alone: +/// - the `"whenever "` site → `WheneverEvent` (multi-fire). Ghired's +/// Belligerence and Reckless Blaze, both of which spread damage over many +/// creatures, so many deaths can qualify; +/// - the `"when "` site → `WhenNextEvent { ThisTurn }` (one-shot). Skeletonize, +/// whose damage goes to a SINGLE target creature, so at most one death can +/// ever qualify — one occurrence, correctly consumed. /// -/// A BROAD-FILTER event form ("when a creature dies this turn, if X, …") is -/// exactly what CR 603.7b's "unless it has a stated duration" clause keeps -/// watching: the first non-qualifying death must NOT destroy the ability for the -/// rest of the turn. Those stay installed and have their gate re-checked on the -/// next occurrence; their stated duration is enforced by the cleanup purge -/// (`DelayedTriggerLifetime` / `WheneverEventExpiry`), never by this discard. +/// Those are the only three cards in the pool with that wording, and the split is +/// exact for all three. So "broad filter" alone never implies multi-fire here, +/// and this arm does not need to inspect the filter: the routing decision was +/// already made, correctly, one layer up. /// /// Exhaustive on purpose: a new `DelayedTriggerCondition` must decide whether its /// stated event is a single occurrence before it can be discarded on a false gate. @@ -9419,12 +9459,14 @@ fn false_gate_consumes_one_shot(condition: &DelayedTriggerCondition) -> bool { | DelayedTriggerCondition::WhenDiesOrExiled { filter } => { filter.names_bound_single_object() } - // CR 603.4 + CR 603.7b (+ CR 603.12 for `Reflexive`): every lifetime this - // variant carries names ONE occurrence, and the matched event has now - // spent it — see the per-lifetime breakdown above. Retaining it would - // silently rewrite "when you next X, if C" into "when you next X for - // which C holds", letting a later X fire an ability CR 603.4 already - // resolved as doing nothing. + // CR 603.4 + CR 603.7b (+ CR 603.12 for `Reflexive`): this variant IS the + // one-shot half of the CR 603.7b split — its own doc calls it the + // "one-shot variant of `WheneverEvent`" — so the matched event has spent + // its single occurrence whatever lifetime it carries. Retaining it would + // silently rewrite "when X, if C" into "when X for which C holds", letting + // a later X fire an ability CR 603.4 already resolved as doing nothing. + // A trigger that must keep watching is a `WheneverEvent` and cannot reach + // here at all — see the doc above. DelayedTriggerCondition::WhenNextEvent { .. } => true, // Never one-shot (`effects::delayed_trigger` computes `one_shot` as // "not `WheneverEvent`"), so this arm is unreachable from the caller; @@ -19174,6 +19216,158 @@ pub mod tests { ); } + /// The OTHER half of CR 603.7b, and the discriminating counterpart to the + /// test above: a STATED-DURATION delayed ability whose first matching event + /// fails the intervening-`if` must SURVIVE and still fire on a later matching + /// event in the same turn. + /// + /// CR 603.7b caps a delayed ability at one trigger "unless it has a stated + /// duration, such as 'this turn.'" The engine models that carve-out as + /// `DelayedTriggerCondition::WheneverEvent` (multi-fire), the sibling of the + /// one-shot `WhenNextEvent` — so the protection is structural, and this test + /// pins the structure rather than trusting it: `effects::delayed_trigger` + /// computes `one_shot = !matches!(condition, WheneverEvent { .. })`, and the + /// discard in `check_delayed_triggers` is gated on `delayed.one_shot`, so a + /// false gate here must decline the fire WITHOUT consuming the ability. + /// + /// Two matching events, gate false then true, is what makes this + /// discriminating: a one-event fixture would pass even if the ability were + /// wrongly discarded, because both readings put nothing on the stack the + /// first time. + /// + /// REVERT-TO-RED: drop the `delayed.one_shot &&` conjunct from the discard + /// condition in `check_delayed_triggers` (so every false gate consumes) and + /// `after_first` becomes `0` and the second death fires nothing. + #[test] + fn stated_duration_multi_fire_survives_a_false_intervening_if() { + use crate::types::triggers::TriggerMode; + + /// Installs "whenever you play a land this turn, if you control your + /// commander, you become the monarch" — the stated-duration shape. + /// Deliberately the same fixture as the one-shot test above, differing + /// ONLY in the condition sibling, so the two tests isolate exactly the + /// `WhenNextEvent` / `WheneverEvent` distinction and nothing else. + fn install(state: &mut GameState) -> ObjectId { + let controller = PlayerId(0); + state.active_player = controller; + state.priority_player = controller; + + let source = create_object( + state, + CardId(0x0603_0409), + controller, + "Stated Duration Rider".to_string(), + Zone::Battlefield, + ); + let land = create_object( + state, + CardId(0x0603_040a), + controller, + "Played Land".to_string(), + Zone::Battlefield, + ); + + // CR 305.1 + CR 603.2: scope the delayed event to the controller's + // own land drop, exactly as the one-shot fixture above does. + let mut trigger_def = TriggerDefinition::new(TriggerMode::LandPlayed); + trigger_def.valid_target = Some(TargetFilter::Controller); + + let mut ability = + ResolvedAbility::new(Effect::BecomeMonarch, vec![], source, controller); + ability.condition = Some(AbilityCondition::ControlsCommander { + ownership: CommanderOwnership::Own, + }); + let source_object = state.objects.get(&source).expect("installed source"); + ability.trigger_source = Some(trigger_source_context_for_latch(state, source_object)); + + state.delayed_triggers.push(DelayedTrigger { + condition: DelayedTriggerCondition::WheneverEvent { + trigger: Box::new(trigger_def), + expiry: crate::types::ability::WheneverEventExpiry::EndOfTurn, + }, + ability: Box::new(ability), + controller, + source_id: source, + // CR 603.7b: the stated duration is what makes this multi-fire. + // Mirrors `effects::delayed_trigger`'s own computation + // (`one_shot = !matches!(condition, WheneverEvent { .. })`). + one_shot: false, + provenance: DelayedInstallIdentity::LegacyDelayed, + }); + + land + } + + fn stage_commander(state: &mut GameState) { + // CR 903.3 + CR 903.3d: owned AND controlled, on the battlefield. + let commander = make_creature(state, PlayerId(0), "Your Commander", 2, 2); + state + .objects + .get_mut(&commander) + .expect("staged commander") + .is_commander = true; + } + + fn land_played(land: ObjectId) -> GameEvent { + GameEvent::LandPlayed { + object_id: land, + player_id: PlayerId(0), + from_zone: Zone::Hand, + } + } + + let mut state = setup(); + let land = install(&mut state); + + // Batch one: a matching event with the gate FALSE (no commander staged). + check_delayed_triggers(&mut state, &[land_played(land)]); + assert_eq!( + state.stack.len(), + 0, + "CR 603.4: a false intervening-`if` means the ability does not trigger on this \ + occurrence" + ); + let after_first = state.delayed_triggers.len(); + assert_eq!( + after_first, 1, + "CR 603.7b: a STATED-DURATION delayed ability is not capped at one trigger, so a \ + false gate must decline the occurrence WITHOUT consuming the ability" + ); + + // Batch two: make the gate TRUE, then a second matching event. The + // ability must still be installed and must now fire. + stage_commander(&mut state); + check_delayed_triggers(&mut state, &[land_played(land)]); + assert_eq!( + state.stack.len(), + 1, + "CR 603.7b + CR 603.4: the ability survived the declined occurrence and its gate is \ + re-checked on the next one, which now passes" + ); + + // CR 603.4 (second half) + CR 608.2a: passing the FIRE-TIME gate must not + // disarm the RESOLUTION-TIME one. The hoist is a pair, not a move: the + // same gate has to ride onto the stack entry so `stack.rs`'s + // `bind_resolution_scope` re-checks it, which is what denies the ability + // if the condition stops holding between triggering and resolution (a + // commander leaving the battlefield in response). An entry carrying + // `condition: None` would resolve unconditionally — the very bug this PR + // fixes, reintroduced one layer later. + let entry = state.stack.last().expect("the fired delayed ability"); + assert!( + matches!( + &entry.kind, + StackEntryKind::TriggeredAbility { + condition: Some(_), + .. + } + ), + "CR 603.4: the hoisted intervening-`if` must also be carried onto the stack entry \ + for the resolution-time recheck, got {:?}", + entry.kind + ); + } + /// HOSTILE fixture for the CR 603.4 hoist's carve-out. /// /// "…, if X, A. Otherwise, B." is NOT an intervening-`if` in the CR 603.4 diff --git a/crates/engine/src/parser/oracle_effect/conditions.rs b/crates/engine/src/parser/oracle_effect/conditions.rs index 8d62e92c38..072ac7b7eb 100644 --- a/crates/engine/src/parser/oracle_effect/conditions.rs +++ b/crates/engine/src/parser/oracle_effect/conditions.rs @@ -7317,7 +7317,7 @@ mod tests { }; use crate::types::counter::{CounterMatch, CounterType}; - /// CR 903.3d + CR 608.2c: the `StaticCondition` -> `AbilityCondition` bridge + /// CR 903.3d + CR 603.4: the `StaticCondition` -> `AbilityCondition` bridge /// must lower a commander-control gate, and must keep the two `ownership` /// arms DISTINCT — CR 903.3 + CR 109.5 "your commander" (owned and /// controlled) is a strictly narrower predicate than CR 903.3d "a commander" diff --git a/crates/engine/src/parser/oracle_effect/tests.rs b/crates/engine/src/parser/oracle_effect/tests.rs index ca7c76136f..e33ab0e900 100644 --- a/crates/engine/src/parser/oracle_effect/tests.rs +++ b/crates/engine/src/parser/oracle_effect/tests.rs @@ -22131,6 +22131,56 @@ fn inline_delayed_trigger_when_damage_this_way_dies_uses_damage_condition() { } } +/// CR 603.7b: the SIBLING of the test above, and the reason that one is allowed +/// to lower onto the one-shot `WhenNextEvent`. +/// +/// CR 603.7b caps a delayed ability at one trigger "unless it has a stated +/// duration, such as 'this turn.'" Both cards here carry the same stated +/// duration and the same broad "a creature dealt damage this way" filter, so the +/// filter cannot be the discriminator — the WotC "When" / "Whenever" templating +/// is, and `parse_dealt_damage_this_way_dies_trigger` is reached from two call +/// sites that lower it accordingly: +/// - "When …" (Skeletonize, damage to a SINGLE target creature, so at most one +/// death can qualify) → one-shot `WhenNextEvent`, pinned above; +/// - "Whenever …" (Ghired's Belligerence, damage DIVIDED among any number of +/// creatures, so many deaths can qualify) → multi-fire `WheneverEvent`, +/// pinned here. +/// +/// This split is what makes `game::triggers::false_gate_consumes_one_shot` safe +/// to consume EVERY `WhenNextEvent` on a false intervening-`if` (CR 603.4): an +/// ability that must keep watching for the rest of the turn is a `WheneverEvent`, +/// whose `one_shot` is false, so it never reaches that discard. Re-route this +/// form onto `WhenNextEvent` and that guarantee is silently lost — which is why +/// the routing is pinned here rather than left to the call sites. +#[test] +fn inline_delayed_trigger_whenever_damage_this_way_dies_is_multi_fire() { + let e = parse_effect( + "Whenever a creature dealt damage this way dies this turn, create a 1/1 black Skeleton creature token", + ); + match e { + Effect::CreateDelayedTrigger { + condition: DelayedTriggerCondition::WheneverEvent { trigger, .. }, + .. + } => { + // Same event shape as the "When" form — only the multiplicity differs. + assert_eq!(trigger.mode, TriggerMode::ChangesZone); + assert_eq!(trigger.origin, Some(Zone::Battlefield)); + assert_eq!(trigger.destination, Some(Zone::Graveyard)); + assert_eq!( + trigger.condition, + Some(TriggerCondition::DealtDamageBySourceThisTurn), + "the 'whenever' form must reuse the same damage-ledger condition as the \ + 'when' form; only its multiplicity differs" + ); + } + other => panic!( + "CR 603.7b: a stated-duration 'whenever … dealt damage this way dies this turn' \ + must lower to the multi-fire WheneverEvent, not the one-shot WhenNextEvent, \ + got {other:?}" + ), + } +} + #[test] fn inline_delayed_trigger_when_leaves() { let e = parse_effect("When that creature leaves the battlefield, return it to the battlefield under its owner's control"); @@ -52298,7 +52348,7 @@ fn instead_override_lowers_the_typed_completed_dungeon_condition() { ); } -/// CR 603.4 + CR 608.2c + CR 903.3: Fight for the Throne's delayed triggered +/// CR 603.4 + CR 903.3: Fight for the Throne's delayed triggered /// ability carries an intervening-`if` — "When the creature an opponent /// controls dies this turn, *if you control your commander*, you become the /// monarch." The gate must survive lowering as a TYPED `AbilityCondition`. diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index ce8aa32198..77b7535603 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -15346,7 +15346,7 @@ impl TargetFilter { } } - /// CR 608.2c + CR 400.7: Returns true when this filter names ONE object that + /// CR 603.7c + CR 400.7: Returns true when this filter names ONE object that /// is already BOUND — a specific permanent or an anaphor to one — rather than /// a CLASS of objects re-matched against the live board on every check. /// @@ -20424,10 +20424,13 @@ pub enum AbilityCondition { HasCityBlessing, /// CR 702.195b: True when the ability controller has the enduring story designation. HasEnduringStory, - /// CR 903.3d + CR 608.2c: Resolution-time commander-control gate — "if you + /// CR 903.3d + CR 608.2a: Resolution-time commander-control gate — "if you /// control your commander" / "if you control a commander". CR 903.3d is the /// authorizing rule: "If an effect refers to controlling a commander, it /// refers to a permanent on the battlefield that is a commander." + /// CR 608.2a is the resolution-time half: an intervening-`if` condition is + /// re-checked as the ability resolves and the ability does nothing if it is + /// then false. /// /// The effect-resolution mirror of `StaticCondition::ControlsCommander` /// (layers), `TriggerCondition::ControlsCommander` (intervening-if on a From b0690c2a257a450773ccf4ece02aa6e90d89acb8 Mon Sep 17 00:00:00 2001 From: Jacob Woodson <38709105+JacobWoodson@users.noreply.github.com> Date: Fri, 14 Aug 2026 12:38:53 -0500 Subject: [PATCH 4/5] test(PR-7389): correct the REVERT-TO-RED recipe to the measured one The claim shipped in the previous commit was wrong, and wrong in exactly the way this PR criticises elsewhere: asserted from reading rather than measured. Measured, one revert at a time: - dropping only the caller's `delayed.one_shot &&` conjunct -> STILL PASSES (`false_gate_consumes_one_shot` answers `false` for `WheneverEvent` anyway); - flipping only that arm to `true` -> STILL PASSES (the caller never consults it for a multi-fire trigger); - BOTH together -> RED, `after_first` 0 vs 1. `WheneverEvent` is protected twice over, so the fixture pins the CONJUNCTION rather than either layer. That is a real property worth recording, not a weakness: the test stays red for any change that removes the protection, and stays green for one that merely moves it between the two layers. Doc-only. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/triggers.rs | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/crates/engine/src/game/triggers.rs b/crates/engine/src/game/triggers.rs index 991c854cdf..f0fba0ff60 100644 --- a/crates/engine/src/game/triggers.rs +++ b/crates/engine/src/game/triggers.rs @@ -19235,9 +19235,19 @@ pub mod tests { /// wrongly discarded, because both readings put nothing on the stack the /// first time. /// - /// REVERT-TO-RED: drop the `delayed.one_shot &&` conjunct from the discard - /// condition in `check_delayed_triggers` (so every false gate consumes) and - /// `after_first` becomes `0` and the second death fires nothing. + /// REVERT-TO-RED, and the exact recipe matters here: `WheneverEvent` is + /// protected TWICE over, so neither half alone reds this test. Flipping only + /// `false_gate_consumes_one_shot`'s `WheneverEvent` arm to `true` changes + /// nothing, because the caller never consults it for a multi-fire trigger; + /// dropping only the caller's `delayed.one_shot &&` conjunct changes nothing, + /// because the arm still answers `false`. Do BOTH — flip the arm to `true` + /// AND drop the conjunct — and `after_first` becomes `0` and the second event + /// fires nothing (measured, not asserted from reading). + /// + /// That redundancy is the finding, not a weakness of the fixture: this test + /// pins the CONJUNCTION that is the actual guarantee, so it stays red for any + /// change that removes the protection outright rather than merely moving it + /// between the two layers. #[test] fn stated_duration_multi_fire_survives_a_false_intervening_if() { use crate::types::triggers::TriggerMode; From 82b19260750306e7d5d2d9360e20c32617902178 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Fri, 14 Aug 2026 18:10:16 -0700 Subject: [PATCH 5/5] fix(PR-7389): correct intervening-if citations CR 603.4 and CR 608.2a govern the creation-time and resolution-time checks for intervening-if clauses; CR 608.2c governs instruction order. --- .../fight_for_the_throne_monarch_gated_on_commander.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs b/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs index 4de8850543..2daef745ca 100644 --- a/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs +++ b/crates/engine/tests/integration/fight_for_the_throne_monarch_gated_on_commander.rs @@ -71,7 +71,7 @@ fn resolve_fight_for_the_throne(stage_commander: impl FnOnce(&mut GameScenario)) let theirs = scenario.add_creature(P1, "Doomed Squire", 1, 1).id(); // The commander is ALWAYS a distinct object from both fighters. The gate is - // evaluated live as the delayed ability resolves (CR 608.2c), so a commander + // evaluated live as the delayed ability resolves (CR 603.4 + CR 608.2a), so a commander // that were itself a fight participant could change zones mid-resolution and // make the expected value depend on the fight outcome rather than on the gate. stage_commander(&mut scenario); @@ -126,7 +126,7 @@ fn own_commander_on_battlefield_grants_the_monarch() { ); } -/// NEGATIVE + reach-guard — CR 603.4 + CR 608.2c: with no commander anywhere, +/// NEGATIVE + reach-guard — CR 603.4 + CR 608.2a: with no commander anywhere, /// the intervening-`if` fails as the delayed ability resolves and nobody becomes /// the monarch. /// @@ -145,7 +145,7 @@ fn no_commander_means_no_monarch() { ); assert_eq!( result.monarch, None, - "CR 608.2c: the intervening-if \"if you control your commander\" fails, so \ + "CR 603.4 + CR 608.2a: the intervening-if \"if you control your commander\" fails, so \ the delayed BecomeMonarch must do nothing. Some(P0) here is the shipped \ misparse — a dropped gate granting the monarch unconditionally" );