fix(engine): escalate entry-incremental flush when an entrant's card types are rewritten - #6846
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIncremental layer evaluation now tracks characteristic dependencies and modification writes. Full and incremental paths share reset logic. Transient effects use centralized gate evaluation. Regression tests cover engine and integration scenarios. ChangesIncremental layer evaluation
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant IncrementalEvaluation
participant LayerPreparation
participant CharacteristicAnalysis
participant TransientGateEvaluation
participant FullEvaluation
IncrementalEvaluation->>LayerPreparation: prepare incremental flush
LayerPreparation->>CharacteristicAnalysis: classify reads and writes
CharacteristicAnalysis-->>LayerPreparation: return dependency results
LayerPreparation->>TransientGateEvaluation: probe duration and retained conditions
TransientGateEvaluation-->>LayerPreparation: return gate perturbations
LayerPreparation->>FullEvaluation: escalate when detected changes invalidate probes
LayerPreparation-->>IncrementalEvaluation: continue incremental evaluation otherwise
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/game/layers.rs`:
- Around line 3642-3659: Close the uncovered controller channel in the
incremental-layer gate by adding a battlefield ChangeController-reaching-entrant
disjunct alongside the existing card-type-writer disjunct. Update the relevant
logic around modification_writes_card_types and
entered_object_blocks_incremental so entrants that may move between
controller-keyed populations after probing force the conservative path;
alternatively, add a focused synthetic tripwire test beside
known_gap_color_keyed_population_probes_entrant_pre_layer that asserts this
divergence.
🪄 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: 60e0e068-d0d9-44fb-af95-973d8cf0a30c
📒 Files selected for processing (5)
crates/engine/src/game/layers.rscrates/engine/src/game/stack.rscrates/engine/tests/integration/ashaya_nontoken_lands.rscrates/engine/tests/integration/life_and_limb_sylvan_advocate.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the entry-flush gate leaves a live controller-channel blind spot.
[HIGH] population_probe_blinded_by_entrant_characteristic_change only proceeds when modification_writes_card_types accepts the effect (layers.rs:3669-3677). The exhaustive classifier explicitly returns false for ChangeController (layers.rs:3750-3755), so an entrant reached by a layer-2 control-changing effect cannot cause the full re-evaluation even when a later population read is controller-scoped. That can leave pre-existing recipients derived from the entrant’s pre-layer controller population.
This is not merely hypothetical documentation debt: the new doc block describes the same open path at layers.rs:3642-3651, and CR 613.1b places control-changing effects before subsequent layer reads. Please either cover this controller writer in a controller-aware escalation disjunct with a discriminating full-versus-incremental test, or add a dedicated executable tripwire that pins the acknowledged divergence before landing this partial gate.
|
Closed the controller channel rather than pinning it — What changed
Parameterizing instead of adding a second bool classifier is the CLAUDE.md sibling-cluster rule: two predicates differing only in which characteristic they name is exactly the shape that compounds. The axis stays inside one CR section — both keys are written within CR 613 — so the categorical-boundary rule holds, and closing the remaining kinds (COLOR, KEYWORD, NAME, P/T) means adding variants here rather than growing a sibling classifier. Controller is deliberately not folded into the "other characteristics" paragraph: CR 109.3 states an object's controller is not one of its characteristics, so those are separate claims. What unifies them for this gate is only that Discriminating test
It is discriminating in both directions:
The fixture also pins CostNil in practice — battlefield Still open, still declaredGRANT CHAINS (an effect that grants a type-writing static rather than writing types itself) and the COLOR/KEYWORD/NAME/P/T matrix. Verification at
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/stack.rs (1)
10272-10326: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRemove the known color-rewrite under-escalation.
This test proves that the incremental path violates CR 613 layer ordering. The layer-5
AddColoreffect makes the entrant green before the layer-7c count applies, but the incremental path leaves existing creatures at 4/4 while full evaluation produces 5/5.Extend the entrant characteristic-write classification to cover color reads and color writes. Then require escalation and full-board identity in this test. Do not retain a passing test that accepts stale derived state.
As per path instructions, “Surface GAPS (missing or wrong behavior), not style nits.”
🤖 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/stack.rs` around lines 10272 - 10326, Extend the characteristic-write classification used by population_probe_blinded_by_entrant_characteristic_change to recognize COLOR reads paired with COLOR writes, including AddColor layer-5 effects, so the entrant’s rewritten color triggers escalation before the layer-7c count. Update known_gap_color_keyed_population_probes_entrant_pre_layer to require escalation, assert normal and forced board identity, and remove the stale incremental 4/4 expectation.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/engine/src/game/stack.rs`:
- Around line 10272-10326: Extend the characteristic-write classification used
by population_probe_blinded_by_entrant_characteristic_change to recognize COLOR
reads paired with COLOR writes, including AddColor layer-5 effects, so the
entrant’s rewritten color triggers escalation before the layer-7c count. Update
known_gap_color_keyed_population_probes_entrant_pre_layer to require escalation,
assert normal and forced board identity, and remove the stale incremental 4/4
expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 42b278a4-518a-4a76-9206-4daefeae6096
📒 Files selected for processing (2)
crates/engine/src/game/layers.rscrates/engine/src/game/stack.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/layers.rs
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — this current head knowingly ships a color-rewrite under-escalation.
Blocker
[HIGH] crates/engine/src/game/layers.rs:3663-3685 only escalates this pre-layer population-probe path when modification_population_key_write reports a writer. That classifier returns Some for card-type and controller changes (:3711-3735), but explicitly returns None for SetColor, AddColor, and AddChosenColor (:3772-3778). The color-count fixture demonstrates the result: an AddColor(Green) effect applies in layer 5 while the count applies in layer 7, yet known_gap_color_keyed_population_probes_entrant_pre_layer asserts the normal path remains incremental and leaves existing Bears at stale 4/4, where the forced full pass correctly derives 5/5 (crates/engine/src/game/stack.rs:10272-10325). That is an active CR 613.1e/g layer-ordering error, not an acceptable passing-tripwire baseline.
Please extend the population read/write classification so a color-keyed population read plus a color writer reaching the entrant escalates conservatively, then make this fixture require escalation and normal/full board identity. Do not retain a passing regression that codifies stale derived state. The current controller-channel fix is intact but does not affect this independent color path.
Evidence was rechecked at b471740e12a5c1e22846f69295ae62d2847599fe. CodeRabbit's current-head review identifies the same unresolved defect. Rust CI is still pending for this head; the already completed checks do not exercise this under-escalation.
|
Blocker addressed in The classifier. The test. Discrimination, both directions. Reverting only the classifier's Restored, it passes. Keeping the arm but breaking the escalation plumbing instead fails on the identity assertion over the Bears' derived power/toughness, so the test cannot pass for the wrong reason in either direction. Fast path unaffected. The gate still requires a classified writer to actually REACH an entrant AND a live population read to exist, so boards whose only effects write P/T are untouched. Both deliberately-pinned fast paths still take the incremental arm — Still open, still declared. KEYWORD, NAME and P/T remain the same shape and the doc comment says so. I did not fold them in here because closing them properly needs the full Verification at Requesting re-review. 🤖 Generated with Claude Code |
|
Current-head maintainer hold for |
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new population-key classifier still leaves a reachable keyword-write under-escalation.
[HIGH] population_probe_blinded_by_entrant_characteristic_change treats any population read as unsafe only when modification_population_key_write recognizes the modification (layers.rs:3650-3672). The current exhaustive match explicitly returns None for AddKeyword, RemoveKeyword, and the other layer-6 ability writers (layers.rs:3769-3783), although the dynamic quantity path can count a TargetFilter with WithKeyword / HasKeywordKind (filter.rs:186-195).
Concrete reachable class: a layer-6 static grants flying to creatures and a later layer-7 count gives creatures +X/+X where X is the number of creatures with flying. A nonflying entrant does not perturb that count at the pre-layer probe; it becomes flying before the count resolves, but this gate stays incremental and leaves pre-existing recipients derived from the old count. The same mismatch applies to the declared name/P/T gaps. The long comment correctly names these as open, but a passing test suite may not codify a known stale-derived-state path.
Please replace the one-sided writer list with a typed population read/write-kind relation at the gate: derive the kinds read by the relevant ObjectCount filters/affected sets/conditions, derive the kinds each modification writes, and escalate when a writer reaching an entrant intersects a live read. Keep it conservative for uncertain forms and exhaustive over both surfaces. Add a discriminating full-vs-incremental keyword fixture (and the closest name/P/T sibling coverage required by the relation) that asserts both escalation and pre-existing-recipient identity. This is larger than a local color arm, but it is the correct general boundary for the abstraction this PR introduced.
Current-head review scope: d8e11e9571fe688ff2544336653b6c013c715e8b. Required CI and the current no-change parse-diff artifact are green; neither exercises this unrepresented layer-6-to-layer-7 path.
|
Both sides are typed now and the gate is their intersection. One commit on the branch. Your read was right, and something I found while building it makes the case stronger than "cleaner." I first traced whether the writer list could simply be extended with keyword/name/colour/P-T. It cannot. The read side was untyped, so any widening of the writer list widens every board, and both fast paths this PR deliberately pins — The lattice
The read set has to include affected filters unconditionallyI had this conditional in an earlier draft and it is unsound. Take a board with no counting and no conditions at all: a layer-3 rename plus a static whose affected set keys on names. The read set is empty, nothing intersects, and a pre-existing permanent whose membership the rename flips keeps stale P/T — the same blindness this gate exists to close, reached through a different door. So the read set is the unconditional union of dynamic magnitudes, live conditions, and every live modification's affected filter. CR 613.6 carves out exactly one shapeA static that reads a kind through its own affected filter and writes that kind through its own modification is not a staleness risk against itself. CR 613.6 fixes a continuous effect's set of affected objects the first time it applies and retains it for the rest of the pass, so an effect cannot push an object out of the filter that admitted it — "artifacts that aren't creatures become creatures" reads and writes card types and is fine. The granularity matters and cost me a round: the exclusion is keyed on the CR 613.6 retention group, not on the individual modification. One OrderingCheapest-first, and entrant-independent for as long as possible. The union of all write kinds is pure enum matches. The read set is computed once per flush with an early exit once it saturates. A global disjointness check exits before any affected filter is matched against any entrant — the token-storm board leaves there, since keyword grants write abilities while the board reads card types and controller, with zero EvidenceEight fixtures. Each was checked by reverting the specific classifier row or union term it depends on, confirming it fails with a concrete stale value, and restoring it. Every reverted side exits 101 on its own assertion; every restored side is in the suite run below.
Both keyword sides are reverted separately because either one alone would leave the fixture passing for the wrong reason. The P/T fixture puts the entrant under the opposing player so the toughness setter is the only writer that reaches it — with P/T as a single kind, a P/T-keyed count anthem that also reached the entrant would satisfy the relation on its own and the fixture would pass with the fix removed. Each positive asserts the derived board against a forced full pass as well as the escalation flag, so a fixture that stopped reaching the entrant would fail rather than pass quietly; each negative asserts the incremental arm and board identity, so it cannot pass by not running. Suite at this head: What I removed, and what I leftThe remaining-gap block is deleted, because the matrix is what it was asking for. Grant chains close through One boundary is inherited unchanged and is now stated explicitly rather than left implicit: the reach probe evaluates affected filters against the previous final state, while full evaluation matches them at intermediate layer states, so a count-thresholded affected filter can diverge from the probe in either direction. Typing the relation neither narrows nor widens that. I did not want CR 613-flavoured reasoning at the gate to read as a completeness claim it does not make. Two adjacent defects I am declaring rather than fixing here, both genuinely outside a gate change:
Ready for another look. 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/engine/src/game/stack.rs (1)
11246-11251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the stale fixture reference.
Replace
controller_theft_count_anthem_boardwithcontroller_keyed_count_anthem_with_control_theft_boardin the doc comment.🤖 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/stack.rs` around lines 11246 - 11251, Update the doc comment near the entrant-reaching writer explanation to replace the stale fixture reference `controller_theft_count_anthem_board` with `controller_keyed_count_anthem_with_control_theft_board`, without changing the surrounding behavior description.crates/engine/src/game/filter.rs (1)
694-697: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the
_wildcard in theDistinctFromarm.The doc comment at Line 535-538 states this classifier is "EXHAUSTIVE, wildcard-free". The inner match on
referencebreaks that invariant. The inner enum isTargetFilter, which is known. A newTargetFiltervariant that reads nothing will silently take theALLbranch, so the compiler cannot force a decision at this seam.Use a
matches!test instead, which keeps the same conservative result without a wildcard match arm on a known enum.♻️ Proposed change
- FilterProp::DistinctFrom { reference } => match reference.as_ref() { - TargetFilter::ParentTarget => CharacteristicKinds::EMPTY, - _ => CharacteristicKinds::ALL, - }, + FilterProp::DistinctFrom { reference } => { + if matches!(reference.as_ref(), TargetFilter::ParentTarget) { + CharacteristicKinds::EMPTY + } else { + CharacteristicKinds::ALL + } + }As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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/filter.rs` around lines 694 - 697, Update the FilterProp::DistinctFrom arm to replace the inner wildcard match on TargetFilter with a matches! test for TargetFilter::ParentTarget, preserving CharacteristicKinds::EMPTY for that variant and CharacteristicKinds::ALL otherwise while keeping the classifier wildcard-free.Sources: Coding guidelines, Path instructions
🤖 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/src/game/filter.rs`:
- Around line 417-424: The FilterProp::Owned classifier currently returns EMPTY
despite resolving controller references through source.controller. Update the
FilterProp::Owned arm in crates/engine/src/game/filter.rs at lines 721-723 to
return CharacteristicKinds::CONTROLLER, ensuring both live-object and
zone-change matching invalidate correctly after control changes; the anchor at
lines 417-424 requires no direct change.
In `@crates/engine/src/game/layers.rs`:
- Around line 1255-1257: Add StaticCondition::DuringOpponentsTurn to the EMPTY
arm in static_condition_characteristic_reads_at, alongside DuringYourTurn. Also
update the sibling classifiers static_condition_uses_object_population,
entered_object_perturbs_static_condition, and static_condition_reads_life so
each handles DuringOpponentsTurn consistently without treating it as reading or
perturbing a characteristic.
- Around line 3718-3747: Update live_characteristic_reads to include
static_condition_characteristic_reads(e.condition) for each active effect in the
gather_transient_continuous_effects dependency loop, alongside the existing
modification and target-filter reads. Ensure recipient-context conditions
retained in ActiveContinuousEffect.condition contribute to global before the
dependency gate can skip reevaluation.
---
Nitpick comments:
In `@crates/engine/src/game/filter.rs`:
- Around line 694-697: Update the FilterProp::DistinctFrom arm to replace the
inner wildcard match on TargetFilter with a matches! test for
TargetFilter::ParentTarget, preserving CharacteristicKinds::EMPTY for that
variant and CharacteristicKinds::ALL otherwise while keeping the classifier
wildcard-free.
In `@crates/engine/src/game/stack.rs`:
- Around line 11246-11251: Update the doc comment near the entrant-reaching
writer explanation to replace the stale fixture reference
`controller_theft_count_anthem_board` with
`controller_keyed_count_anthem_with_control_theft_board`, without changing the
surrounding behavior description.
🪄 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: 56d0cace-d833-428c-804e-98a4734cd0a9
📒 Files selected for processing (4)
crates/engine/src/game/filter.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/stack.rs
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current head does not compile, and the characteristic-read cache misses a runtime condition path.
🔴 Critical — compile blocker
crates/engine/src/game/layers.rs:1232-1273 exhaustively groups DuringYourTurn in static_condition_characteristic_reads_at but omits StaticCondition::DuringOpponentsTurn. The current-head Rust lint, Rust/WASM, and test checks are red as a result.
Add the missing arm and audit the sibling condition classifiers for the same exhaustiveness gap before resubmitting.
🟡 Medium — recipient-context characteristic reads are omitted
live_characteristic_reads at layers.rs:3718-3746 scans only printed static-definition conditions. But gather_transient_continuous_effects retains a recipient-context condition into ActiveContinuousEffect (:5416-5423, :5472-5489), and apply_continuous_effect_filtered evaluates it per recipient (:6594-6621). Its characteristic reads must be included in the live-read calculation; otherwise layer dependency/cache behavior can use stale characteristics.
Include that retained condition's reads and add a discriminating runtime regression that exercises the recipient-context path.
Recommendation: resolve both findings and restore SHA-bound Rust/WASM/lint/test health before requesting re-review.
2a3b55b to
d802570
Compare
|
Force-pushed: rebased onto main, plus one new commit. Flagging it because the branch moved under an open review, and because why it went red is worth a paragraph. CI failed on my previous head with
The rebase itself was conflict-free across all four earlier commits — main's drift on the files this touches was 17 lines in One question I raised against myself and then closedClassifying a turn-relative condition as reading nothing invites the obvious objection: No, and the reason is the binding point rather than the CR. Both arms collect their effect set at the same moment — At this head
🤖 Generated with Claude Code |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current head still bypasses a canonical engine authority and leaves the transient-condition dependency path incomplete.
Medium — bypassed keyword authority
crates/engine/src/game/stack.rs:11548 directly evaluates entrant.keywords.contains(&Keyword::Flying). The current head is red on the engine-authority gate for that bypass. Route this through the canonical keyword query, or add the narrowly justified gate annotation if this site genuinely cannot use it.
Medium — active-effect condition reads are still omitted
crates/engine/src/game/layers.rs:3728-3767 does not include active continuous-effect conditions in live characteristic reads. Yet recipient-context transient conditions are stored in :5432-5439 and :5488-5505, then evaluated per recipient in :6610-6636. Their reads must participate in dependency invalidation (with a fail-closed path when necessary); otherwise the layer cache can use stale characteristics.
The new regression at stack.rs:11095-11119 exercises only a printed condition. Add a discriminating resolution-created transient-continuous-effect case that proves recipient-context condition reads are tracked.
Recommendation: restore the keyword-authority invariant and complete the active-condition read path with the resolution-created regression before resubmitting.
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the repaired compile and keyword-authority issues are welcome, but the active continuous-effect dependency path remains incomplete.
Medium — live characteristic reads omit retained active-effect conditions
crates/engine/src/game/layers.rs:3734-3745 collects active-effect quantities and affected filters, but does not read e.condition. Resolution-created recipient-context transient conditions are retained in ActiveContinuousEffect at :5432-5439 and :5488-5505, then evaluated per recipient at :6610-6636. The layer dependency/cache must include those condition reads, with a fail-closed path where the dependency cannot be described; otherwise it can reuse stale characteristics.
The added regression in stack.rs:11033-11119 is printed-static-only and cannot exercise the retained transient-condition path. Add a production-pipeline resolution-created transient continuous effect test that distinguishes normal behavior from forced full recomputation, demonstrating that the recipient-context condition participates in invalidation.
Recommendation: complete the active-effect condition-read handling and add the real TCE discriminator before resubmitting.
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two current-head dependency reads are still omitted, so the incremental layer cache can retain stale derived state.
🔴 Blocker
[MED] Resolution-created recipient-context conditions are absent from the live read union. Evidence: crates/engine/src/game/layers.rs:3734-3762 unions modification quantities, affected filters, and printed static conditions, but never ActiveContinuousEffect.condition; gather_transient_continuous_effects retains that condition at :5432-5439 and stores it at :5488-5505, while apply_continuous_effect_filtered evaluates it for each recipient at :6610-6637. Why it matters: a layer-written characteristic can flip this live per-recipient gate without the entry-incremental path escalating, leaving pre-existing recipients with stale characteristics. CR 611.3a was verified in docs/MagicCompRules.txt:2922 (a static continuous effect “isn’t locked in”). Suggested fix: union the retained active-effect condition through static_condition_characteristic_reads, conservatively where needed, and add a discriminating production-resolution test that creates a transient continuous effect with a recipient-context condition and compares normal versus forced-full recomputation.
[MED] FilterProp::Owned is classified as reading no layer-writable kind although its controller reference is live. Evidence: crates/engine/src/game/filter.rs:721-724 puts Owned { .. } in the EMPTY group, but both live matching (:5203-5248) and zone-change matching (:5955-5999) resolve You, Opponent, ScopedPlayer, and related scopes through current controller context. Why it matters: a control-changing effect (CR 613.1b, verified at docs/MagicCompRules.txt:2962) can change the ownership-relative filter result while the new read/write relation reports no controller dependency. Suggested fix: classify FilterProp::Owned { .. } as CharacteristicKinds::CONTROLLER and add a focused layer-2 discriminator for the affected read path.
✅ Rechecked
The parse-diff sticky artifact is current-head-bound (e7f856cff57faa23e80083d242f0f7d11bdd33e9) and reports no card-parse changes. The submitted Rust/coverage checks are green, but they do not exercise either omitted dependency path.
Recommendation: complete both classifications and their discriminating runtime coverage, then request re-review on the new head.
…types are rewritten
CR 613.1 + CR 613.1d + CR 613.4a + CR 611.3a.
Ashaya, Soul of the Wild's characteristic-defining ability counts "lands you
control" (layer 7a, CR 613.4a) over a board its own layer-4 static feeds
("nontoken creatures you control are Forest lands", CR 613.1d). Layers apply in
order (CR 613.1), so the type rewrite has already happened by the time the CDA's
population read is evaluated -- but the `EnteredObjects` escalation gate probed
the ENTERING object's membership in the counted population using its PRE-layer
characteristics: a creature, not a land. The gate concluded the count was
unperturbed, took the incremental arm, and left Ashaya's power and toughness
stale at 1/1 where a full re-evaluation derives 2/2.
The gate now escalates when a population READ is live AND some active effect
reaching an entrant rewrites that entrant's card types. The read side spans both
channels: an effect's dynamic magnitude or affected set, and a Continuous
static's enabling condition (CR 611.3a -- a static's condition isn't locked in,
so it re-evaluates as the board changes, and Axis 2b probes it per-entrant with
the same pre-layer blindness).
Precise on the write CLASSIFIER (`modification_writes_card_types` is a
wildcard-free match over every `ContinuousModification` variant, so a future
type-rewriting variant cannot be added without deciding this), conservative on
the read side: a counted population is almost always keyed on card type, so
narrowing it would buy nothing while adding a second 98-arm classifier.
Projecting the entrant forward through layer 4 instead would need a speculative
pass transitively closed over grants that unlock further grants.
The write-side precision is load-bearing, not decorative. Escalating on "the
entrant is a recipient of anything" instead regresses the deliberately-pinned
`count_anthem_nonmatching_entry_does_not_escalate_and_matches_full` and
`devotion_gate_colorless_entry_does_not_escalate_and_matches_full` fast paths,
where the effect reaching the entrant writes only P/T and so cannot move a
type-keyed or devotion-keyed count.
`incremental_flush_must_escalate` (test-only) had drifted from the production
gate -- it re-implemented the axes, omitted the recipient-sourced-effect check,
and ran against a board without the recipient reset. It is now a thin wrapper
delegating to `prepare_incremental_flush` on a scratch clone, so the test
predicate and the production gate answer the same question.
The per-object "back to base" reset is now a single authority,
`reset_recipient_to_base`, used by both arms: the full pass applies it
board-wide, the incremental arm applies it to recipients only. The full pass
previously open-coded the same five steps, which is what let a second copy come
into existence. Its one extra behavior -- collecting face-down permanents so
their CR 708.2 profile is re-applied after layer 1a -- stays as a two-line tail;
no signature change is needed because no reset step touches `face_down`.
Known remaining gap, declared not closed: a population keyed on COLOR, KEYWORD,
NAME or P/T whose entrant has that characteristic rewritten by another layer is
still probed pre-layer, on the read side and on the write-side reach probe
alike. Closing it needs the full characteristic-kind matrix (which kind each
`FilterProp` reads by which kind each `ContinuousModification` writes). Its
current behavior is pinned by the synthetic tripwire
`known_gap_color_keyed_population_probes_entrant_pre_layer`, which is expected
to flip when the matrix lands.
Two further channels are named in the same doc block because neither falls under
"other characteristics": CONTROLLER, since a layer-2 `ChangeController`
(CR 613.1b) can move an entrant between "creatures you control" populations and
CR 109.3 says controller is not a characteristic; and GRANT CHAINS, since an
effect that grants a type-writing static rather than writing types itself is a
second-order path the classifier cannot see.
Found by differential verification against full re-evaluation during
development. The regression test is discriminating: with the new gate disjunct
short-circuited to `false`,
`ashaya_power_counts_a_creature_that_its_own_static_turns_into_a_land` fails at
the stale 1/1 where a full pass derives 2/2, and passes with the disjunct
restored.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…entry-flush gate CR 613.1b + CR 109.3 + CR 613.1. Review follow-up on the CONTROLLER channel this commit's parent named as a known gap rather than closing. A population keyed on controller -- "creatures you control", the overwhelmingly common shape -- was probed with the entrant's PRE-layer controller, while a layer-2 `ChangeController` (CR 613.1b) moves the entrant between players' populations before any later layer counts it. The gate took the incremental arm and left pre-existing recipients stale. `modification_writes_card_types(&ContinuousModification) -> bool` becomes `modification_population_key_write(&ContinuousModification) -> Option<PopulationKeyWrite>`, still an exhaustive wildcard-free match over all 57 variants. Parameterizing rather than adding a second bool classifier is the CLAUDE.md sibling-cluster rule: two predicates differing only in which characteristic they name is the shape that compounds. The axis stays inside one CR section -- card types are written in layer 4 (CR 613.1d), controller in layer 2 (CR 613.1b), both within CR 613 -- and closing the remaining kinds (COLOR, KEYWORD, NAME, P/T) means adding variants here. Controller is deliberately NOT folded into the "other characteristics" gap paragraph: CR 109.3 states an object's controller is not one of its characteristics, so the two are separate claims. What unifies them for this gate is only that `TargetFilter` reads both when counting a population. Fast-path cost is nil in practice -- battlefield `ChangeController` is rare, so the extra classification almost never fires, and the two deliberately-pinned fast paths (`count_anthem_nonmatching_entry_does_not_escalate_and_matches_full`, `devotion_gate_colorless_entry_does_not_escalate_and_matches_full`) still take the incremental arm. The regression test is discriminating. With `ChangeController` classified back as no key write, `controller_change_entry_escalates_when_population_is_controller_keyed` fails on the escalation assertion; with the assertion bypassed it fails on the board comparison at a stale power of 4 where a full pass derives 5. GRANT CHAINS and the COLOR/KEYWORD/NAME/P/T matrix remain declared open, with `known_gap_color_keyed_population_probes_entrant_pre_layer` still pinning the latter's current behavior. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…try-flush gate CR 613.1e. The entry-incremental flush escalates to a full re-evaluation when an entering permanent could move a counted population. It recognised two ways that happens -- a layer-4 card-type rewrite (CR 613.1d) and a layer-2 control change (CR 613.1b) -- and deliberately did not recognise a third. A layer-5 color-changing effect (CR 613.1e) rewrites the very characteristic a color-keyed population reads, and layer 5 runs before the layer-7 count, so the pre-layer probe saw the entrant's printed color rather than its derived one. That gap shipped as a knowingly-passing tripwire test asserting the stale board. It should not have. A test that passes while codifying stale derived state is an active layer-ordering error, not a baseline: the fixture's pre-existing Bears sat at 4/4 where a full pass correctly derives 5/5, and nothing in CI would have gone red if that divergence had widened. `modification_population_key_write` gains a `Color` variant and classifies `SetColor`, `AddColor` and `AddChosenColor` into it. The match stays exhaustive and wildcard-free over all `ContinuousModification` variants, so a future key-rewriting variant still cannot be added without deciding this question at compile time. Parameterizing here rather than adding a sibling classifier keeps the axis inside CR 613: all three keys are written by layers of the same rule section. `known_gap_color_keyed_population_probes_entrant_pre_layer` is replaced by `color_change_entry_escalates_when_population_is_color_keyed`, which requires escalation and full normal-vs-forced board identity instead of pinning the divergence. It discriminates in both directions: revert the classifier's `Color` arm and the escalation assertion fails at stack.rs:10292 with "a layer-5 color wash reaching the entrant moves a color-keyed count"; keep the arm but break the escalation plumbing and the identity assertion fails on the Bears' derived power/toughness. The fast path is unaffected. The gate still requires a classified writer to REACH an entrant AND a live population read to exist, so the two boards pinned as fast paths -- whose only effects write P/T -- keep taking the incremental arm: `count_anthem_nonmatching_entry_does_not_escalate_and_matches_full` and `devotion_gate_colorless_entry_does_not_escalate_and_matches_full` both still pass, along with the other 31 escalation fixtures. KEYWORD, NAME and P/T remain open as the same shape and are still documented as such; closing them needs the full FilterProp-reads x ContinuousModification-writes matrix rather than another per-key arm. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ind relation CR 613.1b / CR 613.1c / CR 613.1d / CR 613.1e / CR 613.1f / CR 613.1g / CR 613.6. The escalation gate had a one-sided writer list. `PopulationKeyWrite` enumerated the two characteristics an entering permanent could have rewritten -- card types and controller -- and any recognized write escalated whenever anything on the board read an object population at all. Keyword, name, colour and P/T rewrites were missing from that list, and the review is right that adding them to it would not have been the fix: the read side was untyped, so widening the writer list widens every board, and the two deliberately pinned fast paths (`count_anthem_nonmatching_entry_does_not_escalate_and_matches_full`, `devotion_gate_colorless_entry_does_not_escalate_and_matches_full`) would have started escalating. That is why the P/T channel could not be closed by extending the enum, and it is what made the sibling cluster the wrong shape to grow. Both sides are now typed and the gate is their intersection. `CharacteristicKinds` is a bitmask over seven kinds, one per CR 613 sublayer that can be written -- Controller (613.1b), NameText (613.1c, with 612.8 for name-setting as a text-changing effect), CardTypes (613.1d), Color (613.1e), Abilities (613.1f), PowerToughness (613.1g) -- plus ManaCost, which has no layer of its own but is copy-writable under 707.9b and is read by devotion, so a copy effect has to be able to move a devotion-keyed population. The parameterisation axis stays inside CR 613's own taxonomy; subtypes and supertypes fold into CardTypes because 613.1d is one sublayer. `modification_characteristic_writes` maps all 57 `ContinuousModification` variants to what they write; `target_filter_characteristic_reads` maps all 98 `FilterProp` variants, and sibling walkers cover `QuantityRef` and `StaticCondition`, to what they read. Every one is exhaustive and wildcard-free, so a new variant on either enum fails to compile until somebody classifies it -- the property the previous classifier had, kept on both surfaces instead of one. Uncertain forms map to the full set, so a classification error can only over-escalate. The read set is the union of three things and is built unconditionally: dynamic magnitudes, live conditions, and every live modification's affected filter. The affected-filter term is not optional. A board with no counting and no conditions at all -- a layer-3 rename plus a static whose affected set keys on names -- has an empty read set without it, so nothing intersects, and a pre-existing permanent whose membership the rename flips keeps stale P/T. That is the same blindness this gate exists to close, reached through a different door. CR 613.6 carves out one shape. It fixes a continuous effect's set of affected objects the first time the effect applies and retains it for the rest of the pass, so an effect cannot push an object out of the filter that admitted it. A static that reads a kind through its own affected filter and writes that kind through its own modification -- "artifacts that aren't creatures become creatures" reads and writes card types -- is therefore not a staleness risk against itself, and `incremental_entry_retains_multi_layer_effect_affected_set` is the board that proves it. The exclusion is keyed on the CR 613.6 retention group, not on the individual modification: one `StaticDefinition` spawns sibling effects that each carry a clone of the same affected filter, so excluding only the modification's own copy leaves the clones contributing the same kind. A kind read by two distinct groups survives any single exclusion, and an effect with no retention identity fails closed. The gate is cheapest-first and entrant-independent for as long as possible: the union of all write kinds is pure enum matches; the read set is computed once per flush with an early exit once it saturates; a global disjointness check exits before any affected filter is matched against any entrant. The scute-storm board leaves at that check -- keyword grants write abilities, the board reads card types and controller -- with zero `matches_target_filter` calls, which is stricter than the ordering it replaces rather than looser. Both fast paths still take the incremental arm, and they do it by typing rather than by exemption: their anthems write P/T while their boards read card types, controller and mana cost. The gate is now narrower in places as well as wider -- a control change against a purely P/T-keyed population no longer escalates -- and both directions are pinned. Eight fixtures cover the channels: keyword, P/T, name, the cross-group affected-filter channel, a condition reading through its own filter, and three negatives for disjoint reads, controller-vs-P/T, and the CR 613.6 self-exclusion. Each one was checked by reverting the specific classifier row or union term it depends on and confirming it fails with a concrete stale value, then restoring it. The P/T fixture puts the entrant under the opposing player so the toughness setter is the only writer that reaches it -- with P/T as one kind, a P/T-keyed count anthem that also reached the entrant would have satisfied the relation on its own and the fixture would have passed with the fix removed. The remaining-gap block is deleted because the matrix is what it asked for. What is left is stated conservatism, not blindness. One boundary is inherited unchanged and is now written down at the gate: the reach probe evaluates affected filters against the previous final state while full evaluation matches them at intermediate layer states, so a count-thresholded affected filter can diverge from the probe in either direction. Typing the relation neither narrows nor widens that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CR 102.1 + CR 102.3 / CR 805.4a. `StaticCondition::DuringOpponentsTurn` landed on main after this branch was cut. `static_condition_characteristic_reads_at` is exhaustive and wildcard-free, so merging main in did not compile until the variant was classified. That is the tripwire behaving as designed rather than an incident -- the property the matrix exists to hold is precisely that a new condition cannot enter the tree unclassified. It joins the `DuringYourTurn` arm. Both consult the active player (CR 102.1), and neither consults a layer-writable characteristic of any object. The two are deliberately distinct conditions rather than one being `Not` of the other -- in a team game a teammate can be the active player while the controller's team still holds the turn (CR 102.3 + CR 805.4a) -- but that distinction is invisible to this classifier, which asks only which characteristics a condition reads. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`check-parser-combinators.sh` flags `entrant.keywords.contains(..)` as a raw keyword query that bypasses the authorities in `game/keywords.rs`: a raw `contains` compares whole values, so it misses parameterised keywords whose payload differs, and it never consults off-zone grants. The non-vacuity assertion in `keyword_grant_entry_stays_incremental_when_population_reads_are_disjoint` now goes through `GameObject::has_keyword`, which matches on discriminant and is the form already used elsewhere in this file. The gate caught this in CI rather than locally because its base defaults to `git merge-base origin/main HEAD` (scripts/check-parser-combinators.sh:48) and this checkout names the upstream remote `upstream`, so the merge-base lookup failed and it fell back to `HEAD~1` -- one commit of the branch instead of the whole PR diff. Re-run against `upstream/main` explicitly it is `Gate A PASS head=a0b60dcf base=96e41b3ab`, and this was the only violation in the branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ous effects `live_characteristic_reads` unioned characteristic kinds from printed and granted-inner static definitions' conditions, but skipped the `condition` a `TransientContinuousEffect` retains after resolution. CR 611.2c locks in a resolved effect's affected SET; it does not lock in the gate, so a retained condition is re-evaluated every pass exactly like the "isn't locked in" static-ability effect of CR 611.3a. With the condition invisible to the read set, an entry-incremental flush that rewrote an entrant's card types could not see that the rewrite perturbed the gate, and recipients kept a stale board. Union `e.condition` for every live `ActiveContinuousEffect` unconditionally, alongside the existing global and affected-filter channels: the gate decides WHETHER the effect applies at all, so it is a board-level read, not a per-modification one (CR 613.6). Also fixes four `characteristic_read_classification` misclassifications in `filter.rs`. `FilterProp::Owned` mapped to `CharacteristicKinds::EMPTY` on the reasoning that ownership is immutable (CR 108.3), but the prop is a two-operand comparison whose LEFT operand is the live controller reference (CR 109.5), which layer 2 can rewrite (CR 613.1b). Same shape for `Unpaired` (CR 702.95e), `ControlledContinuouslySinceTurnBegan`, and `HasHasteOrControlledSinceTurnBegan` (CR 302.6 + CR 702.10). Two CodeRabbit findings: `FilterProp::DistinctFrom` was matched by a `_ =>` wildcard inside a classifier documented as exhaustive and wildcard-free — it is now an explicit arm — and a stale fixture name in a `stack.rs` doc comment now points at `controller_keyed_count_anthem_with_control_theft_board`. Tests exercise the building blocks, not the cards: four classifier-invariant tests in `filter.rs` (including a roster guard that fails when a new `ControllerRef`-carrying prop is added without classifying it), and one entry-incremental fixture in `stack.rs` that installs a conditioned transient through the single construction authority `GameState::add_transient_continuous_effect`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`any_active_static_condition_perturbed_by_entry` walked only
`obj.static_definitions.iter_all()` — printed and granted-inner CONTINUOUS
`StaticDefinition`s — so a source-level enabling condition riding on a
`TransientContinuousEffect` was invisible to the entry-perturbation probe.
CR 611.2c locks in a resolved effect's affected SET, not its gate. A retained
condition therefore flips on entry exactly like a printed one (CR 611.3a), and
every recipient frozen into that effect's set goes stale with it. Walk
`state.transient_continuous_effects` as a second generator channel, reading
`tce.condition` through the same population classifier and entry-narrowing probe
as the printed walk.
The transient walk deliberately has no truth-delta stage: `static_gate_truth` is
keyed by `StaticGateKey { source, def_index }` over printed definitions, and a
transient has no `def_index` in that key space (several transients can share one
`source_id`). Escalating on perturbation alone is the direction the whole gate is
built on, and matches the existing recipient-context arm, which also escalates
with no cache consult.
`FilterContext` is built with `tce.controller`, not the source object's current
controller: CR 109.5 gives a resolved spell or ability its controller for the
effect's whole existence, so "you" in the retained gate names the resolver — the
rebind-to-current-controller reading is only correct for static abilities.
The fixture installs a source-level `IsPresent{creature an opponent controls}`
gate through `GameState::add_transient_continuous_effect`, the single
construction authority, and reaches the probe on the disjunct that only this
walk can satisfy: nothing on the board writes the kinds the gate reads, so the
kind relation exits at stage 3 without it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`live_characteristic_reads` unioned enabling conditions off `active_effects`, which is a projection that structurally drops the two gate shapes an entry can flip. `gather_transient_continuous_effects` skips a transient whose gate is currently OFF (and an OFF gate is exactly the one an entry turns ON), and it retains only a recipient-context condition — a source-level condition is stripped from the effect it pushes. A `Duration::ForAsLongAs` condition (CR 611.2b, Master Thief) reaches no `ActiveContinuousEffect` at all, because no gather copies a duration onto one. So a board whose only live read of a characteristic kind sat in one of those gates computed a `ReadKinds` missing that kind, the kind relation exited early, the entry stayed incremental, and every frozen recipient kept a stale board. Walk `state.transient_continuous_effects` directly as a fifth read channel, through a new `transient_gate_conditions` single authority that yields both gates `transient_effect_is_live` consults: the "for as long as" duration (CR 611.2b) and the retained enabling condition, which is the source definition's own CR 611.3a gate riding along (`effects/counter.rs::apply_source_static`). CR 611.2c freezes such an effect's affected SET and nothing else, so both gates stay live and can flip long after that set is fixed. `any_active_static_condition_perturbed_by_entry` had the same `ForAsLongAs` blind spot — 294ebcd6f gave it `tce.condition` only. It now iterates the same authority, so a second condition-bearing `Duration` variant is wired into the liveness evaluator, the read union, and the probe by editing one function. Its `FilterContext` is hoisted out of the per-condition loop; CR 109.5 keeps "you" in either retained gate bound to `tce.controller`, not to whoever controls the source object now. Docs: 27b8bf71e documented `e.condition` as "the single authority for the condition channel: every `ActiveContinuousEffect` producer converges here". That was false in both directions and is replaced by a WHAT CONVERGES WHERE block naming each producer and the channel that actually sees it, including the ring/emblem/sticker producers that carry no condition today, and the `active_combat_assignment_rule_effects_from_static_definitions` / `collect_transient_combat_assignment_rule_effects` pair, which duplicates the same retain/strip logic for CR 613.11 effects and would need patching if a condition channel is ever added there. Tests: two entry-incremental fixtures whose only live read of NameText is a transient gate — one source-level `tce.condition`, one `ForAsLongAs` duration — plus a probe-channel fixture whose `ForAsLongAs` gate is started by an opponent's entrant. Both read fixtures write with a layer-1 `SetName` (CR 707.9b) rather than a layer-4 `AddType`: `evaluate_layers` gathers between layer 1 and layer 2, so a gate can only observe base + layer-1 state, and a type-based fixture cannot discriminate this seam. Co-Authored-By: Claude <noreply@anthropic.com> Assisted-by: ClaudeCode:claude-opus-5
The roster guard added in 27b8bf71e asserted `props.len() == 8` against the literal array standing two lines above it, so it could only ever restate itself — adding a `ControllerRef`-carrying variant to `FilterProp` left it green, which is the one drift it claimed to catch. Replace it with two authorities the test cannot edit. `carries_controller_ref` is an exhaustive wildcard-free classifier, so a new `FilterProp` variant fails to compile until it is classified; `declared_controller_ref_carriers` source-scans the `FilterProp` declaration in `types/ability.rs` for the variants that actually carry the field, so the sampled roster must equal the enum. Dropping one sample now fails with the two variant-name lists diffed against each other, and a new carrier turns the same assertion red until it is sampled, classified, and shown to report a CONTROLLER read (CR 613.1b). Also withdraws a claim in 27b8bf71e's own body: it said `FilterProp::DistinctFrom` had been "matched by a `_ =>` wildcard inside a classifier documented as exhaustive and wildcard-free — now an explicit arm". `git show e7f856c:crates/engine/src/game/filter.rs` shows the parent already had an explicit `match reference.as_ref()` arm; 27b8bf71e rewrote that arm into an `if matches!(...)`, changing no behavior and no exhaustiveness. This restores the `match` form as a readability change only, and the claim should not be read as evidence of a fixed wildcard. Co-Authored-By: Claude <noreply@anthropic.com> Assisted-by: ClaudeCode:claude-opus-5
Round-5 review of this branch found the "single authority" claim in 27b8bf71e still only half true, and three comments claiming more than the code proves. Six sites re-implemented the dual-gate check inline (`casting.rs`, four in `static_abilities.rs`, `turns.rs`) instead of asking `transient_gate_conditions`. Each would have silently kept the old two-gate shape if a third condition-bearing `Duration` variant were added. They now call the iterator, which is `pub(crate)` for it. No behavior change: the iterator yields exactly the pair each site open-coded. `casting.rs` also led with CR 603.4 + CR 608.2h, neither of which describes "both gates must hold" — it now cites CR 611.2b + CR 611.3a like the other five. Docs corrected where they claimed more than the code delivers: * WHAT CONVERGES WHERE is an enumerated producer table for `ActiveContinuousEffect::condition` rather than prose. It names all four `condition: None` producers, and states the consequence the prose had backwards: a GRANTED-INNER static's condition lives in `inner.condition`, which the source walk (over OUTER `obj.static_definitions.iter_all()`) cannot see until a prior pass has materialized it — so on a never-yet-evaluated state `e.condition` is the only channel that sees it. That is what makes the union load-bearing rather than redundant. * `transient_gate_conditions` no longer claims `transient_effect_is_live` consults exactly this pair: it also applies a CR 400.7 recipient-incarnation check and an `UntilHostLeavesPlay` source-zone check. Both read no layer-writable characteristic, which is why they sit outside the iterator and outside `live_characteristic_reads`. The lapsed-attachment sweep is named as the one deliberate non-consumer — it destructures a specific shape (CR 301.5) to decide expiry, not liveness, so routing it here would lose what it matches on. * `declared_controller_ref_carriers` records its ceiling: the scan is textual, so a `ControllerRef` reached through a nested type stays invisible and the roster tripwire only catches total scan failure. Tests: the source-level-condition fixture asserted the gather strips the condition by inference from what it installed. It now runs `gather_transient_continuous_effects` and asserts every produced effect has `condition: None` — the strip is the whole premise of the test, so it is asserted rather than assumed. Co-Authored-By: Claude <noreply@anthropic.com> Assisted-by: ClaudeCode:claude-opus-5
The previous commit claimed `transient_gate_conditions` is the authority every consumer walks, and enumerated them. The enumeration was incomplete — the same false-completeness defect it was written to fix. Two sites still destructured the pair by hand: * `static_abilities::object_has_active_cant_phase_in` wrapped it in a local `condition_holds` closure taking duration and condition as separate parameters, so it read as a helper rather than a duplicate. * `visibility::viewer_may_look_at_face_down` inlined it under a comment saying it honors "the same duration/condition gates the static-mode TCE queries in `static_abilities.rs` apply" — naming the shared rule while keeping a private copy of it. Both now call the iterator. No behavior change: it yields exactly the pair each site open-coded, in the same order. The doc no longer enumerates consumers, because an enumeration is what went stale. It states the invariant and how to check it instead: outside `layers.rs`, `Duration::ForAsLongAs` appears only in constructors, in the `ability_rw` / `ability_scan` / `coverage` walkers that classify a duration without evaluating it, and in the lapsed-attachment sweep documented as a deliberate non-consumer. A new site that destructures the pair by hand is the regression to look for. Co-Authored-By: Claude <noreply@anthropic.com> Assisted-by: ClaudeCode:claude-opus-5
…aluators The consumer enumeration above `transient_gate_conditions` claimed every consumer of the duration/condition pair routes through it, and offered `Duration::ForAsLongAs` as the grep that would catch a new offender. Both were wrong in the same direction: too broad a claim, too narrow a grep. The claim now covers consumers that EVALUATE liveness. Two classes are named as out of scope instead of silently contradicting it: * Walkers that CLASSIFY a duration without evaluating it -- `analysis::resource` (two hand-destructuring sibling-mutability scans) plus `ability_rw` / `ability_scan` / `coverage`. They stay variant-safe through `ability_scan`'s exhaustive matches, not through this authority. * Gate-blind consumers, disclosed as a tracked pre-existing gap: `casting::apply_static_activated_ability_cost_reduction` and `effects::attach::protection_blocks_attachment` apply a transient effect without consulting either gate. Routing them here changes behavior and needs its own CR analysis and tests. The grep recipe becomes the iteration site `for tce in &state.transient_continuous_effects`. `Duration::ForAsLongAs` only matches sites that already destructure the duration, so it structurally cannot see the gate-blind pair. Also scopes the producer census: `active_continuous_effects_from_base_static_source` is an alternate entry into an already-censused row, not a ninth producer, and never feeds `evaluate_layers`. Documentation only -- no production or test behavior changes. Co-Authored-By: Claude <noreply@anthropic.com> Assisted-by: ClaudeCode:claude-opus-5
e7f856c to
9ec4a13
Compare
|
Re-review requested on new head Your That closed the color and keyword channels as a consequence rather than as two more special cases. Against your findings, in the order you raised them:
Two things I want to flag rather than have you find:
The two coverage-honesty entries in my local pre-push run ( |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Re-review is held for head |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/engine/src/game/stack.rs (1)
11377-11402: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider one predicate-driven selector instead of two near-identical helpers.
pts_namedandpts_base_nameddiffer only in which name field they read. Several later tests then define local closures with the same body and a different predicate (bearsat Line 11630,bearat Line 11800,entrantat Line 12030). A single selector that takes a predicate would cover all five sites.♻️ Proposed consolidation
- fn pts_named(state: &GameState, prefix: &str) -> Vec<(Option<i32>, Option<i32>)> { - let mut pts: Vec<(Option<i32>, Option<i32>)> = state - .battlefield - .iter() - .filter_map(|id| state.objects.get(id)) - .filter(|o| o.name.starts_with(prefix)) - .map(|o| (o.power, o.toughness)) - .collect(); - pts.sort(); - pts - } + /// Sorted `(power, toughness)` of every battlefield object matching + /// `pred`. + fn pts_where( + state: &GameState, + pred: impl Fn(&crate::game::game_object::GameObject) -> bool, + ) -> Vec<(Option<i32>, Option<i32>)> { + let mut pts: Vec<(Option<i32>, Option<i32>)> = state + .battlefield + .iter() + .filter_map(|id| state.objects.get(id)) + .filter(|o| pred(o)) + .map(|o| (o.power, o.toughness)) + .collect(); + pts.sort(); + pts + } + + fn pts_named(state: &GameState, prefix: &str) -> Vec<(Option<i32>, Option<i32>)> { + pts_where(state, |o| o.name.starts_with(prefix)) + } - fn pts_base_named(state: &GameState, prefix: &str) -> Vec<(Option<i32>, Option<i32>)> { - let mut pts: Vec<(Option<i32>, Option<i32>)> = state - .battlefield - .iter() - .filter_map(|id| state.objects.get(id)) - .filter(|o| o.base_name.starts_with(prefix)) - .map(|o| (o.power, o.toughness)) - .collect(); - pts.sort(); - pts - } + fn pts_base_named(state: &GameState, prefix: &str) -> Vec<(Option<i32>, Option<i32>)> { + pts_where(state, |o| o.base_name.starts_with(prefix)) + }Verify the concrete object type name before applying the diff.
🤖 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/stack.rs` around lines 11377 - 11402, Consolidate the duplicated selectors around pts_named and pts_base_named into one predicate-driven helper that iterates battlefield objects, filters by the supplied predicate, collects (power, toughness), and sorts the result. Update both helpers and the later bears, bear, and entrant local closures to call this shared selector with predicates for live name, base name, or their existing conditions; first verify the concrete object type used by GameState.objects so the predicate signature matches it.crates/engine/src/game/casting.rs (1)
7768-7768: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider making
evaluate_cost_mod_static_conditionexhaustive.The new arm adds
DuringOpponentsTurnnext toDuringYourTurn, which is correct:evaluate_conditionrebinds both to the source object's controller, so passingsource_controlleris consistent. The match still ends with a_ =>fallback at Line 7784. A new caster-relative or source-relativeStaticConditionvariant will silently take the caster-bound path, which is exactly the class of drift this arm was added to fix. An exhaustive match makes the compiler flag the next variant.As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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/casting.rs` at line 7768, Make evaluate_cost_mod_static_condition use an exhaustive match over StaticCondition by removing the wildcard fallback at the end of the match. Preserve the existing handling for DuringYourTurn and DuringOpponentsTurn, and add explicit handling for every remaining variant so future StaticCondition additions produce a compiler error instead of silently using the caster-bound path.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.
Inline comments:
In `@crates/engine/src/game/stack.rs`:
- Around line 11789-11820: Strengthen
name_rewrite_entry_escalates_through_affected_filter_reads by capturing and
validating the pre-entry board before add_artifact_entry runs. Assert that the
pre-existing Doppelganger is buffed before the entry, then retain the existing
post-entry and normal-versus-forced comparisons so the test cannot pass when
DifferentNameFrom sees an empty reference set.
- Around line 11971-11988: Update the same-layer discussion in the relevant
layer-7c evaluation logic to cite CR 611.3a, which supports recalculating a
static ability’s affected set when both modifications apply in layer 7c; retain
CR 613.6 only for effects continuing into another layer or sublayer, and
preserve the existing evaluation behavior.
- Around line 12287-12295: Update the rule reference in the
StaticCondition::IsPresent block’s comment from CR 109.5 to CR 608.2c, leaving
the controller and filter logic unchanged. Apply the same comment correction to
the corresponding block near line 12601.
---
Nitpick comments:
In `@crates/engine/src/game/casting.rs`:
- Line 7768: Make evaluate_cost_mod_static_condition use an exhaustive match
over StaticCondition by removing the wildcard fallback at the end of the match.
Preserve the existing handling for DuringYourTurn and DuringOpponentsTurn, and
add explicit handling for every remaining variant so future StaticCondition
additions produce a compiler error instead of silently using the caster-bound
path.
In `@crates/engine/src/game/stack.rs`:
- Around line 11377-11402: Consolidate the duplicated selectors around pts_named
and pts_base_named into one predicate-driven helper that iterates battlefield
objects, filters by the supplied predicate, collects (power, toughness), and
sorts the result. Update both helpers and the later bears, bear, and entrant
local closures to call this shared selector with predicates for live name, base
name, or their existing conditions; first verify the concrete object type used
by GameState.objects so the predicate signature matches it.
🪄 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: 51359721-0ea1-47c3-ba1b-d2ff1ee02a44
📒 Files selected for processing (11)
crates/engine/src/game/casting.rscrates/engine/src/game/filter.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/stack.rscrates/engine/src/game/static_abilities.rscrates/engine/src/game/turns.rscrates/engine/src/game/visibility.rscrates/engine/tests/integration/ashaya_nontoken_lands.rscrates/engine/tests/integration/life_and_limb_sylvan_advocate.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/engine/tests/integration/main.rs
- crates/engine/tests/integration/life_and_limb_sylvan_advocate.rs
- crates/engine/tests/integration/ashaya_nontoken_lands.rs
- crates/engine/src/game/filter.rs
- crates/engine/src/game/quantity.rs
| #[test] | ||
| fn name_rewrite_entry_escalates_through_affected_filter_reads() { | ||
| let (normal, escalated, forced) = | ||
| flush_entry_and_forced(name_rewrite_with_affected_filter_read_board, |s| { | ||
| add_artifact_entry(s, 691) | ||
| }); | ||
| assert!( | ||
| escalated, | ||
| "a layer-3 rename feeding another static's AFFECTED FILTER must \ | ||
| escalate — the affected-filter read channel is unconditional" | ||
| ); | ||
| let bear = |s: &GameState| { | ||
| s.battlefield | ||
| .iter() | ||
| .filter_map(|id| s.objects.get(id)) | ||
| .find(|o| o.base_power == Some(2)) | ||
| .map(|o| (o.power, o.toughness)) | ||
| .expect("pre-existing Doppelganger on battlefield") | ||
| }; | ||
| assert_eq!( | ||
| bear(&forced), | ||
| (Some(2), Some(2)), | ||
| "full pass drops the pre-existing Doppelganger out of the buff \ | ||
| once the renamed artifact joins the reference set" | ||
| ); | ||
| assert_eq!( | ||
| bear(&normal), | ||
| bear(&forced), | ||
| "escalated entry must derive the same board as full re-evaluation" | ||
| ); | ||
| assert_pt_identical(&normal, &forced, "affected-filter read channel"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a pre-entry guard so this test cannot pass vacuously.
This test pins only the post-entry board (bear(&forced) == (2,2)). The pre-existing Doppelganger is a printed 2/2, so (2,2) is also its unbuffed value. If FilterProp::DifferentNameFrom evaluated to false over an empty reference set, the bear would read (2,2) both before and after the entry, and the assertions would still pass while proving nothing about the affected-filter read channel.
Every sibling transient test in this block (type_rewrite_entry_escalates_through_transient_condition_reads, opponent_entry_escalates_through_transient_source_level_gate, and the two duration tests) already flushes a fresh pre-entry board and pins it. Add the same guard here.
💚 Proposed pre-entry guard
assert_eq!(
bear(&forced),
(Some(2), Some(2)),
"full pass drops the pre-existing Doppelganger out of the buff \
once the renamed artifact joins the reference set"
);
+ // Non-vacuity: the buff is genuinely ON before the entry, so the
+ // 2/2 above is the entrant's doing and not an unbuffed board.
+ let mut pre = name_rewrite_with_affected_filter_read_board();
+ flush_layers(&mut pre);
+ assert_eq!(
+ bear(&pre),
+ (Some(5), Some(5)),
+ "pre-entry no artifact carries the name, so the buff applies"
+ );
assert_eq!(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[test] | |
| fn name_rewrite_entry_escalates_through_affected_filter_reads() { | |
| let (normal, escalated, forced) = | |
| flush_entry_and_forced(name_rewrite_with_affected_filter_read_board, |s| { | |
| add_artifact_entry(s, 691) | |
| }); | |
| assert!( | |
| escalated, | |
| "a layer-3 rename feeding another static's AFFECTED FILTER must \ | |
| escalate — the affected-filter read channel is unconditional" | |
| ); | |
| let bear = |s: &GameState| { | |
| s.battlefield | |
| .iter() | |
| .filter_map(|id| s.objects.get(id)) | |
| .find(|o| o.base_power == Some(2)) | |
| .map(|o| (o.power, o.toughness)) | |
| .expect("pre-existing Doppelganger on battlefield") | |
| }; | |
| assert_eq!( | |
| bear(&forced), | |
| (Some(2), Some(2)), | |
| "full pass drops the pre-existing Doppelganger out of the buff \ | |
| once the renamed artifact joins the reference set" | |
| ); | |
| assert_eq!( | |
| bear(&normal), | |
| bear(&forced), | |
| "escalated entry must derive the same board as full re-evaluation" | |
| ); | |
| assert_pt_identical(&normal, &forced, "affected-filter read channel"); | |
| } | |
| #[test] | |
| fn name_rewrite_entry_escalates_through_affected_filter_reads() { | |
| let (normal, escalated, forced) = | |
| flush_entry_and_forced(name_rewrite_with_affected_filter_read_board, |s| { | |
| add_artifact_entry(s, 691) | |
| }); | |
| assert!( | |
| escalated, | |
| "a layer-3 rename feeding another static's AFFECTED FILTER must \ | |
| escalate — the affected-filter read channel is unconditional" | |
| ); | |
| let bear = |s: &GameState| { | |
| s.battlefield | |
| .iter() | |
| .filter_map(|id| s.objects.get(id)) | |
| .find(|o| o.base_power == Some(2)) | |
| .map(|o| (o.power, o.toughness)) | |
| .expect("pre-existing Doppelganger on battlefield") | |
| }; | |
| assert_eq!( | |
| bear(&forced), | |
| (Some(2), Some(2)), | |
| "full pass drops the pre-existing Doppelganger out of the buff \ | |
| once the renamed artifact joins the reference set" | |
| ); | |
| // Non-vacuity: the buff is genuinely ON before the entry, so the | |
| // 2/2 above is the entrant's doing and not an unbuffed board. | |
| let mut pre = name_rewrite_with_affected_filter_read_board(); | |
| flush_layers(&mut pre); | |
| assert_eq!( | |
| bear(&pre), | |
| (Some(5), Some(5)), | |
| "pre-entry no artifact carries the name, so the buff applies" | |
| ); | |
| assert_eq!( | |
| bear(&normal), | |
| bear(&forced), | |
| "escalated entry must derive the same board as full \ | |
| re-evaluation" | |
| ); | |
| assert_pt_identical(&normal, &forced, "affected-filter read channel"); | |
| } |
🤖 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/stack.rs` around lines 11789 - 11820, Strengthen
name_rewrite_entry_escalates_through_affected_filter_reads by capturing and
validating the pre-entry board before add_artifact_entry runs. Assert that the
pre-existing Doppelganger is buffed before the entry, then retain the existing
post-entry and normal-versus-forced comparisons so the test cannot pass when
DifferentNameFrom sees an empty reference set.
| /// (2.8) CR 613.6 SELF-EXCLUSION CARVE-OUT. One Continuous definition | ||
| /// whose modifications WRITE exactly the kind its OWN affected filter | ||
| /// READS, and nothing else on the board reads anything: the buff is | ||
| /// `AddPower`/`AddToughness` (writes `{PowerToughness}`) over "creatures | ||
| /// with power ≤ 1" (reads `{CardTypes, PowerToughness}`). There is no | ||
| /// dynamic magnitude and no static condition, so the affected filter is | ||
| /// the whole read union. | ||
| /// | ||
| /// CR 613.6 locks the effect's affected-object set the first time the | ||
| /// effect applies and retains it for the rest of the pass, so the buff | ||
| /// cannot push the entrant back out of the set it was just admitted to. | ||
| /// Its own write is therefore not a read it can move, and the entry must | ||
| /// stay incremental. | ||
| /// | ||
| /// Revert direction: drop the per-modification exclusion and test stage 4 | ||
| /// against the whole `ReadKinds` union again — `{PowerToughness}` then | ||
| /// intersects its own affected filter's reads and the escalation | ||
| /// assertion flips. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate the engine's CR 613.6 retained-affected-set implementation and its rationale.
set -euo pipefail
rg -n -C 6 '613\.6' --type=rust
rg -n -C 6 'retained_affected|affected_set_retained|retain.*affected' --type=rust -g 'crates/engine/src/game/layers.rs'Repository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target context ---'
sed -n '11940,12010p' crates/engine/src/game/stack.rs
printf '%s\n' '--- CR references in engine ---'
rg -n -C 4 'CR[[:space:]]*613(\.6)?|613\.6|affected.*set|set.*affected|escalat' crates/engine/src crates/engine/tests -g '*.rs' || true
printf '%s\n' '--- relevant files ---'
git ls-files | rg '(^|/)(CLAUDE\.md|layers\.rs|stack\.rs)$'Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-ttulK9
printf '%s\n' '--- target context from prior output ---'
rg -n -C 12 'SELF-EXCLUSION CARVE-OUT|Revert direction|escalation assertion' "$log" || true
printf '%s\n' '--- exact CR 613.6 references ---'
rg -n -C 8 'CR 613\.6|613\.6' crates/engine/src/game/stack.rs crates/engine/src/game/layers.rs crates/engine/tests -g '*.rs' || true
printf '%s\n' '--- affected-set and layer-pass terms in engine ---'
rg -n -C 5 'affected.*set|set.*affected|layer pass|layer_pass|escalat' crates/engine/src/game -g '*.rs' | head -n 400 || trueRepository: phase-rs/phase
Length of output: 50372
🌐 Web query:
Magic Comprehensive Rules CR 613.6 affected objects same set continuous effect
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, specifically rule 613.6, when a continuous effect applies in multiple layers or sublayers, its parts are applied in each of their appropriate layers [1][2][3]. If an effect begins to apply in one layer or sublayer, it will continue to be applied to the same set of objects in each other applicable layer or sublayer, even if the ability generating that effect is removed during this process [1][3]. For example, if an effect makes an object a certain color and also gives it a power/toughness bonus, the color-changing part is applied in layer 5 and the power/toughness-modifying part is applied in layer 7, with the effect tracking the same target objects across both layers regardless of whether the source of the effect persists [1][2].
Citations:
- 1: https://mtg.wiki/page/Layer
- 2: https://ancestral.vision/spells-abilities-and-effects/interaction-of-continuous-effects.html
- 3: https://mtg-rules.vercel.app/chapters/613
🌐 Web query:
site:magic.wizards.com Comprehensive Rules 613.6 611.2c continuous effect same layer affected objects
💡 Result:
In the Magic: The Gathering Comprehensive Rules, sections 611 and 613 govern continuous effects and the interaction of those effects via layers. Rule 611.2c addresses how continuous effects generated by the resolution of a spell or ability identify the objects they affect. It states that such an effect "locks in" the objects it applies to as the spell or ability resolves, meaning the effect applies only to those objects that are present and eligible at that moment, rather than continually checking for new objects that might enter or become eligible later. Rule 613 defines the layer system, which is used to determine the order in which continuous effects are applied to objects. While rule 613.6 is not explicitly detailed in the provided search results, section 613 generally establishes that when multiple continuous effects apply to the same object within the same layer and no dependency is involved, they are applied in timestamp order. A timestamp is typically the time the effect was created, or in the case of counters, the time the counter was added to the object [1][2][3]. The interaction between these rules ensures that effects are applied consistently; once a continuous effect identifies its target objects during resolution (611.2c), those objects are then processed through the layer system (613) alongside any other effects affecting them [1].
Citations:
- 1: https://magic.wizards.com/en/news/announcements/comprehensive-rules-changes-2022-02-09
- 2: https://magic.wizards.com/en/news/announcements/comprehensive-rules-changes-2020-06-23
- 3: https://magic.wizards.com/en/news/announcements/comprehensive-rules-changes-2021-04-20
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- layer application implementation ---'
sed -n '6720,6835p' crates/engine/src/game/layers.rs
printf '%s\n' '--- same-layer ordering and affected-set calls ---'
rg -n -C 8 'apply_continuous_effect_filtered|started_effect_sets|mod_index|affected_filter' crates/engine/src/game/layers.rs | head -n 500
printf '%s\n' '--- target helper and test body ---'
sed -n '11971,12057p' crates/engine/src/game/stack.rsRepository: phase-rs/phase
Length of output: 30280
🌐 Web query:
"613.6. If an effect starts to apply in one layer or sublayer"
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rule 613.6 governs how continuous effects that span multiple layers or sublayers function [1][2][3]. It states that if an effect should be applied in different layers or sublayers, its constituent parts are applied in their respective layers or sublayers [1][2]. Crucially, if an effect starts to apply in one layer or sublayer, it continues to be applied to the same set of objects in every other applicable layer or sublayer, even if the ability that originally generated the effect is removed during the process [1][3]. For example, if an effect states "All noncreature artifacts become 2/2 artifact creatures until end of turn," the type-changing part is applied in layer 4, and the power and toughness-setting part is applied in layer 7b [1][4]. The effect remains applied to those specific objects in layer 7b even though they may no longer be considered "noncreature artifacts" once the layer 4 effect has been processed [1].
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/interaction-of-continuous-effects.html
- 2: https://mtg.wiki/page/Layer
- 3: https://mtg-rules.vercel.app/chapters/613
- 4: https://magic-rulebook.vercel.app/rules/613
🌐 Web query:
"CR 611.3a" static ability continuous effect set of objects determined continuously
💡 Result:
According to the Magic: The Gathering Comprehensive Rules, specifically rule 611.3a, a continuous effect generated by a static ability is not "locked in" [1][2]. Instead, it applies continuously to whatever set of objects its text indicates at any given moment [1][3]. This means that if a static ability affects a set of objects (e.g., "All white creatures get +1/+1"), the effect constantly monitors the battlefield [3]. If a permanent enters the battlefield and meets the criteria, or if an existing permanent changes state to meet the criteria (such as a non-white creature becoming white), the effect immediately applies to it [3]. Conversely, if a permanent ceases to meet the criteria, the effect immediately stops applying to it [3]. This stands in contrast to continuous effects generated by the resolution of a spell or ability that modify characteristics or change control, which are governed by rule 611.2c [1][4]. Under rule 611.2c, the set of objects affected by those types of effects is "locked in" at the moment the effect begins and does not change thereafter, even if objects later meet the criteria that would have otherwise made them eligible [1][5].
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/continuous-effects.html
- 2: https://mtg.wiki/page/Continuous_effect
- 3: https://magic-rulebook.vercel.app/rules/611
- 4: https://tappedout.net/mtg-questions/planeswalker-ability-effects-new-tokens/
- 5: https://boardgames.stackexchange.com/questions/28090/do-spells-that-change-power-toughness-of-all-creatures-until-end-of-turn-affect
Correct the CR citation for the same-layer case. CR 613.6 covers retention when an effect continues into another layer or sublayer. Both modifications here apply in layer 7c, while CR 611.3a states that a static ability’s affected set is not locked in. Keep CR 613.6 for multi-layer retention and cite the rule that supports this layer-7c evaluation behavior.
🤖 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/stack.rs` around lines 11971 - 11988, Update the
same-layer discussion in the relevant layer-7c evaluation logic to cite CR
611.3a, which supports recalculating a static ability’s affected set when both
modifications apply in layer 7c; retain CR 613.6 only for effects continuing
into another layer or sublayer, and preserve the existing evaluation behavior.
Source: Path instructions
| // CR 109.5: a resolved effect RETAINS its controller, so "an | ||
| // opponent" is read against P0. OFF on this board. | ||
| Some(StaticCondition::IsPresent { | ||
| filter: Some(TargetFilter::Typed(TypedFilter { | ||
| type_filters: vec![TypeFilter::Creature], | ||
| controller: Some(ControllerRef::Opponent), | ||
| ..Default::default() | ||
| })), | ||
| }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show how the engine documents a transient effect's retained controller.
set -euo pipefail
rg -n -C 6 'add_transient_continuous_effect' --type=rust -g 'crates/engine/src/**'
rg -n -C 4 'CR 109\.5|CR 611\.2\b|CR 609\.7' --type=rust -g 'crates/engine/src/game/**'Repository: phase-rs/phase
Length of output: 152
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- matching source files ---'
git ls-files 'crates/engine/src/game/stack.rs' 'crates/engine/src/**/*.rs' | head -200
printf '%s\n' '--- relevant identifiers and citations ---'
rg -n -C 8 'add_transient_continuous_effect|install_gated_transient|CR 109\.5|CR 611\.2|CR 609\.7' crates/engine/src/game crates/engine/src --glob '*.rs' || true
printf '%s\n' '--- target regions ---'
sed -n '12260,12320p' crates/engine/src/game/stack.rs
sed -n '12575,12625p' crates/engine/src/game/stack.rsRepository: phase-rs/phase
Length of output: 50371
🌐 Web query:
Magic Comprehensive Rules CR 109.5 resolved effect retains controller CR 609.7 611.2
💡 Result:
In the Magic: The Gathering Comprehensive Rules, rules 109.5, 609.7, and 611.2 govern how objects are identified, how effects interact with sources, and how continuous effects function [1][2][3]. Rule 109.5 defines who "you" and "your" refer to on an object. Specifically, for a static ability, these terms refer to the object's current controller [1]. For an activated ability, they refer to the player who activated it, and for a triggered ability, they refer to the controller of the object when the ability triggered (unless it is a delayed triggered ability) [1]. Rule 609.7 outlines how effects interact with sources, including those that require a player to choose a source of damage or other effects [3]. It notes that the source is chosen when the effect is created, and if that source is a permanent, the effect remains linked to that object regardless of its state [3]. Rule 611.2 describes continuous effects generated by the resolution of a spell or ability [2]. Notably, rule 611.2c states that if a continuous effect generated by a resolution modifies an object's characteristics or changes its controller, the set of objects affected is determined only once, when the continuous effect begins [2]. After that point, the set of affected objects does not change, even if the objects' properties or controllers change later [2]. This is distinct from continuous effects created by static abilities, which are dynamic and re-evaluated [2]. Combining these, while 109.5 establishes the controller for abilities, 611.2c clarifies that when a resolution-based effect creates a continuous effect changing a controller or characteristics, it locks onto the specific set of objects existing at that moment of creation, rather than dynamically updating as the game state changes [2].
Citations:
- 1: https://media.wizards.com/2026/downloads/MagicCompRules%2020260227.pdf
- 2: https://mtg-rules.vercel.app/chapters/611
- 3: https://ancestral.vision/spells-abilities-and-effects/effects.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact target comments ---'
rg -n -C 12 --max-count 20 'CR 109\.5: a resolved effect|resolved effect RETAINS|an opponent.*P0|install_gated_transient' crates/engine/src/game/stack.rs crates/engine/src --glob '*.rs' || true
printf '%s\n' '--- effect-controller and transient-effect definitions ---'
rg -n -C 8 --max-count 30 'effect.?s controller|effect controller|controller.*effect|controller: PlayerId|ResolvedContinuousEffectCommand' crates/engine/src/types/game_state.rs crates/engine/src/game --glob '*.rs' | head -800
printf '%s\n' '--- target regions ---'
sed -n '12270,12310p' crates/engine/src/game/stack.rs
sed -n '12585,12615p' crates/engine/src/game/stack.rsRepository: phase-rs/phase
Length of output: 50371
🌐 Web query:
site:media.wizards.com MagicCompRules 2026 608.2 609.7 611.2 effect controller
💡 Result:
In the Magic: The Gathering Comprehensive Rules (as of April 2026), the specified sections pertain to the following: Rule 608.2 governs the resolution of spells and abilities [1]. Specifically, rule 608.2c defines how a spell or ability resolves, and rule 608.2h notes that if an effect gives a player instructions to perform an action, that player performs the action [1]. Rule 609.7 deals with determining the controller of an effect [1]. While the rules do not have a single "effect controller" rule, they specify that the controller of an effect generated by a spell or ability is the player who controlled that spell or ability [1]. Rule 611.2 covers the creation of continuous effects [1]. It states that a continuous effect generated by the resolution of a spell or ability lasts as long as the effect specifies [1]. If the effect is generated by a permanent's static ability, it lasts as long as that object is on the battlefield and has that ability [1]. Regarding the controller of an effect: - The controller of an effect is generally the player who controlled the source of that effect (the spell, ability, or permanent) at the time the effect was created [1][2]. - For effects generated by spells or abilities on the stack, the controller of the effect is the player who controlled that spell or ability [1][2]. - For static abilities, the controller of the effect is the current controller of the object that has that ability [2]. For more precise applications, such as identifying the controller of a triggered ability, rule 109.5 provides that "you" and "your" refer to the object's controller, the player who activated the ability, or the controller of the object when the ability triggered, depending on the context [2].
Citations:
- 1: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt
- 2: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSL 'https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt' -o "$tmp"
printf '%s\n' '--- official CR 109.5 ---'
awk 'BEGIN{p=0} /^109\.5\./{p=1} p{print} p && /^[0-9]+\.[0-9]+[a-z]?\./ && $0 !~ /^109\.5\./{exit}' "$tmp"
printf '%s\n' '--- official CR 608.2 ---'
awk 'BEGIN{p=0} /^608\.2\./{p=1} p{print} p && /^608\.3\./{exit}' "$tmp"
printf '%s\n' '--- official CR 609.7 ---'
awk 'BEGIN{p=0} /^609\.7\./{p=1} p{print} p && /^609\.8\./{exit}' "$tmp"
printf '%s\n' '--- official CR 611.2 ---'
awk 'BEGIN{p=0} /^611\.2\./{p=1} p{print} p && /^611\.3\./{exit}' "$tmp"Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transient effect type and installation ---'
sed -n '14960,15015p' crates/engine/src/types/game_state.rs
sed -n '18835,18965p' crates/engine/src/types/game_state.rs
printf '%s\n' '--- resolved command controller fields ---'
rg -n -C 10 'ResolvedContinuousEffectCommand|transient_continuous_effects' crates/engine/src/types/resolved_commands.rs crates/engine/src/types --glob '*.rs' | head -500
printf '%s\n' '--- resolution call sites passing controller ---'
rg -n -C 6 'add_transient_continuous_effect\(' crates/engine/src --glob '*.rs' | head -500Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmp="$(mktemp)"
trap 'rm -f "$tmp"' EXIT
curl -fsSL 'https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt' -o "$tmp"
for rule in '109\.5\.' '608\.2\.' '609\.7\.' '611\.2\.'; do
printf '\n--- %s ---\n' "$rule"
line="$(grep -n -m1 "^${rule}" "$tmp" | cut -d: -f1 || true)"
if [ -n "$line" ]; then
start=$((line-2))
end=$((line+18))
sed -n "${start},${end}p" "$tmp"
else
echo 'not found'
fi
done
printf '\n--- all nearby mentions of effect controller ---\n'
grep -in -E 'controller of (the )?effect|effect.*controller|controller.*effect|effect generated by' "$tmp" | head -80Repository: phase-rs/phase
Length of output: 49610
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- focused controller call sites ---'
rg -n -C 5 'add_transient_continuous_effect\(' crates/engine/src --glob '*.rs' | rg -B 5 -A 5 'controller|add_transient|PlayerId' | head -600 || true
printf '%s\n' '--- resolution controller fields ---'
rg -n -C 8 'resolution_source|original_controller|source_controller|ability\.controller|resolved.*controller' crates/engine/src/game/stack.rs crates/engine/src/game/effects crates/engine/src/types --glob '*.rs' | head -700 || trueRepository: phase-rs/phase
Length of output: 50371
Replace CR 109.5 with CR 608.2c. CR 109.5 defines “you” and “your”; it does not state that a resolution-created effect retains its controller. Apply the same correction at line 12601.
🤖 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/stack.rs` around lines 12287 - 12295, Update the rule
reference in the StaticCondition::IsPresent block’s comment from CR 109.5 to CR
608.2c, leaving the controller and filter logic unchanged. Apply the same
comment correction to the corresponding block near line 12601.
Source: Path instructions
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head layer-gate review: typed read/write intersection at prepare_incremental_flush, discriminating full-vs-incremental regressions, current no-change parser artifact, and green required CI. CodeRabbit's remaining comments are non-blocking test/documentation nits; no current functional or architectural blocker found.
Summary
The entry-incremental layers flush shipped in #6777 escalates to a full re-evaluation whenever an entering object can perturb a population-counting effect — but it probed the ENTERING object using that object's PRE-layer characteristics. When another active effect rewrites the entrant earlier in the same pass, the probe answers about the wrong object.
Ashaya, Soul of the Wild is the live case: its layer-4 static makes nontoken creatures you control Forest lands (CR 613.1d) while its characteristic-defining ability counts "lands you control" in layer 7a (CR 613.4). Layers apply in order, so the type rewrite has already happened by the time the count is read — but the gate saw a creature, not a land, concluded the population was unperturbed, took the incremental arm, and left Ashaya stale at 1/1 where a full re-evaluation derives 2/2.
What this PR became. The first version fixed exactly the card-type channel and pinned the rest (color, keyword, name, power/toughness, controller) as a documented known gap with a tripwire test. Six rounds of maintainer review said, correctly, that a per-channel disjunct is the sibling-cluster smell in slow motion. It is now a typed read/write-kind relation instead:
CharacteristicKinds(au8bitflag set,filter.rs:71) is computed on both sides —target_filter_characteristic_reads(filter.rs:367) plus the sibling classifiers overQuantityRef,StaticCondition, andFilterProp, answering which kinds does this population read;modification_characteristic_writes(layers.rs:4090), a wildcard-free match over everyContinuousModificationvariant, answering which kinds does this effect rewrite;and the gate escalates exactly when the two sets intersect for an effect reaching an entrant. The per-channel bool (
modification_writes_card_types) and its intermediate enum (PopulationKeyWrite) are both gone — zero occurrences remain. The known gap is closed, not pinned, so the tripwire test the earlier revision advertised no longer exists.Because the relation is typed and exhaustive, a future
ContinuousModificationvariant cannot be added without deciding which kinds it writes, and a futureFilterPropcannot be added without declaring which kinds it reads. Precision remains load-bearing rather than decorative: escalating on the looser "the entrant is a recipient of anything" regresses the two deliberately-pinned fast paths, where the effect reaching the entrant writes only power/toughness and so cannot move a type-keyed or devotion-keyed count.The read side spans both channels of a static's continuous effect: its dynamic magnitude/affected set, and its enabling condition (CR 611.3a — a static's continuous effect isn't locked in, so its condition re-evaluates as the board changes, and the per-entrant condition probe carried the same pre-layer blindness). Resolution-created continuous effects carry two independent condition gates and both are now read.
Files changed
crates/engine/src/game/layers.rsprepare_incremental_flush:3590), the typed write classifiermodification_characteristic_writes:4096, the dual-channel condition reads, and the transient-gate single-authority refactor (transient_gate_conditions:5710,transient_duration_holds:5718)crates/engine/src/game/filter.rsCharacteristicKinds:71,target_filter_characteristic_reads:367, and theFilterPropread-kind classifiercrates/engine/src/game/quantity.rsQuantityRefread-kind classifiercrates/engine/src/game/static_abilities.rsStaticConditionread-kind classifier; transient gate reads routed to the authoritycrates/engine/src/game/casting.rs,turns.rs,visibility.rscrates/engine/src/game/stack.rscrates/engine/tests/integration/ashaya_nontoken_lands.rscrates/engine/tests/integration/life_and_limb_sylvan_advocate.rscrates/engine/tests/integration/main.rsmodregistration for the new integration moduleDiffstat: 11 files changed, 4183 insertions(+), 202 deletions(-) across 13 commits. The bulk is test fixtures and the read-kind classifier tables. No
Cargo.tomlchange, no new feature flag, no new top-level test binary (the new integration module is registered intests/integration/main.rs).Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Every citation below was grep-verified against
docs/MagicCompRules.txtbefore it was written (file line numbers in parentheses). Verified again at this head.ChangeControllermoves an entrant between players' populations.The
FilterProp::Ownedclassification is worth calling out because it looks wrong at a glance and a reviewer flagged it: CR 108.3 fixes an object's owner for the game, so the naive reading is thatOwnedreads no layer-writable kind. ButOwnedis a two-operand relation — only the right operand is immutable; the left operand is the source's live controller per CR 109.5, which layer 2 rewrites per CR 613.1b. It therefore reportsCONTROLLER, with a dedicated test pinning that reasoning.Verification
9ec4a136c).Tilt was not running on this machine, so the CLAUDE.md direct-cargo fallback applies to everything below. Evidence rather than assertion —
tilt get uiresource clippyexits 1 (cannot reach the Tilt API server).cargo fmt --all -- --check— exit 0../scripts/check-parser-combinators.sh— PASS (output under Gate A).cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings— exit 0 (CI's exact lint line).cargo test-all—Summary [174.480s] 25726 tests run: 25717 passed, 9 failed, 29 skipped. All 9 failures are the pre-existingmtgish-importreds documented under "CI Failures" below; zero engine failures.cargo ai-perf-gate— exit 1, 8 FAIL / 21 PASS. Not a CI-required check for this PR (see "Perf evidence" for why), and every one of the eight failures reproduces at the PR base with this PR's commits absent.Revert-check (the discrimination evidence)
Green alone proves nothing, so here is the FAIL/PASS pair at head
9ec4a136c. The revert neuters exactly this PR's new escalation disjunct — an earlyreturn falseat the top ofpopulation_probe_blinded_by_entrant_characteristic_change(layers.rs:4048), leaving every other production hunk and every test in place — then runs the same filter both ways:cargo nextest run -p phase-engine --no-fail-fast -E 'test(ashaya) or test(life_and_limb) or test(escalat)'With the disjunct neutered —
51 tests run: 39 passed, 12 failed:The stale
(1, 1)is the bug this PR fixes, reproduced on demand. The other 11 reds are one per read/write kind:With the disjunct restored —
51 tests run: 51 passed, andgit status --porcelainis empty at9ec4a136c, so the scaffold left nothing behind.One honest caveat rather than a rounded-up claim: the two pinned fast paths (
recipient_local_dynamic_…,embedded_threshold_token_entry_…) pass in both runs, which is the point of a fast-path pin. So does the integration testsylvan_advocate_condition_counts_a_saproling_that_life_and_limb_turns_into_a_land— it pins the end-to-end condition channel but does not discriminate against this particular neuter. The discriminating coverage for that channel is thecondition_gated_anthem_entry_escalates_when_entrant_types_rewritten/type_rewrite_entry_escalates_through_transient_condition_readspair above, both of which go red.Pinned fast paths still pass and still take the incremental arm:
count_anthem_nonmatching_entry_does_not_escalate_and_matches_full,devotion_gate_colorless_entry_does_not_escalate_and_matches_full, plusrecipient_local_dynamic_does_not_escalate_and_matches_fullandsource_condition_gate_unchanged_does_not_escalate_and_matches_full.Discriminating fixtures, one per read/write kind, so no channel rests on argument alone: controller, color, keyword, power/toughness, name, and type-rewrite-through-a-transient-condition.
Base freshness. This branch is rebased onto
409956671675f1a3951df73e93e86f593130bc8e, which is the currentupstream/maintip —git merge-base HEAD upstream/mainreturns that same commit, so the branch is a strict fast-forward with zero divergence. No conflicts remain.Perf evidence
The AI perf gate does not run on this PR.
ai-gate.yml'spull_requesttrigger carries apaths:filter that excludescrates/engine/**, so an engine-only change never triggers it — it did not run on #6777 either, and the nightly job that would otherwise cover the gap iscontinue-on-error. The local run below is therefore the only perf evidence for this change, and it is stated as such rather than implied to be CI-backed.cargo ai-perf-gateat head9ec4a136c—seed=2654435769 action_cap=3000 sample_count=5 scenarios=["red-mirror", "affinity-mirror", "enchantress-mirror"]— exits 1 with 8 FAIL / 21 PASS / 0 NEW / 0 REMOVED.Reporting the reds rather than rounding them away. All eight are already red at the PR base with this PR's thirteen commits absent. Paired run, identical seed and identical
data/card-data.json, base409956671(the currentupstream/maintip):4099566719ec4a136cEvery PR delta is between −2 and 0, i.e. ≤0.01% and in the cheaper direction. The gate names the real cause itself:
crates/phase-ai/baselines/perf-baseline.jsonrecordscard_data_hash = e2db8a6d…; this machine's regeneratedcard-data.jsonhashes to42e7755f…. Five of the eight reds — mana auras, legend-rule mode gates, SBA snapshots, restriction-static mode gates, legality clones — are in subsystems this PR never touches, which is a hint; the base run turns the hint into evidence.No baseline refresh is included here. The drift is card-data, not this PR, and a refresh belongs to a card-data PR with its own paired-seed report — not smuggled into an engine change.
One limitation stated plainly rather than left for a reviewer to find:
layers_escalatedandlayers_full_evalare byte-identical between base and head (121 / 8814). The escalation disjuncts this PR adds therefore never fire in the three constructed-mirror scenarios the suite runs, so this gate provides no signal — favourable or otherwise — on the escalation change itself. The cost argument for the change rests on the pinned fast-path tests above (count_anthem_nonmatching_entry_does_not_escalate_and_matches_full,devotion_gate_colorless_entry_does_not_escalate_and_matches_full, and the tworecipient_local_dynamic_…/source_condition_gate_unchanged_…pins), which assert that non-matching entries still take the incremental arm.Gate A
Head matches the PR head. (The script reports the immediate parent as
base, not the PR base; pasted unedited.)Anchored on
Both anchors re-read at the current PR base
409956671675f1a3951df73e93e86f593130bc8e:crates/engine/src/game/layers.rs:3469— the existing escalation seam insideprepare_incremental_flush(:3432), whereactive_effects_force_incremental_escalation(:3552) is disjoined withany_active_static_condition_perturbed_by_entry(:3620) at:3469-3470. This change works inside that same boolean, in the same function, with the same "returnNoneto escalate" discipline.crates/engine/src/game/stack.rs:10398—count_anthem_nonmatching_entry_does_not_escalate_and_matches_full, the non-perturbing half of the existing escalate/don't-escalate test pair (its devotion twin is at:10165). Every new fixture extends that pattern and reuses itsflush_entry_and_forcedharness at:10108.Claimed parse impact
None. The parser is untouched.
Scope Expansion
Declared, and larger than the original submission — every item below was added in response to a maintainer review round rather than volunteered:
The per-channel disjunct became a typed read/write-kind relation. This is the substantive expansion: read-kind classifiers over
TargetFilter,FilterProp,QuantityRefandStaticCondition, a write-kind classifier overContinuousModification, and an intersection test at the gate. It closes the color / keyword / name / power-toughness / controller gaps that the first revision only pinned.Transient-gate single authority. Transient continuous effects were gated by hand-rolled duration/condition reads scattered across
layers.rs,static_abilities.rs,casting.rs,turns.rsandvisibility.rs. Every consumer that evaluates liveness now routes throughtransient_gate_conditions/transient_duration_holds— eleven call sites. This was required to make the read side correct: a gate the classifier cannot see is a gate that silently skips escalation. It touches five files and is called out as expansion rather than buried.Two classes sit outside that authority, and the doc block above
transient_gate_conditionsnames both rather than claiming universal coverage:analysis::resource(two sibling-mutability scans),ability_rw/ability_scan/coverage. They ask what a duration reads, never whether it holds, and stay variant-safe throughability_scan's exhaustive matches.casting::apply_static_activated_ability_cost_reductionandeffects::attach::protection_blocks_attachmentapply a transient effect without consulting either gate, so a lapsed condition still reduces a cost or blocks an attachment. That is a behavior change with its own CR analysis and test burden; folding it in here would have widened an already-expanded PR. Tracked as follow-up.The grep that surfaces a new offender is therefore the iteration site
for tce in &state.transient_continuous_effects, notDuration::ForAsLongAs— the latter matches only sites that already destructure the duration, so it structurally cannot see the gate-blind pair.incremental_flush_must_escalate(test-only) is a thin wrapper overprepare_incremental_flushon a scratch clone, so the test predicate and the production gate answer the same question.Validation Failures
None.
CI Failures
None from this change.
Disclosure for reviewers running the suite locally: nine tests in the
mtgish-importcrate are red, and they are red onupstream/mainat this PR's base independently of this branch (sixgolden_structuralfixtures, twoconvert::unit tests, andmanifest_coverage::every_list_field_is_in_ordering_manifest). This PR touches no file incrates/mtgish-import/; its diff is confined tocrates/engine/.Provenance
Cherry-picked onto current main from a development branch, then rebased twice as
upstream/mainadvanced during the review rounds — most recently onto409956671. The history was rewritten by those rebases, which is why this PR was force-pushed.Root cause owned plainly: this is a correctness gap in #6777's escalation gate — my own prior change — found by differential verification against full re-evaluation while developing a follow-up. The diagnostic harness that found it is not in this PR; it is prepared as a separate follow-up so this fix stays small and fully CI-verifiable.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Performance
Tests