From ce79e18c18a7d452033d0ee4bcd4bc40158a8495 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 08:55:02 -0700 Subject: [PATCH 1/7] fix(engine): gather copy-granted static abilities in the pass that copied them CR 613.2a + CR 613.2c. Layer 1a applies copy effects, and after all of layer 1 has been applied the object's characteristics ARE its copiable values. A static ability a permanent has only because a copy effect gave it is therefore part of the board that layers 2-7 of that SAME pass must be derived from. `evaluate_layers` already gathers the pass's continuous effects below the copy application (Step 3 sits after Step 2), but the generator index that decides which permanents get scanned for statics is built at the TOP of the pass, from the just-reset BASE definitions -- which predate the copy. An Embalm token's base is Vizier of Many Faces': a 0/0 Clone carrying no static of its own. A token copying a lord was therefore scanned as a permanent with no ability, and stayed that way on every later pass too, because each one resets to that same base first. Under full evaluation the copy's anthem never applied at all. The sticker branch immediately below had this exact problem and shipped this exact remedy -- "a sticker can turn a non-generator into a continuous static source mid-pass" is the same sentence with a different layer-1 producer. The copy case now shares that guard instead of growing a second one, so the top-of-pass rebuild contract is unchanged. The copy side is narrowed by payload, not by "did layer 1a run". `ContinuousModification::CopyValues` carries the whole copiable static set in the modification, and `apply_copiable_values` assigns that set wholesale, so `copy_grants_continuous_static` mirrors the index's own generator predicate (any `StaticMode::Continuous` def) exactly -- no id plumbing out of the apply path and no battlefield rescan. A clone of a vanilla creature is a permanent-duration TCE that outlives the clone, so a coarse "any copy ran" guard would have charged every later pass on that board a second whole- battlefield rebuild, inside a module whose entire stated purpose is keeping per-flush work off `|battlefield|`. The remaining copy-layer modifications either provably write no static definitions (`SetName`, `CopyChosen`, `RetainPrintedTriggerFromSource`) or read the live source instead of a payload (`RetainPrintedAbilityFromSource`, `RetainAllOtherAbilitiesFromSource`) and are answered conservatively; over-including is the safe direction under the index's stated doctrine. The rebuild sits below the layer-1b face-down reseed rather than above it, so it observes the whole of layer 1 -- the invariant CR 613.2c actually states. The other order is inert today only because 1b just clears statics, a non-local fact rather than the rule cited at the seam. `apply_layers_incremental` is deliberately left alone, and now says why. A copy applied there could only add a generator by landing on a recipient, and it cannot: `recipient_ids` is `entered_ids` alone, because the one way a pre-existing host joins the set -- an attached entrant -- is rejected outright by `entered_object_blocks_incremental` guard (3); and a copy TCE naming a fresh entrant can only have been installed by that same entry, which called `layers_dirty.mark_full()` and so made this flush a full pass. An earlier draft of this change carried the disjunct into that arm for symmetry. That was wrong twice over: it guards nothing reachable, and because the arm applies layers 2-7 only to `recipient_ids`, the granted static would fan out over the recipients alone while the full pass derives it board-wide -- a different wrong answer rather than agreement. A comment at the seam is the honest artifact; an untestable guard is not. Found by the differential harness on this branch's sibling: a copy of Painter's Servant granted its chosen color under the entry-incremental arm -- which resets only the entering objects, so the copy's post-layer-1 static was still live at gather time -- and granted nothing under the full arm. The incremental arm was the correct one. The new test copies a lord rather than the harness's Painter's Servant because an anthem needs no as-enters choice, so the assertion is over P/T alone. It reuses the existing Embalm scaffolding in the same file. Verification: `cargo fmt --all -- --check` clean; `cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings` clean; `./scripts/check-parser-combinators.sh upstream/main` Gate A PASS, Gate G PASS; `cargo test-all` green apart from the pre-existing `mtgish-import` set this branch does not touch, zero engine failures. Revert-check ran and discriminates: dropping `copy_added_generator ||` from the guard fails the new test with `left: (Some(3), Some(3)) right: (Some(4), Some(4))` -- only the ORIGINAL lord's anthem applied -- while the two pre-existing tests in the file stay green, which is why the module's existing coverage never caught this. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/layers.rs | 101 ++++++++++- ...er_of_many_faces_embalm_copy_panic_5278.rs | 158 ++++++++++++++++++ 2 files changed, 251 insertions(+), 8 deletions(-) diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 185747d46a..2f8692783c 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -2131,14 +2131,18 @@ pub fn evaluate_layers(state: &mut GameState) { &mut started_effect_sets, ); } - if crate::game::stickers::apply_battlefield_name_and_ability_stickers(state, &bf_ids) { - // Sticker ability text is appended after the top-of-pass reset/copy - // application, so a sticker can turn a non-generator into a continuous - // static source mid-pass. Refresh the generator index before the main - // gather so those sticker-granted statics participate in this pass - // without broadening the non-sticker top-of-pass rebuild contract. - crate::types::game_state::StaticSourceIndex::rebuild_from_state(state); - } + let stickers_applied = + crate::game::stickers::apply_battlefield_name_and_ability_stickers(state, &bf_ids); + // Narrowed by payload rather than by "did layer 1a run": + // `CopyValues` carries the whole copiable static set in the modification, so + // `copy_grants_continuous_static` answers exactly, with no plumbing and no + // battlefield rescan. A clone of a vanilla creature — a permanent-duration + // TCE that outlives the clone — therefore costs no second rebuild per pass, + // which is the whole point of an index whose stated job is keeping per-flush + // work off `|battlefield|`. + let copy_added_generator = ordered_copy + .iter() + .any(|effect| copy_grants_continuous_static(&effect.modification)); // CR 613.2b + CR 708.2a + CR 708.10: Layer 1b. After Layer-1a copiable effects // (copy per CR 707, merge per CR 730) are applied, re-set each face-down @@ -2152,6 +2156,31 @@ pub fn evaluate_layers(state: &mut GameState) { } } + // Both producers say the same thing: layer 1 can turn a non-generator into a + // continuous static source mid-pass, and the top-of-pass index was built from + // the just-reset BASE definitions, which predate that. Refresh the generator + // index before the main gather so those statics participate in this pass, + // without broadening the top-of-pass rebuild contract for boards that have + // neither. + // + // CR 613.2a + CR 613.2c: a copy effect can hand a permanent a static ability + // its copiable base does not carry (an embalm token copying a Painter's + // Servant). After all of layer 1 has been applied the object's characteristics + // ARE its copiable values, so a static granted there must generate effects for + // layers 2-7 of THIS pass, not the next one. + // + // Sticker ability text is likewise appended after the top-of-pass reset/copy + // application. + // + // Placed below the 1b face-down reseed so the rebuild observes the whole of + // layer 1, which is the invariant CR 613.2c actually states. 1b only ever + // clears statics (`apply_face_down_creature_characteristics` clears live and + // base alike), so ordering the two the other way is inert today — but only via + // that non-local fact, not via the rule cited here. + if copy_added_generator || stickers_applied { + crate::types::game_state::StaticSourceIndex::rebuild_from_state(state); + } + // Step 3: Gather active continuous effects after layer 1 is applied. let mut effects_by_layer = gather_active_continuous_effects(state); crate::game::stickers::append_battlefield_pt_sticker_effects(state, &mut effects_by_layer); @@ -3810,6 +3839,51 @@ fn effect_is_restricted_to_incremental_recipients( } } +/// CR 613.2a + CR 613.2c: does applying this layer-1 copy modification hand its +/// recipient a continuous static ability — i.e. can it turn a non-generator into a +/// `StaticSourceIndex` generator mid-pass? +/// +/// Mirrors the index's own classification predicate +/// (`static_source_index::object_sources_continuous_effect`): a generator is an +/// object carrying a `StaticMode::Continuous` def. `CopyValues` carries its whole +/// copiable static set in the modification payload and `apply_copiable_values` +/// assigns that set wholesale, so the answer is exact — no id plumbing out of the +/// apply path, and no battlefield rescan. The other copy-layer modifications +/// either provably write no static definitions or read the live source instead of +/// a payload; those are answered conservatively, over-including in the direction +/// the index's own doctrine calls safe (over-include, never under-include). +fn copy_grants_continuous_static(modification: &ContinuousModification) -> bool { + match modification { + ContinuousModification::CopyValues { values, .. } => values + .static_definitions + .iter() + .any(|def| def.mode == StaticMode::Continuous), + // CR 707.9b: name-only override; never writes `static_definitions`. + ContinuousModification::SetName { .. } => false, + // CR 707.2c: parse-time marker whose apply arm is an explicit no-op. The + // real copy is the latched `CopyValues` TCE, which sits in this same + // bucket and is classified on its own payload. + ContinuousModification::CopyChosen => false, + // CR 707.9a: trigger-set retention only. + ContinuousModification::RetainPrintedTriggerFromSource { .. } => false, + // CR 707.9a: these read the live source rather than a payload, so the + // retained set is not inspectable from the modification. + ContinuousModification::RetainPrintedAbilityFromSource { .. } + | ContinuousModification::RetainAllOtherAbilitiesFromSource => true, + // Not a copy-layer modification. Its caller filters to `Layer::Copy` + // first, so this is unreachable by construction; it answers in the safe + // direction rather than silently claiming "adds no generator". + other => { + debug_assert_eq!( + other.layer(), + Layer::Copy, + "copy_grants_continuous_static is defined only for layer-1 copy modifications" + ); + true + } + } +} + /// Incremental layer re-derivation for a set of freshly-entered objects. /// /// Mirrors the PER-OBJECT subset of `evaluate_layers` for `entered_ids` only: @@ -3857,6 +3931,17 @@ fn apply_layers_incremental(state: &mut GameState, prepared: PreparedIncremental let recipient_vec: Vec = recipient_ids.iter().copied().collect(); let stickers_changed = crate::game::stickers::apply_battlefield_name_and_ability_stickers(state, &recipient_vec); + // CR 613.2a + CR 613.2c: deliberately no copy disjunct on this rebuild, unlike + // the full pass. A copy applied here could only add a generator by landing on + // a recipient, and it cannot: `recipient_ids` is `entered_ids` alone, because + // an attached entrant — the one way a pre-existing host joins the set — is + // rejected outright by `entered_object_blocks_incremental` guard (3); and a + // copy TCE naming a fresh entrant can only have been installed by that same + // entry, which called `layers_dirty.mark_full()` (`game_state.rs`, + // `apply_resolved_continuous_effect`) and so made this flush a full pass. A + // rebuild here would guard nothing reachable, and the version of this guard + // that fans a granted static over `recipient_ids` alone would not even agree + // with the full pass, which derives it board-wide. let active_effects = if stickers_changed { // Incremental resets clear the entered/attached recipients back to base, // so retained stickers must be re-applied before the restricted main diff --git a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs index fe55e047c5..dc6321e2a4 100644 --- a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs +++ b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs @@ -118,6 +118,55 @@ fn build_scenario() -> ( (runner, vizier, bear) } +/// A lord whose ONLY ability is a board-wide static. Copying it is what hands +/// the Embalm token a static ability that its copiable base — a 0/0 Clone with +/// no static of its own — does not carry. +const LORD_ORACLE: &str = "Other creatures you control get +1/+1."; + +/// Same shape as [`build_scenario`], except the creature the token will copy is +/// a lord and a third, vanilla creature is present to receive the anthem. +/// Returns `(runner, vizier, lord, vanilla)`. +fn build_lord_scenario() -> ( + GameRunner, + engine::types::identifiers::ObjectId, + engine::types::identifiers::ObjectId, + engine::types::identifiers::ObjectId, +) { + let mut scenario = GameScenario::new(); + scenario.at_phase(Phase::PreCombatMain); + + let vizier = scenario + .add_creature_to_graveyard(P0, "Vizier of Many Faces", 0, 0) + .with_mana_cost(engine::types::mana::ManaCost::Cost { + generic: 3, + shards: vec![engine::types::mana::ManaCostShard::Blue], + }) + .from_oracle_text_with_keywords(&["Embalm"], VIZIER_ORACLE) + .id(); + + // 1/1 lord and a 2/2 vanilla: the two anthem instances (original + copy) + // are each worth +1/+1, so the vanilla's toughness alone distinguishes + // "one anthem applied" (3/3) from "both applied" (4/4). + let lord = scenario + .add_creature(P0, "Bear Umbra Lord", 1, 1) + .from_oracle_text(LORD_ORACLE) + .id(); + let vanilla = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); + + let mut runner = scenario.build(); + add_mana( + &mut runner, + &[ + ManaType::Blue, + ManaType::Blue, + ManaType::Colorless, + ManaType::Colorless, + ManaType::Colorless, + ], + ); + (runner, vizier, lord, vanilla) +} + /// Activate the Embalm ability and drain any mana-payment prompt so the token /// is created and parked on its first entry-choice prompt. fn activate_embalm(runner: &mut GameRunner, vizier: engine::types::identifiers::ObjectId) { @@ -325,3 +374,112 @@ fn embalm_copy_declined_enters_as_zero_zero_and_dies() { "a declined 0/0 Embalm copy of Vizier must die to SBA, not remain on the battlefield" ); } + +/// CR 613.2a + CR 613.2c: layer 1a applies copy effects, and after all of layer 1 +/// has been applied the object's characteristics ARE its copiable values. A static +/// ability a permanent has ONLY because of a copy effect is therefore part of the +/// board that layers 2–7 of that SAME pass must be derived from — it does not wait +/// for the next evaluation. +/// +/// The generator index that decides which permanents are scanned for statics is +/// built at the top of the pass, from the just-reset BASE definitions. An Embalm +/// token's base is Vizier's (a 0/0 Clone with no static at all), so before the fix +/// the copied anthem was invisible to the very pass that applied the copy, and it +/// stayed invisible on every later pass too — each one resets to that same base +/// first. The engine therefore ran a copy of a lord as a permanent with no ability. +/// +/// REVERT-PROBE (discriminating, RUN): drop `copy_added_generator ||` from the +/// post-layer-1 `StaticSourceIndex::rebuild_from_state` guard in `evaluate_layers` +/// ⇒ the vanilla creature holds at 3/3 (only the ORIGINAL lord's anthem applies) +/// and this test fails on both P/T assertions, while the two tests above stay +/// green (neither copies a permanent that carries a static ability). +#[test] +fn a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy() { + let (mut runner, vizier, lord, vanilla) = build_lord_scenario(); + activate_embalm(&mut runner, vizier); + + let token = 'drive: loop { + match runner.state().waiting_for.clone() { + WaitingFor::ReplacementChoice { .. } => { + runner + .act(GameAction::ChooseReplacement { index: 0 }) + .expect("accept enter-as-copy replacement"); + } + WaitingFor::CopyTargetChoice { + source_id, + valid_targets, + .. + } => { + assert!( + valid_targets.contains(&lord), + "the lord must be a legal copy target, got {valid_targets:?}" + ); + runner + .act(GameAction::ChooseTarget { + target: Some(TargetRef::Object(lord)), + }) + .expect("choose copy target (the lord)"); + break 'drive source_id; + } + WaitingFor::Priority { .. } => { + runner.act(GameAction::PassPriority).expect("pass priority"); + } + other => panic!("unexpected waiting_for during entry: {other:?}"), + } + }; + + for _ in 0..16 { + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } if runner.state().stack.is_empty() => break, + WaitingFor::Priority { .. } => { + runner.act(GameAction::PassPriority).expect("pass priority"); + } + _ => break, + } + } + + // POSITIVE reach-guards: the copy really happened AND it really is the copy + // that put a static ability on the token, so the P/T assertions below cannot + // pass for the wrong reason (a token that copied nothing has no anthem to + // contribute, and a token whose base already carried the anthem would not + // exercise the layer-1 binding at all). + let token_obj = runner + .state() + .objects + .get(&token) + .expect("the Embalm token must be on the battlefield"); + assert_eq!( + token_obj.name, "Bear Umbra Lord", + "the token must be a copy of the chosen lord" + ); + assert!( + !token_obj.static_definitions.is_empty(), + "the copy must have given the token the lord's static ability" + ); + assert!( + token_obj.base_static_definitions.is_empty(), + "the token's own copiable base must carry no static, or its anthem would not \ + depend on the layer-1 copy effect and this test would stop exercising the \ + binding it is written for" + ); + + // Two anthems now apply to the vanilla creature: the original lord's and the + // token's copied one. Before the fix only the original was gathered. + assert_eq!( + ( + runner.state().objects[&vanilla].power, + runner.state().objects[&vanilla].toughness + ), + (Some(4), Some(4)), + "the vanilla 2/2 must get +1/+1 from BOTH the original lord and its copy" + ); + assert_eq!( + ( + runner.state().objects[&lord].power, + runner.state().objects[&lord].toughness + ), + (Some(2), Some(2)), + "the original 1/1 lord must get +1/+1 from the token's copied anthem \ + (\"other creatures\", so its own anthem does not pump it)" + ); +} From 563b67b317677185b008ce5b40bfdacab6b19867 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 13:35:25 -0700 Subject: [PATCH 2/7] fix(engine): reach a layer-1 fixed point for copy-granted copy abilities CR 613.2a + CR 613.2c: applying a copy effect can hand its recipient a static ability that itself generates a layer-1 (copy) continuous effect, and CR 613.2c makes the characteristics standing after layer 1 the object's copiable values -- so that second-generation effect belongs to the SAME layer 1, not to the next pass. Sublayer 1a now re-gathers and re-applies to a fixed point (`apply_copy_sublayer_to_fixed_point`), rebuilding `StaticSourceIndex` between generations and bounded by `MAX_COPY_SUBLAYER_GENERATIONS` so a mutually-copying pair cannot spin. Each generation re-orders and re-applies the WHOLE started set (CR 613.2 timestamp order + CR 613.8a dependency order), because `depends_on` sorts every `CopyValues` ahead of other layer-1 modifications; the `Layer::Copy` attribution bucket is cleared first so a re-applied effect is not listed twice. A board with no copy-granted copy ability pays exactly today's cost. The incremental arm cannot absorb the same state. It applies effects only to `recipient_ids`, so a board-wide static a recipient acquires from a copy would never reach the pre-existing objects it must apply to -- the state `entered_object_blocks_incremental` already escalates for, reached one step later. And a PRE-EXISTING copy effect reaches a fresh entrant by FILTER MATCH, since `apply_continuous_effect_to` narrows an ordinary board-wide `affected_filter` to the recipients rather than requiring `TargetFilter::SpecificObject`; neither the magnitude/population classifier nor the source-side guard sees that case. `prepare_incremental_flush` now escalates whenever an active copy effect's payload grants a continuous static, before any copy is applied, which also keeps the generator set of the index it just built true for the whole flush. Also: - `copy_grants_continuous_static` enumerates the six `Layer::Copy` variants explicitly instead of an `other =>` catch-all that called `other.layer()`. Six of that method's arms are `unreachable!()` panics (`AddCounterOnEnter`, `SetStartingLoyalty`, `RemoveManaCost`, and the three combat-assignment variants), so the `debug_assert_eq!` meant to make the arm safe could abort inside itself. - CR 707.9a: `RetainPrintedAbilityFromSource` no longer claims to add a generator. Its apply arm pushes one `AbilityDefinition` onto `obj.abilities` and never touches `static_definitions`. Only `RetainAllOtherAbilitiesFromSource`, which merges the source's `base_static_definitions`, still does. - The `CopyValues` payload question routes through the index's own classification predicate, shared as `static_source_index::defs_source_continuous_effect`, so the two answers cannot drift apart. - Bound the `CopyTargetChoice` drive loop in the #5278 integration test (and share it between tests) so an engine change that stops surfacing the prompt fails instead of hanging CI. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/layers.rs | 446 ++++++++++++++++-- crates/engine/src/game/static_source_index.rs | 14 +- ...er_of_many_faces_embalm_copy_panic_5278.rs | 196 ++++++-- 3 files changed, 562 insertions(+), 94 deletions(-) diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 2f8692783c..0c5f684506 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -2120,19 +2120,6 @@ pub fn evaluate_layers(state: &mut GameState) { // Step 2: Apply copy effects first so copied static abilities exist before later layers. let mut zone_cache = LayerZoneObjectCache::default(); let mut started_effect_sets = StartedContinuousEffectSets::new(); - let copy_effects = gather_active_effects_for_layer(state, Layer::Copy); - let ordered_copy = order_active_continuous_effects(Layer::Copy, ©_effects, state); - for effect in &ordered_copy { - apply_continuous_effect( - state, - effect, - &mut abilities_suppressed, - &mut zone_cache, - &mut started_effect_sets, - ); - } - let stickers_applied = - crate::game::stickers::apply_battlefield_name_and_ability_stickers(state, &bf_ids); // Narrowed by payload rather than by "did layer 1a run": // `CopyValues` carries the whole copiable static set in the modification, so // `copy_grants_continuous_static` answers exactly, with no plumbing and no @@ -2140,9 +2127,14 @@ pub fn evaluate_layers(state: &mut GameState) { // TCE that outlives the clone — therefore costs no second rebuild per pass, // which is the whole point of an index whose stated job is keeping per-flush // work off `|battlefield|`. - let copy_added_generator = ordered_copy - .iter() - .any(|effect| copy_grants_continuous_static(&effect.modification)); + let copy_added_generator = apply_copy_sublayer_to_fixed_point( + state, + &mut abilities_suppressed, + &mut zone_cache, + &mut started_effect_sets, + ); + let stickers_applied = + crate::game::stickers::apply_battlefield_name_and_ability_stickers(state, &bf_ids); // CR 613.2b + CR 708.2a + CR 708.10: Layer 1b. After Layer-1a copiable effects // (copy per CR 707, merge per CR 730) are applied, re-set each face-down @@ -2177,6 +2169,13 @@ pub fn evaluate_layers(state: &mut GameState) { // clears statics (`apply_face_down_creature_characteristics` clears live and // base alike), so ordering the two the other way is inert today — but only via // that non-local fact, not via the rule cited here. + // + // `apply_copy_sublayer_to_fixed_point` already rebuilt mid-1a whenever a + // generation of copies added a generator, but that rebuild is scoped to 1a's + // own discovery loop and predates 1b by construction. This one is the layer-1 + // exit rebuild the CR 613.2c invariant asks for; on the rare board that pays + // both it is one extra O(battlefield) walk, the same order as the Step-1 reset + // that already runs unconditionally. if copy_added_generator || stickers_applied { crate::types::game_state::StaticSourceIndex::rebuild_from_state(state); } @@ -3500,6 +3499,34 @@ fn prepare_incremental_flush( { return None; } + // CR 613.2a + CR 613.2c: a copy effect applied on this path can hand a + // recipient a continuous static ability its copiable base does not carry, and + // after layer 1 that static is part of the board layers 2-7 derive from. + // + // The incremental arm cannot absorb that. `entered_object_blocks_incremental` + // (1) escalates for an entrant that arrives sourcing a continuous effect, + // precisely because effects here are re-applied only to `recipient_ids` — + // pre-existing permanents are never reset, so a board-wide anthem a recipient + // acquires would never reach the objects it must apply to. A copy that grants + // a static reaches the same state one step later, so it takes the same exit. + // Escalating BEFORE any copy is applied also keeps the generator set of the + // index built above true for the whole of `apply_layers_incremental`. + // + // Reached by filter match, not only by name: the copy set below is applied + // with `apply_continuous_effect_to(state, effect, &recipient_ids, ..)`, which + // restricts an ordinary board-wide `affected_filter` to the recipients rather + // than requiring `TargetFilter::SpecificObject`. Neither + // `active_effects_force_incremental_escalation` (magnitude / affected-set + // population sensitivity) nor the source-side guard above sees that case: the + // copy's source is a pre-existing permanent and its filter counts nothing. + // + // O(active-effect-count), and zero on the overwhelmingly common board with no + // copy effect at all. + if active_effects.iter().any(|effect| { + effect.layer == Layer::Copy && copy_grants_continuous_static(&effect.modification) + }) { + return None; + } Some(PreparedIncrementalFlush { recipient_ids, @@ -3843,21 +3870,24 @@ fn effect_is_restricted_to_incremental_recipients( /// recipient a continuous static ability — i.e. can it turn a non-generator into a /// `StaticSourceIndex` generator mid-pass? /// -/// Mirrors the index's own classification predicate -/// (`static_source_index::object_sources_continuous_effect`): a generator is an -/// object carrying a `StaticMode::Continuous` def. `CopyValues` carries its whole -/// copiable static set in the modification payload and `apply_copiable_values` -/// assigns that set wholesale, so the answer is exact — no id plumbing out of the -/// apply path, and no battlefield rescan. The other copy-layer modifications -/// either provably write no static definitions or read the live source instead of -/// a payload; those are answered conservatively, over-including in the direction -/// the index's own doctrine calls safe (over-include, never under-include). +/// Shares the index's own classification predicate +/// (`static_source_index::defs_source_continuous_effect`, the body of +/// `object_sources_continuous_effect`): a generator is an object carrying a +/// `StaticMode::Continuous` def. `CopyValues` carries its whole copiable static +/// set in the modification payload and `apply_copiable_values` assigns that set +/// wholesale, so the answer is exact — no id plumbing out of the apply path, and +/// no battlefield rescan. +/// +/// The six variants below are exactly `ContinuousModification::layer()`'s +/// `Layer::Copy` set; every other variant belongs to layers 2-7 and is filtered +/// out by both callers before they ask. fn copy_grants_continuous_static(modification: &ContinuousModification) -> bool { match modification { - ContinuousModification::CopyValues { values, .. } => values - .static_definitions - .iter() - .any(|def| def.mode == StaticMode::Continuous), + ContinuousModification::CopyValues { values, .. } => { + crate::game::static_source_index::defs_source_continuous_effect( + &values.static_definitions, + ) + } // CR 707.9b: name-only override; never writes `static_definitions`. ContinuousModification::SetName { .. } => false, // CR 707.2c: parse-time marker whose apply arm is an explicit no-op. The @@ -3866,22 +3896,149 @@ fn copy_grants_continuous_static(modification: &ContinuousModification) -> bool ContinuousModification::CopyChosen => false, // CR 707.9a: trigger-set retention only. ContinuousModification::RetainPrintedTriggerFromSource { .. } => false, - // CR 707.9a: these read the live source rather than a payload, so the - // retained set is not inspectable from the modification. - ContinuousModification::RetainPrintedAbilityFromSource { .. } - | ContinuousModification::RetainAllOtherAbilitiesFromSource => true, - // Not a copy-layer modification. Its caller filters to `Layer::Copy` - // first, so this is unreachable by construction; it answers in the safe - // direction rather than silently claiming "adds no generator". - other => { - debug_assert_eq!( - other.layer(), - Layer::Copy, - "copy_grants_continuous_static is defined only for layer-1 copy modifications" + // CR 707.9a: single-ability retention. Its apply arm pushes one + // `AbilityDefinition` read from the source's `base_abilities` onto + // `obj.abilities` and touches no other set — `static_definitions` is + // structurally out of its reach, so the Sakashima-class single-ability + // retain provably adds no generator. + ContinuousModification::RetainPrintedAbilityFromSource { .. } => false, + // CR 707.9a: the unbounded retain genuinely merges the source's + // `base_static_definitions` back onto the copy, so it can add a + // generator. Answered `true` without inspecting the retained set, which + // lives on the live source rather than in this payload. + ContinuousModification::RetainAllOtherAbilitiesFromSource => true, + // Layers 2-7. `ContinuousModification` spans all eleven layers in one + // enum, so this arm is required for totality; it is EXACT rather than a + // fallback guess — no modification outside layer 1 writes copied + // `static_definitions`, and both callers filter to `Layer::Copy` first. + // Deliberately does not consult `other.layer()`: six of that method's + // arms are `unreachable!()` panics (`AddCounterOnEnter`, + // `SetStartingLoyalty`, `RemoveManaCost`, and the three + // combat-assignment variants), so the guard that was meant to make this + // arm safe could abort inside itself. + _ => false, + } +} + +/// CR 613.2: how many discovery generations sublayer 1a may run before it stops. +/// +/// Each generation applies every started copy effect and then re-gathers; a copy +/// effect first seen in generation `k` can only exist because a copy applied in +/// generation `k-1` handed its recipient the static that generates it. So the +/// bound is the maximum length of a copy-grants-a-copy-ability chain. No printed +/// card forms a chain longer than two links (a clone of a permanent whose own +/// static is a copy effect), and every generation past the first requires a +/// board that already paid for one; eight leaves generous headroom while keeping +/// the mutually-copying case — two permanents that rewrite each other's static +/// sets and therefore keep producing payload-distinct effects forever — from +/// spinning. `debug_assert` on exhaustion so a real non-convergent board is a +/// test failure rather than a silent truncation; release builds stop +/// deterministically with the last fully-ordered application in place. +const MAX_COPY_SUBLAYER_GENERATIONS: usize = 8; + +/// CR 613.2a + CR 613.2c: apply sublayer 1a to a fixed point. +/// +/// A copy effect can hand a permanent a static ability its copiable base does not +/// carry, and CR 613.2c says that after all of layer 1 has been applied the +/// object's characteristics ARE its copiable values. When that granted static is +/// itself a copy effect, it belongs to THIS sublayer — so gathering layer 1 once +/// is not enough: the sublayer has to be iterated until no new copy effect +/// appears. +/// +/// Ordering is preserved ACROSS generations, not just within one: each generation +/// re-orders and re-applies the whole started set (CR 613.2 timestamp order, +/// CR 613.8a dependency order), never just the newly discovered tail. That +/// matters because `depends_on` sorts every `CopyValues` ahead of the other +/// layer-1 modifications, so a copy discovered late can legitimately need to +/// apply before one already applied. Re-application is safe: `CopyValues` / +/// `SetName` assign wholesale from a payload snapshot, the retains dedupe +/// per-item, `CopyChosen` is a no-op, and `started_effect_sets` hands an +/// already-started effect back its original affected set (CR 613.6). +/// +/// Returns whether any applied copy added a `StaticSourceIndex` generator, which +/// the caller needs for the layer-1 exit rebuild that feeds layers 2-7. +fn apply_copy_sublayer_to_fixed_point( + state: &mut GameState, + abilities_suppressed: &mut HashSet, + zone_cache: &mut LayerZoneObjectCache, + started_effect_sets: &mut StartedContinuousEffectSets, +) -> bool { + let mut started: Vec = Vec::new(); + let mut copy_added_generator = false; + + for _ in 0..MAX_COPY_SUBLAYER_GENERATIONS { + let fresh: Vec = + gather_active_effects_for_layer(state, Layer::Copy) + .into_iter() + .filter(|effect| !started.iter().any(|seen| same_copy_effect(seen, effect))) + .collect(); + if fresh.is_empty() { + // Fixed point: this generation's gather produced nothing the + // previous ones had not already started applying. + return copy_added_generator; + } + + let fresh_added_generator = fresh + .iter() + .any(|effect| copy_grants_continuous_static(&effect.modification)); + copy_added_generator |= fresh_added_generator; + + let reapplying = !started.is_empty(); + started.extend(fresh); + let ordered = order_active_continuous_effects(Layer::Copy, &started, state); + if reapplying { + // 1a re-runs from the top, so its display attribution is re-derived + // from the top too — otherwise a re-applied effect would be listed + // twice, in stale order. Only the Copy bucket exists yet: Step 1 + // cleared the table and no later layer has run. + for (_, attribution) in state.attribution.iter_mut() { + attribution.by_layer.remove(&Layer::Copy); + } + } + for effect in &ordered { + apply_continuous_effect( + state, + effect, + abilities_suppressed, + zone_cache, + started_effect_sets, ); - true } + + if !fresh_added_generator { + // Nothing this generation could add a `StaticSourceIndex` generator, + // so the next gather would read the same index and return the same + // set. Stop without paying the O(battlefield) rebuild. + return copy_added_generator; + } + // The index was built from definitions that predate these copies; refresh + // it so the next gather can see a copy-granted copy ability. + crate::types::game_state::StaticSourceIndex::rebuild_from_state(state); } + + debug_assert!( + false, + "layer 1a did not converge in {MAX_COPY_SUBLAYER_GENERATIONS} generations; \ + mutually-copying permanents rewriting each other's static definitions?" + ); + copy_added_generator +} + +/// CR 613.2a: identity of a layer-1 copy effect for the fixed-point loop. +/// +/// Compares provenance AND payload. Provenance alone is not a stable name across +/// generations: a copy effect rewrites its own recipient's `static_definitions`, +/// so `(source_id, def_index, mod_index)` can denote a DIFFERENT modification in +/// the next generation. Including the payload makes "already started" exact, at +/// the cost of letting a genuinely non-convergent board keep producing new +/// identities — which is what `MAX_COPY_SUBLAYER_GENERATIONS` bounds. +fn same_copy_effect(a: &ActiveContinuousEffect, b: &ActiveContinuousEffect) -> bool { + a.source_id == b.source_id + && a.def_index == b.def_index + && a.transient_id == b.transient_id + && a.mod_index == b.mod_index + && a.timestamp == b.timestamp + && a.modification == b.modification } /// Incremental layer re-derivation for a set of freshly-entered objects. @@ -3938,10 +4095,26 @@ fn apply_layers_incremental(state: &mut GameState, prepared: PreparedIncremental // rejected outright by `entered_object_blocks_incremental` guard (3); and a // copy TCE naming a fresh entrant can only have been installed by that same // entry, which called `layers_dirty.mark_full()` (`game_state.rs`, - // `apply_resolved_continuous_effect`) and so made this flush a full pass. A - // rebuild here would guard nothing reachable, and the version of this guard - // that fans a granted static over `recipient_ids` alone would not even agree - // with the full pass, which derives it board-wide. + // `apply_resolved_continuous_effect`) and so made this flush a full pass. + // + // That argument covers only a copy that NAMES a recipient. A pre-existing copy + // effect also reaches a recipient by plain filter match, because + // `apply_continuous_effect_to` restricts an ordinary board-wide + // `affected_filter` to `recipient_ids` rather than requiring + // `TargetFilter::SpecificObject`. What rules that case out is the escalation + // guard in `prepare_incremental_flush`: any active copy whose payload grants a + // continuous static sends the whole flush to `evaluate_layers`, so no copy + // surviving to this line can turn a recipient into a `StaticSourceIndex` + // generator. The index built there still names exactly the right sources and a + // rebuild would find the identical set. (It would also be the wrong repair + // anyway: a guard that fans a copy-granted static over `recipient_ids` alone + // would not agree with the full pass, which derives it board-wide.) + // + // The re-collect below is not dead weight, though. A copy changes its + // recipients' characteristics, and a pre-existing generator's + // `GrantStaticAbility` fan-out is computed per matching recipient when that + // generator is visited — same generator set, different collected effects. So + // the effect set must be re-read after the copy even though the index need not. let active_effects = if stickers_changed { // Incremental resets clear the entered/attached recipients back to base, // so retained stickers must be re-applied before the restricted main @@ -20433,6 +20606,187 @@ mod tests { ); } + /// CR 613.2a + CR 613.2c: an entry-incremental flush must ESCALATE while a + /// copy effect whose payload carries a continuous static is active. + /// + /// The incremental arm applies every active effect through + /// `apply_continuous_effect_to(state, effect, &recipient_ids, ..)`, which + /// NARROWS an ordinary board-wide `affected_filter` down to the entrants — so + /// a PRE-EXISTING copy effect reaches a fresh entrant by FILTER MATCH, never + /// having to name it with `TargetFilter::SpecificObject`. The entrant would + /// then hold a continuous static (here an anthem) that the incremental arm + /// can never fan out over the pre-existing board, because pre-existing + /// objects are not reset — the same state `entered_object_blocks_incremental` + /// already escalates for, reached one step later. Neither existing guard sees + /// it: the entrant's own BASE carries no static, and the copy's source is a + /// pre-existing permanent whose filter counts nothing. + /// + /// REVERT-PROBE (discriminating, RUN): delete the `Layer::Copy` guard at the + /// end of `prepare_incremental_flush` ⇒ `layers_incremental == 1` and + /// `layers_full_eval == 0`, and this test fails on the branch assertions. + #[test] + fn entry_incremental_escalates_when_a_live_copy_payload_carries_a_static() { + let mut state = setup(); + let player = PlayerId(0); + let template = make_creature(&mut state, "Template", 2, 2, player); + let mut copied_values = intrinsic_copiable_values(&state.objects[&template]); + Arc::make_mut(&mut copied_values.static_definitions).push( + StaticDefinition::continuous() + .affected(TargetFilter::Typed( + TypedFilter::creature().controller(ControllerRef::You), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + // Board-wide `affected`, NOT `SpecificObject`: this is the shape that + // reaches an object which did not exist when the effect was registered. + let caster = make_creature(&mut state, "Caster", 1, 1, player); + state.add_transient_continuous_effect( + caster, + player, + Duration::Permanent, + TargetFilter::Typed(TypedFilter::creature().controller(ControllerRef::You)), + vec![ContinuousModification::CopyValues { + values: Box::new(copied_values), + display_source: crate::game::game_object::DisplaySource::Card, + printed_ref: None, + token_image_ref: None, + }], + None, + ); + evaluate_layers(&mut state); + + let entrant = make_creature(&mut state, "Entrant", 1, 1, player); + assert!( + state.objects[&entrant].base_static_definitions.is_empty(), + "the entrant's own base must carry no static, or the existing \ + `entered_object_blocks_incremental` guard would escalate instead and \ + this test would stop probing the copy guard" + ); + + crate::game::perf_counters::reset(); + state.layers_dirty = LayersDirty::EnteredObjects([entrant].into()); + flush_layers(&mut state); + let counters = crate::game::perf_counters::snapshot(); + + assert_eq!( + counters.layers_full_eval, 1, + "a live copy effect whose payload grants a continuous static must force \ + a full pass" + ); + assert_eq!( + counters.layers_incremental, 0, + "the incremental arm must not run for this entry" + ); + // POSITIVE reach-guard: the copy really did reach the entrant by filter + // match, so the escalation above is not vacuous. + assert!( + !state.objects[&entrant].static_definitions.is_empty(), + "the board-wide copy must have handed the entrant the payload's static" + ); + } + + /// CR 613.2a: `copy_grants_continuous_static` answers over the COMPLETE + /// `Layer::Copy` modification set, and its `_ => false` arm is exact rather + /// than a fallback guess. Every variant here is also asserted to report + /// `Layer::Copy` (and the layers-2-7 sample NOT to), which pins the claim the + /// catch-all rests on. `layer()` itself is deliberately not consulted by the + /// function under test: six of its arms are `unreachable!()` panics, so the + /// `debug_assert_eq!(other.layer(), Layer::Copy)` guard this replaced could + /// abort inside itself — `AddCounterOnEnter` below is one such variant and is + /// answered without ever asking for its layer. + #[test] + fn copy_grants_continuous_static_covers_every_copy_layer_variant() { + let mut state = setup(); + let player = PlayerId(0); + let donor = make_creature(&mut state, "Donor", 2, 2, player); + let plain_values = intrinsic_copiable_values(&state.objects[&donor]); + assert!( + plain_values.static_definitions.is_empty(), + "the vanilla donor must carry no static, or the negative case below \ + would not be a negative case" + ); + let mut anthem_values = plain_values.clone(); + Arc::make_mut(&mut anthem_values.static_definitions).push( + StaticDefinition::continuous() + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + let copy_of = |values: CopiableValues| ContinuousModification::CopyValues { + values: Box::new(values), + display_source: crate::game::game_object::DisplaySource::Card, + printed_ref: None, + token_image_ref: None, + }; + + // The whole `Layer::Copy` set, with the expected answer for each. + let copy_layer_cases = [ + // Payload-exact: the copiable static set travels inside the + // modification, so no live-source read and no battlefield rescan. + (copy_of(anthem_values), true), + (copy_of(plain_values), false), + // CR 707.9a: merges the source's whole printed set, INCLUDING its + // `base_static_definitions`, onto the recipient (see the apply arm). + ( + ContinuousModification::RetainAllOtherAbilitiesFromSource, + true, + ), + // CR 707.9a: pushes ONE `AbilityDefinition` onto `obj.abilities`; + // `static_definitions` is structurally out of reach. + ( + ContinuousModification::RetainPrintedAbilityFromSource { + source_ability_index: 0, + }, + false, + ), + // CR 707.9a: pushes one trigger; likewise never a static. + ( + ContinuousModification::RetainPrintedTriggerFromSource { + source_trigger_index: 0, + }, + false, + ), + // CR 707.9b: name-only override. + ( + ContinuousModification::SetName { + name: "Renamed".into(), + }, + false, + ), + // CR 707.2c: parse-time marker whose apply arm is an explicit no-op. + (ContinuousModification::CopyChosen, false), + ]; + for (modification, expected) in ©_layer_cases { + assert_eq!( + modification.layer(), + Layer::Copy, + "{modification:?} must be a layer-1 modification for this table to be \ + the complete `Layer::Copy` set" + ); + assert_eq!( + copy_grants_continuous_static(modification), + *expected, + "wrong generator answer for {modification:?}" + ); + } + + // Layers 2-7 fall to the catch-all. `AddCounterOnEnter` is one of the six + // variants whose `layer()` is `unreachable!()`, so it is exactly the input + // that made the replaced `debug_assert_eq!(other.layer(), ..)` unsafe. + for modification in [ + ContinuousModification::AddCounterOnEnter { + counter_type: CounterType::Plus1Plus1, + count: QuantityExpr::Fixed { value: 1 }, + if_type: None, + }, + ContinuousModification::AddPower { value: 1 }, + ] { + assert!( + !copy_grants_continuous_static(&modification), + "{modification:?} is not a layer-1 modification and must not claim to \ + add a generator" + ); + } + } + /// Shared reach-guard: run ONLY the guard from a Clean baseline and assert it /// forces a full escalation (the `mark_layers_full` the guarded site would /// otherwise call unconditionally). diff --git a/crates/engine/src/game/static_source_index.rs b/crates/engine/src/game/static_source_index.rs index 95e9ab6094..38ceea223b 100644 --- a/crates/engine/src/game/static_source_index.rs +++ b/crates/engine/src/game/static_source_index.rs @@ -64,9 +64,17 @@ use super::game_object::GameObject; /// That is the soundness foundation — the index over-includes (condition-failing /// statics) but never under-includes a real source. pub(crate) fn object_sources_continuous_effect(obj: &GameObject) -> bool { - obj.static_definitions - .iter_all() - .any(|def| def.mode == StaticMode::Continuous) + defs_source_continuous_effect(obj.static_definitions.as_slice()) +} + +/// The predicate above, over a bare definition slice. Layer 1 has to ask the same +/// question of a set of definitions that is not (yet) on any object — the copiable +/// static set inside a `CopyValues` payload — to decide whether applying that copy +/// turns its recipient into a generator mid-pass (`layers.rs`, +/// `copy_grants_continuous_static`). Sharing one body makes the two answers +/// impossible to drift apart. +pub(crate) fn defs_source_continuous_effect(defs: &[crate::types::StaticDefinition]) -> bool { + defs.iter().any(|def| def.mode == StaticMode::Continuous) } impl StaticSourceIndex { diff --git a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs index dc6321e2a4..f33f393cc3 100644 --- a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs +++ b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs @@ -30,7 +30,10 @@ //! `Duration::Permanent` transient), the accept case panics again. use engine::game::scenario::{GameRunner, GameScenario, P0}; -use engine::types::ability::{ContinuousModification, Effect, TargetRef}; +use engine::types::ability::{ + ContinuousModification, Effect, FilterProp, StaticDefinition, TargetFilter, TargetRef, + TypeFilter, TypedFilter, +}; use engine::types::actions::GameAction; use engine::types::game_state::WaitingFor; use engine::types::mana::{ManaColor, ManaType, ManaUnit}; @@ -131,6 +134,20 @@ fn build_lord_scenario() -> ( engine::types::identifiers::ObjectId, engine::types::identifiers::ObjectId, engine::types::identifiers::ObjectId, +) { + build_lord_scenario_with(None) +} + +/// [`build_lord_scenario`], plus an extra static definition on the lord's PRINTED +/// base — so it is part of the lord's copiable values (CR 707.2) and rides along +/// onto the Embalm token when the token copies the lord. +fn build_lord_scenario_with( + extra_lord_static: Option, +) -> ( + GameRunner, + engine::types::identifiers::ObjectId, + engine::types::identifiers::ObjectId, + engine::types::identifiers::ObjectId, ) { let mut scenario = GameScenario::new(); scenario.at_phase(Phase::PreCombatMain); @@ -147,10 +164,12 @@ fn build_lord_scenario() -> ( // 1/1 lord and a 2/2 vanilla: the two anthem instances (original + copy) // are each worth +1/+1, so the vanilla's toughness alone distinguishes // "one anthem applied" (3/3) from "both applied" (4/4). - let lord = scenario - .add_creature(P0, "Bear Umbra Lord", 1, 1) - .from_oracle_text(LORD_ORACLE) - .id(); + let mut lord_builder = scenario.add_creature(P0, "Bear Umbra Lord", 1, 1); + lord_builder.from_oracle_text(LORD_ORACLE); + if let Some(static_def) = extra_lord_static { + lord_builder.with_static_definition(static_def); + } + let lord = lord_builder.id(); let vanilla = scenario.add_creature(P0, "Grizzly Bears", 2, 2).id(); let mut runner = scenario.build(); @@ -198,6 +217,61 @@ fn activate_embalm(runner: &mut GameRunner, vizier: engine::types::identifiers:: panic!("Embalm activation never reached an entry choice"); } +/// Drive a token parked on its entry choices all the way onto the battlefield as +/// a copy of `copy_target`: accept the enter-as-copy replacement, name the target, +/// then let the stack settle. Returns the token's `ObjectId`. +fn resolve_embalm_copy_of( + runner: &mut GameRunner, + copy_target: engine::types::identifiers::ObjectId, +) -> engine::types::identifiers::ObjectId { + // Bounded, not `loop`: the `Priority` arm acts on every trip, so an engine + // change that stops surfacing `CopyTargetChoice` would spin here forever and + // hang CI instead of failing. Same bound as `activate_embalm`. + let mut chosen = None; + for _ in 0..64 { + match runner.state().waiting_for.clone() { + WaitingFor::ReplacementChoice { .. } => { + runner + .act(GameAction::ChooseReplacement { index: 0 }) + .expect("accept enter-as-copy replacement"); + } + WaitingFor::CopyTargetChoice { + source_id, + valid_targets, + .. + } => { + assert!( + valid_targets.contains(©_target), + "the intended copy target must be legal, got {valid_targets:?}" + ); + runner + .act(GameAction::ChooseTarget { + target: Some(TargetRef::Object(copy_target)), + }) + .expect("choose copy target"); + chosen = Some(source_id); + break; + } + WaitingFor::Priority { .. } => { + runner.act(GameAction::PassPriority).expect("pass priority"); + } + other => panic!("unexpected waiting_for during entry: {other:?}"), + } + } + let token = chosen.expect("the Embalm token never reached its copy-target choice"); + + for _ in 0..16 { + match runner.state().waiting_for.clone() { + WaitingFor::Priority { .. } if runner.state().stack.is_empty() => break, + WaitingFor::Priority { .. } => { + runner.act(GameAction::PassPriority).expect("pass priority"); + } + _ => break, + } + } + token +} + #[test] fn embalm_copy_of_bear_survives_and_carries_stamped_exceptions() { let (mut runner, vizier, _bear) = build_scenario(); @@ -397,46 +471,7 @@ fn embalm_copy_declined_enters_as_zero_zero_and_dies() { fn a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy() { let (mut runner, vizier, lord, vanilla) = build_lord_scenario(); activate_embalm(&mut runner, vizier); - - let token = 'drive: loop { - match runner.state().waiting_for.clone() { - WaitingFor::ReplacementChoice { .. } => { - runner - .act(GameAction::ChooseReplacement { index: 0 }) - .expect("accept enter-as-copy replacement"); - } - WaitingFor::CopyTargetChoice { - source_id, - valid_targets, - .. - } => { - assert!( - valid_targets.contains(&lord), - "the lord must be a legal copy target, got {valid_targets:?}" - ); - runner - .act(GameAction::ChooseTarget { - target: Some(TargetRef::Object(lord)), - }) - .expect("choose copy target (the lord)"); - break 'drive source_id; - } - WaitingFor::Priority { .. } => { - runner.act(GameAction::PassPriority).expect("pass priority"); - } - other => panic!("unexpected waiting_for during entry: {other:?}"), - } - }; - - for _ in 0..16 { - match runner.state().waiting_for.clone() { - WaitingFor::Priority { .. } if runner.state().stack.is_empty() => break, - WaitingFor::Priority { .. } => { - runner.act(GameAction::PassPriority).expect("pass priority"); - } - _ => break, - } - } + let token = resolve_embalm_copy_of(&mut runner, lord); // POSITIVE reach-guards: the copy really happened AND it really is the copy // that put a static ability on the token, so the P/T assertions below cannot @@ -483,3 +518,74 @@ fn a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy() { (\"other creatures\", so its own anthem does not pump it)" ); } + +/// CR 613.2a + CR 613.2c: sublayer 1a must reach a FIXED POINT before layer 2. +/// Applying a copy effect can hand its recipient a static ability that ITSELF +/// generates a layer-1 continuous effect (CR 707.2c), and CR 613.2c says the +/// characteristics standing after layer 1 finishes ARE the object's copiable +/// values — so that second-generation copy effect belongs to the same layer 1, +/// not to the next pass. +/// +/// Board: the lord's printed base carries "each OTHER creature is named Faceless +/// Reflection" (`FilterProp::Another` excludes the source from its own effect). +/// The Embalm token copies the lord (CR 707.2), which makes the TOKEN a second +/// source of that same static — and the token's instance, unlike the lord's, +/// does cover the lord. So `lord.name` flips if and only if sublayer 1a ran a +/// second generation: the lord's own instance can never rename the lord, and no +/// later layer applies `SetName` (it is a `Layer::Copy` modification). +/// +/// REVERT-PROBE (discriminating, RUN): cap `apply_copy_sublayer_to_fixed_point` +/// at a single generation (return right after the first apply) ⇒ the vanilla is +/// still renamed by generation 1 (the lord's own instance) but the lord keeps its +/// printed name, and this test fails on the `lord.name` assertion alone. +#[test] +fn a_copy_granted_layer_one_static_applies_inside_the_same_layer_one_pass() { + const RENAMED: &str = "Faceless Reflection"; + let renamer = StaticDefinition::continuous() + .affected(TargetFilter::Typed( + TypedFilter::new(TypeFilter::Creature).properties(vec![FilterProp::Another]), + )) + .modifications(vec![ContinuousModification::SetName { + name: RENAMED.to_string(), + }]); + let (mut runner, vizier, lord, vanilla) = build_lord_scenario_with(Some(renamer)); + activate_embalm(&mut runner, vizier); + let token = resolve_embalm_copy_of(&mut runner, lord); + + // POSITIVE reach-guards: the copy really delivered a LAYER-1 static onto the + // token, and the token's own copiable base carries none — so the rename + // asserted below cannot come from anywhere but the copy effect. + let token_obj = runner + .state() + .objects + .get(&token) + .expect("the Embalm token must be on the battlefield"); + assert!( + token_obj.base_static_definitions.is_empty(), + "the token's own copiable base must carry no static, or the second \ + generation this test probes would not depend on the copy at all" + ); + assert!( + token_obj.static_definitions.as_slice().iter().any(|sd| sd + .modifications + .iter() + .any(|m| matches!(m, ContinuousModification::SetName { .. }))), + "the copy must have handed the token the lord's copy-LAYER static, or the \ + second generation could not exist: {:?}", + token_obj.static_definitions.as_slice() + ); + + // Generation 1 evidence: the lord's own instance renames every OTHER creature. + assert_eq!( + runner.state().objects[&vanilla].name, + RENAMED, + "the lord's own layer-1 static must rename the vanilla creature" + ); + // Generation 2 evidence: only the token's copied instance can rename the lord. + assert_eq!( + runner.state().objects[&lord].name, + RENAMED, + "the copy-granted layer-1 static must be applied inside the SAME layer-1 \ + pass that applied the copy" + ); +} From c25d506146169f7f5b6828c966591ada73a5dc1d Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:22:52 -0700 Subject: [PATCH 3/7] fix(engine): identify layer-1 copy effects by provenance, not payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `apply_copy_sublayer_to_fixed_point` identified an already-applied copy effect with `same_copy_effect`, which compared provenance AND the boxed `CopiableValues` payload AND the timestamp. Payload-in-identity makes the loop non-convergent by construction: applying a copy rewrites its recipient, the next gather re-reads the rewritten payload, and the "same" effect looks new forever. That is why `MAX_COPY_SUBLAYER_GENERATIONS = 8` and its `debug_assert!` existed — and in release/WASM, where the assert is compiled out, exhausting the range silently returned a WRONG board with no diagnostic. CR 707.2c: "If a static ability generates a continuous effect that's a copy effect, the copiable values that effect grants are determined only at the time that effect first starts to apply." Identity is therefore PROVENANCE ONLY. `CopySublayerEffectId` wraps the engine's existing canonical identity, `ContinuousEffectGroupKey` — the same key `started_effect_sets` uses for CR 613.6 affected-set retention, which already distinguishes a `GrantedStatic` by grant origin and recipient. `mod_index` rides alongside because the group key is deliberately `mod_index`-blind and would otherwise collapse two modifications of one definition; `source_id` keeps the id total for the synthetic producers whose group key is `None`. The linear `iter().any(same_copy_effect)` scan and its deep payload compare are gone with it — membership is now one `HashSet::insert`. Termination, in place of the cap: - The provenance space is finite and does not GROW during the layer-1 pass. It is (battlefield object x definition index x modification index), plus the fixed set of transient continuous-effect ids, plus granted-static origins. No object enters the battlefield inside the pass. - No arm can mint unbounded fresh provenance. `apply_copiable_values` ASSIGNS `static_definitions` wholesale from the payload (it does not append), so a definition index cannot climb; and `RetainAllOtherAbilitiesFromSource` dedupes against what is already there. - Each iteration either inserts at least one identity into a monotonically growing set, or returns. Also splits the loop-continuation question from the index-rebuild question, which the previous commit conflated. `copy_grants_continuous_static` answers "does this payload grant ANY continuous static", which is the right gate for rebuilding `StaticSourceIndex` — but it was also gating whether to take another generation, and a cloned LORD answers it yes while being unable to extend a layer-1 chain (an anthem is layer 7c). Cloning a lord is common, so that mis-gate bought a `StaticSourceIndex::rebuild_from_state` plus a board-wide `collect_shared_active_continuous_effects` on boards that need neither, and the previous commit message's "a board with no copy-granted copy ability pays exactly today's cost" was false. Continuation now asks the narrow `copy_grants_copy_layer_static`. `ContinuousModification::is_copy_layer` is the panic-free companion to `layer()`, six of whose arms are `unreachable!()` — asking an arbitrary modification read out of a payload for its layer could abort. `started` is deliberately still never pruned: under provenance-only identity CR 707.2c says the latched snapshot is the correct thing to keep applying. Tests: - `mutually_copying_permanents_reach_a_layer_one_fixed_point` — the exact construction the deleted cap named. Renames land on neutral watchers, not on the mutually-copying pair, because two permanents copying each other is a CR 613.8b dependency LOOP whose timestamp fallback would otherwise be what the assertion pinned. - `only_a_copy_layer_payload_buys_a_layer_one_discovery_generation` — three boards differing only in the payload's static set, counting real `collect_shared_active_continuous_effects` calls: the anthem board must match the vanilla board exactly, the copy-layer board must buy one more. - `copy_grants_continuous_static_covers_every_copy_layer_variant` grows a third column so the two questions must disagree on the anthem row. Both new tests were revert-probed: forcing a single generation fails both. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/layers.rs | 481 ++++++++++++++++++++++++++---- crates/engine/src/types/layers.rs | 23 ++ 2 files changed, 441 insertions(+), 63 deletions(-) diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 0c5f684506..55adbda7bd 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -2120,13 +2120,18 @@ pub fn evaluate_layers(state: &mut GameState) { // Step 2: Apply copy effects first so copied static abilities exist before later layers. let mut zone_cache = LayerZoneObjectCache::default(); let mut started_effect_sets = StartedContinuousEffectSets::new(); - // Narrowed by payload rather than by "did layer 1a run": - // `CopyValues` carries the whole copiable static set in the modification, so - // `copy_grants_continuous_static` answers exactly, with no plumbing and no - // battlefield rescan. A clone of a vanilla creature — a permanent-duration - // TCE that outlives the clone — therefore costs no second rebuild per pass, - // which is the whole point of an index whose stated job is keeping per-flush - // work off `|battlefield|`. + // Narrowed by payload rather than by "did layer 1a run": `CopyValues` carries + // the whole copiable static set in the modification, so the payload answers + // both questions 1a needs with no plumbing and no battlefield rescan. They are + // two different questions and are asked separately — `copy_grants_continuous_static` + // for the flag returned here (does the recipient become a generator layers 2-7 + // must see?) and the narrower `copy_grants_copy_layer_static` for continuing + // 1a's own discovery loop. A clone of a vanilla creature — a permanent-duration + // TCE that outlives the clone — therefore pays nothing extra at all, and a + // clone of an ordinary lord pays one rebuild here rather than a second + // gather-and-rebuild generation inside 1a. Only a copy-granted COPY ability + // pays for a generation, which is the point of an index whose stated job is + // keeping per-flush work off `|battlefield|`. let copy_added_generator = apply_copy_sublayer_to_fixed_point( state, &mut abilities_suppressed, @@ -2171,8 +2176,9 @@ pub fn evaluate_layers(state: &mut GameState) { // that non-local fact, not via the rule cited here. // // `apply_copy_sublayer_to_fixed_point` already rebuilt mid-1a whenever a - // generation of copies added a generator, but that rebuild is scoped to 1a's - // own discovery loop and predates 1b by construction. This one is the layer-1 + // generation of copies added a COPY-LAYER generator, but that rebuild is scoped + // to 1a's own discovery loop and predates 1b by construction. This one is the + // layer-1 // exit rebuild the CR 613.2c invariant asks for; on the rare board that pays // both it is one extra O(battlefield) walk, the same order as the Step-1 reset // that already runs unconditionally. @@ -3920,21 +3926,51 @@ fn copy_grants_continuous_static(modification: &ContinuousModification) -> bool } } -/// CR 613.2: how many discovery generations sublayer 1a may run before it stops. +/// CR 613.2a: can applying this copy modification produce ANOTHER generation of +/// sublayer 1a — i.e. does its payload grant a continuous static that itself +/// generates a LAYER-1 effect? /// -/// Each generation applies every started copy effect and then re-gathers; a copy -/// effect first seen in generation `k` can only exist because a copy applied in -/// generation `k-1` handed its recipient the static that generates it. So the -/// bound is the maximum length of a copy-grants-a-copy-ability chain. No printed -/// card forms a chain longer than two links (a clone of a permanent whose own -/// static is a copy effect), and every generation past the first requires a -/// board that already paid for one; eight leaves generous headroom while keeping -/// the mutually-copying case — two permanents that rewrite each other's static -/// sets and therefore keep producing payload-distinct effects forever — from -/// spinning. `debug_assert` on exhaustion so a real non-convergent board is a -/// test failure rather than a silent truncation; release builds stop -/// deterministically with the last fully-ordered application in place. -const MAX_COPY_SUBLAYER_GENERATIONS: usize = 8; +/// Strictly narrower than [`copy_grants_continuous_static`], and the two are not +/// interchangeable. That one asks "does the recipient become a +/// `StaticSourceIndex` generator?", which is the right question for the layer-1 +/// exit rebuild that feeds layers 2-7 and for the incremental-flush guard. +/// Cloning an ordinary lord answers it yes — an anthem is a continuous static — +/// but an anthem is a layer-7c effect and can never appear in a `Layer::Copy` +/// gather however many times 1a re-runs. Continuing the loop on that answer buys +/// a `StaticSourceIndex::rebuild_from_state` plus a board-wide +/// `collect_shared_active_continuous_effects` for a generation that is +/// guaranteed to find nothing, on the very common board that merely contains a +/// cloned lord. Only a copy-LAYER static can extend the chain. +/// +/// Asks [`ContinuousModification::is_copy_layer`] rather than `layer()`: these +/// are payload modifications that have never been through the gather filter, and +/// six of `layer()`'s arms are `unreachable!()` panics. +fn copy_grants_copy_layer_static(modification: &ContinuousModification) -> bool { + match modification { + ContinuousModification::CopyValues { values, .. } => values + .static_definitions + .iter() + // CR 604.2 + CR 613.1: a STATIC ability is what creates a continuous + // effect here, so only a `Continuous` def counts. (CR 611.2, which + // `defs_source_continuous_effect` cites for the same test, is the + // resolution-generated case; the answer is identical, the rule for a + // static is 604.2.) + .filter(|def| def.mode == StaticMode::Continuous) + .flat_map(|def| def.modifications.iter()) + .any(ContinuousModification::is_copy_layer), + // CR 707.9a: the unbounded retain merges the LIVE source's + // `base_static_definitions`, which are not in this payload to inspect, so + // it is answered conservatively — as in `copy_grants_continuous_static`. + // Over-answering costs at most one extra generation, which then gathers + // nothing fresh and exits. + ContinuousModification::RetainAllOtherAbilitiesFromSource => true, + // The remaining `Layer::Copy` variants write no `static_definitions` at + // all (`SetName` is name-only, `CopyChosen`'s apply arm is a no-op, and + // the two single-item retains push one ability / one trigger), and every + // layers-2-7 variant is out of both callers' `Layer::Copy` filter. + _ => false, + } +} /// CR 613.2a + CR 613.2c: apply sublayer 1a to a fixed point. /// @@ -3950,10 +3986,41 @@ const MAX_COPY_SUBLAYER_GENERATIONS: usize = 8; /// CR 613.8a dependency order), never just the newly discovered tail. That /// matters because `depends_on` sorts every `CopyValues` ahead of the other /// layer-1 modifications, so a copy discovered late can legitimately need to -/// apply before one already applied. Re-application is safe: `CopyValues` / -/// `SetName` assign wholesale from a payload snapshot, the retains dedupe -/// per-item, `CopyChosen` is a no-op, and `started_effect_sets` hands an -/// already-started effect back its original affected set (CR 613.6). +/// apply before one already applied. Re-application is safe, and CR 707.2c is +/// why: the copiable values a copy effect grants are fixed the first time it +/// starts to apply, so re-applying a started effect re-applies that same locked +/// snapshot. Mechanically, `CopyValues` / `SetName` assign wholesale from the +/// payload, the retains dedupe per item, `CopyChosen` is a no-op, and +/// `started_effect_sets` hands an already-started effect back its original +/// affected set (CR 613.6). Nothing is ever pruned from the started set for the +/// same reason: an effect whose provenance slot has since been overwritten still +/// applies the values it locked in, so dropping it would be the CR 707.2c +/// violation, not keeping it. +/// +/// Termination. The loop's identity ([`CopySublayerEffectId`]) is provenance, +/// never payload, so the space it draws from is finite and does not grow during +/// the pass: +/// +/// - The battlefield object set is fixed for the duration of layer 1 — nothing +/// in sublayer 1a creates, destroys or moves an object — so the `Static` and +/// `GrantedStatic` arms of `ContinuousEffectGroupKey` range over a fixed set of +/// `ObjectIncarnationRef`s (`incarnation` changes only on a zone change), and +/// the `Transient` arm over the fixed id set of +/// `state.transient_continuous_effects`. +/// - `definition_index` and `mod_index` are bounded by the largest static set any +/// object can hold during the pass, and no arm can mint fresh ones without +/// bound: `apply_copiable_values` ASSIGNS `obj.static_definitions` wholesale +/// from the payload's snapshot rather than appending, +/// `RetainAllOtherAbilitiesFromSource` merges the source's +/// `base_static_definitions` behind a per-item `contains` dedupe, and every +/// other `Layer::Copy` arm writes no static at all. Each payload is itself a +/// snapshot taken before the pass began, so the reachable static sets are fixed +/// too. +/// +/// Each iteration therefore either inserts at least one identity into the +/// monotonically growing `started_ids` — bounded above by that finite space — or +/// gathers nothing fresh and returns. So the loop terminates, with no generation +/// cap needed to force it. /// /// Returns whether any applied copy added a `StaticSourceIndex` generator, which /// the caller needs for the layer-1 exit rebuild that feeds layers 2-7. @@ -3964,13 +4031,16 @@ fn apply_copy_sublayer_to_fixed_point( started_effect_sets: &mut StartedContinuousEffectSets, ) -> bool { let mut started: Vec = Vec::new(); + let mut started_ids: HashSet = HashSet::new(); let mut copy_added_generator = false; - for _ in 0..MAX_COPY_SUBLAYER_GENERATIONS { + loop { + // `insert` returns `false` for an identity already started, so this both + // selects the fresh effects and records them in one O(1)-per-effect pass. let fresh: Vec = gather_active_effects_for_layer(state, Layer::Copy) .into_iter() - .filter(|effect| !started.iter().any(|seen| same_copy_effect(seen, effect))) + .filter(|effect| started_ids.insert(copy_sublayer_effect_id(state, effect))) .collect(); if fresh.is_empty() { // Fixed point: this generation's gather produced nothing the @@ -3978,10 +4048,15 @@ fn apply_copy_sublayer_to_fixed_point( return copy_added_generator; } - let fresh_added_generator = fresh + copy_added_generator |= fresh .iter() .any(|effect| copy_grants_continuous_static(&effect.modification)); - copy_added_generator |= fresh_added_generator; + // Loop continuation is the NARROW question: only a copy-layer static can + // show up in the next `Layer::Copy` gather. A cloned lord answers + // `copy_grants_continuous_static` yes and still cannot extend the chain. + let fresh_added_copy_generator = fresh + .iter() + .any(|effect| copy_grants_copy_layer_static(&effect.modification)); let reapplying = !started.is_empty(); started.extend(fresh); @@ -4005,40 +4080,64 @@ fn apply_copy_sublayer_to_fixed_point( ); } - if !fresh_added_generator { - // Nothing this generation could add a `StaticSourceIndex` generator, - // so the next gather would read the same index and return the same - // set. Stop without paying the O(battlefield) rebuild. + if !fresh_added_copy_generator { + // Nothing this generation can put a copy-layer static on the board, + // so the next gather would return the same set. Stop without paying + // the O(battlefield) rebuild. If a NON-copy generator was added, the + // returned flag makes the caller do the layer-1 exit rebuild instead. return copy_added_generator; } // The index was built from definitions that predate these copies; refresh // it so the next gather can see a copy-granted copy ability. crate::types::game_state::StaticSourceIndex::rebuild_from_state(state); } - - debug_assert!( - false, - "layer 1a did not converge in {MAX_COPY_SUBLAYER_GENERATIONS} generations; \ - mutually-copying permanents rewriting each other's static definitions?" - ); - copy_added_generator } -/// CR 613.2a: identity of a layer-1 copy effect for the fixed-point loop. +/// CR 707.2c: identity of a layer-1 copy effect for the fixed-point loop. /// -/// Compares provenance AND payload. Provenance alone is not a stable name across -/// generations: a copy effect rewrites its own recipient's `static_definitions`, -/// so `(source_id, def_index, mod_index)` can denote a DIFFERENT modification in -/// the next generation. Including the payload makes "already started" exact, at -/// the cost of letting a genuinely non-convergent board keep producing new -/// identities — which is what `MAX_COPY_SUBLAYER_GENERATIONS` bounds. -fn same_copy_effect(a: &ActiveContinuousEffect, b: &ActiveContinuousEffect) -> bool { - a.source_id == b.source_id - && a.def_index == b.def_index - && a.transient_id == b.transient_id - && a.mod_index == b.mod_index - && a.timestamp == b.timestamp - && a.modification == b.modification +/// PROVENANCE ONLY — never the payload. CR 707.2c: "If a static ability generates +/// a continuous effect that's a copy effect, the copiable values that effect +/// grants are determined only at the time that effect first starts to apply." So +/// once a slot has started applying, re-reading it later in the same sublayer is +/// the SAME effect granting the SAME locked values; a rewritten payload does not +/// mint a new one. Keying on the payload instead both contradicts that rule and +/// makes the loop non-convergent by construction — two permanents that rewrite +/// each other's static sets would produce payload-distinct identities forever, +/// which is exactly what a generation cap used to have to paper over. +/// +/// Built on [`ContinuousEffectGroupKey`], this file's canonical "which effect is +/// this" answer, rather than a hand-rolled tuple. Two reasons: it is what +/// `started_effect_sets` already keys on, so the loop and `apply_continuous_effect` +/// can no longer disagree about whether an effect has started (a disagreement is a +/// CR 613.6 bug — the loop calls an effect new while the applier hands it back an +/// already-locked affected set); and it distinguishes +/// `GrantedStatic { grant_origin, recipient }` occurrences that a raw +/// `(source_id, def_index, transient_id)` tuple collides on. +/// +/// `mod_index` rides alongside because the group key deliberately drops it — every +/// modification of one definition shares one CR 613.6 affected set — while this +/// loop must re-apply EACH modification of a multi-modification copy definition +/// (`CopyValues` + its `SetName` exception, say), not just the first. +#[derive(Debug, Clone, PartialEq, Eq, Hash)] +struct CopySublayerEffectId { + /// `None` only for a synthetic effect carrying no static, transient or grant + /// provenance at all (the Ring emblem, stickers). No synthetic producer emits + /// a `Layer::Copy` modification today, so this is unreachable here; carrying + /// `source_id` keeps the identity total instead of resting on that fact. + group: Option, + source_id: ObjectId, + mod_index: usize, +} + +fn copy_sublayer_effect_id( + state: &GameState, + effect: &ActiveContinuousEffect, +) -> CopySublayerEffectId { + CopySublayerEffectId { + group: continuous_effect_group_key(state, effect), + source_id: effect.source_id, + mod_index: effect.mod_index, + } } /// Incremental layer re-derivation for a set of freshly-entered objects. @@ -20710,6 +20809,14 @@ mod tests { StaticDefinition::continuous() .modifications(vec![ContinuousModification::AddPower { value: 1 }]), ); + // Same shape, but the payload's static is a LAYER-1 one — the only thing + // that can extend sublayer 1a's discovery chain. + let mut copy_static_values = plain_values.clone(); + Arc::make_mut(&mut copy_static_values.static_definitions).push( + StaticDefinition::continuous().modifications(vec![ContinuousModification::SetName { + name: "Renamed".into(), + }]), + ); let copy_of = |values: CopiableValues| ContinuousModification::CopyValues { values: Box::new(values), display_source: crate::game::game_object::DisplaySource::Card, @@ -20717,17 +20824,26 @@ mod tests { token_image_ref: None, }; - // The whole `Layer::Copy` set, with the expected answer for each. + // The whole `Layer::Copy` set: (modification, adds ANY generator, adds a + // COPY-LAYER generator). The two columns are the two different questions + // layer 1 asks, and the anthem row is where they must disagree. let copy_layer_cases = [ // Payload-exact: the copiable static set travels inside the // modification, so no live-source read and no battlefield rescan. - (copy_of(anthem_values), true), - (copy_of(plain_values), false), + // An anthem makes its recipient a generator for layers 2-7 but can + // never appear in a `Layer::Copy` gather, so it must NOT buy a + // discovery generation. + (copy_of(anthem_values), true, false), + (copy_of(copy_static_values), true, true), + (copy_of(plain_values), false, false), // CR 707.9a: merges the source's whole printed set, INCLUDING its // `base_static_definitions`, onto the recipient (see the apply arm). + // The live source is not in the payload, so both answers are + // conservative. ( ContinuousModification::RetainAllOtherAbilitiesFromSource, true, + true, ), // CR 707.9a: pushes ONE `AbilityDefinition` onto `obj.abilities`; // `static_definitions` is structurally out of reach. @@ -20736,6 +20852,7 @@ mod tests { source_ability_index: 0, }, false, + false, ), // CR 707.9a: pushes one trigger; likewise never a static. ( @@ -20743,6 +20860,7 @@ mod tests { source_trigger_index: 0, }, false, + false, ), // CR 707.9b: name-only override. ( @@ -20750,27 +20868,41 @@ mod tests { name: "Renamed".into(), }, false, + false, ), // CR 707.2c: parse-time marker whose apply arm is an explicit no-op. - (ContinuousModification::CopyChosen, false), + (ContinuousModification::CopyChosen, false, false), ]; - for (modification, expected) in ©_layer_cases { + for (modification, expected_any, expected_copy_layer) in ©_layer_cases { assert_eq!( modification.layer(), Layer::Copy, "{modification:?} must be a layer-1 modification for this table to be \ the complete `Layer::Copy` set" ); + // Pins the panic-free companion to `layer()` over the same set — the + // invariant `is_copy_layer`'s doc claims. + assert!( + modification.is_copy_layer(), + "{modification:?} is in the `Layer::Copy` set, so `is_copy_layer` \ + must agree with `layer()`" + ); assert_eq!( copy_grants_continuous_static(modification), - *expected, + *expected_any, "wrong generator answer for {modification:?}" ); + assert_eq!( + copy_grants_copy_layer_static(modification), + *expected_copy_layer, + "wrong copy-layer generator answer for {modification:?}" + ); } // Layers 2-7 fall to the catch-all. `AddCounterOnEnter` is one of the six // variants whose `layer()` is `unreachable!()`, so it is exactly the input - // that made the replaced `debug_assert_eq!(other.layer(), ..)` unsafe. + // that made the replaced `debug_assert_eq!(other.layer(), ..)` unsafe — + // and exactly why `is_copy_layer` exists to be asked instead. for modification in [ ContinuousModification::AddCounterOnEnter { counter_type: CounterType::Plus1Plus1, @@ -20779,12 +20911,235 @@ mod tests { }, ContinuousModification::AddPower { value: 1 }, ] { + assert!( + !modification.is_copy_layer(), + "{modification:?} is not a layer-1 modification" + ); assert!( !copy_grants_continuous_static(&modification), "{modification:?} is not a layer-1 modification and must not claim to \ add a generator" ); + assert!( + !copy_grants_copy_layer_static(&modification), + "{modification:?} is not a layer-1 modification and must not buy a \ + discovery generation" + ); + } + } + + /// CR 707.2c + CR 613.2c: sublayer 1a runs to a FIXED POINT on the exact + /// construction the deleted `MAX_COPY_SUBLAYER_GENERATIONS = 8` named — "two + /// permanents that rewrite each other's static sets". Reaching the assertions + /// at all IS the termination evidence; the assertions are the CR-correctness + /// evidence. + /// + /// Each half of the pair is copied from a donor whose copiable values carry a + /// copy-LAYER static (CR 707.2), so BOTH second-generation effects exist only + /// because the first generation applied, in both directions at once. The board + /// is synthetic in the sense that no printed pair does exactly this, but every + /// piece is a real mechanism: a latched `CopyValues` TCE (the only way a copy + /// effect can originate — `apply_continuous_effect` requires `transient_id` for + /// it) whose payload carries a `SetName` copy exception. + /// + /// The two copy-granted renames land on neutral watchers rather than on the + /// pair itself, ON PURPOSE. Two permanents copying each other is a CR 613.8b + /// dependency LOOP, so the engine correctly discards the dependency edges and + /// falls back to timestamp order, under which each half's older static applies + /// before the newer `CopyValues` overwrites its name. That is a separate rule + /// from the one under test, and asserting on the pair's own names would pin + /// 613.8b's tie-break instead of 1a's fixed point. + /// + /// REVERT-PROBE (discriminating, run): make the loop return after its first + /// generation ⇒ both watchers keep their printed names and both rename + /// assertions fail, while the copy assertions stay green. + #[test] + fn mutually_copying_permanents_reach_a_layer_one_fixed_point() { + let mut state = setup(); + let player = PlayerId(0); + let left = make_creature(&mut state, "Left", 1, 1, player); + let right = make_creature(&mut state, "Right", 1, 1, player); + let watcher_l = make_creature(&mut state, "Watcher L", 1, 1, player); + let watcher_r = make_creature(&mut state, "Watcher R", 1, 1, player); + let donor_p = make_creature(&mut state, "Donor P", 4, 4, player); + let donor_q = make_creature(&mut state, "Donor Q", 5, 5, player); + + // Donor payloads: each carries a copy-layer static that renames a watcher. + let renames = |state: &GameState, donor: ObjectId, target: ObjectId, name: &str| { + let mut values = intrinsic_copiable_values(&state.objects[&donor]); + Arc::make_mut(&mut values.static_definitions).push( + StaticDefinition::continuous() + .affected(TargetFilter::SpecificObject { id: target }) + .modifications(vec![ContinuousModification::SetName { + name: name.to_string(), + }]), + ); + values + }; + let p_values = renames(&state, donor_p, watcher_l, "Marked by right"); + let q_values = renames(&state, donor_q, watcher_r, "Marked by left"); + + let mut copy_onto = |source: ObjectId, target: ObjectId, values: CopiableValues| { + state.add_transient_continuous_effect( + source, + player, + Duration::Permanent, + TargetFilter::SpecificObject { id: target }, + vec![ContinuousModification::CopyValues { + values: Box::new(values), + display_source: crate::game::game_object::DisplaySource::Card, + printed_ref: None, + token_image_ref: None, + }], + None, + ); + }; + // `left` copies donor P onto `right`; `right` copies donor Q onto `left`. + copy_onto(left, right, p_values); + copy_onto(right, left, q_values); + + // POSITIVE reach-guards, on the live field the top-of-pass reset writes and + // `for_each_static_effect_source` gathers: neither permanent carries a + // PRINTED static, so every second-generation effect below can only exist + // because generation 1's copy put it there. + assert!( + state.objects[&left].base_static_definitions.is_empty() + && state.objects[&right].base_static_definitions.is_empty(), + "neither half of the pair may carry a printed static, or the renames \ + would not depend on the copies at all" + ); + + evaluate_layers(&mut state); + + assert!( + !state.objects[&left].static_definitions.is_empty() + && !state.objects[&right].static_definitions.is_empty(), + "each copy must have handed its recipient the donor's copy-layer static, \ + or there is no second generation to reach a fixed point over" + ); + // CR 707.2: each half took its donor's copiable P/T. + assert_eq!( + (state.objects[&right].power, state.objects[&right].toughness), + (Some(4), Some(4)), + "the right half must be a copy of donor P" + ); + assert_eq!( + (state.objects[&left].power, state.objects[&left].toughness), + (Some(5), Some(5)), + "the left half must be a copy of donor Q" + ); + // Generation 2, both directions at once: each half's copy-granted static + // reached its watcher inside this same layer-1 pass. + assert_eq!( + state.objects[&watcher_l].name, "Marked by right", + "the copy-granted static on the RIGHT half must apply inside the same \ + layer-1 pass that applied the copy" + ); + assert_eq!( + state.objects[&watcher_r].name, "Marked by left", + "the copy-granted static on the LEFT half must apply inside the same \ + layer-1 pass that applied the copy" + ); + } + + /// CR 613.2a: `copy_grants_continuous_static` is the WRONG question for + /// CONTINUING sublayer 1a's discovery loop. An anthem makes its recipient a + /// `StaticSourceIndex` generator, which layers 2-7 must see, but it can never + /// appear in a `Layer::Copy` gather — so answering the broad question at the + /// continuation site charged every board holding a cloned lord for an extra + /// index rebuild plus an extra board-wide + /// `collect_shared_active_continuous_effects`, inside the module whose stated + /// job is keeping per-flush work off `|battlefield|`. + /// + /// Three boards differing ONLY in the copy payload's static set, measured in + /// board-wide gathers. + /// + /// REVERT-PROBE (discriminating, run): put `copy_grants_continuous_static` + /// back at the loop-continuation site ⇒ the anthem board costs one gather more + /// than the vanilla board and the first assertion fails. + #[test] + fn only_a_copy_layer_payload_buys_a_layer_one_discovery_generation() { + /// Returns (board-wide gathers during the pass, statics landed on the + /// recipient). + fn probe(payload_static: Option) -> (usize, usize) { + let mut state = setup(); + let player = PlayerId(0); + let template = make_creature(&mut state, "Template", 2, 2, player); + let recipient = make_creature(&mut state, "Recipient", 1, 1, player); + let caster = make_creature(&mut state, "Caster", 1, 1, player); + let mut values = intrinsic_copiable_values(&state.objects[&template]); + assert!( + values.static_definitions.is_empty(), + "the template must be vanilla so the payload static is the only variable" + ); + if let Some(def) = payload_static { + Arc::make_mut(&mut values.static_definitions).push(def); + } + state.add_transient_continuous_effect( + caster, + player, + Duration::Permanent, + TargetFilter::SpecificObject { id: recipient }, + vec![ContinuousModification::CopyValues { + values: Box::new(values), + display_source: crate::game::game_object::DisplaySource::Card, + printed_ref: None, + token_image_ref: None, + }], + None, + ); + reset_active_effect_collection_count(); + evaluate_layers(&mut state); + ( + active_effect_collection_count(), + state.objects[&recipient].static_definitions.len(), + ) } + + let board_wide = + || TargetFilter::Typed(TypedFilter::creature().controller(ControllerRef::You)); + let (vanilla_gathers, vanilla_statics) = probe(None); + let (anthem_gathers, anthem_statics) = probe(Some( + StaticDefinition::continuous() + .affected(board_wide()) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + )); + // Same shape, but a `Layer::Copy` modification — the only kind that can + // show up in the next gather. + let (copy_layer_gathers, copy_layer_statics) = probe(Some( + StaticDefinition::continuous() + .affected(board_wide()) + .modifications(vec![ContinuousModification::SetName { + name: "Renamed".into(), + }]), + )); + + // POSITIVE reach-guards: the payload static really landed on the recipient + // in both static cases, so the gather counts below are not comparing two + // boards where the copy did nothing. + assert_eq!( + vanilla_statics, 0, + "the vanilla control must land no static" + ); + assert_eq!( + anthem_statics, 1, + "the anthem payload must reach the recipient" + ); + assert_eq!( + copy_layer_statics, 1, + "the copy-layer payload must reach the recipient" + ); + + assert_eq!( + anthem_gathers, vanilla_gathers, + "cloning an ordinary lord must buy no layer-1 discovery generation: an \ + anthem is a layer-7c effect and cannot appear in a `Layer::Copy` gather" + ); + assert_eq!( + copy_layer_gathers, + vanilla_gathers + 1, + "a copy-LAYER payload static must buy exactly one discovery generation" + ); } /// Shared reach-guard: run ONLY the guard from a Clean baseline and assert it diff --git a/crates/engine/src/types/layers.rs b/crates/engine/src/types/layers.rs index 702fec6fc8..57259f7ad8 100644 --- a/crates/engine/src/types/layers.rs +++ b/crates/engine/src/types/layers.rs @@ -77,6 +77,29 @@ impl Layer { } impl ContinuousModification { + /// CR 613.2a: whether this modification is applied in layer 1 (copy). + /// + /// Panic-free companion to [`Self::layer`]. Six of `layer`'s arms are + /// `unreachable!()` — `AddCounterOnEnter`, `SetStartingLoyalty`, + /// `RemoveManaCost` and the three combat-assignment variants are consumed at + /// copy resolution and never layered — so asking an arbitrary modification + /// (one read out of a `CopyValues` payload, say) for its layer can abort. + /// This answers the only layer question sublayer 1a needs without that + /// hazard. The variant list is exactly `layer`'s `Layer::Copy` set; + /// `copy_grants_continuous_static_covers_every_copy_layer_variant` + /// (`game/layers.rs`) pins the two together. + pub fn is_copy_layer(&self) -> bool { + matches!( + self, + ContinuousModification::CopyValues { .. } + | ContinuousModification::CopyChosen + | ContinuousModification::SetName { .. } + | ContinuousModification::RetainPrintedTriggerFromSource { .. } + | ContinuousModification::RetainPrintedAbilityFromSource { .. } + | ContinuousModification::RetainAllOtherAbilitiesFromSource + ) + } + /// Returns the appropriate Layer for this modification type. pub fn layer(&self) -> Layer { match self { From 8acf866e9e256de2435c43c95312dd44b68c969f Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:23:27 -0700 Subject: [PATCH 4/7] perf(engine): escalate only for a copy that can reach an incremental recipient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The entry-incremental escalation guard fired on the mere PRESENCE of an active copy effect whose payload grants a continuous static. That includes the ordinary shape — a resolved Clone / Vizier carries a `SelfRef` or `SpecificObject` copy naming one pre-existing permanent — so any board that had ever resolved a clone paid the O(|battlefield|) full pass on every subsequent entry, which is exactly the cost this module exists to keep off the flush. Such a copy cannot hand a recipient anything: the incremental arm applies copies through `apply_continuous_effect_to(state, effect, &recipient_ids, ..)`, and a filter naming a non-recipient intersects `recipient_ids` to the empty set. The clone's own copy-granted static is already live on it from the earlier full pass, is never reset here, and is picked up by the top-of-pass `StaticSourceIndex` rebuild like any other generator. The guard now also asks `effect_can_reach_incremental_recipients`, and `copy_effects` is filtered by the same predicate so `copy_effects.is_empty()` — which decides whether the flush pays a second board-wide gather — is accurate again. A copy that DOES name a recipient still escalates, and so does any copy whose affected set is a predicate rather than an id, since that set is not decidable here. The review asked for `effect_is_restricted_to_incremental_recipients` to be reused directly. It cannot be: the two questions agree on an id-naming filter (a set of one is confined iff it is reached) but their conservative answers for an unknown affected set are OPPOSITE, because both must escalate on doubt and doubt sits on different sides. They now share the id-naming classifier `effect_names_single_affected_object` and differ only in `is_some_and` vs `is_none_or`, which is where the asymmetry belongs. Tests: - `entry_incremental_stays_incremental_for_a_copy_naming_a_pre_existing_object` is the negative sibling of the existing escalation test, with a positive reach-guard proving the clone really carries the payload's static (so `copy_grants_continuous_static` answers true and only the reach test is keeping the flush incremental), and a CR 613.2c assertion that the entrant still comes out wearing the clone's copy-granted anthem. - Both escalation tests now reach-guard on `entered_object_blocks_incremental` itself rather than on `base_static_definitions`. The guard reads the LIVE `static_definitions`; asserting the neighbouring field was not vacuous but was not the predicate under test either. Revert-probed: dropping the reach term flips `layers_incremental` to 0 and `layers_full_eval` to 1 and fails the new test. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/layers.rs | 192 ++++++++++++++++++++++++++++--- 1 file changed, 174 insertions(+), 18 deletions(-) diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 55adbda7bd..858cdc8928 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -3526,10 +3526,23 @@ fn prepare_incremental_flush( // population sensitivity) nor the source-side guard above sees that case: the // copy's source is a pre-existing permanent and its filter counts nothing. // + // That same `restrict_to` intersection is why the guard asks + // `effect_can_reach_incremental_recipients` rather than firing on the mere + // presence of a copy: a `SelfRef` / `SpecificObject` copy that names a + // PRE-EXISTING permanent — the ordinary shape, every resolved clone on the + // board carries one — applies to the empty set on this path, so it can hand no + // recipient anything, and escalating for it would put every board that has + // ever cast a clone back on the O(|battlefield|) full pass this module exists + // to avoid. A copy that does name a recipient still escalates; so does any + // copy whose affected set is a predicate, since that set is not decidable + // here. + // // O(active-effect-count), and zero on the overwhelmingly common board with no // copy effect at all. if active_effects.iter().any(|effect| { - effect.layer == Layer::Copy && copy_grants_continuous_static(&effect.modification) + effect.layer == Layer::Copy + && effect_can_reach_incremental_recipients(effect, &recipient_ids) + && copy_grants_continuous_static(&effect.modification) }) { return None; } @@ -3861,15 +3874,45 @@ fn incremental_recipient_ids( recipients } +/// The single object an effect's `affected_filter` provably names, when it names +/// one by id at all. `None` means the filter is a PREDICATE over the board +/// (`Typed`, `And`, `Or`, ...) whose membership can only be decided by evaluating +/// it against every object — which neither caller below does, so each supplies +/// its own conservative answer for that case. +/// +/// An id-naming filter is exactly the case where the affected set is known +/// without a board scan. (No CR annotation: this decides nothing about the rules, +/// it only reports what a filter's shape already tells us.) +fn effect_names_single_affected_object(effect: &ActiveContinuousEffect) -> Option { + match &effect.affected_filter { + TargetFilter::SelfRef => Some(effect.source_id), + TargetFilter::SpecificObject { id } => Some(*id), + _ => None, + } +} + +/// Is this effect provably CONFINED to the incremental recipients — i.e. can the +/// restricted pass reproduce it in full? Unknown affected sets answer `false` +/// (escalate on doubt): a board-wide filter also touches pre-existing objects +/// that the incremental arm never resets. fn effect_is_restricted_to_incremental_recipients( effect: &ActiveContinuousEffect, recipient_ids: &BTreeSet, ) -> bool { - match &effect.affected_filter { - TargetFilter::SelfRef => recipient_ids.contains(&effect.source_id), - TargetFilter::SpecificObject { id } => recipient_ids.contains(id), - _ => false, - } + effect_names_single_affected_object(effect).is_some_and(|id| recipient_ids.contains(&id)) +} + +/// Can this effect REACH at least one incremental recipient? The dual question to +/// [`effect_is_restricted_to_incremental_recipients`], and deliberately not the +/// same predicate: the two agree on an id-naming filter (a set of one is confined +/// iff it is reached) but their conservative answers for an unknown affected set +/// are OPPOSITE, because both must escalate on doubt and doubt sits on different +/// sides of the two questions. +fn effect_can_reach_incremental_recipients( + effect: &ActiveContinuousEffect, + recipient_ids: &BTreeSet, +) -> bool { + effect_names_single_affected_object(effect).is_none_or(|id| recipient_ids.contains(&id)) } /// CR 613.2a + CR 613.2c: does applying this layer-1 copy modification hand its @@ -4167,9 +4210,18 @@ fn apply_layers_incremental(state: &mut GameState, prepared: PreparedIncremental // static-source index rebuild and shared active-effect collection. // Step 2: Copy effects first (Layer 1), restricted to recipient objects. + // + // `effect_can_reach_incremental_recipients` drops a copy that provably names a + // NON-recipient — the ordinary resolved-clone shape. `apply_continuous_effect_to` + // would skip every one of its objects at the `restrict_to` test anyway, so this + // changes no state; what it buys is an accurate `copy_effects.is_empty()` below, + // which is what decides whether this flush pays a second board-wide gather. let copy_effects: Vec = active_effects .iter() - .filter(|effect| effect.layer == Layer::Copy) + .filter(|effect| { + effect.layer == Layer::Copy + && effect_can_reach_incremental_recipients(effect, &recipient_ids) + }) .cloned() .collect(); let ordered_copy = order_active_continuous_effects(Layer::Copy, ©_effects, state); @@ -4201,13 +4253,16 @@ fn apply_layers_incremental(state: &mut GameState, prepared: PreparedIncremental // `apply_continuous_effect_to` restricts an ordinary board-wide // `affected_filter` to `recipient_ids` rather than requiring // `TargetFilter::SpecificObject`. What rules that case out is the escalation - // guard in `prepare_incremental_flush`: any active copy whose payload grants a - // continuous static sends the whole flush to `evaluate_layers`, so no copy - // surviving to this line can turn a recipient into a `StaticSourceIndex` - // generator. The index built there still names exactly the right sources and a - // rebuild would find the identical set. (It would also be the wrong repair - // anyway: a guard that fans a copy-granted static over `recipient_ids` alone - // would not agree with the full pass, which derives it board-wide.) + // guard in `prepare_incremental_flush`: any active copy that can REACH a + // recipient and whose payload grants a continuous static sends the whole flush + // to `evaluate_layers`. The copies it lets through are exactly those confined + // to a named non-recipient, which the `copy_effects` filter above then drops — + // so either way no copy surviving to this line can turn a recipient into a + // `StaticSourceIndex` generator. The index built there still names exactly the + // right sources and a rebuild would find the identical set. (It would also be + // the wrong repair anyway: a guard that fans a copy-granted static over + // `recipient_ids` alone would not agree with the full pass, which derives it + // board-wide.) // // The re-collect below is not dead weight, though. A copy changes its // recipients' characteristics, and a pre-existing generator's @@ -20755,11 +20810,14 @@ mod tests { evaluate_layers(&mut state); let entrant = make_creature(&mut state, "Entrant", 1, 1, player); + // Reach-guard asked of the guard ITSELF, not of a neighbouring field: this + // is the exact predicate `prepare_incremental_flush` consults first, and it + // reads the LIVE `static_definitions`. If it answered true the escalation + // below would be its doing and this test would stop probing the copy guard. assert!( - state.objects[&entrant].base_static_definitions.is_empty(), - "the entrant's own base must carry no static, or the existing \ - `entered_object_blocks_incremental` guard would escalate instead and \ - this test would stop probing the copy guard" + !entered_object_blocks_incremental(&state, &state.objects[&entrant]), + "the entrant must be an ordinary entry, or the existing \ + `entered_object_blocks_incremental` guard would escalate instead" ); crate::game::perf_counters::reset(); @@ -20784,6 +20842,104 @@ mod tests { ); } + /// The other side of that guard: a copy effect that provably names a + /// PRE-EXISTING permanent must NOT escalate. This is the ordinary resolved + /// clone — every Clone / Vizier copy on a board carries a `SpecificObject` + /// (or `SelfRef`) filter naming one permanent — so escalating on the mere + /// PRESENCE of a copy would drop every board that has ever resolved one back + /// onto the O(|battlefield|) full pass this module exists to avoid. + /// + /// It is sound because the incremental arm applies copies through + /// `apply_continuous_effect_to(state, effect, &recipient_ids, ..)`: a filter + /// naming a non-recipient intersects `recipient_ids` to the empty set, so it + /// can hand the entrant nothing, and the clone's own copy-granted static — + /// already live on it from the earlier full pass, never reset here — is picked + /// up by the top-of-pass `StaticSourceIndex` rebuild like any other generator. + /// The differential assertion below is what pins that: the entrant must come + /// out of the incremental flush wearing the CLONE's anthem. + /// + /// REVERT-PROBE (discriminating, RUN): drop + /// `effect_can_reach_incremental_recipients` from the guard ⇒ + /// `layers_full_eval == 1` / `layers_incremental == 0` and the two branch + /// assertions fail. + #[test] + fn entry_incremental_stays_incremental_for_a_copy_naming_a_pre_existing_object() { + let mut state = setup(); + let player = PlayerId(0); + let template = make_creature(&mut state, "Template", 2, 2, player); + let mut copied_values = intrinsic_copiable_values(&state.objects[&template]); + Arc::make_mut(&mut copied_values.static_definitions).push( + StaticDefinition::continuous() + .affected(TargetFilter::Typed( + TypedFilter::creature().controller(ControllerRef::You), + )) + .modifications(vec![ContinuousModification::AddPower { value: 1 }]), + ); + // The clone exists BEFORE the flush under test, and the copy effect names + // it — the shape a resolved clone actually has on a live board. + let clone = make_creature(&mut state, "Clone", 1, 1, player); + let caster = make_creature(&mut state, "Caster", 1, 1, player); + state.add_transient_continuous_effect( + caster, + player, + Duration::Permanent, + TargetFilter::SpecificObject { id: clone }, + vec![ContinuousModification::CopyValues { + values: Box::new(copied_values), + display_source: crate::game::game_object::DisplaySource::Card, + printed_ref: None, + token_image_ref: None, + }], + None, + ); + evaluate_layers(&mut state); + + // POSITIVE reach-guards, both on the live fields the guard consults: the + // copy really did land, so `copy_grants_continuous_static` answers TRUE for + // this effect and the only thing keeping the flush incremental is the reach + // test. Without this the assertions below would pass for the wrong reason. + assert!( + !state.objects[&clone].static_definitions.is_empty(), + "the clone must be carrying the payload's continuous static, or the \ + guard's copy branch is never exercised" + ); + + let entrant = make_creature(&mut state, "Entrant", 1, 1, player); + // Same reach-guard as the escalating sibling, asked of the predicate itself + // (it reads the LIVE `static_definitions`, not the base). + assert!( + !entered_object_blocks_incremental(&state, &state.objects[&entrant]), + "the entrant must be an ordinary entry, or the incremental arm would be \ + refused for a reason unrelated to the copy guard" + ); + + crate::game::perf_counters::reset(); + state.layers_dirty = LayersDirty::EnteredObjects([entrant].into()); + flush_layers(&mut state); + let counters = crate::game::perf_counters::snapshot(); + + assert_eq!( + counters.layers_incremental, 1, + "a copy confined to a pre-existing permanent cannot reach the entrant, \ + so the flush must stay incremental" + ); + assert_eq!( + counters.layers_full_eval, 0, + "no full pass may be forced by a copy the entrant is out of reach of" + ); + // CR 613.2c: after layer 1 the clone's copied static is part of the board, + // so its anthem must reach the entrant on THIS path — 1/1 printed, +1/+0 + // from the clone. + assert_eq!( + ( + state.objects[&entrant].power, + state.objects[&entrant].toughness + ), + (Some(2), Some(1)), + "the entrant must still receive the clone's copy-granted anthem" + ); + } + /// CR 613.2a: `copy_grants_continuous_static` answers over the COMPLETE /// `Layer::Copy` modification set, and its `_ => false` arm is exact rather /// than a fallback guess. Every variant here is also asserted to report From 50d17e814db9fbafe49eca022b97e9f426dfeb4a Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 15:23:48 -0700 Subject: [PATCH 5/7] test(engine): bound the #5278 drive loops and mark the synthetic SetName board MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two unbounded `loop`s survived the previous commit's bounding pass. Both act on their `Priority` arm every trip, so an engine change that stops surfacing `CopyTargetChoice` / the enter-as-copy `ReplacementChoice` would spin forever and hang CI instead of failing. Both are now `for _ in 0..64` with an `expect` naming the prompt that never arrived — the same bound and shape `activate_embalm` and `resolve_embalm_copy_of` already use. The previous commit message claimed this was already done for the whole file; it was true of one loop. `a_copy_granted_layer_one_static_applies_inside_the_same_layer_one_pass` builds a board-wide `SetName` static, which no printed card has — on real cards `SetName` appears as a copy EXCEPTION paired with `CopyValues` inside `additional_modifications`. The doc comment now says so, and says why the synthetic shape is the right one to test the building block with: `SetName` is the only `Layer::Copy` modification with an observable no later layer can also write, which is what lets the assertion attribute the change to sublayer 1a and nothing else. The mechanism it stands in for — a copy handing its recipient a static that is itself a layer-1 effect — is real and is what CR 707.2c is about. Co-Authored-By: Claude Opus 5 --- ...er_of_many_faces_embalm_copy_panic_5278.rs | 34 +++++++++++++++---- 1 file changed, 27 insertions(+), 7 deletions(-) diff --git a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs index f33f393cc3..53e707c21c 100644 --- a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs +++ b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs @@ -279,7 +279,11 @@ fn embalm_copy_of_bear_survives_and_carries_stamped_exceptions() { // Accept the enter-as-copy replacement, then pick the Bear to copy. This is // the exact sequence that panicked before the fix. - let token = 'drive: loop { + // Bounded, not `loop`, for the same reason as `resolve_embalm_copy_of`: the + // `Priority` arm acts on every trip, so an engine change that stops surfacing + // `CopyTargetChoice` would spin here forever and hang CI instead of failing. + let mut chosen = None; + for _ in 0..64 { match runner.state().waiting_for.clone() { WaitingFor::ReplacementChoice { .. } => { runner @@ -299,14 +303,16 @@ fn embalm_copy_of_bear_survives_and_carries_stamped_exceptions() { target: Some(TargetRef::Object(target)), }) .expect("choose copy target (Bear)"); - break 'drive source_id; + chosen = Some(source_id); + break; } WaitingFor::Priority { .. } => { runner.act(GameAction::PassPriority).expect("pass priority"); } other => panic!("unexpected waiting_for during entry: {other:?}"), } - }; + } + let token = chosen.expect("the Embalm token never reached its copy-target choice"); // Drain any residual priority so SBAs run. for _ in 0..16 { @@ -394,7 +400,9 @@ fn embalm_copy_declined_enters_as_zero_zero_and_dies() { // Decline the enter-as-copy replacement (index 1 = decline on an optional // replacement). The token stays a copy of Vizier (0/0) with the Embalm // exceptions already stamped at creation, then dies to CR 704.5f. - let token = loop { + // Bounded for the same CI-hang reason as the drive loop above. + let mut declined = None; + for _ in 0..64 { match runner.state().waiting_for.clone() { WaitingFor::ReplacementChoice { candidates, .. } => { // Positive reach-guard: the enter-as-copy replacement really @@ -415,14 +423,16 @@ fn embalm_copy_declined_enters_as_zero_zero_and_dies() { runner .act(GameAction::ChooseReplacement { index: 1 }) .expect("decline enter-as-copy replacement"); - break entering; + declined = Some(entering); + break; } WaitingFor::Priority { .. } => { runner.act(GameAction::PassPriority).expect("pass priority"); } other => panic!("unexpected waiting_for before decline: {other:?}"), } - }; + } + let token = declined.expect("the Embalm token never reached its enter-as-copy choice"); // Drain to let SBAs run. for _ in 0..16 { @@ -526,7 +536,17 @@ fn a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy() { /// values — so that second-generation copy effect belongs to the same layer 1, /// not to the next pass. /// -/// Board: the lord's printed base carries "each OTHER creature is named Faceless +/// Board: SYNTHETIC, and deliberately so. No printed card carries a board-wide +/// `SetName` static — on real cards `SetName` shows up as a copy EXCEPTION, +/// paired with `CopyValues` inside `additional_modifications` (Vizier's own +/// Embalm line is one). What is being tested here is the building block, not a +/// card: `SetName` is the only `Layer::Copy` modification with an observable that +/// no later layer can also write, which is what makes the assertion below +/// attribute the change to sublayer 1a and nothing else. The mechanism it stands +/// in for — a copy handing its recipient a static that is itself a layer-1 effect +/// — is real and is what CR 707.2c is about. +/// +/// So: the lord's printed base carries "each OTHER creature is named Faceless /// Reflection" (`FilterProp::Another` excludes the source from its own effect). /// The Embalm token copies the lord (CR 707.2), which makes the TOKEN a second /// source of that same static — and the token's instance, unlike the lord's, From 6ee0d248a97b802eec92cf23e923d7a7cce8c400 Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 16:09:44 -0700 Subject: [PATCH 6/7] docs(engine): state what the copy-sublayer identity actually guarantees MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CopySublayerEffectId` claimed more than provenance-only identity delivers. It wraps `ContinuousEffectGroupKey`, and one of that key's three arms — `Static { source, definition_index }` — is a POSITION in `obj.static_definitions`, a vector `apply_copiable_values` assigns wholesale. So the old wording ("once a slot has started applying, re-reading it later is the SAME effect") is exact for `Transient` and `GrantedStatic` but only exact-under-a-caveat for `Static`: in principle slot i could denote a different ability in generation N+1, and the newcomer would be filtered out as already-started while the effect it displaced kept re-applying. That is the case the deleted `same_copy_effect` payload compare used to cover. The payload compare stays gone — it is what made termination unprovable and what the generation cap was hiding. The residual is documented as accepted-unreachable instead, with the invariant stated and the argument spelled out rather than asserted: no board can produce a `Static`-keyed `Layer::Copy` effect. Both engine construction sites of a copy-layer modification install through `add_transient_continuous_effect`; `expand_granted_static_effects` sets `def_index: None`; and card data — the only other producer of `StaticDefinition`s — never puts a copy-layer modification in a printed `static_abilities`. In the generated pool the six `is_copy_layer` variants appear inside a `StaticDefinition` in exactly four places, all `GenericEffect` payloads (Awakening of Vitu-Ghazi, Tenth District Hero, The Curse of Fenric, The Irencrag, all `SetName`), which resolve through `register_transient_effect`. So no route into an object's `static_definitions` — copy payload, `GrantStaticAbility` graft, `RetainPrintedAbilityFromSource` graft — can carry one either. No test: closing this means de-positioning a key shared by all seven layers, and pinning today's behaviour in an assertion would encode the wrong answer and turn the suite red on the day it is fixed. Two smaller notes on seams a reviewer read as claiming more than they do: - `copy_grants_copy_layer_static` and `apply_continuous_effect` disagree about whether a `CopyValues` nested in a payload's `static_definitions` is possible — this one buys a discovery generation for it, that one panics on it. Recorded as deliberate: the expect is where the invariant is enforced, so a future reachable case is a construction-site bug, not a case to admit by special-casing it out of `is_copy_layer`'s totality. - The `GrantedStatic` half of the "why build on `ContinuousEffectGroupKey`" argument is forward-looking, not load-bearing: that arm is live in the layers sharing the key, but reaching it from the copy sublayer needs a `GrantStaticAbility` whose inner definition is copy-layer, and no card prints one. Comments only; no behavioural change. Co-Authored-By: Claude Opus 5 --- crates/engine/src/game/layers.rs | 65 ++++++++++++++++++++++++++++++-- 1 file changed, 61 insertions(+), 4 deletions(-) diff --git a/crates/engine/src/game/layers.rs b/crates/engine/src/game/layers.rs index 858cdc8928..3fa67da3ae 100644 --- a/crates/engine/src/game/layers.rs +++ b/crates/engine/src/game/layers.rs @@ -4000,6 +4000,20 @@ fn copy_grants_copy_layer_static(modification: &ContinuousModification) -> bool // static is 604.2.) .filter(|def| def.mode == StaticMode::Continuous) .flat_map(|def| def.modifications.iter()) + // `is_copy_layer` is total over `Layer::Copy`, so it answers `true` for + // a nested `CopyValues` too. That sub-case is unreachable, for the same + // reason `apply_continuous_effect`'s `"CopyValues must originate from a + // transient continuous effect"` expect is sound: every construction of + // `CopyValues` installs it through `add_transient_continuous_effect` + // (`become_copy::apply_precomputed_copy_values`, + // `merge::install_merge_layer_effect`) and card data never authors one, + // so no `StaticDefinition` — and hence no `CopiableValues` snapshot of + // one — can hold it. The two seams therefore disagree about whether the + // state is possible — this one buys a discovery generation for it, that + // one panics on it — and the disagreement is deliberate: the expect is + // where the invariant is enforced, so a future reachable case is a + // construction-site bug to fix there, not a case to quietly admit by + // special-casing it out of this totality. .any(ContinuousModification::is_copy_layer), // CR 707.9a: the unbounded retain merges the LIVE source's // `base_static_definitions`, which are not in this payload to inspect, so @@ -4141,9 +4155,9 @@ fn apply_copy_sublayer_to_fixed_point( /// PROVENANCE ONLY — never the payload. CR 707.2c: "If a static ability generates /// a continuous effect that's a copy effect, the copiable values that effect /// grants are determined only at the time that effect first starts to apply." So -/// once a slot has started applying, re-reading it later in the same sublayer is -/// the SAME effect granting the SAME locked values; a rewritten payload does not -/// mint a new one. Keying on the payload instead both contradicts that rule and +/// re-reading a provenance that has already started is the SAME effect granting +/// the SAME locked values; a rewritten payload does not mint a new one. Keying on +/// the payload instead both contradicts that rule and /// makes the loop non-convergent by construction — two permanents that rewrite /// each other's static sets would produce payload-distinct identities forever, /// which is exactly what a generation cap used to have to paper over. @@ -4155,7 +4169,50 @@ fn apply_copy_sublayer_to_fixed_point( /// CR 613.6 bug — the loop calls an effect new while the applier hands it back an /// already-locked affected set); and it distinguishes /// `GrantedStatic { grant_origin, recipient }` occurrences that a raw -/// `(source_id, def_index, transient_id)` tuple collides on. +/// `(source_id, def_index, transient_id)` tuple collides on. That second reason is +/// forward-looking here, not load-bearing: `GrantedStatic` is live in the layers +/// that share this key, but reaching it from THIS loop needs a +/// `GrantStaticAbility` whose inner definition is copy-layer, and no card prints +/// one (see the pool argument below). Sharing the key is still the right call — +/// the alternative is a second identity that can drift from the one +/// `apply_continuous_effect` enforces. +/// +/// The three provenances are not equally precise, and this identity claims no more +/// than each one carries. `Transient { continuous_effect_id }` names an allocated +/// id and `GrantedStatic { grant_origin, recipient }` names a grant origin — +/// nothing in a layer pass can make either denote a different ability. +/// `Static { source, definition_index }` names a POSITION in +/// `obj.static_definitions`, and [`apply_copiable_values`] assigns that vector +/// wholesale from the copy payload. So slot `i` could in principle denote a +/// different static ability in generation N+1 than in generation N, and the +/// newcomer would be filtered out as already-started while the effect it displaced +/// kept re-applying. +/// +/// That board is not constructible today, and the residual is accepted rather than +/// closed: `ContinuousEffectGroupKey` is the shared key `started_effect_sets` uses +/// in every layer, so de-positioning its `Static` arm is a change to all seven +/// layers rather than to this loop. Reaching the hazard needs a `Layer::Copy` +/// effect that is `Static`-keyed, and no producer makes one: +/// +/// - Both engine construction sites of a `Layer::Copy` modification — +/// `become_copy::apply_precomputed_copy_values` and +/// `merge::install_merge_layer_effect` — install through +/// `add_transient_continuous_effect`, so the effect is `Transient`-keyed. +/// - `expand_granted_static_effects` sets `def_index: None`, so a granted +/// copy-layer static is `GrantedStatic`-keyed. +/// - Card data is the only other producer of `StaticDefinition`s, and in the +/// generated pool every copy-layer modification that sits inside one at all sits +/// inside a `GenericEffect` payload — Awakening of Vitu-Ghazi, Tenth District +/// Hero, The Curse of Fenric, The Irencrag, all `SetName` — which +/// `effects::effect` resolves through `register_transient_effect`. No card +/// carries a copy-layer modification in its printed `static_abilities`, so no +/// route into an object's `static_definitions` can carry one either: the copy +/// payload ([`apply_copiable_values`]), the `GrantStaticAbility` graft, and the +/// `RetainPrintedAbilityFromSource` graft all replay card-data statics. +/// +/// The first card to print a copy-layer static ability directly — rather than +/// creating its copy effect from a resolving spell or ability — is the trigger to +/// revisit this. /// /// `mod_index` rides alongside because the group key deliberately drops it — every /// modification of one definition shares one CR 613.6 affected set — while this From 2087430704e3663caabe9c588bf653983f7ea4cd Mon Sep 17 00:00:00 2001 From: mcbradd <35860549+mcbradd@users.noreply.github.com> Date: Sun, 2 Aug 2026 21:54:22 -0700 Subject: [PATCH 7/7] test(engine): pin the copy token's own P/T against its self-excluding anthem MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The anthem regression asserted the vanilla (both instances apply) and the original lord (the copy's instance only), but never the token itself — the third recipient class, and the only object the two anthem instances must disagree about. The token is excluded from the anthem it copied (`FilterProp::Another`, CR 613.1g) and pumped by the original lord's, so a bug that swapped which instance excludes which recipient would move this total while leaving both existing assertions plausible. Co-Authored-By: Claude Opus 5 --- .../vizier_of_many_faces_embalm_copy_panic_5278.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs index 53e707c21c..9b6842a064 100644 --- a/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs +++ b/crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs @@ -527,6 +527,20 @@ fn a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy() { "the original 1/1 lord must get +1/+1 from the token's copied anthem \ (\"other creatures\", so its own anthem does not pump it)" ); + // The third recipient class, and the only one where the two anthem instances + // must disagree about the SAME object: the token is excluded from the anthem + // it copied (`FilterProp::Another`) and pumped by the original lord's, so a + // bug that swapped which instance excludes which recipient would move this + // total while leaving the two above plausible. + assert_eq!( + ( + runner.state().objects[&token].power, + runner.state().objects[&token].toughness + ), + (Some(2), Some(2)), + "the 1/1 token must get +1/+1 from the ORIGINAL lord's anthem only — its \ + own copied anthem says \"other creatures\" and cannot pump itself" + ); } /// CR 613.2a + CR 613.2c: sublayer 1a must reach a FIXED POINT before layer 2.