From df37c8f1738d31fca25b08598b48ecb11dc989d6 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 16:07:12 -0700 Subject: [PATCH 1/2] feat(engine): apply the CR 605.1a library-movement criterion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CR 605.1a was amended to add a fourth criterion to the mana-ability test: "An activated ability is a mana ability if it meets all of the following criteria: it doesn't require a target (see rule 115.6), it could add mana to a player's mana pool when it resolves, it's not a loyalty ability (see rule 606, "Loyalty Abilities"), and its cost and effect don't move any card to or from a library. Do not take into account replacement effects that may apply, other than self-replacement effects, when evaluating these criteria." Two deltas versus the prior printing: the library-movement criterion, and the trailing replacement-effect clause. What this adds: - `Effect::moves_card_to_or_from_library()` and `AbilityCost::moves_card_to_or_from_library()` — exhaustive, wildcard-free per-node predicates over all 229 `Effect` and 33 `AbilityCost` variants. Every reasoned verdict carries its CR annotation inline. - `ResolutionScope { OwnResolutionOnly, IncludeRegisteredLater }` in `types/ability_visit.rs`, threaded through the walkers. `OwnResolutionOnly` stops at any effect that merely REGISTERS a separate ability, replacement, or continuous effect to apply later — delayed triggers (CR 603.7a), reflexive triggers (CR 603.12), registered replacements (CR 614.1), and `Effect::Mana`'s `grants` (CR 603.3). Existing entry points keep `IncludeRegisteredLater`, so no current caller changes behavior. - `is_mana_ability` is now the single authority for all four criteria. The three-criteria core is extracted as `produces_mana_on_activation` so that `is_renewable_mana_ability` — which asks a different question, "is this permanent part of a standing manabase?" — does not narrow. Composing the development predicate on the rules predicate would drop Millikin, Deranged Assistant, and Codie out of `phase-ai`'s `is_intrinsic_mana_source` -> `card_value::mana_role` -> mulligan `keep_tier`, for a reason unrelated to manabase development. The replacement-effect clause is satisfied by construction: `is_mana_ability` is a pure function of the printed `AbilityDefinition` AST, takes no `&GameState`, and therefore cannot consult replacement effects at all. Self-replacement riders that ARE printed on the ability (CR 614.15) — e.g. `Counter { countered_spell_zone: Some(Library) }`, Memory Lapse's "put it on top of its owner's library instead" — are counted, which is what the closing sentence's carve-out requires. One committed insta snapshot is re-accepted: `manamorphose_lowered.snap` loses its `is_mana_ability: true`. Sixteen committed snapshots pin that field; the suite exercised all sixteen and exactly this one moved, which is an independent confirmation of the census's predicted blast radius (see F-6 for why the new value is the rules-correct one). The plan predicted the classification flip but did not predict that a snapshot pinned it, so this path is a scope addition. Known divergences, none fixed here and none pinned as correct by any test: - F-1 Codie, Vociferous Codex is reclassified and under CR 605.1a should not be. This is an accepted, documented divergence traceable to a parser mis-attachment: `CastFromZone` / `PutAtLibraryPosition` are parsed as chain siblings of the `CreateDelayedTrigger` rather than as its payload. A follow-up PR fixes that attachment, after which `OwnResolutionOnly` prunes them and Codie classifies correctly with no further change here. Codie is already rules-incorrect today for the same root cause: its free-cast continuation resolves immediately instead of when the delayed trigger fires. - F-2 `feasible_mana_capacity` / `available_mana` / `strategy_helpers` under-report reachable mana for the affected permanents, because they model mana reachable inside the payment window and these sources now require floating at priority first. Payment-planner change. - F-3 `WhenYouDo` reflexive sub-chains run inline on the off-stack mana path; under CR 603.12 + CR 603.3 they belong on the stack. Pre-existing and unchanged in severity. - F-4 `triggers.rs`'s `AbilityActivated` comment describes a construction-based invariant that is now classification-based. Behavior pinned by test; wording left alone (out of scope). - F-5 The three `ActivationExemption::ManaAbilities` gates are copy-shaped and not deduplicated. - F-6 Pre-existing CR 605.5b violation: `is_mana_ability` never checks `AbilityKind`, so `kind: "Spell"` abilities serialize `is_mana_ability: true`. CR 605.5b: "A spell can never be a mana ability." This change incidentally flips exactly one of them (Manamorphose) for the right outcome by the wrong criterion — its "Draw a card" moves a card from a library, which is the CR 605.1a criterion, not the CR 605.5b one that should have caught it. 31 remain misflagged, counted over top-level `.abilities[]` nodes; a recursive walk including nested sub-abilities counts more (210 nodes / 197 cards), so any follow-up must state its population before quoting a number. Adding an `AbilityKind` guard needs its own census and is deliberately not done here. - F-7 Informational: one of the reclassified cards, Manakin and Millikin, is `set_type: funny` / `Unknown Event` and `not_legal` in every format. It is in `card-data.json` so it counts in the census, but no tournament- relevant behavior rests on it. - F-8 Two shipped `CR 701.58e` citations overstate the rule (it is the multi-cloak ordering rule, not authority for cloak's source zone), at `types/ability.rs` and `game/effects/cloak.rs`. A third at `game/effects/cloak.rs` cites it correctly for the ordering it does state and must NOT be swept up by a cleanup. Verified on Tilt: clippy clean, and test-engine build 7331 ran 23,356 tests with 23,355 passing — the four new integration tests, the re-accepted Manamorphose snapshot, and the other fifteen snapshots that pin `is_mana_ability` all pass. Both verification builds started after the last edit to these paths. One failure remains and is NOT from this change: `battlefield_entry_authority_census::every_token_created_construction_lives_in_the_single_emitter`. That test is a source-scanning census over `TokenCreated` constructions, its file carries another agent's uncommitted in-flight edits, and none of the six paths in this commit contains a `TokenCreated` construction, so this change cannot move it. Left to its owner rather than fixed here. --- crates/engine/src/game/mana_abilities.rs | 1389 ++++++++++++++++- ..._snapshot_tests__manamorphose_lowered.snap | 3 +- crates/engine/src/types/ability.rs | 788 ++++++++++ crates/engine/src/types/ability_visit.rs | 461 +++++- .../integration/cr605_1a_library_criterion.rs | 266 ++++ crates/engine/tests/integration/main.rs | 1 + 6 files changed, 2841 insertions(+), 67 deletions(-) create mode 100644 crates/engine/tests/integration/cr605_1a_library_criterion.rs diff --git a/crates/engine/src/game/mana_abilities.rs b/crates/engine/src/game/mana_abilities.rs index ac8538bcf4..d25ef6008f 100644 --- a/crates/engine/src/game/mana_abilities.rs +++ b/crates/engine/src/game/mana_abilities.rs @@ -5,6 +5,9 @@ use crate::types::ability::{ QuantityExpr, QuantityRef, ResolvedAbility, TargetFilter, REMOVE_COUNTER_COST_ALL, REMOVE_COUNTER_COST_ANY_NUMBER, }; +use crate::types::ability_visit::{ + visit_ability_def_costs_scoped, visit_ability_def_scoped, ResolutionScope, +}; use crate::types::counter::{CounterMatch, CounterType}; use crate::types::events::{GameEvent, ManaTapState}; use crate::types::game_state::{ @@ -21,6 +24,7 @@ use crate::types::player::PlayerId; use crate::types::statics::StaticModeKind; use crate::types::zones::Zone; use std::collections::HashSet; +use std::ops::ControlFlow; use super::cost_payability::{eligible_exile_cost_objects, exile_cost_effective_zone}; use super::effects::mana::resolve_restrictions; @@ -33,13 +37,28 @@ use super::mana_sources::{mana_color_to_type, mana_type_to_color}; use super::sacrifice; use super::zone_pipeline::{self, ZoneMoveRequest, ZoneMoveResult}; -/// Check if a typed ability definition represents a mana ability (CR 605). -/// CR 605.3: Mana abilities produce mana and resolve immediately without using the stack. +/// CR 605.1a, criteria (1)-(3) ONLY — no target (CR 115.6), the root effect adds +/// mana, and it's not a loyalty ability (CR 606.2). Deliberately EXCLUDES the +/// fourth criterion ("its cost and effect don't move any card to or from a +/// library"), which is why this is NOT the mana-ability test and must never be +/// used for activation routing — use [`is_mana_ability`] for that. +/// +/// This exists because [`is_renewable_mana_ability`] asks a different question: +/// "is this permanent part of a standing manabase?" A Millikin +/// ("{T}, Mill a card: Add {C}") stops being a rules mana ability under the +/// library clause but does not stop being a manabase permanent. Composing the +/// development predicate on the rules predicate would delete Millikin, Deranged +/// Assistant, and Codie from `phase-ai`'s `is_intrinsic_mana_source` -> +/// `card_value::mana_role` -> mulligan `keep_tier`, for a reason unrelated to +/// manabase development. +/// +/// CR 605.3: Mana abilities produce mana and resolve immediately without using +/// the stack. /// CR 605.1a: A mana ability cannot have targets. `Effect::Mana` carries a /// `ManaTargetRole` naming its recipient and/or count-source player targets; /// any declared role means the ability targets and must use the stack. The /// `multi_target` mechanism is checked alongside it. -pub fn is_mana_ability(ability_def: &AbilityDefinition) -> bool { +fn produces_mana_on_activation(ability_def: &AbilityDefinition) -> bool { // CR 605.1a: A mana ability "doesn't require a target." Read the ROLE's // declared filters: ANY declared role — recipient or count source — means // the ability names a target and therefore uses the stack (Jeska's Will @@ -73,6 +92,101 @@ pub fn is_mana_ability(ability_def: &AbilityDefinition) -> bool { true } +/// CR 605.1a + CR 608.2c: does any effect this ability executes during its OWN +/// resolution move a card to or from a library? Walks the head effect, the +/// cost's embedded effects, and the `sub_ability` / `else_ability` / +/// `mode_abilities` chain, stopping at the CR 603.3 boundary owned by +/// [`ResolutionScope::OwnResolutionOnly`] — so a payload that is merely +/// *registered* to resolve later (a CR 603.7a delayed trigger, a CR 603.12 +/// reflexive trigger, a CR 614.1 replacement, an emblem, a token's granted +/// abilities) is not attributed to this ability. +fn chain_moves_card_to_or_from_library(ability_def: &AbilityDefinition) -> bool { + visit_ability_def_scoped( + ability_def, + ResolutionScope::OwnResolutionOnly, + &mut |effect| { + if effect.moves_card_to_or_from_library() { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + }, + ) + .is_break() +} + +/// CR 605.1a "its cost": the root activation cost (CR 602.1a — "the activation +/// cost is everything before the colon"), PLUS every cost paid during this +/// ability's own resolution, which CR 118.12a -> CR 118.12 classifies as a cost +/// ("the action [do something] is a cost, paid when the spell or ability +/// resolves") and CR 608.2c therefore places under "its effect": +/// `unless_pay.cost` and the `cost` on every `sub_ability` / `else_ability` / +/// `mode_abilities` link. +/// +/// This CANNOT be folded into [`chain_moves_card_to_or_from_library`]: that +/// walk's visitor is `FnMut(&Effect)`, and `AbilityCost::Mill` / `Exile` / +/// `ExileWithAggregate` / `ReturnToHand` carry no nested `Effect` at all, so they +/// are structurally invisible to it. That is a type-level gap, not a missing +/// match arm — see `ability_visit::visit_ability_def_costs_scoped`. +fn cost_moves_card_to_or_from_library(ability_def: &AbilityDefinition) -> bool { + visit_ability_def_costs_scoped( + ability_def, + ResolutionScope::OwnResolutionOnly, + &mut |cost| { + if cost.moves_card_to_or_from_library() { + ControlFlow::Break(()) + } else { + ControlFlow::Continue(()) + } + }, + ) + .is_break() +} + +/// CR 605.1a: the single authority for "is this activated ability a mana +/// ability?" — all four criteria. +/// +/// CR 605.1a (final sentence): "Do not take into account replacement effects +/// that may apply, other than self-replacement effects, when evaluating these +/// criteria." This function is a pure function of the printed +/// `AbilityDefinition` AST — it takes no `&GameState` and therefore CANNOT +/// observe a replacement effect. That purity IS the implementation of the +/// clause, not an accident of the signature: do NOT add a `&GameState` +/// parameter or consult the replacement registry here. Self-replacement effects +/// (CR 614.15), which the rule DOES admit, are printed on the ability itself and +/// so are already in the AST this function reads — see the +/// `Effect::Counter { countered_spell_zone }` arm of +/// `Effect::moves_card_to_or_from_library`, which counts Memory Lapse's +/// "instead" precisely because it is a self-replacement effect. +/// +/// CR 605.2 is the second reason the signature must stay pure: "A mana ability +/// remains a mana ability even if the game state doesn't allow it to produce +/// mana." A classification that could read game state would invite exactly the +/// state-dependent answer CR 605.2 forbids. (This is also why `Effect::Dig` is +/// unconditionally true: its only non-moving configuration is state-dependent.) +pub fn is_mana_ability(ability_def: &AbilityDefinition) -> bool { + produces_mana_on_activation(ability_def) + // CR 605.1a: "...and its cost and effect don't move any card to or from + // a library." Chromatic Sphere ("{1}, {T}, Sacrifice this artifact: Add + // one mana of any color. Draw a card.") is the canonical effect case; + // Millikin ("{T}, Mill a card: Add {C}") is the cost-side case. + // + // The cost axis runs off its OWN walk, not the root cost alone: CR + // 602.1a scopes "its cost" to the activation cost, but CR 118.12a -> + // CR 118.12 makes an `unless [player] pays` action a cost paid AT + // RESOLUTION, which CR 608.2c places under "its effect" — as are the + // costs on chain links. + // + // NOT reclassified, and each for a different CR reason: + // - Chromatic Star — the draw is a separate ChangesZone trigger. + // - Barbed Sextant — CR 603.7a, a delayed triggered ability. + // - Shaun & Rebecca — CR 603.12, a reflexive triggered ability. + // - Gilanra — CR 603.3, a TriggerOnSpend mana-spend grant. + // - The Secret Lair — CR 701.22a, Scry reorders WITHIN a library. + && !cost_moves_card_to_or_from_library(ability_def) + && !chain_moves_card_to_or_from_library(ability_def) +} + /// CR 701.21a: Detects when this ability's cost sacrifices **the source itself**. /// /// Also detects a self-`ReturnToHand` cost: either form removes the source from @@ -112,8 +226,9 @@ fn cost_removes_self_from_battlefield(cost: &Option) -> bool { }) } -/// CR 605.1a + CR 701.21: a *renewable* mana ability — one that produces mana -/// (per [`is_mana_ability`]) without consuming its own source to do it. +/// CR 605.1a criteria (1)-(3) + CR 701.21: a *renewable* mana ability — one that +/// produces mana (per [`produces_mana_on_activation`]) without consuming its own +/// source to do it. /// /// This is the **development** predicate: it answers "is this permanent part of a /// standing manabase," not "can this produce mana right now." A Treasure, Gold, @@ -125,11 +240,27 @@ fn cost_removes_self_from_battlefield(cost: &Option) -> bool { /// genuinely is one mana available right now, which is why the two predicates must /// not be unified. /// +/// **DELIBERATELY COMPOSED ON [`produces_mana_on_activation`], NOT ON +/// [`is_mana_ability`] — do not "simplify" this back.** The two predicates answer +/// different questions, so the CR 605.1a library criterion (criterion 4) must NOT +/// reach this one. A **Millikin** or **Deranged Assistant** (`{T}, Mill a card: +/// Add {C}`) and **Codie, Vociferous Codex** stop being rules mana abilities +/// under the library clause, but they do not stop being manabase permanents: +/// they still turn a tap into mana every turn without consuming themselves. +/// Composing the development predicate on the rules predicate would demote all +/// three to `ManaRole::None` through `phase-ai`'s `is_intrinsic_mana_source` -> +/// `card_value::mana_role` -> `plan::controlled_mana_sources`, deleting them from +/// manabase development and from the `mana_behind` deficit that drives mulligan +/// `keep_tier` — an AI-strength regression for a reason that has nothing to do +/// with manabase development. This composition keeps the value **unchanged for +/// every input** across the CR 605.1a amendment. +/// /// Takes a single ability so callers compose with `.any()`; a permanent counts if /// **at least one** of its mana abilities is renewable (Crystal Vein carries both /// a renewable `{T}: Add {C}` and a self-sac `{T}, Sac: Add {C}{C}`). pub fn is_renewable_mana_ability(ability_def: &AbilityDefinition) -> bool { - is_mana_ability(ability_def) && !cost_removes_self_from_battlefield(&ability_def.cost) + produces_mana_on_activation(ability_def) + && !cost_removes_self_from_battlefield(&ability_def.cost) } /// CR 605.1b: A triggered ability is a mana ability iff all three hold: @@ -3104,9 +3235,14 @@ where } } } - // CR 605.1a + CR 701.17a: Bare `Mill` mana-ability cost. The Millikin - // `{T}, Mill a card: Add {C}` shape routes through the Composite arm; this - // arm covers a hypothetical mill-only mana ability for completeness. + // CR 605.1a (2026 amendment): unreachable by construction — an activated + // ability whose cost moves a card to or from a library is no longer a mana + // ability, so no `Mill` cost reaches this payer. Retained rather than + // deleted because the `match` over `AbilityCost` is exhaustive and this is + // the shared mana-ability cost payer; deleting the arm would require + // inventing an error path for a case the classifier already prevents. If + // `is_mana_ability` is ever relaxed, this arm is already correct. + // CR 701.17a: mill puts cards from the top of a library into a graveyard. Some(AbilityCost::Mill { count }) => mill_for_mana_cost(state, player, *count, events)?, Some(AbilityCost::PayLife { amount }) => { // CR 119.4 + CR 903.4: QuantityExpr resolves against the activator's @@ -3377,9 +3513,15 @@ fn ability_cost_sacrifices_source(cost: &AbilityCost) -> bool { /// graveyard. Routes through the replacement pipeline (mirroring `mill::resolve` /// and the rad-counter handler) so graveyard-redirect replacements (Rest in /// Peace / Leyline of the Void) apply and "a card was put into a graveyard" -/// triggers see the milled cards. Millikin (`{T}, Mill a card: Add {C}`) is the -/// canonical case — mill is a non-mana cost component and the {C} is produced -/// unconditionally. +/// triggers see the milled cards. +/// +/// Millikin (`{T}, Mill a card: Add {C}`) **was** the canonical case and is no +/// longer a mana ability under CR 605.1a's 2026 library criterion, so this +/// function is unreachable from the mana fast path — see the +/// `Some(AbilityCost::Mill { .. })` arm of the mana-ability cost payer above, +/// which records why the arm is retained rather than deleted. The mill mechanics +/// below remain correct for a relaxed classifier or a future non-library mill +/// cost. fn mill_for_mana_cost( state: &mut GameState, player: PlayerId, @@ -4523,6 +4665,1222 @@ mod tests { ))) } + // ─────────────────────────────────────────────────────────────────────── + // CR 605.1a (2026 amendment) — the library-movement criterion. + // + // "An activated ability is a mana ability if ... its cost and effect don't + // move any card to or from a library." + // + // Rows V1-V13 of the plan's verification matrix. Every negative below is + // paired with a positive reach-guard in the SAME test, built from the SAME + // builder with the minimal one-node delta, so a fixture that never reaches + // the seam cannot pass vacuously. + // ─────────────────────────────────────────────────────────────────────── + + /// `{T}: Add {C}` — the minimal mana ability every row below perturbs. + fn colorless_tap_mana_ability() -> AbilityDefinition { + make_mana_ability(ManaProduction::Colorless { + count: QuantityExpr::Fixed { value: 1 }, + }) + } + + /// `{T}: Add {C}` with the root activation cost replaced (CR 602.1a). + fn mana_ability_with_cost(cost: AbilityCost) -> AbilityDefinition { + colorless_tap_mana_ability().cost(cost) + } + + /// A bare chain link carrying `effect` and no cost. + fn link(effect: Effect) -> AbilityDefinition { + AbilityDefinition::new(AbilityKind::Activated, effect) + } + + /// `{T}: Add {C}` with `effect` chained as the `sub_ability` — an + /// instruction this ability follows during its own resolution (CR 608.2c). + fn mana_ability_with_sub_effect(effect: Effect) -> AbilityDefinition { + let mut def = colorless_tap_mana_ability(); + def.sub_ability = Some(Box::new(link(effect))); + def + } + + /// `{T}: Add {C}` with a fully-specified chain link. + fn mana_ability_with_sub(sub: AbilityDefinition) -> AbilityDefinition { + let mut def = colorless_tap_mana_ability(); + def.sub_ability = Some(Box::new(sub)); + def + } + + fn draw_one() -> Effect { + Effect::Draw { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + } + } + + fn surveil_one() -> Effect { + Effect::Surveil { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + } + } + + fn scry_one() -> Effect { + Effect::Scry { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + } + } + + fn exile_cost(zone: Option) -> AbilityCost { + AbilityCost::Exile { + count: 1, + zone, + filter: None, + } + } + + fn pay_life_one() -> AbilityCost { + AbilityCost::PayLife { + amount: QuantityExpr::Fixed { value: 1 }, + } + } + + /// A `CreateDelayedTrigger` wrapping `effect` — CR 603.7a, a separate + /// ability that resolves later (CR 603.3). + fn delayed(effect: Effect) -> Effect { + Effect::CreateDelayedTrigger { + condition: DelayedTriggerCondition::AtNextPhase { + phase: Phase::Upkeep, + }, + effect: Box::new(link(effect)), + uses_tracked_set: false, + } + } + + /// V1 — CR 605.1a + CR 701.17a: a **root** cost-side `Mill` disqualifies. + /// Millikin / Deranged Assistant: `{T}, Mill a card: Add {C}`. + #[test] + fn mill_cost_is_not_a_mana_ability() { + let millikin = mana_ability_with_cost(AbilityCost::Composite { + costs: vec![AbilityCost::Tap, AbilityCost::Mill { count: 1 }], + }); + assert!( + !is_mana_ability(&millikin), + "CR 605.1a: a Mill cost moves a card from a library" + ); + + // Reach-guard, same builder, one-node delta: swap Mill for PayLife. + let paid = mana_ability_with_cost(AbilityCost::Composite { + costs: vec![AbilityCost::Tap, pay_life_one()], + }); + assert!( + is_mana_ability(&paid), + "the identical shape with a non-library cost IS a mana ability" + ); + } + + /// V2 — cost recursion reaches `Composite`, `OneOf`, and `PerCounter.base`. + /// The last two are exactly what `mana_sources::cost_has_component` cannot + /// see, which is why this criterion has its own recursive predicate. + #[test] + fn nested_cost_shapes_reach_the_library_predicate() { + let composite = AbilityCost::Composite { + costs: vec![ + AbilityCost::Mana { + cost: ManaCost::generic(1), + }, + AbilityCost::Tap, + AbilityCost::Mill { count: 1 }, + ], + }; + let one_of = AbilityCost::Composite { + costs: vec![ + AbilityCost::Tap, + AbilityCost::OneOf { + costs: vec![ + AbilityCost::Mill { count: 1 }, + AbilityCost::Sacrifice(SacrificeCost::count(TargetFilter::SelfRef, 1)), + ], + }, + ], + }; + let per_counter = AbilityCost::PerCounter { + counter: CounterType::Generic("charge".to_string()), + target: TargetFilter::SelfRef, + base: Box::new(AbilityCost::Mill { count: 1 }), + }; + for (label, cost) in [ + ("Composite", composite), + ("OneOf nested in Composite", one_of), + ("PerCounter base", per_counter), + ] { + assert!( + !is_mana_ability(&mana_ability_with_cost(cost)), + "{label}: nested Mill must disqualify" + ); + } + + // Reach-guards: the same three shapes with PayLife in place of Mill. + let composite_ok = AbilityCost::Composite { + costs: vec![ + AbilityCost::Mana { + cost: ManaCost::generic(1), + }, + AbilityCost::Tap, + pay_life_one(), + ], + }; + let one_of_ok = AbilityCost::Composite { + costs: vec![ + AbilityCost::Tap, + AbilityCost::OneOf { + costs: vec![ + pay_life_one(), + AbilityCost::Sacrifice(SacrificeCost::count(TargetFilter::SelfRef, 1)), + ], + }, + ], + }; + let per_counter_ok = AbilityCost::PerCounter { + counter: CounterType::Generic("charge".to_string()), + target: TargetFilter::SelfRef, + base: Box::new(pay_life_one()), + }; + for (label, cost) in [ + ("Composite", composite_ok), + ("OneOf nested in Composite", one_of_ok), + ("PerCounter base", per_counter_ok), + ] { + assert!( + is_mana_ability(&mana_ability_with_cost(cost)), + "{label}: the non-library twin must stay a mana ability" + ); + } + } + + /// V2b — a cost on a **nested chain link** disqualifies. A root-only + /// application of the cost criterion passes all three of these wrongly, + /// because a `Mill` cost is not an `Effect` and no effect-shaped visitor can + /// ever see it (CR 605.1a "its cost and effect" + CR 608.2c). + #[test] + fn mill_cost_on_a_chain_link_is_not_a_mana_ability() { + let mill_link = || link(Effect::NoOp).cost(AbilityCost::Mill { count: 1 }); + let paid_link = || link(Effect::NoOp).cost(pay_life_one()); + + let mut sub = colorless_tap_mana_ability(); + sub.sub_ability = Some(Box::new(mill_link())); + assert!(!is_mana_ability(&sub), "sub_ability link cost"); + + let mut els = colorless_tap_mana_ability(); + els.else_ability = Some(Box::new(mill_link())); + assert!(!is_mana_ability(&els), "else_ability link cost"); + + let mut modal = colorless_tap_mana_ability(); + modal.mode_abilities = vec![mill_link()]; + assert!(!is_mana_ability(&modal), "mode_abilities link cost"); + + // Reach-guards: the same three links with a non-library cost. These + // prove the walker reaches nested links at all, so the negatives above + // are prunes of a real read rather than a miss. + let mut sub_ok = colorless_tap_mana_ability(); + sub_ok.sub_ability = Some(Box::new(paid_link())); + assert!(is_mana_ability(&sub_ok), "sub_ability link reached"); + + let mut else_ok = colorless_tap_mana_ability(); + else_ok.else_ability = Some(Box::new(paid_link())); + assert!(is_mana_ability(&else_ok), "else_ability link reached"); + + let mut modal_ok = colorless_tap_mana_ability(); + modal_ok.mode_abilities = vec![paid_link()]; + assert!(is_mana_ability(&modal_ok), "mode_abilities link reached"); + } + + /// V2c — an `unless_pay` cost disqualifies. CR 118.12a routes the "unless + /// [a player does something]" form into CR 118.12, which supplies "the + /// action [do something] is a cost, **paid when the spell or ability + /// resolves**" — so this arrives under CR 605.1a's *effect* limb via + /// CR 608.2c, not under the CR 602.1a *activation cost* limb. Bare CR 118.12 + /// is the wrong citation for an "unless" form. + #[test] + fn unless_pay_mill_cost_is_not_a_mana_ability() { + let mill_unless = crate::types::ability::UnlessPayModifier { + cost: AbilityCost::Mill { count: 1 }, + payer: TargetFilter::Opponent, + }; + let paid_unless = crate::types::ability::UnlessPayModifier { + cost: pay_life_one(), + payer: TargetFilter::Opponent, + }; + + assert!( + !is_mana_ability(&colorless_tap_mana_ability().unless_pay(mill_unless.clone())), + "CR 118.12a -> CR 118.12: an unless-pay Mill is a cost paid at resolution" + ); + assert!( + is_mana_ability(&colorless_tap_mana_ability().unless_pay(paid_unless.clone())), + "reach-guard: the unless_pay leg is walked" + ); + + // Nested: an `unless_pay` on a chain link is reached too. + assert!(!is_mana_ability(&mana_ability_with_sub( + link(Effect::NoOp).unless_pay(mill_unless) + ))); + assert!(is_mana_ability(&mana_ability_with_sub( + link(Effect::NoOp).unless_pay(paid_unless) + ))); + } + + /// V2e — the three **conditional** cost arms read their typed zone fields. + /// + /// This is the highest-consequence surface in the criterion, and it is the + /// only one that fails DANGEROUS. Every other conditional fails safe (an + /// ability wrongly keeps mana-ability status; zero cards affected today). + /// These three fail by STRIPPING status: writing + /// `AbilityCost::Exile { .. } => true` — dropping the zone read, a one-token + /// slip — strips mana-ability status from 13 shipping cards: Elvish Spirit + /// Guide, Simian Spirit Guide, Food Chain, Black Tulip, Cadaverous Bloom, + /// Ether, Jack-o'-Lantern, Mirrored Lotus, Molt Tender, Rubble Rouser, + /// Sunken Palace, Thornvault Forager, Titans' Nest. + /// + /// Both mutation directions are covered, and which assertion catches which + /// is not symmetric: + /// - the **library** assertions fail under the `=> false` mutation; + /// - the **non-library** assertions fail under the `=> true` mutation. + #[test] + fn cost_axis_conditional_arms_read_their_typed_zone_fields() { + // Library == disqualifying. Revert-failing for `=> false`. + assert!(!is_mana_ability(&mana_ability_with_cost(exile_cost(Some( + Zone::Library + ))))); + assert!(!is_mana_ability(&mana_ability_with_cost( + AbilityCost::ExileWithAggregate { + filter: TargetFilter::SelfRef, + function: crate::types::ability::AggregateFunction::Sum, + property: crate::types::ability::ObjectProperty::ManaValue, + comparator: Comparator::GE, + value: 1, + zone: Zone::Library, + } + ))); + assert!(!is_mana_ability(&mana_ability_with_cost( + AbilityCost::ReturnToHand { + count: 1, + filter: None, + from_zone: Some(Zone::Library), + } + ))); + + // Non-library == still a mana ability. Revert-failing for `=> true`, + // the strip-status direction, and therefore the PRIMARY guard for the + // dangerous mutation — not optional decoration. + // + // `zone: None` is asserted EXPLICITLY and is the modal corpus value + // (Black Tulip / Ether / Food Chain / Mirrored Lotus). It is `false` + // because the classifier is static and cannot decide a missing zone on + // EITHER payment path: `cost_payability::exile_cost_effective_zone` is + // the authority for non-self costs only, and the `TargetFilter::SelfRef` + // path short-circuits before it and resolves to the source's own current + // zone (game state, which CR 605.2 forbids this classifier from + // reading). + for zone in [ + None, // black tulip / ether / food chain / mirrored lotus + Some(Zone::Hand), // elvish spirit guide / simian spirit guide + Some(Zone::Graveyard), // jack-o'-lantern / molt tender / titans' nest + Some(Zone::Battlefield), // no shipping card, but the inferred default + ] { + assert!( + is_mana_ability(&mana_ability_with_cost(exile_cost(zone))), + "Exile {{ zone: {zone:?} }} must KEEP mana-ability status" + ); + } + assert!(is_mana_ability(&mana_ability_with_cost( + AbilityCost::ExileWithAggregate { + filter: TargetFilter::SelfRef, + function: crate::types::ability::AggregateFunction::Sum, + property: crate::types::ability::ObjectProperty::ManaValue, + comparator: Comparator::GE, + value: 1, + zone: Zone::Graveyard, + } + ))); + // Grinning Ignus: `from_zone: None` means BATTLEFIELD. + assert!(is_mana_ability(&mana_ability_with_cost( + AbilityCost::ReturnToHand { + count: 1, + filter: None, + from_zone: None, + } + ))); + } + + /// V3 — effect-side at the root `sub_ability` link. Chromatic Sphere: + /// `{1}, {T}, Sacrifice this artifact: Add one mana of any color. Draw a + /// card.` + #[test] + fn draw_in_sub_ability_is_not_a_mana_ability() { + assert!(!is_mana_ability(&mana_ability_with_sub_effect(draw_one()))); + // Reach-guard: the identical fixture with Draw replaced by NoOp. + assert!(is_mana_ability(&mana_ability_with_sub_effect(Effect::NoOp))); + } + + /// V4 — effect-side at a **nested** `sub_ability` (depth >= 2). Deleting the + /// recursive chain arm makes this pass wrongly. + #[test] + fn draw_at_nested_sub_ability_depth_is_not_a_mana_ability() { + let mut inner = link(Effect::NoOp); + inner.sub_ability = Some(Box::new(link(draw_one()))); + assert!(!is_mana_ability(&mana_ability_with_sub(inner))); + + let mut inner_ok = link(Effect::NoOp); + inner_ok.sub_ability = Some(Box::new(link(Effect::NoOp))); + assert!(is_mana_ability(&mana_ability_with_sub(inner_ok))); + } + + /// V5 — effect-side at an `else_ability` link. + #[test] + fn draw_in_else_branch_is_not_a_mana_ability() { + let mut def = colorless_tap_mana_ability(); + def.else_ability = Some(Box::new(link(draw_one()))); + assert!(!is_mana_ability(&def)); + + let mut ok = colorless_tap_mana_ability(); + ok.else_ability = Some(Box::new(link(Effect::NoOp))); + assert!(is_mana_ability(&ok)); + } + + /// V5b — effect-side in a `mode_abilities` entry. + #[test] + fn draw_in_a_mode_is_not_a_mana_ability() { + let mut def = colorless_tap_mana_ability(); + def.mode_abilities = vec![link(draw_one())]; + assert!(!is_mana_ability(&def)); + + let mut ok = colorless_tap_mana_ability(); + ok.mode_abilities = vec![link(Effect::NoOp)]; + assert!(is_mana_ability(&ok)); + } + + /// V6 — the criterion does NOT narrow ordinary mana abilities. This guards + /// the over-narrowing direction across every shape the corpus actually + /// carries, including a real `Exile`-cost card. + #[test] + fn library_criterion_does_not_narrow_ordinary_mana_abilities() { + // Plain `{T}: Add {C}`. + assert!(is_mana_ability(&colorless_tap_mana_ability())); + // "Sacrifice a Goblin: Add {R}" — the existing builder. + assert!(is_mana_ability(&skirk_prospector_mana_ability())); + // Loot, the Pathfinder: `Exhaust — {G}, {T}: Add three mana of any one + // color.` The only mana ability in the corpus carrying an `ability_tag`. + let loot = make_mana_ability(ManaProduction::AnyOneColor { + count: QuantityExpr::Fixed { value: 3 }, + color_options: ManaColor::ALL.to_vec(), + contribution: ManaContribution::Base, + }) + .cost(AbilityCost::Composite { + costs: vec![ + AbilityCost::Mana { + cost: ManaCost::generic(1), + }, + AbilityCost::Tap, + ], + }); + assert!(is_mana_ability(&loot)); + + // Elvish Spirit Guide: "Exile this creature from your hand: Add {G}." + // NOTE the wording — "this **creature**", not "this card"; "Exile this + // card from your hand" is SIMIAN Spirit Guide. Same AST shape either + // way: `Exile { zone: Some(Hand), filter: Some(SelfRef) }`. + let spirit_guide = |zone: Option| { + make_mana_ability(ManaProduction::Fixed { + colors: vec![ManaColor::Green], + contribution: ManaContribution::Base, + }) + .cost(AbilityCost::Exile { + count: 1, + zone, + filter: Some(TargetFilter::SelfRef), + }) + }; + assert!( + is_mana_ability(&spirit_guide(Some(Zone::Hand))), + "Elvish Spirit Guide must remain a mana ability" + ); + // Minimal one-field delta, so the pair isolates the zone read itself. + assert!( + !is_mana_ability(&spirit_guide(Some(Zone::Library))), + "the same cost with zone=Library is disqualifying" + ); + + // Paired negative for each positive shape: add a Mill cost. + for def in [ + colorless_tap_mana_ability(), + skirk_prospector_mana_ability(), + loot, + ] { + let base_cost = def.cost.clone().unwrap_or(AbilityCost::Tap); + let milled = def.cost(AbilityCost::Composite { + costs: vec![base_cost, AbilityCost::Mill { count: 1 }], + }); + assert!( + !is_mana_ability(&milled), + "a Mill cost disqualifies every shape" + ); + } + } + + /// V7 — `Scry` does NOT disqualify, but `Surveil` does. The two keyword + /// actions differ on exactly the axis under test, which is why they must + /// never share an arm: CR 701.22a scry puts cards on the bottom or top of + /// **your library** (every card starts and ends in the same library), while + /// CR 701.25a surveil can put them **into your graveyard**. + /// + /// A real shipping card depends on this: The Secret Lair, `{T}, Say the + /// secret word: Add one mana of any color. Scry 1. You gain 1 life.` + #[test] + fn scry_does_not_disqualify_but_surveil_does() { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(scry_one())), + "CR 701.22a: scry reorders WITHIN a library — The Secret Lair" + ); + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(surveil_one())), + "CR 701.25a: surveil can put cards into a graveyard" + ); + } + + /// V8 — library-adjacent effects that move nothing to or from a library. + #[test] + fn library_reorder_reveal_and_other_decks_do_not_disqualify() { + let benign = [ + // CR 701.24a: "randomize the cards WITHIN it". + Effect::Shuffle { + target: TargetFilter::Controller, + }, + // CR 701.20b: "Revealing a card doesn't cause it to leave the zone + // it's in." + Effect::RevealTop { + player: TargetFilter::Controller, + count: 1, + }, + // CR 701.30a: the top card goes to the bottom or stays on top — of + // its own library either way. + Effect::Clash, + // CR 901.4: plane and phenomenon cards remain in the COMMAND ZONE. + Effect::ArrangePlanarDeckTop { + count: QuantityExpr::Fixed { value: 2 }, + keep_on_top: QuantityExpr::Fixed { value: 1 }, + }, + // CR 701.51b + CR 717.2: the Attraction deck is in the command zone. + Effect::OpenAttractions { count: 1 }, + ]; + for effect in benign { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(effect.clone())), + "{effect:?} moves no card to or from a library" + ); + } + // Paired negative in the same test: a Mill link in the same position. + assert!(!is_mana_ability(&mana_ability_with_sub_effect( + Effect::Mill { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + destination: Zone::Graveyard, + } + ))); + } + + /// V9 — the registered-later boundary holds at a chain-link root. Barbed + /// Sextant / Brass Infiniscope put their draw inside a delayed triggered + /// ability (CR 603.7a), which goes on the stack later as its own object + /// (CR 603.3), so it is not an instruction THIS ability follows (CR 608.2c). + #[test] + fn delayed_trigger_payload_is_not_this_abilitys_effect() { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(delayed(draw_one()))), + "CR 603.7a: a delayed trigger's payload is a separate ability" + ); + // Reach-guard: Chromatic Sphere — the SAME Draw, not wrapped. + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(draw_one())), + "the unwrapped Draw in the same position DOES disqualify" + ); + } + + /// V9b — the boundary holds at DEPTH >= 1. This is the central falsifier: a + /// design that prunes only at chain-link roots and then delegates to an + /// unscoped walker reaches the delayed trigger's payload through any inline + /// branch carrier and wrongly disqualifies. + #[test] + fn boundary_holds_under_an_inline_choice_carrier() { + let wrapped = delayed(draw_one()); + + let carriers: Vec<(&str, Effect)> = vec![ + ( + "ChooseOneOf", + Effect::ChooseOneOf { + chooser: PlayerFilter::Controller, + branches: vec![link(wrapped.clone())], + }, + ), + ( + "FlipCoin win branch", + Effect::FlipCoin { + win_effect: Some(Box::new(link(wrapped.clone()))), + lose_effect: None, + flipper: TargetFilter::Controller, + }, + ), + ( + "RollDie result branch", + Effect::RollDie { + count: QuantityExpr::Fixed { value: 1 }, + sides: 20, + results: vec![crate::types::ability::DieResultBranch { + min: 1, + max: 20, + effect: Box::new(link(wrapped.clone())), + }], + modifier: None, + }, + ), + ( + "RevealFromHand on_decline", + Effect::RevealFromHand { + filter: TargetFilter::Controller, + on_decline: Some(Box::new(link(wrapped.clone()))), + }, + ), + ]; + for (label, carrier) in &carriers { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(carrier.clone())), + "{label}: the boundary must hold one level down" + ); + } + + // `AbilityCost::EffectCost` re-enters the effect walk from the cost + // axis, so the scope must be threaded there too. + assert!(is_mana_ability(&mana_ability_with_cost( + AbilityCost::EffectCost { + effect: Box::new(wrapped), + } + ))); + + // Reach-guards: the same carriers with a BARE Draw, no wrapper. These + // prove each carrier is descended at all, so the positives above are + // boundary prunes rather than unreached subtrees. + let bare_carriers: Vec<(&str, Effect)> = vec![ + ( + "ChooseOneOf", + Effect::ChooseOneOf { + chooser: PlayerFilter::Controller, + branches: vec![link(draw_one())], + }, + ), + ( + "FlipCoin win branch", + Effect::FlipCoin { + win_effect: Some(Box::new(link(draw_one()))), + lose_effect: None, + flipper: TargetFilter::Controller, + }, + ), + ( + "RollDie result branch", + Effect::RollDie { + count: QuantityExpr::Fixed { value: 1 }, + sides: 20, + results: vec![crate::types::ability::DieResultBranch { + min: 1, + max: 20, + effect: Box::new(link(draw_one())), + }], + modifier: None, + }, + ), + ( + "RevealFromHand on_decline", + Effect::RevealFromHand { + filter: TargetFilter::Controller, + on_decline: Some(Box::new(link(draw_one()))), + }, + ), + ]; + for (label, carrier) in &bare_carriers { + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(carrier.clone())), + "{label}: reach-guard — the carrier IS descended" + ); + } + assert!(!is_mana_ability(&mana_ability_with_cost( + AbilityCost::EffectCost { + effect: Box::new(draw_one()), + } + ))); + } + + /// V9c — the boundary covers the replacement family, the emblem, and the + /// token's granted abilities. Each REGISTERS something rather than moving a + /// card during this resolution: CR 603.3 primary (a replacement applying to + /// a later event or to another object is NOT a self-replacement effect under + /// CR 614.15, so CR 605.1a's carve-out does not reach it; CR 614.1 + /// secondary), CR 114.1 for the emblem, CR 111.1 for the token, CR 611.2 for + /// a granted continuous effect. + #[test] + fn replacement_emblem_and_token_payloads_are_not_this_abilitys_effect() { + fn granting_static(effect: Effect) -> StaticDefinition { + let mut def = StaticDefinition::new(StaticMode::Continuous); + def.modifications = vec![ContinuousModification::GrantAbility { + definition: Box::new(link(effect)), + }]; + def + } + + let wrapped: Vec<(&str, Effect)> = vec![ + ( + "CreateDrawReplacement", + Effect::CreateDrawReplacement { + replacement_effect: Box::new(draw_one()), + }, + ), + ( + "CreateEmblem", + Effect::CreateEmblem { + statics: vec![granting_static(draw_one())], + triggers: vec![], + }, + ), + ( + "GenericEffect granted ability", + Effect::GenericEffect { + static_abilities: vec![granting_static(draw_one())], + duration: Some(Duration::UntilEndOfTurn), + target: None, + end_cost: None, + }, + ), + ( + "Token granted ability", + Effect::Token { + name: "Test".to_string(), + power: crate::types::ability::PtValue::Fixed(1), + toughness: crate::types::ability::PtValue::Fixed(1), + types: vec!["Creature".to_string()], + colors: vec![], + keywords: vec![], + tapped: false, + count: QuantityExpr::Fixed { value: 1 }, + owner: TargetFilter::Controller, + attach_to: None, + enters_attacking: false, + supertypes: vec![], + static_abilities: vec![granting_static(Effect::Mill { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + destination: Zone::Graveyard, + })], + enter_with_counters: vec![], + }, + ), + ]; + for (label, effect) in &wrapped { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(effect.clone())), + "{label}: the registered payload belongs to a later resolution \ + or to another object" + ); + } + + // Reach-guards: the unwrapped mover in the same chain position. + assert!(!is_mana_ability(&mana_ability_with_sub_effect(draw_one()))); + assert!(!is_mana_ability(&mana_ability_with_sub_effect( + Effect::Mill { + count: QuantityExpr::Fixed { value: 1 }, + target: TargetFilter::Controller, + destination: Zone::Graveyard, + } + ))); + } + + /// V9d — inline carriers are STILL descended (guards over-pruning). These + /// are branches of this resolution (CR 608.2c), not separate abilities. + #[test] + fn inline_carriers_are_still_descended() { + let movers: Vec<(&str, Effect)> = vec![ + ( + "ChooseOneOf", + Effect::ChooseOneOf { + chooser: PlayerFilter::Controller, + branches: vec![link(draw_one()), link(Effect::NoOp)], + }, + ), + ( + "FlipCoin lose branch", + Effect::FlipCoin { + win_effect: None, + lose_effect: Some(Box::new(link(draw_one()))), + flipper: TargetFilter::Controller, + }, + ), + ( + "SeparateIntoPiles chosen pile", + Effect::SeparateIntoPiles { + partition_subject: crate::types::ability::VoterScope::AllPlayers, + object_filter: TargetFilter::Controller, + chooser: PlayerScope::Controller, + chosen_pile_effect: Box::new(link(draw_one())), + pile_source: crate::types::ability::PileSource::Battlefield, + unchosen_pile_effect: None, + }, + ), + ( + "Vote outcome template", + Effect::Vote { + choices: vec!["a".to_string(), "b".to_string()], + per_choice_effect: vec![ + Box::new(link(draw_one())), + Box::new(link(Effect::NoOp)), + ], + starting_with: ControllerRef::You, + voter_scope: crate::types::ability::VoterScope::AllPlayers, + tally_mode: crate::types::ability::VoteTally::PerVote, + subject: crate::types::ability::VoteSubject::Named, + visibility: crate::types::ability::VoteVisibility::Open, + }, + ), + ]; + for (label, effect) in &movers { + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(effect.clone())), + "{label}: an inline branch is part of THIS resolution" + ); + } + + // Reach-guards: the same carriers with NoOp in place of Draw. + let benign: Vec = vec![ + Effect::ChooseOneOf { + chooser: PlayerFilter::Controller, + branches: vec![link(Effect::NoOp), link(Effect::NoOp)], + }, + Effect::FlipCoin { + win_effect: None, + lose_effect: Some(Box::new(link(Effect::NoOp))), + flipper: TargetFilter::Controller, + }, + Effect::SeparateIntoPiles { + partition_subject: crate::types::ability::VoterScope::AllPlayers, + object_filter: TargetFilter::Controller, + chooser: PlayerScope::Controller, + chosen_pile_effect: Box::new(link(Effect::NoOp)), + pile_source: crate::types::ability::PileSource::Battlefield, + unchosen_pile_effect: None, + }, + Effect::Vote { + choices: vec!["a".to_string(), "b".to_string()], + per_choice_effect: vec![Box::new(link(Effect::NoOp)), Box::new(link(Effect::NoOp))], + starting_with: ControllerRef::You, + voter_scope: crate::types::ability::VoterScope::AllPlayers, + tally_mode: crate::types::ability::VoteTally::PerVote, + subject: crate::types::ability::VoteSubject::Named, + visibility: crate::types::ability::VoteVisibility::Open, + }, + ]; + for effect in benign { + assert!(is_mana_ability(&mana_ability_with_sub_effect(effect))); + } + } + + /// V10 — CR 603.12 reflexive links are excluded. Shaun & Rebecca, Agents: + /// `{T}: Add {C}. When you do, mill two cards.` A reflexive triggered + /// ability follows the rules for delayed triggered abilities (CR 603.7) and + /// goes on the stack the next time a player would receive priority + /// (CR 603.3) — the CR 603.12 exception is about WHEN the trigger condition + /// is checked, not about when the ability resolves. + #[test] + fn reflexive_when_you_do_link_is_a_separate_ability() { + let mill_two = || { + link(Effect::Mill { + count: QuantityExpr::Fixed { value: 2 }, + target: TargetFilter::Controller, + destination: Zone::Graveyard, + }) + }; + + let reflexive = mill_two().condition(AbilityCondition::WhenYouDo); + assert!( + is_mana_ability(&mana_ability_with_sub(reflexive)), + "CR 603.12 -> CR 603.7 -> CR 603.3: a 'when you do' link is a \ + SEPARATE triggered ability" + ); + + // Reach-guard: the same chain with no condition at all. + assert!( + !is_mana_ability(&mana_ability_with_sub(mill_two())), + "an unconditioned Mill link is part of this resolution" + ); + + // And the guard must key on `WhenYouDo` ALONE. "If you do, ..." is + // CR 608.2c — one instruction conditional on another within the SAME + // resolution — and must keep being descended. Widening the guard to the + // engine's broader reflexive predicate (which unions the two because it + // answers the skip-on-decline question) fails this assertion. + let if_you_do = mill_two().condition(AbilityCondition::EffectOutcome { + signal: crate::types::ability::EffectOutcomeSignal::OptionalEffectPerformed, + }); + assert!( + !is_mana_ability(&mana_ability_with_sub(if_you_do)), + "an 'if you do' rider is CR 608.2c, not CR 603.12" + ); + } + + /// V10b — the reflexive boundary holds on the COST axis too, because both + /// walkers consult ONE authority (`scope_prunes_nested_ability`). Removing + /// that call from the cost walker fails this row while leaving V10 green. + #[test] + fn reflexive_link_cost_is_also_excluded() { + let reflexive_cost = link(Effect::NoOp) + .cost(AbilityCost::Mill { count: 1 }) + .condition(AbilityCondition::WhenYouDo); + assert!( + is_mana_ability(&mana_ability_with_sub(reflexive_cost)), + "the reflexive link's cost is the SEPARATE ability's cost" + ); + + // Reach-guard: the identical link without the condition (V2b's shape), + // proving the cost walker reaches nested links at all — so the positive + // above is a prune, not a miss. + let plain_cost = link(Effect::NoOp).cost(AbilityCost::Mill { count: 1 }); + assert!(!is_mana_ability(&mana_ability_with_sub(plain_cost))); + } + + /// V11 — `Effect::Mana`'s `grants` are deliberately NOT descended. Gilanra, + /// Caller of Wirewood: `{T}: Add {G}. When you spend this mana to cast a + /// spell with mana value 6 or greater, draw a card.` The rider is a + /// `ManaSpellGrant::TriggerOnSpend` — CR 603.3, a separate triggered ability + /// that fires when the mana is LATER spent, in a different resolution. + /// + /// `Effect::Mana` is the root of 100% of this classifier's inputs, so a + /// "helpful" descent into `grants` here would misclassify Gilanra and + /// Path of Ancestry. A future descent fails this test. + #[test] + fn mana_spend_grant_rider_is_a_separate_ability() { + let gilanra = { + let mut def = make_mana_ability(ManaProduction::Fixed { + colors: vec![ManaColor::Green], + contribution: ManaContribution::Base, + }); + if let Effect::Mana { grants, .. } = &mut *def.effect { + grants.push(crate::types::mana::ManaSpellGrant::TriggerOnSpend { + filter: TargetFilter::Any, + ability: Box::new(link(draw_one())), + }); + } else { + panic!("make_mana_ability must build an Effect::Mana"); + } + def + }; + assert!( + is_mana_ability(&gilanra), + "CR 603.3: a TriggerOnSpend rider is a separate triggered ability" + ); + + // Reach-guard: the SAME Draw moved from `grants` to a plain chain link. + assert!(!is_mana_ability(&mana_ability_with_sub_effect(draw_one()))); + } + + /// V12 — the zone-conditional effect arms read their typed fields, and + /// `Effect::Dig` is UNCONDITIONAL. + /// + /// `DigSource` is **not** a library-vs-not axis: under `PriorLook` the cards + /// are still in `player.library` (the look-only pass takes an iterator slice + /// and returns without removing them), so the library is the origin under + /// BOTH variants. A `source ==` test here — or a test on + /// `destination`/`rest_destination` — reproduces the same error on a + /// different field. + #[test] + fn zone_conditional_arms_read_their_typed_fields() { + fn dig(source: crate::types::ability::DigSource) -> Effect { + Effect::Dig { + player: TargetFilter::Controller, + count: QuantityExpr::Fixed { value: 1 }, + destination: None, + keep_count: Some(1), + keep_count_expr: None, + up_to: false, + filter: TargetFilter::Any, + rest_destination: None, + reveal: false, + enter_tapped: false, + source, + } + } + fn change_zone(origin: Option, destination: Zone, target: TargetFilter) -> Effect { + Effect::ChangeZone { + origin, + destination, + target, + owner_library: false, + enter_transformed: false, + enters_under: None, + enter_tapped: crate::types::zones::EtbTapState::Unspecified, + enters_attacking: false, + up_to: false, + enter_with_counters: vec![], + conditional_enter_with_counters: vec![], + face_down_profile: None, + enters_modified_if: None, + } + } + fn search(source_zones: Vec) -> Effect { + Effect::SearchLibrary { + source_zones, + filter: TargetFilter::Any, + count: QuantityExpr::Fixed { value: 1 }, + reveal: false, + target_player: None, + selection_constraint: crate::types::ability::SearchSelectionConstraint::None, + split: None, + } + } + fn counter( + zone: Option, + ) -> Effect { + Effect::Counter { + target: TargetFilter::Any, + source_rider: None, + countered_spell_zone: zone, + } + } + fn pay_cost(cost: AbilityCost) -> Effect { + Effect::PayCost { + cost, + scale: None, + payer: TargetFilter::Controller, + } + } + + // Library-touching configurations disqualify. + let disqualifying: Vec<(&str, Effect)> = vec![ + ("SearchLibrary[Library]", search(vec![Zone::Library])), + ( + "ChangeZone destination=Library", + change_zone(None, Zone::Library, TargetFilter::SelfRef), + ), + ( + "ChangeZone origin=Library", + change_zone(Some(Zone::Library), Zone::Graveyard, TargetFilter::SelfRef), + ), + ( + "ChangeZone origin=None, zone in the filter", + change_zone( + None, + Zone::Battlefield, + TargetFilter::Typed(TypedFilter::new(TypeFilter::Creature).properties(vec![ + FilterProp::InZone { + zone: Zone::Library, + }, + ])), + ), + ), + ( + "Counter countered_spell_zone=Library", + counter(Some( + crate::types::ability::SpellStackToGraveyardReplacement::Library { + position: crate::types::ability::LibraryPosition::Top, + }, + )), + ), + ("PayCost{Mill}", pay_cost(AbilityCost::Mill { count: 1 })), + ( + "Dig{Library}", + dig(crate::types::ability::DigSource::Library), + ), + ( + "Dig{PriorLook}", + dig(crate::types::ability::DigSource::PriorLook), + ), + ]; + for (label, effect) in &disqualifying { + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(effect.clone())), + "{label} moves a card to or from a library" + ); + } + + // Each CONDITIONAL arm with its non-library value — the reach-guards + // that prove the arms are evaluated rather than hardcoded. + let keeps: Vec<(&str, Effect)> = vec![ + ("SearchLibrary[Graveyard]", search(vec![Zone::Graveyard])), + ( + "ChangeZone graveyard->battlefield", + change_zone(Some(Zone::Graveyard), Zone::Battlefield, TargetFilter::Any), + ), + ("Counter{None}", counter(None)), + ("PayCost{PayLife}", pay_cost(pay_life_one())), + // For `Dig` the reach-guard is `Scry` — a genuine look-at-a-library + // WITHOUT moving anything, which is the axis Dig actually differs + // on. Round 2 used `Dig{PriorLook} => true` as this guard; that + // pinned the wrong answer and is deliberately NOT reinstated. + ("Scry", scry_one()), + ]; + for (label, effect) in &keeps { + assert!( + is_mana_ability(&mana_ability_with_sub_effect(effect.clone())), + "{label} must keep mana-ability status" + ); + } + } + + /// V12b — exactly ONE of `SpellStackToGraveyardReplacement`'s four carriers + /// is read, and the asymmetry is the design. + /// + /// CR 605.1a scopes the criterion to "**its** cost and effect", so the + /// question is not "does this field mention a library" but "whose resolution + /// does the movement happen in". + /// - `Counter.countered_spell_zone` IS read. CR 608.2c cites Memory Lapse's + /// exact text ("Counter target spell. If that spell is countered this + /// way, put it on top of its owner's library instead of into its owner's + /// graveyard") as its OWN worked example of instructions this ability + /// follows; CR 701.6a puts the countered spell in the graveyard during + /// this resolution and the rider redirects that same event. Per CR 614.15 + /// it is a SELF-replacement effect, which CR 605.1a's closing sentence + /// explicitly does NOT exclude. + /// - `FreeCastFromZones.graveyard_replacement` and + /// `CastingPermission::ExileWithAltCost.graveyard_replacement` are NOT + /// read. Each replaces the CAST SPELL'S OWN LATER RESOLUTION at its + /// CR 608.2n graveyard step ("as the final part of an instant or sorcery + /// spell's resolution"). That later resolution belongs to a different + /// object, so the rider is not this ability's own effect, so it is not a + /// self-replacement effect under CR 614.15, so CR 605.1a's closing + /// sentence says do not take it into account. + /// + /// Making the three arms symmetric fails this test, which is exactly its + /// purpose. The configuration has ZERO cards today, so no census, coverage + /// report, or card-level test can see it — this row is what makes the + /// verdict durable against a later round re-deriving it. + #[test] + fn only_counter_reads_the_stack_to_graveyard_replacement() { + use crate::types::ability::{ + CastingPermission, LibraryPosition, SpellStackToGraveyardReplacement, + }; + + let library_rider = || SpellStackToGraveyardReplacement::Library { + position: LibraryPosition::Top, + }; + let exile_with_alt_cost = |graveyard_replacement: Option< + SpellStackToGraveyardReplacement, + >| CastingPermission::ExileWithAltCost { + cost: ManaCost::generic(0), + cast_transformed: false, + constraint: None, + granted_to: None, + resolution_cleanup: None, + duration: None, + graveyard_replacement, + enters_with_counter: None, + enters_with_modifications: vec![], + mana_spend_permission: None, + }; + let grant = |graveyard_replacement: Option| { + Effect::GrantCastingPermission { + permission: exile_with_alt_cost(graveyard_replacement), + target: TargetFilter::Any, + grantee: crate::types::ability::PermissionGrantee::AbilityController, + } + }; + let free_cast = + |zones: Vec, graveyard_replacement: Option| { + Effect::FreeCastFromZones { + count: 1, + max_total_mv: None, + filter: TargetFilter::Any, + zones, + graveyard_replacement, + } + }; + + // (1) `Counter`'s rider IS read: this ability's own resolution moves the + // card from the stack to a library. + assert!(!is_mana_ability(&mana_ability_with_sub_effect( + Effect::Counter { + target: TargetFilter::Any, + source_rider: None, + countered_spell_zone: Some(library_rider()), + } + ))); + // ... and its positive control: the same node with no rider. + assert!(is_mana_ability(&mana_ability_with_sub_effect( + Effect::Counter { + target: TargetFilter::Any, + source_rider: None, + countered_spell_zone: None, + } + ))); + + // (2) `FreeCastFromZones` reads `zones` ONLY. + assert!( + is_mana_ability(&mana_ability_with_sub_effect(free_cast( + vec![Zone::Graveyard], + Some(library_rider()) + ))), + "graveyard_replacement is a rider on the CAST SPELL's later resolution" + ); + // Positive control via the `zones` leg — proves the arm is reached and + // genuinely discriminating rather than hardcoded `false`. + assert!( + !is_mana_ability(&mana_ability_with_sub_effect(free_cast( + vec![Zone::Library], + None + ))), + "the `zones` leg IS read" + ); + + // (3) `GrantCastingPermission` is not descended: the same answer with + // and without the field, proving it is genuinely not consulted rather + // than accidentally agreeing. + assert!(is_mana_ability(&mana_ability_with_sub_effect(grant(Some( + library_rider() + ))))); + assert!(is_mana_ability(&mana_ability_with_sub_effect(grant(None)))); + + // `GrantCastingPermission` is UNCONDITIONALLY false, so no input to it + // can ever produce a `false` — both halves of the pair above assert + // `true` and would also pass on a malformed fixture that never reached + // the walked tree at all. This same-position control closes that hole: + // a library mover at the identical depth MUST disqualify. + assert!( + !is_mana_ability(&mana_ability_with_sub_effect( + Effect::PutAtLibraryPosition { + target: TargetFilter::SelfRef, + count: QuantityExpr::Fixed { value: 1 }, + position: LibraryPosition::Top, + } + )), + "positive control: the chain position the grant occupies IS walked" + ); + } + + /// V13 — `is_renewable_mana_ability` is NOT narrowed by the library + /// criterion. The divergence IS the assertion: a Millikin stops being a + /// rules mana ability (CR 605.1a criterion 4) while remaining a manabase + /// permanent, which is why the development predicate composes on + /// `produces_mana_on_activation` and not on `is_mana_ability`. + #[test] + fn renewable_predicate_survives_the_library_criterion() { + let millikin = mana_ability_with_cost(AbilityCost::Composite { + costs: vec![AbilityCost::Tap, AbilityCost::Mill { count: 1 }], + }); + assert!( + !is_mana_ability(&millikin), + "CR 605.1a criterion 4: Millikin's Mill cost disqualifies it" + ); + assert!( + is_renewable_mana_ability(&millikin), + "but Millikin is still a standing manabase permanent — composing \ + the development predicate on the rules predicate would delete it \ + from manabase development and mulligan keep_tier" + ); + } + /// Row 4a — CR 701.21: one-shot self-sacrificing mana sources are NOT /// renewable. Gold is the constraint discriminator: its cost is a **bare** /// `Sacrifice` (not wrapped in a `Composite` like Treasure's), so a @@ -5732,6 +7090,13 @@ mod tests { /// (`Unsupported mana ability sub-cost: Mill`), so the readiness simulation /// in `can_activate_mana_ability_now` failed and the ability was never /// offered — the user could not tap Millikin for mana. + /// + /// **Premise note (CR 605.1a 2026 amendment):** the *ability-level* mill + /// mechanics asserted below remain correct, but Millikin's ability is no + /// longer reachable *as a mana ability* — its `Mill` cost moves a card from a + /// library, so `is_mana_ability` now returns `false` for it. This test still + /// passes because it drives the cost payer directly and never consults the + /// classifier; see `mill_cost_is_not_a_mana_ability` for the classification. #[test] fn millikin_mills_a_card_and_adds_colorless() { let mut state = GameState::new_two_player(42); diff --git a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_lowered.snap b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_lowered.snap index 98c448985e..fb7cd665c7 100644 --- a/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_lowered.snap +++ b/crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_lowered.snap @@ -53,8 +53,7 @@ expression: "&lowered" "condition": null, "optional_targeting": false, "optional": false, - "forward_result": false, - "is_mana_ability": true + "forward_result": false } ], "triggers": [], diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index 318dcbbf92..aee8c860f1 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -9181,6 +9181,156 @@ impl AbilityCost { } } + /// CR 605.1a (2026 amendment): does paying this cost move a card to or from + /// a **library**? + /// + /// **Recursive over the compositional forms.** `Composite` and `OneOf` are + /// `true` if any sub-cost is; `PerCounter` delegates to its `base`. Calling + /// this on a root node therefore already answers for the whole cost subtree, + /// which is why `ability_visit::visit_ability_def_costs_scoped` yields only + /// top-level cost nodes and does not descend compositions itself. A second + /// wildcard-free `AbilityCost` walk would be a drift hazard for no gain. + /// + /// `EffectCost` delegates ONE node to `Effect::moves_card_to_or_from_library` + /// without descending further: that keeps this method correct for standalone + /// callers, and it overlaps harmlessly (idempotent boolean OR) with the + /// effect walk's own surfacing of the same inner effect. + /// + /// **`mana_sources::cost_has_component` is deliberately NOT used and NOT + /// widened here.** Widening it would also alter the loyalty criterion and + /// `cost_removes_self_from_battlefield`, which are two different shipping + /// predicates; and it cannot see nested composition, which is exactly what + /// this criterion needs. + /// + /// **Which limb of CR 605.1a each application site falls under.** CR 602.1a + /// ("the activation cost is everything before the colon") scopes CR 605.1a's + /// *"its cost"* to the root activation cost. Every other position is reached + /// under *"its effect"*: CR 118.12a routes the "unless [a player does + /// something]" form into CR 118.12, which supplies "the action [do something] + /// is a cost, **paid when the spell or ability resolves**" — so an + /// `unless_pay` cost, and likewise any cost carried by a chain link (which is + /// never separately activated), is an instruction this ability follows during + /// its own resolution under CR 608.2c. Note CR 605.1a says "a library", not + /// "your library", so an opponent paying an `unless_pay` mill still counts. + /// + /// The match is **wildcard-free on purpose**: a new `AbilityCost` variant is + /// a compile error here, forcing a `true` / `false` / recursive decision. + pub fn moves_card_to_or_from_library(&self) -> bool { + match self { + // CR 701.17a: mill puts cards from the top of a library into a + // graveyard. Millikin and Deranged Assistant ("{T}, Mill a card: Add + // {C}") are the shipping cost-side cases. + AbilityCost::Mill { .. } => true, + + // `AbilityCost::Exile { zone }` is true IFF `zone == + // Some(Zone::Library)`, and `None` is deliberately FALSE. + // + // DO NOT widen this arm to inspect `filter`, and do not reach for a + // payability helper to resolve `None`. There are two payment paths and + // NEITHER is statically decidable: + // `game::cost_payability::exile_cost_effective_zone` is the authority + // for NON-SELF exile costs only — it resolves a missing zone to + // `Battlefield` (permanent-implying filter) or `Hand` (otherwise) — + // while the `TargetFilter::SelfRef` path short-circuits BEFORE it and + // treats a missing zone as the source's own CURRENT zone, which at + // runtime may be any zone including Library. + // + // `is_mana_ability` is a pure static classification over the printed + // AST and takes no `&GameState` (CR 605.2), so it can consult neither + // resolution. The explicit `Some(Zone::Library)` is therefore the only + // decidable test, and being the only one, it is the complete one. If + // that ever stops being true it is a parser-REPRESENTATION change, not + // a classifier change, and it would arrive as a new `zone` value + // rather than a new arm. + // + // ⚠️ 13 shipping mana abilities carry an `Exile` cost — Elvish Spirit + // Guide, Simian Spirit Guide, Food Chain, Black Tulip, Cadaverous + // Bloom, Ether, Jack-o'-Lantern, Mirrored Lotus, Molt Tender, Rubble + // Rouser, Sunken Palace, Thornvault Forager, Titans' Nest — and NONE + // carries `Library`. Writing `AbilityCost::Exile { .. } => true` here, + // dropping the zone read, strips mana-ability status from every one of + // them. `cost_axis_conditional_arms_read_their_typed_zone_fields` is + // the guard. + AbilityCost::Exile { zone, .. } => *zone == Some(Zone::Library), + + // Non-optional `zone`, so there is no default to reason about. + AbilityCost::ExileWithAggregate { zone, .. } => *zone == Zone::Library, + + // The field's own doc states the default: `from_zone: None` means + // BATTLEFIELD (the standard "return a permanent you control to its + // owner's hand" cost shape — Grinning Ignus is the one shipping mana + // ability using it). `Some(Zone::Graveyard)` is the Harvest Wurm + // unless-cost shape. Only an explicit `Library` counts. + AbilityCost::ReturnToHand { from_zone, .. } => *from_zone == Some(Zone::Library), + + // Compositional forms — recurse. This recursion is the SOLE authority + // for cost composition; `cost_has_component` cannot express it. + AbilityCost::Composite { costs } | AbilityCost::OneOf { costs } => { + costs.iter().any(AbilityCost::moves_card_to_or_from_library) + } + AbilityCost::PerCounter { base, .. } => base.moves_card_to_or_from_library(), + + // CR 602.1a: "The activation cost is everything before the colon (:)" + // — an effect written there is performed as a cost. Delegate one node. + AbilityCost::EffectCost { effect } => effect.moves_card_to_or_from_library(), + + // ---------- FALSE: no library endpoint ---------- + // Mana, energy, speed, life, and loyalty payments move no card at all. + AbilityCost::Mana { .. } + | AbilityCost::ManaDynamic { .. } + | AbilityCost::Waterbend { .. } + | AbilityCost::PayEnergy { .. } + // CR 702.179f: speed is a player designation, not a card. + | AbilityCost::PaySpeed { .. } + // CR 119.4: paying life moves no card. + | AbilityCost::PayLife { .. } + | AbilityCost::Loyalty { .. } + // CR 122.1: "A counter is a marker placed on an object or player ... + // Counters are not objects and have no characteristics." Giving the + // paying player counters (The Serpent Society's "Ward—Get five poison + // counters") places markers on a PLAYER — no card and no zone change, + // so no library can be an endpoint. + | AbilityCost::GetPlayerCounters { .. } + // CR 122.1 again: removing counters from a permanent is a marker + // change on an object already on the battlefield. + | AbilityCost::RemoveCounter { .. } + // Blight puts -1/-1 counters on a creature you control. + | AbilityCost::Blight { .. } + // CR 701.26a/b: tap/untap are status changes, not zone changes. + | AbilityCost::Tap + | AbilityCost::Untap + | AbilityCost::TapCreatures { .. } + | AbilityCost::Exert + // CR 701.3d: unattaching leaves the object on the battlefield. + | AbilityCost::Unattach + | AbilityCost::UnattachFrom { .. } + // Battlefield -> graveyard. + | AbilityCost::Sacrifice(_) + // Hand -> graveyard. + | AbilityCost::Discard { .. } + // CR 702.167a/b: craft materials come from the battlefield and/or your + // graveyard — a dual-zone union that never includes a library. + | AbilityCost::ExileMaterials { .. } + // CR 701.59a: collect evidence exiles from your GRAVEYARD. + | AbilityCost::CollectEvidence { .. } + // CR 701.20b: "Revealing a card doesn't cause it to leave the zone + // it's in." + | AbilityCost::Reveal { .. } + // CR 701.4a: behold is choose-or-reveal from hand/battlefield, with no + // zone change. + | AbilityCost::Behold { .. } + // CR 702.49a: ninjutsu returns an unblocked attacker to its owner's + // hand and puts the ninja onto the battlefield from hand. + | AbilityCost::NinjutsuFamily { .. } + // CR 118.9: a borrowed keyword mana cost is still a mana cost. + | AbilityCost::KeywordCostOfCastSpell { .. } + // The parser could not classify the cost fragment. Asserting `true` + // would strip mana-ability status on a guess; `false` is the honest + // answer, matching `Effect::Unimplemented`. + | AbilityCost::Unimplemented { .. } => false, + } + } + /// CR 605.3a + CR 106.12 + CR 107.6: True iff every component of this cost is /// conclusively decided by the non-simulating mana-ability cheap gate /// (`mana_ability_ready_without_simulation`) — i.e. the cost is built solely @@ -15576,6 +15726,644 @@ impl Effect { } } + /// CR 605.1a (2026 amendment): does resolving this effect move a card to or + /// from a **library**? + /// + /// Returns `true` iff resolving this effect can cause at least one card to + /// change zones with a library as the origin or the destination. Reordering + /// *within* a library is not a move to or from it. Revealing or looking at a + /// card is not a move (CR 701.20b: "Revealing a card doesn't cause it to + /// leave the zone it's in"). + /// + /// The answer must be decidable from the **static AST alone**, never from + /// game state — CR 605.1a is a static classification, and CR 605.2 ("A mana + /// ability remains a mana ability even if the game state doesn't allow it to + /// produce mana") forbids a state-dependent answer. Where a variant is + /// conditionally library-touching, decide from the variant's **own typed + /// fields**. + /// + /// This method answers for **THIS NODE ONLY**; nested payloads are the + /// walker's business (`ability_visit::visit_ability_def_scoped` under + /// `ResolutionScope::OwnResolutionOnly`). + /// + /// The match is **wildcard-free on purpose**: a new `Effect` variant is a + /// compile error here, forcing its author to answer "does this move a card + /// to or from a library?" at the one place that owns the answer. Do not add + /// an `_ =>` arm. + pub fn moves_card_to_or_from_library(&self) -> bool { + match self { + // ---------- Unconditionally TRUE: a library is always an endpoint ---------- + // CR 121.1: "A player draws a card by putting the top card of their + // library into their hand." + Effect::Draw { .. } + // CR 701.17a: mill puts cards from the top of a library into a + // graveyard; the library is always the origin. + | Effect::Mill { .. } + // CR 701.25a: surveil looks at the top cards and may "put any number + // of them into your graveyard" — they can leave the library. This is + // exactly the axis on which surveil differs from scry (CR 701.22a), + // which is why the two must never share an arm. + | Effect::Surveil { .. } + // CR 701.44a: explore "reveals the top card of their library", then + // puts it into hand or (optionally) graveyard. CR 701.44a is the SOLE + // authority — do NOT cite CR 701.44d, which is the simultaneous-explore + // APNAP ordering rule and says nothing about card movement. + | Effect::Explore + | Effect::ExploreAll { .. } + // Exiles the top N cards of a player's library. + | Effect::ExileTop { .. } + // Exiles one explicit object AND the top `count` cards of a library. + | Effect::ExileFaceDownPile { .. } + // CR 701.50a: connive "draws a card, then discards". + | Effect::Connive { .. } + // CR 728.1: a player with rad counters "mills a number of cards equal + // to the number of rad counters they have". + | Effect::ProcessRadCounters + // CR 701.13a: exiles from the top of a player's library. + | Effect::ExileFromTopUntil { .. } + // Reveal-until. TWO clauses, TWO authorities — do not collapse them. + // CR 701.20a is the authority for the REVEAL LOOP ONLY; per CR 701.20b + // revealing does NOT move a card. The movement is done by this + // variant's own `kept_destination` / `rest_destination` fields, which + // take the revealed cards out of the library. + | Effect::RevealUntil { .. } + // CR 701.57a: "Exile cards from the top of your library until ..." + | Effect::Discover { .. } + // CR 702.85a: cascade exiles from the top of your library, then + // bottoms the cards not cast. + | Effect::Cascade + // CR 702.60a: ripple reveals the top N and "put the rest on the bottom". + | Effect::Ripple { .. } + // Destination is a library position. + | Effect::PutAtLibraryPosition { .. } + // Hand -> top of library. + | Effect::ChooseDrawnThisTurnPayOrTopdeck { .. } + // Destination is a library. + | Effect::PutOnTopOrBottom { .. } + // CR 701.62a: manifest dread "Look at the top two cards of your + // library", then manifests one and bins the other. + | Effect::ManifestDread + // CR 701.48a: learn — "If you do, draw a card". + | Effect::Learn + // Randomly picks card(s) FROM a library matching a filter; the library + // is always the origin. + | Effect::Seek { .. } => true, + + // `Effect::Manifest` is TRUE because the ENGINE's manifest is always + // top-of-library (all 8 shipping cards; Ghastly Conscription's + // manifest-from-a-graveyard-pile routes to `ChangeZoneAll` instead), + // NOT because CR 701.40a says so — CR 701.40a names no source zone + // ("Put that card onto the battlefield face down"), and CR 701.40e + // ("If an effect instructs a player to manifest multiple cards from + // their library ...") and CR 701.40f ("it remains in its previous + // zone") together imply non-library sources are legal. A CR-derived + // verdict would have to be conditional; there is no source field to + // condition on. + // + // THIS ARM BECOMES WRONG the moment a card manifests from hand, + // graveyard, or exile AND parses to this variant. That change arrives + // as a new nested/source STRUCT FIELD — field access, not a match arm + // — and therefore compiles silently. If you are adding a source field + // to `Effect::Manifest`, make this arm conditional in the same commit. + Effect::Manifest { .. } => true, + + // No CR entry exists: "heist" does not appear anywhere in + // `docs/MagicCompRules.txt` (verified case-insensitively). Do not + // invent a CR number for this arm. The verdict is grounded in card + // Oracle text — "Heist target opponent's library" (Grave Expectations, + // Weave the Nightmare). + // + // `Heist` is TRUE even though the exile is performed by `HeistExile`, + // because `HeistExile` is an INTERNAL resolver continuation stashed by + // `Heist` rather than an AST chain sibling: the walker treats both as + // leaves and can never reach `HeistExile` from a `Heist` node. Marking + // `Heist` false "because the exile happens elsewhere" would under-narrow. + Effect::Heist { .. } | Effect::HeistExile => true, + + // `DigSource` is NOT a library-vs-not axis, and this arm is + // unconditional for that reason. Under `DigSource::PriorLook` the + // cards are STILL in `player.library` — the look-only pass takes an + // iterator slice rather than a drain and returns without removing + // them (`game/effects/dig.rs`), stashing their ids in + // `private_look_ids` — so the library is the origin under BOTH + // variants. Do not re-introduce a `source ==` test here, and do not + // condition on `destination` / `rest_destination` either: under + // `PriorLook` the move is out of the library regardless of where the + // cards land. The one non-moving configuration (an empty library, or + // a keep count of zero with no rest destination) is state-dependent, + // and CR 605.2 forbids a state-dependent classification. + Effect::Dig { .. } => true, + + // ---------- CONDITIONAL: decided from this variant's own typed fields ---------- + + // CR 614.15 + CR 701.6a: "put it on top of its owner's library + // INSTEAD of into its owner's graveyard" (Memory Lapse, Spell + // Crumple, Kylox's Voltstrider) is a SELF-replacement effect — an + // effect of this resolving ability replacing its own effect. CR 605.1a's + // closing sentence excludes replacement effects "other than + // self-replacement effects", so this one IS taken into account. + // + // CR 605.1a scopes the criterion to "ITS cost and effect" — whose + // resolution the movement happens in, not whether a field mentions a + // library. `SpellStackToGraveyardReplacement` has FOUR carriers and + // only this one is read, deliberately: + // - `Counter.countered_spell_zone` IS read. CR 608.2c cites "Counter + // target spell. If that spell is countered this way, put it on top + // of its owner's library instead of into its owner's graveyard" + // AS ITS OWN WORKED EXAMPLE of instructions this ability follows. + // CR 701.6a puts the countered spell into the graveyard during this + // resolution, and the rider redirects that same event. + // - `FreeCastFromZones.graveyard_replacement` and + // `CastingPermission::ExileWithAltCost.graveyard_replacement` are + // NOT read. Each replaces the CAST SPELL'S OWN LATER RESOLUTION at + // its CR 608.2n graveyard step ("as the final part of an instant or + // sorcery spell's resolution"). The cast itself is not later — what + // is later is the cast spell's own resolution, which belongs to a + // different object. So the rider is not this ability's own effect, + // hence not a self-replacement effect under CR 614.15, hence + // excluded by CR 605.1a's closing sentence. + // + // THIS ASYMMETRY IS THE DESIGN, NOT A BUG. Do not "fix" it by making + // the arms symmetric. Pinned by + // `only_counter_reads_the_stack_to_graveyard_replacement`. + Effect::Counter { + countered_spell_zone, + .. + } => matches!( + countered_spell_zone, + Some(SpellStackToGraveyardReplacement::Library { .. }) + ), + + // Three legs, all required. `origin: None` means "derive the origin + // from the target", and the zone then lives in the filter's + // `InZone`/`InAnyZone` property — so the filter must be consulted. + // Use `extract_zones()`, NOT `extract_in_zone()`, which collapses + // multi-zone filters to a single answer. + Effect::ChangeZone { + origin, + destination, + target, + .. + } => { + *destination == Zone::Library + || *origin == Some(Zone::Library) + || target.extract_zones().contains(&Zone::Library) + } + + // Same three-leg test as `ChangeZone`, plus: a `library_position` is + // only meaningful for a library destination, so its presence implies one. + Effect::ChangeZoneAll { + origin, + destination, + target, + library_position, + .. + } => { + *destination == Zone::Library + || *origin == Some(Zone::Library) + || library_position.is_some() + || target.extract_zones().contains(&Zone::Library) + } + + // `Some(Zone::Library)` is the top-of-library bounce class; the + // default (`None`) is the owner's hand. + Effect::Bounce { destination, .. } | Effect::BounceAll { destination, .. } => { + *destination == Some(Zone::Library) + } + + // CR 701.23a: search. The default is library-only, so the ordinary + // tutor is true; the multi-zone class is true whenever a library is + // among the searched zones. + Effect::SearchLibrary { source_zones, .. } => source_zones.contains(&Zone::Library), + + // CR 707.12 / CR 601.2a: casting moves the card to the stack, so a + // library among the source zones is a move out of a library. + Effect::CastCopyOfCard { target, .. } | Effect::CastFromZone { target, .. } => { + target.extract_zones().contains(&Zone::Library) + } + + // The augment/host combination names its own source zones. + Effect::ChooseAugmentAndCombineWithHost { zones, .. } => zones.contains(&Zone::Library), + + // CR 605.1a says "its cost AND effect"; a resolution-time `PayCost` + // carrying a library-moving cost is reached under the EFFECT limb + // (CR 608.2c), so delegate to the cost's own classification. + Effect::PayCost { cost, .. } => cost.moves_card_to_or_from_library(), + + // `zones` is where the cards are cast FROM, during this resolution + // (CR 601.2a). `graveyard_replacement` is deliberately NOT read — see + // the `Effect::Counter` arm above for the full four-carrier rationale. + Effect::FreeCastFromZones { zones, .. } => zones.contains(&Zone::Library), + + Effect::ChooseFromZone { + zone, + additional_zones, + .. + } => *zone == Zone::Library || additional_zones.contains(&Zone::Library), + + // CR 608.2c: the per-member action runs inside THIS resolution, so it + // is an inline conditional rather than a boundary. + Effect::ForEachCategory { action, .. } => matches!( + action, + ForEachCategoryAction::ExileFromPool { + zone: Zone::Library, + .. + } + ), + + // CR 121.1: only the `Card` gift is a draw ("Opponent draws a card"). + // `Treasure` / `Food` / `TappedFish` create tokens (CR 111.1). + Effect::GiftDelivery { kind } => { + matches!(kind, crate::types::keywords::GiftKind::Card) + } + + // `object_source: None` is TRUE because the ENGINE's default cloak + // source is the top of a library — the same empirical grounding as + // `Effect::Manifest`, NOT a CR derivation. CR 701.58a names no source + // zone ("Put that card onto the battlefield face down"), and CR 701.58e + // is the multi-cloak ORDERING rule ("If an effect instructs a player to + // cloak multiple cards from a single library, those cards are cloaked + // one at a time") — its conditional PRESUPPOSES a library source, it + // does not establish one, exactly as CR 701.40e does for manifest. No + // subrule in the CR 701.58 block names cloak's source zone. Cite + // CR 701.58a for "turn it face down / put onto the battlefield" and + // CR 701.58e only for the multi-cloak ordering it actually states; + // do not cite either as authority for the library source. + // + // `Some(filter)` is the good shape: the variant names its own source + // axis, so read it rather than assume it. + Effect::Cloak { object_source, .. } => match object_source { + None => true, + Some(filter) => filter.extract_zones().contains(&Zone::Library), + }, + + // A conjured card APPEARS IN a library it was not previously in — + // precisely the disruption CR 605.1a guards against. Digital-only + // keyword action with no CR entry, so the reasoning is carried inline + // rather than cited. + Effect::Conjure { destination, .. } + | Effect::DraftFromSpellbook { destination, .. } => *destination == Zone::Library, + + // ---------- Reasoned FALSE: library-adjacent but not a move ---------- + + // CR 701.22a: scry looks at the top N cards, "then put any number of + // them on the BOTTOM of your library ... and the rest on TOP of your + // library." Every card starts and ends in the same library, so nothing + // moves to or from it. Contrast `Surveil` (CR 701.25a), which is true + // precisely because its cards can leave for the graveyard — the two + // keyword actions differ on exactly this axis. A real shipping card + // depends on this staying false: The Secret Lair. + Effect::Scry { .. } => false, + + // CR 701.30a: "To clash, a player reveals the top card of their + // library. That player may then put that card on the bottom of their + // library." Either way the card stays in its own library. + Effect::Clash => false, + + // CR 701.24a: shuffling randomizes "the cards within it" — a reorder, + // not a move. `Effect::Shuffle` carries a player filter and no + // zone-move field at all. + Effect::Shuffle { .. } => false, + + // CR 701.20b: "Revealing a card doesn't cause it to leave the zone + // it's in." (`RevealFromHand`'s `on_decline` is an inline carrier the + // walker descends; the node itself moves nothing.) + Effect::RevealTop { .. } + | Effect::Reveal { .. } + | Effect::RevealHand { .. } + | Effect::RevealFromHand { .. } => false, + + // CR 400.11: "Outside the game is not a zone" — so a sideboard/wish + // search moves nothing to or from a library. CR 701.23j covers the + // outside-the-game search itself. + Effect::SearchOutsideGame { .. } => false, + + // CR 901.4: "All plane and phenomenon cards remain in the COMMAND ZONE + // throughout the game, both while they're part of a planar deck and + // while they're face up." CR 311.2 says the same for plane cards + // specifically. The planar deck lives in the command zone, so + // reordering its top moves nothing to or from a library. (Do NOT cite + // CR 901.15 here: that is the "Single Planar Deck Option", the + // shared-deck variant, and it states nothing about the deck's zone.) + Effect::ArrangePlanarDeckTop { .. } => false, + + // CR 701.51b + CR 717.2: the Attraction deck exists in the command + // zone, not a library. + Effect::OpenAttractions { .. } => false, + + // The Contraption deck likewise is not a library — exact siblings of + // `ArrangePlanarDeckTop`. + Effect::AssembleContraptions { .. } | Effect::AssembleContraptionsFromRollDifference => { + false + } + + // The engine decomposes hideaway into `Effect::Dig` + this conceal + // step. This node turns the JUST-EXILED card face down — it acts on a + // card already in exile. The paired `Effect::Dig` is the library mover + // and is independently true above. Noted honestly: CR 702.75a DOES + // describe a library look, so this false rests on the engine's + // decomposition rather than on the CR denying a library move. + // Double-counting here would obscure which link is load-bearing. + Effect::HideawayConceal { .. } => false, + + // Both variants carry `{ cost: ManaCost }` and nothing else — no zone + // field, no library reference. CR 702.94a casts the revealed card FROM + // HAND; CR 702.35a casts FROM EXILE. The preceding draw or discard is a + // separate effect that classifies on its own merits. + Effect::MiracleCast { .. } | Effect::MadnessCast { .. } => false, + + // CR 701.4a: behold is the choose-or-reveal keyword action, with no + // zone change. + Effect::Behold { .. } => false, + + // CR 701.61a: forage exiles from a graveyard or sacrifices a Food. + Effect::Forage => false, + + // CR 701.59a: "To 'collect evidence N' means to exile any number of + // cards from your GRAVEYARD with total mana value N or greater." + Effect::CollectEvidence { .. } => false, + + // The parser could not classify the fragment. Asserting `true` would + // reclassify unknown text on a guess, and this classifier must not + // double as a coverage signal — coverage already marks these cards + // unsupported, so `false` is the honest answer. + Effect::Unimplemented { .. } => false, + + // Names a card from a list of strings; no zone move. + Effect::ChooseCard { .. } => false, + + // CR 702.49a: ninjutsu returns an unblocked attacker to hand and puts + // the ninja from hand onto the battlefield. No library involved. + Effect::RuntimeHandled { .. } => false, + + // CR 701.49: the dungeon is a command-zone object, not a library card. + Effect::VentureIntoDungeon | Effect::VentureInto { .. } => false, + + // Granting a casting permission moves no card at THIS ability's + // resolution; the permitted cast happens later, as a separate object's + // cast (CR 601.2a) and resolution (CR 608.2n). Do NOT descend into + // `permission`. Three distinct limbs license this false, and they must + // not be collapsed into one sentence — the subtree is not homogeneous: + // + // (A) RULES, covering only the two `graveyard_replacement` fields + // (`CastingPermission::ExileWithAltCost` and + // `ResolutionCastSuccessAction::FreeCastOfferRemaining`): each is a + // rider on the CAST SPELL'S OWN LATER RESOLUTION at its CR 608.2n + // graveyard step, so per CR 614.15 it is not a self-replacement + // effect of this ability and CR 605.1a's closing sentence excludes + // it. THIS LIMB DOES NOT REACH THE OTHER TWO VARIANTS. + // + // (B) RULES, covering `ResolutionCastSuccessAction`'s other two + // variants. `BottomMisses` (the `#[default]`) and + // `RippleOfferRemaining` GENUINELY DO bottom cards to a library, + // and per CR 702.85a / CR 701.57a / CR 702.60a that bottoming + // happens inside the CASCADE / DISCOVER / RIPPLE ability's OWN + // resolution — not a later object's. They are false HERE because + // that ability's printed AST node is `Effect::Cascade` / + // `Effect::Discover` / `Effect::Ripple`, each already + // unconditionally true in this same match. `ResolutionCastCleanup` + // is the runtime continuation those resolvers mint WHILE resolving + // (`game/effects/cast_from_zone.rs`, + // `game/engine_resolution_choices.rs`), not the printed + // representation of the movement. Reading it here would + // double-count a verdict already correct one node over. + // + // (C) REACHABILITY FLOOR, empirical: the parser emits + // `resolution_cleanup: None` at both of its construction sites + // (`parser/oracle_effect/imperative.rs`, + // `parser/oracle_effect/mod.rs`), and `data/card-data.json` + // contains zero occurrences of `resolution_cleanup`, + // `success_action`, `BottomMisses`, or `RippleOfferRemaining`. + // Every `Some(..)` construction is a runtime resolver or a test. + // No parsed `AbilityDefinition` — this classifier's only input — + // reaches these variants at all. + // + // THIS ARM BECOMES WRONG the moment a parser production emits + // `resolution_cleanup: Some(..)`. That arrives as a CONSTRUCTION-SITE + // change, not a match arm, and COMPILES SILENTLY. If you are changing + // either parser site from `None`, re-derive this arm in the same commit: + // limb C is gone, and limb B holds only while the printed + // cascade/ripple/discover node carries the library verdict. + Effect::GrantCastingPermission { .. } => false, + + // ---------- Boundary carriers: false AT THE NODE ---------- + // Each registers a separate ability, replacement, or continuous effect + // rather than moving a card itself. Descent into their payloads is + // gated by `ResolutionScope` in `ability_visit`, not here. + // + // CR 603.3: `Effect::Mana`'s `grants` carry a `TriggerOnSpend` + // reflexive rider that fires when the mana is LATER spent — a separate + // triggered ability (Gilanra). `AddKeywordUntilEndOfTurn` is a CR 611.2 + // continuous effect on another object; `CantBeCountered` is a fieldless + // leaf. Deliberately NOT descended — see `ability_visit`'s leaf arm. + Effect::Mana { .. } + // CR 603.7a: a delayed triggered ability, created now, resolving later + // as its own ability (CR 603.3). + | Effect::CreateDelayedTrigger { .. } + // CR 603.3 primary / CR 614.1 secondary: these register a replacement + // that applies to a later event or to another object, so none is a + // self-replacement effect (CR 614.15) and CR 605.1a's carve-out does + // not reach them. + | Effect::CreateDrawReplacement { .. } + | Effect::CreatePlaneswalkReplacement { .. } + | Effect::AddTargetReplacement { .. } + | Effect::CreateDamageReplacement { .. } + // CR 114.1: an emblem is a distinct object in the command zone; its + // abilities are the emblem's. + | Effect::CreateEmblem { .. } + // CR 611.2: a continuous effect; any `GrantAbility` inside belongs to + // the affected object. + | Effect::GenericEffect { .. } + // CR 111.1: the token is a distinct permanent; its granted abilities + // are the token's. + | Effect::Token { .. } => false, + + // ---------- Inline branch carriers: false at the node ---------- + // These are branches of THIS resolution (CR 608.2c), not separate + // abilities, so the walker always descends into them — but the node + // itself moves no card. + Effect::Vote { .. } + | Effect::SeparateIntoPiles { .. } + | Effect::FlipCoin { .. } + | Effect::FlipCoins { .. } + | Effect::FlipCoinUntilLose { .. } + | Effect::RollDie { .. } + | Effect::ChooseOneOf { .. } => false, + + // ---------- Other reasoned FALSE ---------- + // CR 707.10: a copy of a spell is created on the stack. + Effect::CopySpell { .. } | Effect::EpicCopy { .. } => false, + // CR 707.2 governs the COPY SEMANTICS ("the copy acquires the copiable + // values of the original object's characteristics"). The token is + // created from a FORMAT CARD POOL, which is not a game zone at all, so + // no card changes zones — that point carries no CR, because CR 707.2 + // addresses neither pools nor libraries. + Effect::CreateTokenCopyFromPool { .. } => false, + // CR 701.42: meld moves the two cards from exile to the battlefield. + Effect::Meld { .. } => false, + // CR 702.55a: haunt exiles from a graveyard. + Effect::ExileHaunting { .. } => false, + // Stack -> exile. + Effect::ExileResolvingSpellInsteadOfGraveyard { .. } => false, + // Hand -> graveyard. + Effect::Discard { .. } | Effect::DiscardCard { .. } => false, + // Battlefield -> graveyard. + Effect::Sacrifice { .. } => false, + // Digital-only: mana plus a discard; no library endpoint. + Effect::Specialize => false, + // CR 701.16: investigate creates a Clue token (CR 111.1). + Effect::Investigate => false, + // CR 701.56a: time travel adjusts time counters (CR 122.1). + Effect::TimeTravel => false, + // CR 701.53a: incubate creates an Incubator token. + Effect::Incubate { .. } => false, + // CR 701.47a: amass creates an Army token and/or adds +1/+1 counters. + Effect::Amass { .. } => false, + + // ---------- Bulk FALSE ---------- + // Everything not named above. These move no card at all, or move cards + // between zones none of which is a library (battlefield, hand, + // graveyard, exile, stack, command). The match is wildcard-free, so a + // new variant omitted from every bucket is a COMPILE ERROR and a + // variant named twice is an unreachable-pattern warning — the compiler + // is the census, and this arm needs no hand-maintained list. + Effect::Adapt { .. } + | Effect::AdditionalPhase { .. } + | Effect::AddPendingEntersModifications { .. } + | Effect::AddPendingETBCounters { .. } + | Effect::AddRestriction { .. } + | Effect::Animate { .. } + | Effect::ApplyPerpetual { .. } + | Effect::ApplyPostReplacementDamage { .. } + | Effect::ApplySticker { .. } + | Effect::AssembleContraptionOnSprocket { .. } + | Effect::Attach { .. } + | Effect::BecomeBlocked { .. } + | Effect::BecomeCopy { .. } + | Effect::BecomeMonarch + | Effect::BecomePrepared { .. } + | Effect::BecomeSaddled { .. } + | Effect::BecomeUnprepared { .. } + | Effect::BlightEffect { .. } + | Effect::Bolster { .. } + | Effect::ChangeSpeed { .. } + | Effect::ChangeTargets { .. } + | Effect::ChaosEnsues + | Effect::Choose { .. } + | Effect::ChooseAndSacrificeRest { .. } + | Effect::ChooseCounterAdjustment { .. } + | Effect::ChooseCounterKind { .. } + | Effect::ChooseDamageSource { .. } + | Effect::ChooseObjectsIntoTrackedSet { .. } + | Effect::ChoosePermanent { .. } + | Effect::Cleanup { .. } + | Effect::CombineHost { .. } + | Effect::ControlNextTurn { .. } + | Effect::CopyTokenBlockingAttacker { .. } + | Effect::CopyTokenOf { .. } + | Effect::CounterAll { .. } + | Effect::CrankContraptions { .. } + | Effect::DamageAll { .. } + | Effect::DamageEachPlayer { .. } + | Effect::DealDamage { .. } + | Effect::Destroy { .. } + | Effect::DestroyAll { .. } + | Effect::Detain { .. } + | Effect::Double { .. } + | Effect::DoublePT { .. } + | Effect::DoublePTAll { .. } + | Effect::EachDealsDamageEqualToPower { .. } + | Effect::EachPlayerCopyChosen { .. } + | Effect::EachSourceDealsDamage { .. } + | Effect::Encore + | Effect::EndCombatPhase + | Effect::EndTheTurn + | Effect::Endure { .. } + | Effect::ExchangeControl { .. } + | Effect::ExchangeLifeTotals { .. } + | Effect::ExchangeLifeWithStat { .. } + | Effect::Exploit { .. } + | Effect::ExtraTurn { .. } + | Effect::Fight { .. } + | Effect::FlipPermanent { .. } + | Effect::ForceAttack { .. } + | Effect::ForceBlock { .. } + | Effect::GainActivatedAbilitiesOfTarget { .. } + | Effect::GainControl { .. } + | Effect::GainControlAll { .. } + | Effect::GainEnergy { .. } + | Effect::GainLife { .. } + | Effect::GiveControl { .. } + | Effect::GivePlayerCounter { .. } + | Effect::Goad { .. } + | Effect::GoadAll { .. } + | Effect::GrantExtraLoyaltyActivations { .. } + | Effect::GrantNextSpellAbility { .. } + | Effect::Harness + | Effect::Intensify { .. } + | Effect::LoseAllPlayerCounters { .. } + | Effect::LoseLife { .. } + | Effect::LoseTheGame { .. } + | Effect::Monstrosity { .. } + | Effect::MoveCounters { .. } + | Effect::MultiplyCounter { .. } + | Effect::Myriad + | Effect::NoOp + | Effect::NoteManaSpent + | Effect::OpponentGuess { .. } + | Effect::PairWith { .. } + | Effect::PhaseIn { .. } + | Effect::PhaseOut { .. } + | Effect::Planeswalk + | Effect::Populate + | Effect::PreventDamage { .. } + | Effect::Proliferate + | Effect::ProliferateTarget { .. } + | Effect::Pump { .. } + | Effect::PumpAll { .. } + | Effect::PutChosenCounter { .. } + | Effect::PutCounter { .. } + | Effect::PutCounterAll { .. } + | Effect::PutSticker { .. } + | Effect::ReassembleContraption { .. } + | Effect::ReassembleContraptionOnSprocket { .. } + | Effect::RedistributeLifeTotals + | Effect::ReduceNextSpellCost { .. } + | Effect::Regenerate { .. } + | Effect::RegisterBending { .. } + | Effect::RememberCard { .. } + | Effect::RemoveAllDamage { .. } + | Effect::RemoveCounter { .. } + | Effect::RemoveFromCombat { .. } + | Effect::Renown { .. } + | Effect::ReturnAsAura { .. } + | Effect::ReverseTurnOrder + | Effect::RingTemptsYou + | Effect::RollToVisitAttractions + | Effect::SetClassLevel { .. } + | Effect::SetDayNight { .. } + | Effect::SetLifeTotal { .. } + | Effect::SetRoomDoorLock { .. } + | Effect::SetTapState { .. } + | Effect::SkipNextStep { .. } + | Effect::SkipNextTurn { .. } + | Effect::SolveCase + | Effect::StartYourEngines { .. } + | Effect::Suspect { .. } + | Effect::SwapChosenLabels { .. } + | Effect::SwitchPT { .. } + | Effect::TakeTheInitiative + | Effect::TargetOnly { .. } + | Effect::Transform { .. } + | Effect::Tribute { .. } + | Effect::TurnFaceDown { .. } + | Effect::TurnFaceUp { .. } + | Effect::UnattachAll { .. } + | Effect::Unsuspect { .. } + | Effect::WinTheGame { .. } => false, + } + } + /// Visits every `QuantityExpr` carried by this effect — /// including the secondary quantity slots that `count_expr()` (the /// PRIMARY count/amount accessor) intentionally does not expose, such as diff --git a/crates/engine/src/types/ability_visit.rs b/crates/engine/src/types/ability_visit.rs index 8defc94643..21811f3dd7 100644 --- a/crates/engine/src/types/ability_visit.rs +++ b/crates/engine/src/types/ability_visit.rs @@ -27,34 +27,232 @@ //! only `Token` / `ChooseOneOf` / `sub_ability` / `else_ability` — broadening it //! would change replacement behavior). Neither is migrated here, because either //! migration would change behavior. +//! +//! # [`ResolutionScope`] — the own-resolution boundary +//! +//! Some callers need to know only what an ability does during **its own** +//! resolution, not what it merely *registers* to happen later. [`ResolutionScope`] +//! names that axis: CR 608.2c ("the controller of the spell or ability follows +//! its instructions in the order written") versus CR 603.3 (a triggered ability +//! is put on the stack "the next time a player would receive priority" — a +//! separate object, resolving separately). `is_mana_ability`'s CR 605.1a +//! library criterion is the first consumer. +//! +//! Under [`ResolutionScope::OwnResolutionOnly`] the walk visits the boundary +//! node itself but does **not** descend into payloads that belong to a later or +//! separate resolution: delayed triggers (CR 603.7a), registered replacements +//! (CR 614.1), emblem abilities (CR 114.1), token abilities (CR 111.1), granted +//! statics (CR 611.2), mana-spend grants (CR 603.3), and reflexive "when you do" +//! links (CR 603.12). Every existing public entry point passes +//! [`ResolutionScope::IncludeRegisteredLater`], so their behavior is unchanged. +//! +//! **The boundary has exactly two axes and one authority.** The effect walk +//! ([`visit_ability_def_scoped`]) and the cost walk +//! ([`visit_ability_def_costs_scoped`]) are separate because the effect +//! visitor is `FnMut(&Effect)` and an `AbilityCost::Mill` is not an `Effect` — +//! a type-level gap, not a missing match arm. Both consult +//! `scope_prunes_nested_ability` for the CR 603.12 decision and neither +//! re-implements it. +//! +//! **Scope-reset invariant.** `visit_trigger_scoped`, `visit_replacement_scoped`, +//! `visit_static_scoped`, `visit_continuous_mod_scoped`, and +//! `visit_copiable_values_scoped` are unreachable under `OwnResolutionOnly` +//! today, because every `visit_effect_scoped` arm that reaches them is a gated +//! boundary carrier. That is asserted by a `debug_assert!` at the head of each, +//! but correctness does **not** depend on the assertion: `scope` is propagated +//! into every nested `visit_ability_def_scoped` call, so the invariant holds by +//! construction in release builds too. The assertion is a decision-forcing +//! tripwire, not the mechanism. use crate::types::ability::{ - AbilityCost, AbilityDefinition, ContinuousModification, CopiableValues, CounterSourceRider, - Effect, ReplacementDefinition, ReplacementMode, StaticDefinition, TriggerDefinition, - VoteSubject, + AbilityCondition, AbilityCost, AbilityDefinition, ContinuousModification, CopiableValues, + CounterSourceRider, Effect, ReplacementDefinition, ReplacementMode, StaticDefinition, + TriggerDefinition, VoteSubject, }; use std::ops::ControlFlow; +/// CR 608.2c vs CR 603.3: which nested abilities a traversal attributes to the +/// ability it started from. +/// +/// CR 608.2c: "The controller of the spell or ability follows its instructions in +/// the order written" — those instructions are this ability's own resolution. +/// CR 603.3: an ability that *triggers* is put on the stack "the next time a +/// player would receive priority" — a separate object, resolving separately. +/// The two rules are the two sides of one binary, and this enum names it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ResolutionScope { + /// Visit only what THIS ability does during its own resolution. Stop at any + /// effect that merely REGISTERS a separate ability, replacement, or + /// continuous effect to apply later or to another object. + OwnResolutionOnly, + /// Visit the entire printed subtree, including separately-registered + /// payloads. The historical behavior of every existing entry point. + IncludeRegisteredLater, +} + +/// CR 603.12 + CR 603.7 + CR 603.3: a reflexive triggered ability ("when you do") +/// follows the rules for delayed triggered abilities and goes on the stack the +/// next time a player would receive priority. It is a SEPARATE ability, not an +/// instruction this ability follows during its own resolution (CR 608.2c). +/// +/// SINGLE AUTHORITY for the reflexive boundary. Both the effect walk +/// ([`visit_nested_ability_def_scoped`]) and the cost walk +/// ([`visit_ability_def_costs_scoped`]) consult this and nothing else. +/// +/// Deliberately keys on `WhenYouDo` ALONE. `AbilityCondition::EffectOutcome` +/// ("if you do, ...") is CR 608.2c — one instruction conditional on another +/// within the SAME resolution — and must keep being descended. Do not reach for +/// `effects::sub_ability_is_reflexive`, which unions the two because it answers a +/// different question (skip-on-decline), not this one. +fn scope_prunes_nested_ability(def: &AbilityDefinition, scope: ResolutionScope) -> bool { + scope == ResolutionScope::OwnResolutionOnly + && matches!(def.condition, Some(AbilityCondition::WhenYouDo)) +} + +/// Scope-reset trapdoor tripwire, shared by the five traversal functions that +/// are reachable only through a CR 603.3 boundary carrier. +/// +/// Those five — `visit_trigger_scoped`, `visit_replacement_scoped`, +/// `visit_static_scoped`, `visit_continuous_mod_scoped`, and +/// `visit_copiable_values_scoped` — are unreachable under `OwnResolutionOnly` +/// today, because every `visit_effect_scoped` arm that reaches them is a gated +/// boundary carrier: `GenericEffect`, `AddTargetReplacement`, `Counter`, +/// `Token`, and `CreateEmblem`. (Named by variant, not by line: the arm heads +/// move whenever this file is edited, and a stale number in a shipped comment +/// is worse than no number.) +/// +/// Correctness does NOT depend on this assertion: `scope` is propagated into +/// every nested `visit_ability_def_scoped` call, so an un-gated arm would carry +/// `OwnResolutionOnly` correctly rather than silently resetting it. The +/// assertion exists to FORCE A DECISION — reaching here under the narrow scope +/// means someone un-gated a CR 603.3 boundary carrier, and that is a rules +/// judgment a human must make deliberately rather than inherit by accident. +/// +/// Accepted residual: `debug_assert!` compiles out of release builds. That is +/// acceptable only because the tripwire is not the mechanism. +fn debug_assert_own_resolution_unreachable(scope: ResolutionScope, fn_name: &str) { + debug_assert!( + scope == ResolutionScope::IncludeRegisteredLater, + "boundary-carrier arm reached {fn_name} under OwnResolutionOnly — a CR 603.3 \ + boundary was un-gated; see the ability_visit module docs before proceeding" + ); +} + +/// Effect-axis wrapper over [`scope_prunes_nested_ability`]. Every recursion +/// into a nested `AbilityDefinition` that is *not* already behind a boundary +/// gate goes through here. +fn visit_nested_ability_def_scoped( + def: &AbilityDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + if scope_prunes_nested_ability(def, scope) { + return ControlFlow::Continue(()); + } + visit_ability_def_scoped(def, scope, visit) +} + pub fn visit_ability_def(def: &AbilityDefinition, visit: &mut F) -> ControlFlow<()> where F: FnMut(&Effect) -> ControlFlow<()>, { - visit_effect(&def.effect, visit)?; + visit_ability_def_scoped(def, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_ability_def_scoped( + def: &AbilityDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + visit_effect_scoped(&def.effect, scope, visit)?; if let Some(cost) = &def.cost { - visit_cost(cost, visit)?; + visit_cost_scoped(cost, scope, visit)?; } if let Some(sub) = &def.sub_ability { - visit_ability_def(sub, visit)?; + visit_nested_ability_def_scoped(sub, scope, visit)?; } if let Some(else_ability) = &def.else_ability { - visit_ability_def(else_ability, visit)?; + visit_nested_ability_def_scoped(else_ability, scope, visit)?; } for mode in &def.mode_abilities { - visit_ability_def(mode, visit)?; + visit_nested_ability_def_scoped(mode, scope, visit)?; } // "unless [player] pays {cost}" — the cost may be an EffectCost that conjures. if let Some(unless_pay) = &def.unless_pay { - visit_cost(&unless_pay.cost, visit)?; + visit_cost_scoped(&unless_pay.cost, scope, visit)?; + } + ControlFlow::Continue(()) +} + +/// CR 605.1a "its cost and effect" — the COST axis companion to +/// [`visit_ability_def_scoped`]. +/// +/// WHY THIS EXISTS AND CANNOT BE FOLDED INTO THE EFFECT WALK: the effect +/// visitor is `FnMut(&Effect)`, and `visit_cost` surfaces an `Effect` for +/// exactly one variant (`EffectCost`). `AbilityCost::Mill`, `Exile`, +/// `ExileWithAggregate`, and `ReturnToHand` carry no nested `Effect` at all, so +/// they are structurally invisible to any effect-shaped visitor. This is a +/// type-level gap, not a missing match arm. +/// +/// Yields every `AbilityCost` in the same own-resolution tree the effect walk +/// covers: the node's `cost` (CR 602.1a — the activation cost, at the root), +/// the node's `unless_pay.cost` (CR 118.12a -> CR 118.12 — a cost paid when the +/// ability RESOLVES, therefore reached under CR 608.2c as part of "its effect"), +/// and every `sub_ability` / `else_ability` / `mode_abilities` link, recursing +/// through the SAME `scope_prunes_nested_ability` authority so the CR 603.12 +/// reflexive boundary holds identically on both axes. +/// +/// Only TOP-LEVEL cost nodes are yielded; composition (`Composite`, `OneOf`, +/// `PerCounter`) is the consuming predicate's own recursion to do. A second +/// wildcard-free `AbilityCost` match in this module would be a drift hazard +/// against `visit_cost_scoped`, which is exactly the pattern this module's docs +/// reject. +/// +/// The alternative — an `AbilityNode<'a> { Effect(..), Cost(..) }` unified +/// visitor — was rejected: it would change the visitor signature for every +/// existing caller of the eight public entry points, for no correctness gain. +/// Recorded here so a future reader does not re-litigate it. +pub(crate) fn visit_ability_def_costs_scoped( + def: &AbilityDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&AbilityCost) -> ControlFlow<()>, +{ + // CR 602.1a: the activation cost — "everything before the colon (:)". + if let Some(cost) = &def.cost { + visit(cost)?; + } + // CR 118.12a -> CR 118.12: an "unless [a player] pays" action is a cost paid + // WHEN THE ABILITY RESOLVES, so CR 608.2c places it under "its effect". + if let Some(unless_pay) = &def.unless_pay { + visit(&unless_pay.cost)?; + } + // Chain links, each gated by the ONE CR 603.12 boundary authority. Three + // explicit blocks, mirroring `visit_ability_def_scoped`'s shape above, so a + // reviewer diffing the two sibling walkers sees the gate as the only + // difference. + if let Some(sub) = &def.sub_ability { + if !scope_prunes_nested_ability(sub, scope) { + visit_ability_def_costs_scoped(sub, scope, visit)?; + } + } + if let Some(else_ability) = &def.else_ability { + if !scope_prunes_nested_ability(else_ability, scope) { + visit_ability_def_costs_scoped(else_ability, scope, visit)?; + } + } + for mode in &def.mode_abilities { + if !scope_prunes_nested_ability(mode, scope) { + visit_ability_def_costs_scoped(mode, scope, visit)?; + } } ControlFlow::Continue(()) } @@ -63,11 +261,23 @@ pub fn visit_trigger(trigger: &TriggerDefinition, visit: &mut F) -> ControlFl where F: FnMut(&Effect) -> ControlFlow<()>, { + visit_trigger_scoped(trigger, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_trigger_scoped( + trigger: &TriggerDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + debug_assert_own_resolution_unreachable(scope, "visit_trigger_scoped"); if let Some(execute) = &trigger.execute { - visit_ability_def(execute, visit)?; + visit_ability_def_scoped(execute, scope, visit)?; } if let Some(unless_pay) = &trigger.unless_pay { - visit_cost(&unless_pay.cost, visit)?; + visit_cost_scoped(&unless_pay.cost, scope, visit)?; } ControlFlow::Continue(()) } @@ -76,21 +286,33 @@ pub fn visit_replacement(replacement: &ReplacementDefinition, visit: &mut F) where F: FnMut(&Effect) -> ControlFlow<()>, { + visit_replacement_scoped(replacement, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_replacement_scoped( + replacement: &ReplacementDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + debug_assert_own_resolution_unreachable(scope, "visit_replacement_scoped"); if let Some(execute) = &replacement.execute { - visit_ability_def(execute, visit)?; + visit_ability_def_scoped(execute, scope, visit)?; } // The mode carries the decline continuation (and, for MayCost, a cost), // either of which may conjure. Descend into both. match &replacement.mode { ReplacementMode::MayCost { cost, decline } => { - visit_cost(cost, visit)?; + visit_cost_scoped(cost, scope, visit)?; if let Some(decline) = decline { - visit_ability_def(decline, visit)?; + visit_ability_def_scoped(decline, scope, visit)?; } } ReplacementMode::Optional { decline } => { if let Some(decline) = decline { - visit_ability_def(decline, visit)?; + visit_ability_def_scoped(decline, scope, visit)?; } } ReplacementMode::Mandatory => {} @@ -104,8 +326,20 @@ pub fn visit_static(static_def: &StaticDefinition, visit: &mut F) -> ControlF where F: FnMut(&Effect) -> ControlFlow<()>, { + visit_static_scoped(static_def, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_static_scoped( + static_def: &StaticDefinition, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + debug_assert_own_resolution_unreachable(scope, "visit_static_scoped"); for modification in &static_def.modifications { - visit_continuous_mod(modification, visit)?; + visit_continuous_mod_scoped(modification, scope, visit)?; } ControlFlow::Continue(()) } @@ -117,19 +351,33 @@ pub fn visit_continuous_mod( where F: FnMut(&Effect) -> ControlFlow<()>, { + visit_continuous_mod_scoped(modification, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_continuous_mod_scoped( + modification: &ContinuousModification, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + debug_assert_own_resolution_unreachable(scope, "visit_continuous_mod_scoped"); match modification { ContinuousModification::GrantAbility { definition } => { - visit_ability_def(definition, visit)? + visit_ability_def_scoped(definition, scope, visit)? + } + ContinuousModification::GrantTrigger { trigger } => { + visit_trigger_scoped(trigger, scope, visit)? } - ContinuousModification::GrantTrigger { trigger } => visit_trigger(trigger, visit)?, ContinuousModification::GrantReplacement { replacement } => { - visit_replacement(replacement, visit)? + visit_replacement_scoped(replacement, scope, visit)? } ContinuousModification::GrantStaticAbility { definition } => { - visit_static(definition, visit)? + visit_static_scoped(definition, scope, visit)? } ContinuousModification::CopyValues { values, .. } => { - visit_copiable_values(values, visit)? + visit_copiable_values_scoped(values, scope, visit)? } // Remaining modifications carry no nested ability/effect carriers. // GrantAllActivatedAbilitiesOf / GrantAllTriggeredAbilitiesOf only hold a @@ -197,33 +445,56 @@ pub fn visit_copiable_values(values: &CopiableValues, visit: &mut F) -> Contr where F: FnMut(&Effect) -> ControlFlow<()>, { + visit_copiable_values_scoped(values, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_copiable_values_scoped( + values: &CopiableValues, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + debug_assert_own_resolution_unreachable(scope, "visit_copiable_values_scoped"); for ability in values.abilities.iter() { - visit_ability_def(ability, visit)?; + visit_ability_def_scoped(ability, scope, visit)?; } for trigger in values.trigger_definitions.iter() { - visit_trigger(trigger, visit)?; + visit_trigger_scoped(trigger, scope, visit)?; } for static_def in values.static_definitions.iter() { - visit_static(static_def, visit)?; + visit_static_scoped(static_def, scope, visit)?; } for replacement in values.replacement_definitions.iter() { - visit_replacement(replacement, visit)?; + visit_replacement_scoped(replacement, scope, visit)?; } ControlFlow::Continue(()) } pub fn visit_cost(cost: &AbilityCost, visit: &mut F) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + visit_cost_scoped(cost, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_cost_scoped( + cost: &AbilityCost, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> where F: FnMut(&Effect) -> ControlFlow<()>, { match cost { - AbilityCost::EffectCost { effect } => visit_effect(effect, visit)?, + AbilityCost::EffectCost { effect } => visit_effect_scoped(effect, scope, visit)?, AbilityCost::Composite { costs } | AbilityCost::OneOf { costs } => { for sub in costs { - visit_cost(sub, visit)?; + visit_cost_scoped(sub, scope, visit)?; } } - AbilityCost::PerCounter { base, .. } => visit_cost(base, visit)?, + AbilityCost::PerCounter { base, .. } => visit_cost_scoped(base, scope, visit)?, // Remaining costs carry no nested effect/cost carriers. AbilityCost::Mana { .. } | AbilityCost::ManaDynamic { .. } @@ -270,6 +541,17 @@ where /// are the complementary safety nets for those cases — extend both whenever a /// carrier is added. pub fn visit_effect(effect: &Effect, visit: &mut F) -> ControlFlow<()> +where + F: FnMut(&Effect) -> ControlFlow<()>, +{ + visit_effect_scoped(effect, ResolutionScope::IncludeRegisteredLater, visit) +} + +pub(crate) fn visit_effect_scoped( + effect: &Effect, + scope: ResolutionScope, + visit: &mut F, +) -> ControlFlow<()> where F: FnMut(&Effect) -> ControlFlow<()>, { @@ -280,14 +562,28 @@ where // CR 614.11: A one-shot draw replacement nests its substitute Effect // (Words of Worship/Wilding). Walk it so any conjure name it carries is // surfaced (GainLife/Token carry none today, but it is a nested carrier). + // + // BOUNDARY CARRIER (CR 603.3 primary / CR 614.1 secondary): this + // registers a replacement that applies to a LATER event. It is therefore + // NOT a self-replacement effect (CR 614.15, which scopes those to an + // effect of a resolving spell or ability replacing "that spell or + // ability's own effect(s)"), so CR 605.1a's closing carve-out does not + // reach it and the substitute effect is not part of THIS resolution. Effect::CreateDrawReplacement { replacement_effect } => { - visit_effect(replacement_effect, visit)? + if scope == ResolutionScope::IncludeRegisteredLater { + visit_effect_scoped(replacement_effect, scope, visit)? + } } // CR 614.1a: A planeswalk replacement nests its substitute Effect (Fixed // Point in Time: chaos ensues). Walk it so any conjure name it carries is // surfaced (ChaosEnsues carries none today, but it is a nested carrier). + // + // BOUNDARY CARRIER — same reason as `CreateDrawReplacement` above: + // CR 603.3 primary, CR 614.1 secondary, not a CR 614.15 self-replacement. Effect::CreatePlaneswalkReplacement { replacement_effect } => { - visit_effect(replacement_effect, visit)? + if scope == ResolutionScope::IncludeRegisteredLater { + visit_effect_scoped(replacement_effect, scope, visit)? + } } // Heist exiles a card from an opponent's library at random; it does not // name a conjure card, so there is no static face to preload. @@ -319,7 +615,7 @@ where .. } => { for sub in per_choice_effect { - visit_ability_def(sub, visit)?; + visit_nested_ability_def_scoped(sub, scope, visit)?; } // CR 701.38b: object-pool votes (Council's Judgment, Prime // Minister's Cabinet Room) leave `per_choice_effect` empty and @@ -330,7 +626,7 @@ where outcome_template, .. } = subject { - visit_ability_def(outcome_template, visit)?; + visit_nested_ability_def_scoped(outcome_template, scope, visit)?; } } Effect::SeparateIntoPiles { @@ -338,20 +634,29 @@ where unchosen_pile_effect, .. } => { - visit_ability_def(chosen_pile_effect, visit)?; + visit_nested_ability_def_scoped(chosen_pile_effect, scope, visit)?; if let Some(unchosen) = unchosen_pile_effect { - visit_ability_def(unchosen, visit)?; + visit_nested_ability_def_scoped(unchosen, scope, visit)?; } } Effect::RevealFromHand { on_decline, .. } => { if let Some(sub) = on_decline { - visit_ability_def(sub, visit)?; + visit_nested_ability_def_scoped(sub, scope, visit)?; } } // Only the delayed `effect` is walked; the `condition`'s embedded // TriggerDefinition has `execute: None` by construction (it is a matcher, // not a payload), so it carries no conjure name. - Effect::CreateDelayedTrigger { effect, .. } => visit_ability_def(effect, visit)?, + // + // BOUNDARY CARRIER (CR 603.7a): a delayed triggered ability is created + // now and resolves later as its own ability on the stack (CR 603.3). Its + // payload is not an instruction THIS ability follows during its own + // resolution (CR 608.2c), so `OwnResolutionOnly` stops here. + Effect::CreateDelayedTrigger { effect, .. } => { + if scope == ResolutionScope::IncludeRegisteredLater { + visit_ability_def_scoped(effect, scope, visit)? + } + } Effect::FlipCoin { win_effect, lose_effect, @@ -363,57 +668,92 @@ where .. } => { if let Some(sub) = win_effect { - visit_ability_def(sub, visit)?; + visit_nested_ability_def_scoped(sub, scope, visit)?; } if let Some(sub) = lose_effect { - visit_ability_def(sub, visit)?; + visit_nested_ability_def_scoped(sub, scope, visit)?; } } - Effect::FlipCoinUntilLose { win_effect } => visit_ability_def(win_effect, visit)?, + Effect::FlipCoinUntilLose { win_effect } => { + visit_nested_ability_def_scoped(win_effect, scope, visit)? + } Effect::RollDie { results, .. } => { for branch in results { - visit_ability_def(&branch.effect, visit)?; + visit_nested_ability_def_scoped(&branch.effect, scope, visit)?; } } Effect::ChooseOneOf { branches, .. } => { for branch in branches { - visit_ability_def(branch, visit)?; + visit_nested_ability_def_scoped(branch, scope, visit)?; } } // GenericEffect applies static abilities at resolution; their // modifications can grant abilities/triggers that themselves conjure. // Descend into the granted definitions rather than treating it as a leaf. + // + // BOUNDARY CARRIER (CR 611.2): "A continuous effect may be generated by + // the resolution of a spell or ability." Any `GrantAbility` inside + // belongs to the AFFECTED object, not to this ability's own resolution. Effect::GenericEffect { static_abilities, .. } => { - for static_def in static_abilities { - visit_static(static_def, visit)?; + if scope == ResolutionScope::IncludeRegisteredLater { + for static_def in static_abilities { + visit_static_scoped(static_def, scope, visit)?; + } } } // Carries a nested ReplacementDefinition whose execute/decline/cost may conjure. - Effect::AddTargetReplacement { replacement, .. } => visit_replacement(replacement, visit)?, + // + // BOUNDARY CARRIER (CR 603.3 primary / CR 614.1 secondary): registers a + // replacement that applies to a later event AND to another object, so it + // is not a CR 614.15 self-replacement effect of this ability and falls + // outside CR 605.1a's carve-out. + Effect::AddTargetReplacement { replacement, .. } => { + if scope == ResolutionScope::IncludeRegisteredLater { + visit_replacement_scoped(replacement, scope, visit)? + } + } // Counter's `source_rider` may apply a static to the countered source // (LosesAbilities) that grants an ability that conjures. The Destroy // rider carries no static. + // + // BOUNDARY CARRIER (CR 611.2): the rider is a continuous effect applied + // to the countered source — another object. NOTE this gates only + // `source_rider`; `Counter`'s OWN `countered_spell_zone` field is a + // CR 614.15 self-replacement effect read directly by + // `Effect::moves_card_to_or_from_library`, not by this walk. Effect::Counter { source_rider, .. } => { - if let Some(CounterSourceRider::LosesAbilities { static_def, .. }) = source_rider { - visit_static(static_def, visit)?; + if scope == ResolutionScope::IncludeRegisteredLater { + if let Some(CounterSourceRider::LosesAbilities { static_def, .. }) = source_rider { + visit_static_scoped(static_def, scope, visit)?; + } } } // Tokens and emblems can host granted static/triggered abilities that conjure. + // + // BOUNDARY CARRIER (CR 111.1): "A token is a marker used to represent any + // permanent that isn't represented by a card." The token is a distinct + // permanent and its granted abilities are the token's, not this one's. Effect::Token { static_abilities, .. } => { - for static_def in static_abilities { - visit_static(static_def, visit)?; + if scope == ResolutionScope::IncludeRegisteredLater { + for static_def in static_abilities { + visit_static_scoped(static_def, scope, visit)?; + } } } + // BOUNDARY CARRIER (CR 114.1): an emblem is a distinct object in the + // command zone; its abilities are the emblem's, not this ability's. Effect::CreateEmblem { statics, triggers } => { - for static_def in statics { - visit_static(static_def, visit)?; - } - for trigger in triggers { - visit_trigger(trigger, visit)?; + if scope == ResolutionScope::IncludeRegisteredLater { + for static_def in statics { + visit_static_scoped(static_def, scope, visit)?; + } + for trigger in triggers { + visit_trigger_scoped(trigger, scope, visit)?; + } } } // Leaf effects with no nested ability/effect carrier. @@ -505,6 +845,21 @@ where | Effect::Animate { .. } | Effect::RegisterBending { .. } | Effect::Cleanup { .. } + // `Effect::Mana { grants }` is a DELIBERATE no-descend, not an accident + // of leaf-ness, and all three `ManaSpellGrant` variants are swept: + // - `TriggerOnSpend { filter, ability }` is the only one carrying a + // nested `AbilityDefinition`. CR 603.3: it is a separate triggered + // ability that goes on the stack when the mana is LATER spent, in a + // different resolution entirely (Gilanra's `Draw` lives here). + // - `AddKeywordUntilEndOfTurn { .. }` is CR 611.2 — a continuous effect + // granting a keyword to ANOTHER object (the spell the mana is spent + // on) for a duration. It carries no nested ability. + // - `CantBeCountered` is a fieldless leaf. + // Both CR reasons are recorded because a future reader who sees only + // CR 603.3 will not know why the other two are safe. A "helpful" descent + // here would break `parser::oracle_tests`'s Path of Ancestry guard, + // which asserts the delayed-trigger rider does not disqualify the mana + // ability under CR 605.1a. | Effect::Mana { .. } | Effect::Discard { .. } | Effect::Shuffle { .. } diff --git a/crates/engine/tests/integration/cr605_1a_library_criterion.rs b/crates/engine/tests/integration/cr605_1a_library_criterion.rs new file mode 100644 index 0000000000..3e5a0ec6ad --- /dev/null +++ b/crates/engine/tests/integration/cr605_1a_library_criterion.rs @@ -0,0 +1,266 @@ +//! CR 605.1a (2026 amendment) — the library-movement criterion, end to end. +//! +//! > 605.1a An activated ability is a mana ability if it meets all of the +//! > following criteria: it doesn't require a target (see rule 115.6), it could +//! > add mana to a player's mana pool when it resolves, it's not a loyalty +//! > ability (see rule 606, "Loyalty Abilities"), and **its cost and effect +//! > don't move any card to or from a library.** +//! +//! The classifier itself is unit-tested in `game/mana_abilities.rs` (rows +//! V1-V13). This file covers the four consequences that are only observable by +//! driving the real pipeline, and it deliberately asserts **no** AST-internal +//! flag: `is_mana_ability` never appears in an assertion here. +//! +//! | Row | Claim | Seam | +//! |---|---|---| +//! | V14 | a reclassified ability still produces its mana, **via the stack** | `engine.rs` dispatch fork -> `casting::handle_activate_ability` -> `stack::push_to_stack` | +//! | V14b | it leaves the instant-speed auto-tap pool | `mana_sources::activatable_mana_actions_for_player` | +//! | V15 | `FilterProp::HasManaAbility` stops matching it | `game/filter.rs` | +//! | V16 | `TriggerCondition::ActivatedAbilityIsNonMana` now fires on it | `game/triggers.rs` (unchanged code, changed input) | +//! +//! Every negative is paired with a positive reach-guard **in the same test**, +//! built from a still-qualifying mana source on the same battlefield, so a +//! fixture that never reached the seam cannot pass vacuously. + +use engine::game::mana_sources::activatable_mana_actions_for_player; +use engine::game::scenario::{GameScenario, P0}; +use engine::types::actions::GameAction; +use engine::types::events::GameEvent; +use engine::types::identifiers::ObjectId; +use engine::types::mana::{ManaType, ManaUnit}; +use engine::types::phase::Phase; +use engine::types::zones::Zone; + +/// Verbatim Oracle text. A paraphrase can take a different parser branch and go +/// green while the real card stays broken, so these are exact. +const CHROMATIC_SPHERE: &str = + "{1}, {T}, Sacrifice this artifact: Add one mana of any color. Draw a card."; +const MILLIKIN: &str = "{T}, Mill a card: Add {C}."; +const LLANOWAR_ELVES: &str = "{T}: Add {G}."; +const RAGGADRAGGA: &str = "Each creature you control with a mana ability gets +2/+2."; +const BURNING_TREE_SHAMAN: &str = "Whenever a player activates an ability that isn't a mana \ + ability, this creature deals 1 damage to that player."; + +fn generic_unit() -> ManaUnit { + ManaUnit::new(ManaType::Colorless, ObjectId(0), false, vec![]) +} + +/// **V14** — a reclassified ability still produces its mana and performs its +/// draw, and it does so through the **stack**. +/// +/// Chromatic Sphere's draw makes its own resolution move a card from a library, +/// so under CR 605.1a it is no longer a mana ability. CR 605.3b ("an activated +/// mana ability doesn't use the stack") therefore stops applying to it: the +/// dispatch fork falls through to the ordinary activated-ability path and the +/// ability is put on the stack, where opponents get priority before the draw. +/// That is the point of the reclassification, not a side effect of it. +#[test] +fn chromatic_sphere_produces_mana_and_draws_through_the_stack() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + // Make the draw observable rather than inferred from a hand count alone. + scenario.with_library_top(P0, &["Forest", "Library Bottom"]); + // The Sphere's cost is `{1}, {T}, Sacrifice this artifact`. Without a funded + // pool the `{1}` is unpayable and the activation cannot even begin. + scenario.with_mana_pool(P0, vec![generic_unit()]); + let sphere = scenario + .add_creature(P0, "Chromatic Sphere", 0, 0) + .as_artifact() + .from_oracle_text(CHROMATIC_SPHERE) + .id(); + + let mut runner = scenario.build(); + let outcome = runner.activate(sphere, 0).resolve(); + + // The ability resolved: one card drawn, and the artifact was sacrificed as + // part of paying the cost. + outcome.assert_hand_drawn(P0, 1); + outcome.assert_zone(&[sphere], Zone::Graveyard); + // And the mana was still produced — the reclassification changes the ROUTE, + // never the payload. + assert!( + outcome.mana_pool_total(P0) >= 1, + "the mana ability's mana must still reach the pool" + ); + // The ROUTE itself, which the three assertions above cannot see: all of them + // hold identically on the off-stack mana fast path, so without this the test + // would pass unchanged if the Sphere were still classified as a mana + // ability. `GameEvent::AbilityActivated` is documented on the variant as + // never emitted for mana abilities (CR 605.3b — they resolve immediately on + // a separate path that never reaches the emission site), so its presence is + // the discriminator between the stack path and the fast path. + assert!( + outcome + .events() + .iter() + .any(|event| matches!(event, GameEvent::AbilityActivated { .. })), + "CR 605.3b: no longer a mana ability, so the activation must use the \ + stack and emit AbilityActivated (events: {:?})", + outcome.events() + ); +} + +/// **V14b** — a reclassified ability drops out of the instant-speed auto-tap +/// pool, because CR 605.3a only lets a player activate a **mana ability** while +/// casting or paying. An affordance a player cannot legally use would be a UI +/// lie, so the payment picker must stop offering it. +/// +/// The paired reach-guard is a Llanowar Elves on the same battlefield: it is +/// still a mana ability, so it must still be offered. Without that pair the +/// negative could pass because the harness produced no actions at all. +#[test] +fn chromatic_sphere_is_not_offered_as_an_instant_speed_mana_source() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_library_top(P0, &["Forest", "Library Bottom"]); + scenario.with_mana_pool(P0, vec![generic_unit()]); + let sphere = scenario + .add_creature(P0, "Chromatic Sphere", 0, 0) + .as_artifact() + .from_oracle_text(CHROMATIC_SPHERE) + .id(); + let elves = scenario + .add_creature(P0, "Llanowar Elves", 1, 1) + .from_oracle_text(LLANOWAR_ELVES) + .id(); + + let runner = scenario.build(); + let actions = activatable_mana_actions_for_player(runner.state(), P0); + + // Match the typed `source_id`, never a substring of the debug rendering: a + // stringified `ObjectId` of `1` is a substring of `10`, `11`, `21`, ... so a + // debug-text `contains` would answer for the wrong object. + let mentions = |target: ObjectId| { + actions.iter().any(|action| { + matches!(action, GameAction::ActivateAbility { source_id, .. } if *source_id == target) + }) + }; + + // Reach-guard first: a still-qualifying mana source IS offered, so the + // enumeration ran and produced a non-empty, correctly-scoped result. + assert!( + mentions(elves), + "Llanowar Elves is still a mana ability and must stay in the pool \ + (actions: {actions:?})" + ); + assert!( + !mentions(sphere), + "Chromatic Sphere is no longer a mana ability (CR 605.1a) so CR 605.3a \ + forbids activating it during a payment window (actions: {actions:?})" + ); +} + +/// **V15** — the in-game behavioral negative: `FilterProp::HasManaAbility` stops +/// matching a reclassified ability, because the prop is *defined by reference +/// to* CR 605.1a. Narrowing 605.1a therefore necessarily narrows every card that +/// keys off it. +/// +/// Raggadragga, Goreguts Boss grants `+2/+2` to "each creature you control with +/// a mana ability" — a CR 613 layer-7c continuous effect, so the displayed P/T +/// changes on the board the instant this ships. Millikin's `{T}, Mill a card: +/// Add {C}` moves a card from a library as its **cost**, so it loses the anthem. +/// +/// Paired reach-guard, same battlefield, same Raggadragga: Llanowar Elves is +/// still a mana ability and still gets the +2/+2. Without it the negative could +/// pass because Raggadragga was absent, under the wrong controller, or its +/// static never applied at all. +#[test] +fn raggadragga_no_longer_pumps_a_mill_cost_mana_creature() { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_library_top(P0, &["Forest", "Library Bottom"]); + scenario + .add_creature(P0, "Raggadragga, Goreguts Boss", 4, 4) + .from_oracle_text(RAGGADRAGGA); + let millikin = scenario + .add_creature(P0, "Millikin", 0, 3) + .as_artifact() + .from_oracle_text(MILLIKIN) + .id(); + let elves = scenario + .add_creature(P0, "Llanowar Elves", 1, 1) + .from_oracle_text(LLANOWAR_ELVES) + .id(); + + // The layer system materializes post-continuous-effect P/T back into + // `GameObject::power` / `toughness` during `apply`, so drive one real action + // (activating the still-qualifying Elves) to get a post-pipeline state + // rather than reading pre-layer fields off a freshly-built runner. + let mut runner = scenario.build(); + let outcome = runner.activate(elves, 0).resolve(); + + // Reach-guard: the anthem is installed and applying to a qualifying + // creature, so the negative below is a genuine non-match. + assert_eq!( + outcome.power_toughness(elves), + (3, 3), + "Llanowar Elves is still a mana ability and must get Raggadragga's +2/+2" + ); + assert_eq!( + outcome.power_toughness(millikin), + (0, 3), + "Millikin's Mill cost moves a card from a library, so under CR 605.1a it \ + has no mana ability and must NOT get the +2/+2" + ); +} + +/// **V16** — the in-game behavioral negative in the other direction: +/// `TriggerCondition::ActivatedAbilityIsNonMana` now fires on a reclassified +/// activation. +/// +/// This needs no code change — it is unchanged code seeing a changed input. A +/// mana ability never reaches a `GameEvent::AbilityActivated` emission site, +/// because all three sit downstream of a `stack::push_to_stack` and the mana +/// fast path forks before it. Once Chromatic Sphere is not a mana ability it +/// takes the stack path, `AbilityActivated` is emitted, and the trigger sees it. +/// +/// Under CR 605.1a that is **correct**: the Sphere's ability genuinely is not a +/// mana ability, so a trigger reading "whenever a player activates an ability +/// that isn't a mana ability" *should* see it. This row also pins the behavior +/// so that `triggers.rs`'s comment — which today says `AbilityActivated` is +/// emitted only by stack-using activations "by construction" — cannot silently +/// rot into a false claim once the invariant becomes classification-based. +#[test] +fn burning_tree_shaman_now_sees_a_chromatic_sphere_activation() { + fn activate_and_measure_life(oracle: &str, name: &str, fund_pool: bool) -> i32 { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + scenario.with_library_top(P0, &["Forest", "Library Bottom"]); + if fund_pool { + scenario.with_mana_pool(P0, vec![generic_unit()]); + } + scenario + .add_creature(P0, "Burning-Tree Shaman", 3, 4) + .from_oracle_text(BURNING_TREE_SHAMAN); + let mut builder = scenario.add_creature(P0, name, 0, 0); + if name == "Chromatic Sphere" { + builder.as_artifact(); + } + let source = builder.from_oracle_text(oracle).id(); + + let mut runner = scenario.build(); + let outcome = runner.activate(source, 0).resolve(); + outcome.life_delta(P0) + } + + // Reach-guard FIRST — and here the reach-guard is the POSITIVE case. Only a + // -1 life delta is evidence that the Shaman is on the battlefield with a + // live trigger; the 0 below would also pass on a fixture with no Shaman at + // all, because "nothing happened" and "nothing could have happened" are the + // same observation. Do not reorder these or drop this one: the negative + // carries no reachability evidence on its own. + assert_eq!( + activate_and_measure_life(CHROMATIC_SPHERE, "Chromatic Sphere", true), + -1, + "Chromatic Sphere is no longer a mana ability, so it uses the stack, \ + emits AbilityActivated, and Burning-Tree Shaman pings its controller" + ); + // The discriminating negative: a still-qualifying mana ability must NOT + // trigger it, so the reclassification is what moved and not the fixture. + assert_eq!( + activate_and_measure_life(LLANOWAR_ELVES, "Llanowar Elves", false), + 0, + "Llanowar Elves is still a mana ability — CR 605.3b keeps it off the \ + stack, so no AbilityActivated event and no trigger" + ); +} diff --git a/crates/engine/tests/integration/main.rs b/crates/engine/tests/integration/main.rs index 9050ceea7e..6135df6f60 100644 --- a/crates/engine/tests/integration/main.rs +++ b/crates/engine/tests/integration/main.rs @@ -101,6 +101,7 @@ mod counter_anaphor_created_token_binding; mod counter_double_redirect_choice; mod counter_spell_zone_redirect; mod court_of_cunning_multi_target_mill; +mod cr605_1a_library_criterion; mod cr733_resolved_attachment; mod cr733_resolved_combat_membership; mod cr733_resolved_commands_p0; From f5d8bc34efb5df4c9bb37eaeee19621a95a50df4 Mon Sep 17 00:00:00 2001 From: matthewevans Date: Sat, 8 Aug 2026 16:31:14 -0700 Subject: [PATCH 2/2] fix(engine): correct CR annotations on the 605.1a library criterion MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses all twelve findings from the implementation review of the previous commit. No arm verdict changes and no card is reclassified: eleven are comment corrections on annotations that carry the design's rules argument, and the twelfth reads a field that has no reachable non-default value today. The two that could have caused a future regression: - `Effect::Ripple`'s comment quoted only the reveal and the bottom-of-library legs of CR 702.60a. This file classifies revealing as `false` (CR 701.20b) and bottoming-within-a-library as `false` (the `Scry` arm), so the comment argued for the opposite of its own `true`. The warrant is the CASTING leg — CR 702.60a lets you "cast any of those cards ... without paying their mana costs", which CR 601.2a makes a library -> stack move. Now cited, with an explicit note not to "correct" it by reading only the other two clauses. - CR 603.3 was cited as the PRIMARY authority for the replacement-registration boundary at five sites. CR 603.3 is exclusively about a *triggered* ability going on the stack; `CreateDrawReplacement`, `CreatePlaneswalkReplacement`, `AddTargetReplacement` and `CreateDamageReplacement` create replacement effects, which never trigger and never use the stack. Swapped to CR 614.1 primary / CR 614.15 secondary. CR 603.3 is retained on the `CreateDelayedTrigger` arm, where a delayed triggered ability genuinely does go on the stack later. Also corrected: - `Effect::SearchOutsideGame` now reads its `destination` rather than returning an unconditional `false`. CR 400.11 licenses only the ORIGIN half ("outside the game is not a zone"); a `Zone::Library` destination would be a move *to* a library. All 11 shipping nodes are `Hand`, so no behavior changes. - `Effect::Manifest`'s empirical warrant said "all 8 shipping cards"; there are 24 `"type":"Manifest"` nodes in `data/card-data.json`. The arm's `true` rests entirely on that census, so the number is the whole argument. Verdict unaffected — every one is "manifest the top card of your library". - `Effect::ExileFromTopUntil` no longer cites CR 701.13a for a library origin; 701.13a says only "move it to the exile zone from wherever it is" and names no source zone. Matches the adjacent `ExileTop` arm, which cites nothing. - `Effect::Meld` cites CR 701.42a (puts the pair onto the battlefield) instead of CR 701.42, which never names exile as the source. - `AbilityCost::PaySpeed` cites CR 702.179b (speed is a numeric player value) instead of CR 702.179f, and drops "designation" — a CR term of art reserved for permanent-level markers. - `AbilityCost::KeywordCostOfCastSpell`'s comment asserted a keyword cost "is still a mana cost"; CR 118.9 says an alternative cost is paid "rather than paying the spell's mana cost". Reworded to match the rule and the variant's own doc. - `Effect::Mana`'s `TriggerOnSpend` rider is no longer called "reflexive". Under CR 603.12 a reflexive trigger is checked immediately, within the resolution that created it; this one fires in a later resolution. The sibling comments already said "a separate triggered ability"; this was the outlier. - `mana_abilities.rs` cites CR 605.3b for "resolve immediately without using the stack" instead of the CR 605.3 header, which is only a preamble. - The V14b test no longer describes CR 605.3a as a prohibition. It is a permission ("a player may activate an activated mana ability..."); once an ability stops being a mana ability that exception simply stops covering it and CR 117.1b governs again. And one documented gap, not a code change: - `visit_ability_def_costs_scoped`'s doc claimed it yields every `AbilityCost` "in the same own-resolution tree the effect walk covers". It does not: the effect walk descends nested `AbilityDefinition`s under `Vote`, `SeparateIntoPiles`, `RevealFromHand.on_decline`, `FlipCoin`/`FlipCoins`, `FlipCoinUntilLose`, `RollDie.results` and `ChooseOneOf.branches`; the cost walk does not, and `Effect::PayCost` is reached only by a one-node delegation. The claim is narrowed to the chain-link axis and the gap is named explicitly. `data/card-data.json` carries zero costs on an effect-payload-nested `AbilityDefinition`, so nothing is reachable today. Closing it means driving both walks from one shared carrier list, which widens cost coverage for every existing `IncludeRegisteredLater` caller and needs its own census. Every CR number cited here was verified against docs/MagicCompRules.txt before being written, including the three newly introduced (CR 117.1b, CR 601.2a, CR 701.42a). --- crates/engine/src/game/mana_abilities.rs | 8 ++-- crates/engine/src/types/ability.rs | 37 +++++++++++++------ crates/engine/src/types/ability_visit.rs | 32 +++++++++++++--- .../integration/cr605_1a_library_criterion.rs | 14 ++++--- 4 files changed, 64 insertions(+), 27 deletions(-) diff --git a/crates/engine/src/game/mana_abilities.rs b/crates/engine/src/game/mana_abilities.rs index d25ef6008f..f7f593aee6 100644 --- a/crates/engine/src/game/mana_abilities.rs +++ b/crates/engine/src/game/mana_abilities.rs @@ -52,7 +52,7 @@ use super::zone_pipeline::{self, ZoneMoveRequest, ZoneMoveResult}; /// `card_value::mana_role` -> mulligan `keep_tier`, for a reason unrelated to /// manabase development. /// -/// CR 605.3: Mana abilities produce mana and resolve immediately without using +/// CR 605.3b: Mana abilities produce mana and resolve immediately without using /// the stack. /// CR 605.1a: A mana ability cannot have targets. `Effect::Mana` carries a /// `ManaTargetRole` naming its recipient and/or count-source player targets; @@ -5320,10 +5320,10 @@ mod tests { /// V9c — the boundary covers the replacement family, the emblem, and the /// token's granted abilities. Each REGISTERS something rather than moving a - /// card during this resolution: CR 603.3 primary (a replacement applying to + /// card during this resolution: CR 614.1 primary (a replacement applying to /// a later event or to another object is NOT a self-replacement effect under - /// CR 614.15, so CR 605.1a's carve-out does not reach it; CR 614.1 - /// secondary), CR 114.1 for the emblem, CR 111.1 for the token, CR 611.2 for + /// CR 614.15, so CR 605.1a's carve-out does not reach it), CR 114.1 for the + /// emblem, CR 111.1 for the token, CR 611.2 for /// a granted continuous effect. #[test] fn replacement_emblem_and_token_payloads_are_not_this_abilitys_effect() { diff --git a/crates/engine/src/types/ability.rs b/crates/engine/src/types/ability.rs index aee8c860f1..72d35223d3 100644 --- a/crates/engine/src/types/ability.rs +++ b/crates/engine/src/types/ability.rs @@ -9280,7 +9280,8 @@ impl AbilityCost { | AbilityCost::ManaDynamic { .. } | AbilityCost::Waterbend { .. } | AbilityCost::PayEnergy { .. } - // CR 702.179f: speed is a player designation, not a card. + // CR 702.179b: speed is a numeric value a rule or effect sets on a + // PLAYER ("players do not have speed until..."), never a card. | AbilityCost::PaySpeed { .. } // CR 119.4: paying life moves no card. | AbilityCost::PayLife { .. } @@ -9322,7 +9323,8 @@ impl AbilityCost { // CR 702.49a: ninjutsu returns an unblocked attacker to its owner's // hand and puts the ninja onto the battlefield from hand. | AbilityCost::NinjutsuFamily { .. } - // CR 118.9: a borrowed keyword mana cost is still a mana cost. + // CR 118.9: a keyword ALTERNATIVE cost is paid "rather than paying + // the spell's mana cost"; either way no card changes zones. | AbilityCost::KeywordCostOfCastSpell { .. } // The parser could not classify the cost fragment. Asserting `true` // would strip mana-ability status on a guess; `false` is the honest @@ -15779,7 +15781,9 @@ impl Effect { // CR 728.1: a player with rad counters "mills a number of cards equal // to the number of rad counters they have". | Effect::ProcessRadCounters - // CR 701.13a: exiles from the top of a player's library. + // The variant's own semantics: exiles from the TOP OF A LIBRARY. No + // CR is cited because CR 701.13a ("move it to the exile zone from + // wherever it is") names no source zone — cf. the `ExileTop` arm. | Effect::ExileFromTopUntil { .. } // Reveal-until. TWO clauses, TWO authorities — do not collapse them. // CR 701.20a is the authority for the REVEAL LOOP ONLY; per CR 701.20b @@ -15792,7 +15796,11 @@ impl Effect { // CR 702.85a: cascade exiles from the top of your library, then // bottoms the cards not cast. | Effect::Cascade - // CR 702.60a: ripple reveals the top N and "put the rest on the bottom". + // CR 702.60a + CR 601.2a: ripple may CAST revealed cards, moving them + // library -> stack. It is the CASTING leg that carries this `true` — + // the reveal (CR 701.20b) and the bottoming (same library, cf. the + // `Scry` arm) are both non-moves on their own, so do not "correct" + // this to `false` by reading only those two clauses. | Effect::Ripple { .. } // Destination is a library position. | Effect::PutAtLibraryPosition { .. } @@ -15810,7 +15818,9 @@ impl Effect { | Effect::Seek { .. } => true, // `Effect::Manifest` is TRUE because the ENGINE's manifest is always - // top-of-library (all 8 shipping cards; Ghastly Conscription's + // top-of-library (all 24 `"type":"Manifest"` nodes in + // `data/card-data.json`, each "manifest the top card of your + // library"; Ghastly Conscription's // manifest-from-a-graveyard-pile routes to `ChangeZoneAll` instead), // NOT because CR 701.40a says so — CR 701.40a names no source zone // ("Put that card onto the battlefield face down"), and CR 701.40e @@ -16033,10 +16043,12 @@ impl Effect { | Effect::RevealHand { .. } | Effect::RevealFromHand { .. } => false, - // CR 400.11: "Outside the game is not a zone" — so a sideboard/wish - // search moves nothing to or from a library. CR 701.23j covers the - // outside-the-game search itself. - Effect::SearchOutsideGame { .. } => false, + // CR 400.11: "Outside the game is not a zone" — so the ORIGIN half + // never touches a library. CR 701.23j covers the outside-the-game + // search itself. The destination is still read, because a + // `Zone::Library` destination WOULD be a move *to* a library; every + // one of the 11 shipping nodes is `Hand` today. + Effect::SearchOutsideGame { destination, .. } => *destination == Zone::Library, // CR 901.4: "All plane and phenomenon cards remain in the COMMAND ZONE // throughout the game, both while they're part of a planar deck and @@ -16152,7 +16164,7 @@ impl Effect { // gated by `ResolutionScope` in `ability_visit`, not here. // // CR 603.3: `Effect::Mana`'s `grants` carry a `TriggerOnSpend` - // reflexive rider that fires when the mana is LATER spent — a separate + // rider that fires when the mana is LATER spent — a separate // triggered ability (Gilanra). `AddKeywordUntilEndOfTurn` is a CR 611.2 // continuous effect on another object; `CantBeCountered` is a fieldless // leaf. Deliberately NOT descended — see `ability_visit`'s leaf arm. @@ -16160,7 +16172,7 @@ impl Effect { // CR 603.7a: a delayed triggered ability, created now, resolving later // as its own ability (CR 603.3). | Effect::CreateDelayedTrigger { .. } - // CR 603.3 primary / CR 614.1 secondary: these register a replacement + // CR 614.1 primary / CR 614.15 secondary: these register a replacement // that applies to a later event or to another object, so none is a // self-replacement effect (CR 614.15) and CR 605.1a's carve-out does // not reach them. @@ -16199,7 +16211,8 @@ impl Effect { // no card changes zones — that point carries no CR, because CR 707.2 // addresses neither pools nor libraries. Effect::CreateTokenCopyFromPool { .. } => false, - // CR 701.42: meld moves the two cards from exile to the battlefield. + // CR 701.42a: meld puts the pair onto the battlefield; no library + // endpoint at either end. Effect::Meld { .. } => false, // CR 702.55a: haunt exiles from a graveyard. Effect::ExileHaunting { .. } => false, diff --git a/crates/engine/src/types/ability_visit.rs b/crates/engine/src/types/ability_visit.rs index 21811f3dd7..ee19a9943f 100644 --- a/crates/engine/src/types/ability_visit.rs +++ b/crates/engine/src/types/ability_visit.rs @@ -200,14 +200,31 @@ where /// they are structurally invisible to any effect-shaped visitor. This is a /// type-level gap, not a missing match arm. /// -/// Yields every `AbilityCost` in the same own-resolution tree the effect walk -/// covers: the node's `cost` (CR 602.1a — the activation cost, at the root), +/// Yields every `AbilityCost` on the CHAIN-LINK axis of the own-resolution +/// tree: the node's `cost` (CR 602.1a — the activation cost, at the root), /// the node's `unless_pay.cost` (CR 118.12a -> CR 118.12 — a cost paid when the /// ability RESOLVES, therefore reached under CR 608.2c as part of "its effect"), /// and every `sub_ability` / `else_ability` / `mode_abilities` link, recursing /// through the SAME `scope_prunes_nested_ability` authority so the CR 603.12 /// reflexive boundary holds identically on both axes. /// +/// **KNOWN GAP — inline branch carriers are NOT descended on the cost axis.** +/// `visit_effect_scoped` descends nested `AbilityDefinition`s under +/// `Vote.per_choice_effect` / `VoteSubject.outcome_template`, +/// `SeparateIntoPiles`, `RevealFromHand.on_decline`, `FlipCoin` / `FlipCoins`, +/// `FlipCoinUntilLose`, `RollDie.results`, and `ChooseOneOf.branches` (all via +/// `visit_nested_ability_def_scoped`); this walk does not. A cost sitting on one +/// of those nested defs — e.g. `ChooseOneOf { branches: [AbilityDefinition { +/// cost: AbilityCost::Mill { .. }, .. }] }` — is therefore invisible here, which +/// would leave CR 605.1a's cost criterion unapplied to it. The same is true of +/// `Effect::PayCost`, whose cost is reached only by the one-node delegation in +/// `Effect::moves_card_to_or_from_library`'s `PayCost` arm. **Unreachable +/// today:** `data/card-data.json` carries zero costs of any type on an +/// effect-payload-nested `AbilityDefinition`. Closing it means driving both +/// walks from one shared carrier→nested-def list; that widens cost coverage for +/// every existing `IncludeRegisteredLater` caller too, so it needs its own +/// census rather than riding along here. +/// /// Only TOP-LEVEL cost nodes are yielded; composition (`Composite`, `OneOf`, /// `PerCounter`) is the consuming predicate's own recursion to do. A second /// wildcard-free `AbilityCost` match in this module would be a drift hazard @@ -563,8 +580,11 @@ where // (Words of Worship/Wilding). Walk it so any conjure name it carries is // surfaced (GainLife/Token carry none today, but it is a nested carrier). // - // BOUNDARY CARRIER (CR 603.3 primary / CR 614.1 secondary): this - // registers a replacement that applies to a LATER event. It is therefore + // BOUNDARY CARRIER (CR 614.1 primary / CR 614.15 secondary): this + // registers a replacement that applies to a LATER event. CR 614.1: + // replacement effects "watch for a particular event that would happen" + // and "aren't locked in ahead of time" — they never trigger and never + // use the stack, so CR 603.3 is NOT the authority here. It is therefore // NOT a self-replacement effect (CR 614.15, which scopes those to an // effect of a resolving spell or ability replacing "that spell or // ability's own effect(s)"), so CR 605.1a's closing carve-out does not @@ -579,7 +599,7 @@ where // surfaced (ChaosEnsues carries none today, but it is a nested carrier). // // BOUNDARY CARRIER — same reason as `CreateDrawReplacement` above: - // CR 603.3 primary, CR 614.1 secondary, not a CR 614.15 self-replacement. + // CR 614.1 primary, not a CR 614.15 self-replacement. Effect::CreatePlaneswalkReplacement { replacement_effect } => { if scope == ResolutionScope::IncludeRegisteredLater { visit_effect_scoped(replacement_effect, scope, visit)? @@ -705,7 +725,7 @@ where } // Carries a nested ReplacementDefinition whose execute/decline/cost may conjure. // - // BOUNDARY CARRIER (CR 603.3 primary / CR 614.1 secondary): registers a + // BOUNDARY CARRIER (CR 614.1 primary / CR 614.15 secondary): registers a // replacement that applies to a later event AND to another object, so it // is not a CR 614.15 self-replacement effect of this ability and falls // outside CR 605.1a's carve-out. diff --git a/crates/engine/tests/integration/cr605_1a_library_criterion.rs b/crates/engine/tests/integration/cr605_1a_library_criterion.rs index 3e5a0ec6ad..a599bac8a2 100644 --- a/crates/engine/tests/integration/cr605_1a_library_criterion.rs +++ b/crates/engine/tests/integration/cr605_1a_library_criterion.rs @@ -101,9 +101,12 @@ fn chromatic_sphere_produces_mana_and_draws_through_the_stack() { } /// **V14b** — a reclassified ability drops out of the instant-speed auto-tap -/// pool, because CR 605.3a only lets a player activate a **mana ability** while -/// casting or paying. An affordance a player cannot legally use would be a UI -/// lie, so the payment picker must stop offering it. +/// pool, because CR 605.3a grants a player permission to activate a **mana +/// ability** mid-cast or mid-payment. That permission is an exception; once an +/// ability stops being a mana ability the exception no longer covers it and the +/// general priority rule (CR 117.1b) governs again. An affordance a player +/// cannot legally use would be a UI lie, so the payment picker must stop +/// offering it. /// /// The paired reach-guard is a Llanowar Elves on the same battlefield: it is /// still a mana ability, so it must still be offered. Without that pair the @@ -145,8 +148,9 @@ fn chromatic_sphere_is_not_offered_as_an_instant_speed_mana_source() { ); assert!( !mentions(sphere), - "Chromatic Sphere is no longer a mana ability (CR 605.1a) so CR 605.3a \ - forbids activating it during a payment window (actions: {actions:?})" + "Chromatic Sphere is no longer a mana ability (CR 605.1a), so CR 605.3a's \ + permission no longer covers it and the general priority rule governs — \ + it must not be offered in a payment window (actions: {actions:?})" ); }