fix(engine): gather copy-granted static abilities in the pass that copied them - #6932
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughLayer evaluation now applies Layer 1 copy effects to a provenance-based fixed point. It rebuilds static-source data when copy effects or stickers add continuous generators. Incremental evaluation escalates only for reachable copies that can add such generators. Tests cover same-pass effects and Embalm copies. ChangesCopy-generated static abilities
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant LayerEvaluation
participant CopyEffects
participant StaticSourceIndex
LayerEvaluation->>CopyEffects: collect and apply ordered Layer 1 effects
CopyEffects->>LayerEvaluation: expose copied characteristics and new provenance
LayerEvaluation->>StaticSourceIndex: rebuild sources for new continuous generators
StaticSourceIndex-->>LayerEvaluation: return sources for further copy discovery
LayerEvaluation->>CopyEffects: repeat until the fixed point
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/engine/src/game/layers.rs (1)
3855-3885: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an exhaustive match over the
other =>wildcard.
copy_grants_continuous_staticmatches specificContinuousModificationvariants, then falls back to a wildcardother =>arm for everything else. Thedebug_assert_eq!documents intent, but it only fires in debug and test builds. If a futureLayer::Copyvariant is added toContinuousModification, this function silently classifies it via the wildcard instead of forcing a compile error that flags the new variant for explicit review.List every remaining variant explicitly (even if several arms return
truefor the same reason) so the compiler enforces exhaustiveness the next timeContinuousModificationgrows.As per path instructions, "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants" is a flagged pattern forcrates/**/*.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/layers.rs` around lines 3855 - 3885, Replace the wildcard `other` arm in `copy_grants_continuous_static` with explicit arms for every remaining `ContinuousModification` variant, preserving each variant’s current classification and removing the debug assertion. Keep the existing special-case arms unchanged, so the match becomes exhaustive and future enum additions require explicit handling.Source: Path instructions
crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs (1)
466-485: 📐 Maintainability & Code Quality | 🔵 TrivialConsider asserting the token's own final P/T.
The test checks
vanillaandlordpower/toughness, but not the copiedtokenitself. Adding an assertion that the token ends at (2, 2) (excluded from its own copied anthem, but pumped by the original lord's "other creatures" anthem) would tighten the regression against a future bug where the token's own anthem incorrectly excludes or includes the wrong recipients.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs` around lines 466 - 485, Extend the regression assertions after the existing vanilla and lord checks to validate the copied anthem token’s final power and toughness are both 2. Use the existing token object identifier and preserve the expected recipient behavior: the token receives the original lord’s anthem but not its own copied anthem.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs`:
- Around line 401-429: Bound the 'drive loop that waits for
WaitingFor::CopyTargetChoice using the same iteration-limited pattern as
activate_embalm. Keep the existing state handling, but add a finite limit and
panic with a clear diagnostic if the target-choice state is not reached before
the limit.
---
Nitpick comments:
In `@crates/engine/src/game/layers.rs`:
- Around line 3855-3885: Replace the wildcard `other` arm in
`copy_grants_continuous_static` with explicit arms for every remaining
`ContinuousModification` variant, preserving each variant’s current
classification and removing the debug assertion. Keep the existing special-case
arms unchanged, so the match becomes exhaustive and future enum additions
require explicit handling.
In
`@crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs`:
- Around line 466-485: Extend the regression assertions after the existing
vanilla and lord checks to validate the copied anthem token’s final power and
toughness are both 2. Use the existing token object identifier and preserve the
expected recipient behavior: the token receives the original lord’s anthem but
not its own copied anthem.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cbe5458-d879-4d88-ad09-91a96f76a046
📒 Files selected for processing (2)
crates/engine/src/game/layers.rscrates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Copy-granted Layer-1 statics are never applied. Evidence: crates/engine/src/game/layers.rs:2180-2192 rebuilds StaticSourceIndex after the initial copy pass, gathers the copied source's static effects, then unconditionally skips the resulting Layer::Copy bucket. Why it matters: copying a permanent whose copied static itself creates a copy effect (for example, an "other creatures are copies of …" static) leaves that static permanently inert; every full pass resets the source to its non-generator base before repeating the same skip. Suggested fix: make Layer 1's source discovery/application reach a rules-correct fixed point (while retaining timestamp/dependency ordering), and add a real-pipeline regression where the copied static produces a Layer-1 copy effect.
[LOW] The newly added priority driver can hang indefinitely. Evidence: crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs:401-429 uses an unbounded 'drive loop even though activate_embalm directly above uses a bounded wait. Why it matters: a regression that leaves the runner in WaitingFor::Priority turns a failing test into a hung CI shard. Suggested fix: bound this loop (as activate_embalm does) and panic with the last unexpected progression state when CopyTargetChoice is not reached.
…pied 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 <noreply@anthropic.com>
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 phase-rs#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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
…recipient 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 <noreply@anthropic.com>
…tic SetName board 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 <noreply@anthropic.com>
`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 <noreply@anthropic.com>
… anthem 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 <noreply@anthropic.com>
1620a32 to
2087430
Compare
|
Re-review requested on new head Both confirmed and fixed. The
Two design points I want to state rather than have you infer:
Real-pipeline regression, with the discrimination run. Capping Restored: The board is synthetic and the test says so at length: no printed card carries a board-wide
Branch is rebased onto current |
|
Both nitpicks addressed — one taken, one refuted with evidence. Token's own final P/T — taken. Added. The token is a 1/1 copy of the lord that its own copied anthem excludes ( Exhaustive match over the The concern is that a future
That is the complete Also worth noting: the The residual risk you are pointing at is real but sits one seam up and predates this PR: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/layers.rs (1)
3959-3968: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGuard the wildcard arms with
debug_assert!(!modification.is_copy_layer()).Both classifiers end in
_ => false. The comment argues the arm is exact for layers 2-7. That is true today. If a futureLayer::Copyvariant is added, both classifiers answerfalsesilently, and a copy that grants a generator becomes invisible to layers 2-7 again — the exact defect this PR repairs. The pinning testcopy_grants_continuous_static_covers_every_copy_layer_variantuses a hand-maintained table, so it would not catch the new variant either.
ContinuousModification::is_copy_layeris total and panic-free, so it is safe to assert on inside these arms, unlikelayer().♻️ Proposed hardening for both wildcard arms
// 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, + // 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. `is_copy_layer` is the panic-free + // companion, so a new `Layer::Copy` variant fails a debug build here + // rather than being silently classified as a non-generator. + other => { + debug_assert!( + !other.is_copy_layer(), + "a new Layer::Copy modification must be classified explicitly: {other:?}" + ); + false + }Apply the same change to the
_ => falsearm ofcopy_grants_copy_layer_static.Also applies to: 4024-4028
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/layers.rs` around lines 3959 - 3968, Update both wildcard `_ => false` arms in the classifiers near `copy_grants_copy_layer_static` and its companion function to first `debug_assert!(!modification.is_copy_layer())`. Keep returning false afterward, using the total, panic-free `ContinuousModification::is_copy_layer` check to detect future copy-layer variants.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/game/layers.rs`:
- Around line 3959-3968: Update both wildcard `_ => false` arms in the
classifiers near `copy_grants_copy_layer_static` and its companion function to
first `debug_assert!(!modification.is_copy_layer())`. Keep returning false
afterward, using the total, panic-free `ContinuousModification::is_copy_layer`
check to detect future copy-layer variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ca3f440-74d6-4b7d-ba2e-c5d8ad7b6a65
📒 Files selected for processing (4)
crates/engine/src/game/layers.rscrates/engine/src/game/static_source_index.rscrates/engine/src/types/layers.rscrates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs
|
You corrected the weakest part of my last reply — the pinning test is a hand-maintained table and would not catch a new variant, so pointing at it as coverage was wrong. But the proposed guard does not close the gap either, and it fails on exactly the scenario it names.
The assert can only fire in the narrower case where someone adds the variant to Your premise is right, though: the residual risk is real and my last reply under-answered it. The fix that actually closes it is one seam up — make |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the Layer 1 fixed-point repair is at the source-index/layer-evaluation seam, preserves provenance and ordering, and has discriminating production-pipeline coverage for both copied statics and chained Layer-1 effects.
2 participants
Summary
A static ability a permanent has only because a layer-1 copy effect gave it never applied under full layer evaluation — not on the pass that applied the copy, and not on any later pass either. A token copying a lord ran as a permanent with no ability.
Two halves, both of CR 613.2c. The first revision closed the layers-2-7 half (refresh the generator index after copies, so an anthem granted by a copy fans out on that same pass). Maintainer review found the layer-1 half still open and self-perpetuating: a copy-granted static that itself generates a
Layer::Copyeffect was gathered into a bucket nothing ever applied. Layer 1 now runs to a fixed point.Files changed
crates/engine/src/game/layers.rs— extend the post-layer-1StaticSourceIndexrefresh to cover copy effects, not only stickers; move it below the layer-1b face-down reseed; addcopy_grants_continuous_static. Replace the single-shot layer-1 apply withapply_copy_sublayer_to_fixed_point; addcopy_sublayer_effect_idandcopy_grants_copy_layer_static.crates/engine/src/game/static_source_index.rs— exposedefs_source_continuous_effectso the payload-side predicate mirrors the index's own rather than restating it.crates/engine/src/types/layers.rs—ContinuousModification::is_copy_layer, so "is this a layer-1 modification?" has one name instead of an inlinematches!at each seam.crates/engine/tests/integration/vizier_of_many_faces_embalm_copy_panic_5278.rs—build_lord_scenariohelper;a_copy_granted_static_ability_applies_in_the_pass_that_applied_the_copy;a_copy_granted_layer_one_static_applies_inside_the_same_layer_one_pass; bounded drive loops; the copy token's own P/T pinned against its self-excluding anthem.Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 613.2a + CR 613.2c (the defect, both halves); CR 707.2c / CR 707.9a / CR 707.9b (the copy-layer classification in
copy_grants_continuous_staticandcopy_grants_copy_layer_static); CR 604.2 + CR 611.2 (what makes a definition a continuous-static generator); CR 613.7 (timestamp order, retained across generations)The defect
evaluate_layersgathers the pass's continuous effects below the copy application — Step 3 already sits after Step 2, so the ordering was right. What was wrong is the index it consults.StaticSourceIndexdecides which permanents get scanned as static-ability generators, and it is rebuilt 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. So a token copying a lord was scanned as a permanent with no ability. Every later pass repeats the reset, so this is not a one-pass-behind stale read — the copied anthem never applied at all.
CR 613.2a puts copy effects in layer 1a, and CR 613.2c says that after all of layer 1 has been applied the object's characteristics are its copiable values. The abilities layers 2–7 are derived from are therefore the post-layer-1 ones, and a static granted there has to generate its effect on that same pass.
Fix, part 1 — the generator index (layers 2–7)
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:
The copy side is narrowed by payload, not by "did layer 1a run".
ContinuousModification::CopyValuescarries the whole copiable static set in the modification, andapply_copiable_valuesassigns that set wholesale, socopy_grants_continuous_staticcan mirror the index's own generator predicate — anyStaticMode::Continuousdef — exactly, with no id plumbing out of the apply path and no battlefield rescan. That matters for cost, not just tidiness: a clone of a vanilla creature is a permanent-duration TCE that outlives the clone, so a coarse "any copy ran" guard would charge 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 other 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 this index's own 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; that is a non-local fact about
apply_face_down_creature_characteristics, not the rule cited at the seam, and I would rather the placement rest on the rule.Boards with neither copies nor stickers pay nothing, and the top-of-pass rebuild contract — including the
rebuild_static_index_at_topplacement-regression toggle — is unchanged.Fix, part 2 — layer 1 reaches a fixed point (the review finding)
Part 1 refreshed the index and then the pass moved on. If the copy-granted static is itself a layer-1 effect (CR 707.2c: a static "other creatures are copies of …"), it lands in a
Layer::Copybucket that has already been applied — so it never applies, and every full pass resets the source to its non-generator base and repeats the skip. Permanently inert, exactly as the review states.apply_copy_sublayer_to_fixed_pointreplaces the single-shot apply: gatherLayer::Copy, apply inorder_active_continuous_effectsorder, repeat while a generation put a copy-layer static on the board, rebuildingStaticSourceIndexbetween generations so the next gather sees it.Ordering is retained, not approximated. Each generation re-orders the accumulated set rather than the fresh effects alone, and re-applies from scratch after clearing the
Layer::Copyattribution bucket, so a second-generation effect that should sort before a first-generation one still does (CR 613.7).Identity is provenance, never payload.
copy_sublayer_effect_idis built onContinuousEffectGroupKey— source, target, group — and deliberately does not hash the modification. Keying on payload would make the loop non-convergent by construction: applying a copy rewrites the very characteristics the payload describes, so the same effect re-hashes each generation and re-enters the fresh set forever. That is the failure a generation cap papers over, so there is no cap; convergence is structural —started_idsis monotone over a finite battlefield.The re-entry test is strictly narrower than the index test, and the two are not interchangeable.
copy_grants_copy_layer_staticasks "does this payload grant a static that is itself aLayer::Copyeffect?";copy_grants_continuous_staticasks "does the recipient become a generator at all?". An anthem makes its recipient a generator for layers 2–7 but can never appear in aLayer::Copygather, so it must not buy a discovery generation.copy_grants_continuous_static_covers_every_copy_layer_variantpins the wholeLayer::Copyset against both columns, and the anthem row is where they must disagree.apply_layers_incrementalis untouched, and now says whyAn earlier revision of this branch carried the same disjunct into the incremental arm for symmetry. That was wrong twice over, and I removed it.
It guards nothing reachable. A copy applied in that arm could only add a generator by landing on a recipient, and it cannot:
recipient_idsisentered_idsalone. The one way a pre-existing host joins the set is an attached entrant, andentered_object_blocks_incrementalguard (3) rejects any entrant withattached_to.is_some()outright, escalating to a full pass.layers_dirty.mark_full()(apply_resolved_continuous_effect,game_state.rs) — so that flush is a full pass, not this arm.And it would not even have produced agreement. The incremental arm applies layers 2–7 only to
recipient_ids, so a granted static would fan out over the recipients alone while the full pass derives it board-wide — a different wrong answer, not the same one. Escalating instead of rebuilding would have been the correct shape, but it is equally unreachable, so it would have been an untestable guard shipped on speculation.What ships instead is a comment at the seam recording both blockers by name. Everything executable in this PR has a test that fails without it.
How it was found
The differential harness on this branch's sibling, comparing the entry-incremental flush against a full re-evaluation of the same post-entry board. The disagreement is also what identifies the culprit: the incremental arm resets only the entering objects, so a copy token's post-layer-1 static was still live at gather time and its grant applied; the full arm reset the copy first and lost it. The incremental arm was the correct one.
The layer-1 half was not found that way — it came from maintainer review, and the harness would not have caught it, because both arms skip the bucket identically and therefore agree.
Verification
Tilt was down for this session —
tilt get uiresource clippyexits1— so the cargo lines below were run directly rather than read off Tilt resources.Branch is rebased onto current
main(7fa1fc4a1), so the parse-diff baseline is not stale.cargo fmt --all -- --check— clean, exit 0../scripts/check-parser-combinators.sh— exit 0;Gate A PASS,Gate G PASS(full output below).cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings— clean, exit 0. (CI's exact lint line.)cargo test -p phase-engine—22769 passed; 0 failed; 8 ignored, across all five engine binaries.Revert-check chore: update coverage stats and badges #1 (discriminating, run) — the generator index. Drop
copy_added_generator ||from the guard, keep the tests:left: (Some(3), Some(3))is the bug exactly: only the original lord's anthem applied; the copy's contributed nothing. Restore the disjunct and the module is green. The two pre-existing tests in the file stay green in both directions — neither copies a permanent that carries a static ability, which is why the module's existing coverage never caught this.Revert-check chore: update coverage stats and badges #2 (discriminating, run) — the layer-1 fixed point. Cap
apply_copy_sublayer_to_fixed_pointat a single generation (return right after the first apply):Restored:
test result: ok. 1 passed; 0 failed. The vanilla is renamed in both directions — generation 1, the lord's own instance, reaches it either way — so thelord.nameassertion carries the discrimination alone. That is deliberate:SetNameis the onlyLayer::Copymodification with an observable no later layer can also write, andFilterProp::Anothermeans only the token's instance can rename the lord.The first revert-check also demonstrates the narrowed predicate is exercised positively: this board has no stickers, so
stickers_appliedis false and the test can only pass viacopy_grants_continuous_staticreturning true.All CR numbers cited in the diff (
613.2a,613.2c,613.7,604.2,611.2,707.2c,707.9a,707.9b) were grep-verified againstdocs/MagicCompRules.txtand each describes the code it annotates.ai-gatedoes not trigger on this PR:.github/workflows/ai-gate.yml'spull_requestpaths:filter excludescrates/engine/**. No AI behavior changed and no baseline was refreshed.Gate A
Anchored on
crates/engine/src/game/layers.rs:2134(at base70c6f4c51) — the sticker branch: the pre-existing authority for "a layer-1 producer turned a non-generator into a continuous static source mid-pass, so refresh the generator index before the main gather." Same seam, same remedy, now shared rather than duplicated.crates/engine/src/game/static_source_index.rs:66(at base70c6f4c51) —object_sources_continuous_effect, the index's own generator predicate.copy_grants_continuous_staticis deliberately the same question asked of aCopyValuespayload instead of a live object, which is why the narrowing is exact rather than heuristic.Claimed parse impact
None. No parser file is touched and no card's parse result changes.
Scope Expansion
The layer-1 fixed point (
apply_copy_sublayer_to_fixed_point,copy_sublayer_effect_id,copy_grants_copy_layer_static,is_copy_layer, and the second regression test) is beyond the PR as originally opened. It is the direct remedy for the[HIGH]finding on the first review and is declared rather than folded in silently.Validation Failures
None.
CI Failures
None.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests