fix(engine): escalate entry-incremental flush when an entrant's card types are rewritten

[mcbradd]

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's card types 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.4a). Layers apply in order (CR 613.1), 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.

The gate now additionally 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 continuous effect isn't locked in, so its condition re-evaluates as the board changes, and the existing per-entrant condition probe carries the same pre-layer blindness).

Precision is 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. That precision is load-bearing rather than decorative — escalating on the looser "the entrant is a recipient of anything" instead 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.

Files changed

• crates/engine/src/game/layers.rs — the new escalation disjunct population_probe_blinded_by_entrant_characteristic_change, the modification_writes_card_types classifier, effect_population_reads (single definition of the two read channels, consumed by both the new disjunct and the existing Axis 2a guard), any_active_static_condition_reads_object_population, the reset_recipient_to_base extraction, the incremental_flush_must_escalate test-predicate rewrite, and the KNOWN REMAINING GAP annotations
• crates/engine/src/game/stack.rs — two synthetic escalation tests beside the existing entry-flush escalation family: condition_gated_anthem_entry_escalates_when_entrant_types_rewritten (discriminating) and known_gap_color_keyed_population_probes_entrant_pre_layer (tripwire)
• crates/engine/tests/integration/ashaya_nontoken_lands.rs — end-to-end regression test through the real cast pipeline
• crates/engine/tests/integration/life_and_limb_sylvan_advocate.rs — end-to-end condition-channel pin (new file)
• crates/engine/tests/integration/main.rs — mod registration for the new integration module

Diffstat: 5 files changed, 700 insertions(+), 92 deletions(-). No Cargo.toml change, no new feature flag, no new top-level test binary.

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.txt before it was written (file line numbers in parentheses).

New or load-bearing in this change:

• CR 613.1 (2958) — continuous effects apply in a series of layers, in order. This is the rule that makes the layer-4 → layer-7a feed real without any dependency machinery.
• CR 613.1d (2966) — Layer 4: type-changing effects.
• CR 613.4a (2986) — Layer 7a: characteristic-defining abilities that define power and/or toughness.
• CR 611.3a (2922) — a static ability's continuous effect isn't "locked in"; the condition channel.
• CR 613.1e (2968) — Layer 5: color-changing effects (cited by the known-gap tripwire, which rewrites the entrant's color).
• CR 613.1b (2962) — Layer 2: control-changing effects (cited where the recipient reset re-derives from base).
• CR 613.2a (2976) / CR 613.2c (2980) / CR 707.2 (5610) — copiable values, cited for classifying CopyValues/CopyChosen as type writers.
• CR 109.5 (610) — "you"/"your" refers to the object's controller; the entrant reach probe is filter-matched with the effect source's controller context.
• CR 109.3 (592) — an object's controller is explicitly NOT one of its characteristics. Cited in the declared-gap list to justify why the controller channel needs naming separately instead of being folded into "other characteristics".
• CR 701.60c (3811) — a suspected permanent has menace and "can't block" while suspected. Carried into reset_recipient_to_base with the reset unification.
• CR 708.2 (5701) — face-down permanents have only the characteristics their enabling ability lists. Cited for the face-down collection that stays in the full pass.

Deliberately NOT cited for the cross-layer feed: CR 613.8a (3038). Dependency requires the two effects to be "applied in the same layer (and, if applicable, sublayer)"; layer 4 and layer 7a are different layers, so dependency does not apply here and citing it would be wrong. CR 613.8a and CR 613.7d (3016) appear in the diff only inside a pre-existing doc block that was reflowed, not as new claims.

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.
• Gate A output below is for the current committed head.
• Final review-impl below is clean for the current committed head.
• Both anchors cite existing analogous code at the same seam.

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 clippy, exit 1:

Unable to connect to the server: dial tcp 127.0.0.1:56376: connectex: No connection could be made because the target machine actively refused it.

• cargo fmt --all -- --check — exit 0.
• cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings — exit 0 (CI's exact lint line). Only output was the benign warning: excluded package(s) 'phase-tauri' not found in workspace.
• cargo test-all — Summary [202.696s] 25295 tests run: 25286 passed (1 slow), 9 failed, 29 skipped. All 9 failures are the pre-existing mtgish-import reds documented under "CI Failures" below; zero engine failures.
• cargo ai-perf-gate — exit 1, 7 counters over threshold. The identical 7 counters fail on this PR's base commit with this branch not applied, so the regression is not attributable to this change. Control run and diagnosis in the Perf section below.

Revert-check (the discriminating evidence). With the new disjunct population_probe_blinded_by_entrant_characteristic_change short-circuited to false and everything else unchanged, both discriminating tests FAIL at concrete stale values:

FAIL phase-engine::integration ashaya_nontoken_lands::ashaya_power_counts_a_creature_that_its_own_static_turns_into_a_land
  assertion `left == right` failed: both Ashaya and the entering Bears are Forest lands, so the CDA counts 2
    left: (Some(1), Some(1))
   right: (Some(2), Some(2))
  at crates/engine/tests/integration/ashaya_nontoken_lands.rs:197

FAIL phase-engine layers::tests::condition_gated_anthem_entry_escalates_when_entrant_types_rewritten
  entrant becomes a land in layer 4, which flips the threshold gate — must escalate
  at crates/engine/src/game/stack.rs:10432

With the disjunct restored, all six checked tests PASS — the two above, the two deliberately-pinned fast paths, the known-gap tripwire, and the end-to-end condition-channel pin. The working tree was verified byte-identical to the committed head afterwards.

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.

Perf. ai-gate.yml's pull_request trigger carries a paths: filter listing crates/phase-ai/**, data/card-data.json, scripts/lib/mtgjson-fetch.sh, .cargo/config.toml, .github/actions/ai-card-data-cache/** and .github/workflows/ai-gate.yml — it does not include crates/engine/**. The perf gate therefore does not run on this PR, and did not run on engine-only #6777 either; the nightly variant is continue-on-error. The local run above is consequently the only perf evidence for this change, which is why it is attached rather than left to CI.

The local run is red: 7 FAIL, 22 PASS, 0 NEW, 0 REMOVED. It is red identically without this branch. I checked out this PR's base e04514f3c0a16581bc163d324937f11ddba0cbe9 detached, ran the same gate, and got the same verdict — the same seven counters (crew_eligibility_scans, layers_full_eval, legend_rule_mode_gate_scans, mana_aura_trigger_scans, restriction_static_mode_gate_scans, sba_battlefield_snapshot_builds, state_clone_for_legality) over threshold at the same values, and the same tool note:

note: card-data hash changed (e2db8a6d4711e34097b307c454032b29fbce8d4f→3d7b74ea722f0e343bf1510673ca922632dabf8c)
      — likely a card-data-driven trajectory shift, not a cost-per-node regression

That is the explanation and it is checkable: crates/phase-ai/baselines/perf-baseline.json records card_data_hash=e2db8a6d…, my local data/card-data.json hashes to 3d7b74ea…, and data/card-data.json is gitignored and untracked, so no commit on this branch could have changed it. The AI playouts are walking a different card corpus than the baseline was recorded against, so the node counts moved. Corroborating, five of the seven failures are in code this diff never touches, and the two counters that do directly measure this change both PASS — layers_escalated 93→113 (threshold 161) and layers_incremental 491→512 (threshold 579). layers_incremental going up is inconsistent with a new escalation disjunct being what drives layers_full_eval up.

No baseline refresh is proposed here: refreshing on a local corpus, without a paired-seed report, would bake my machine's card data into the repo's baseline.

Base freshness. This branch is based on e04514f3c0a16581bc163d324937f11ddba0cbe9, which was upstream/main when the work started. upstream/main has since advanced by two commits: 7323622de ("stop basic-land mana fallback from bypassing CantBeActivated", #6841) and 3f57f8079 ("reject dominated Colorless convoke-family taps during ManaPayment", #6840). Between them they touch casting_tests.rs, mana_abilities.rs, mana_sources.rs, mana_colors.rs and tactical_gate.rs; this PR touches layers.rs, stack.rs and three integration test files. Zero file overlap, so no conflict. The branch was deliberately not rebased onto it: rebasing produces a new head, and under the ordering rule below that would invalidate Gate A and all three review rounds to gain nothing a clean merge does not already give.

Gate A

Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=042254d4892445bdf2dc159bc71a31734b0b4f21 base=e04514f3c0a16581bc163d324937f11ddba0cbe9

Anchored on

Both anchors read at the PR base e04514f3c0a16581bc163d324937f11ddba0cbe9:

• crates/engine/src/game/layers.rs:3454 — the existing escalation seam inside prepare_incremental_flush (active_effects_force_incremental_escalation at :3454 disjoined with any_active_static_condition_perturbed_by_entry at :3455). The new disjunct is a third member of that same boolean, in the same function, with the same "return None to escalate" discipline.
• crates/engine/src/game/stack.rs:10159 — count_anthem_nonmatching_entry_does_not_escalate_and_matches_full, the non-perturbing half of the existing escalate/don't-escalate test pair (its perturbing twin is at :10175). Both new synthetic tests extend that pattern and reuse its flush_entry_and_forced fixture at :9869.

Final review-impl

The third /engine-implementer review round, run against the current committed head 042254d4892445bdf2dc159bc71a31734b0b4f21 — the same head as the Gate A output above — returned CLEAN: no HIGH, no MEDIUM, and one NIT that is knowingly shipped and disclosed below. The two earlier rounds both returned NOT CLEAN; every finding from them is fixed in this head, not deferred. Round-by-round dispositions, including the ones where the reviewer was right about a claim I had written down as fact, are under "Review disposition" below.

Claimed parse impact

None. The parser is untouched.

Scope Expansion

Declared. Beyond the minimal gate fix, this PR also carries:

1. The known-gap tripwire test. A population keyed on COLOR, KEYWORD, NAME or power/toughness 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. This PR does not close that gap; closing it needs the full characteristic-kind matrix (which kind each FilterProp reads by which kind each ContinuousModification writes). Rather than disclose it in prose only, known_gap_color_keyed_population_probes_entrant_pre_layer builds the synthetic board and pins the current divergence as an executable test in the normal cargo test-all path, so the gap cannot silently widen and the matrix flips a red test when it lands. This exposure predates this PR and does not close when it merges.
2. incremental_flush_must_escalate (test-only) rewritten as a thin wrapper. It 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 now delegates to prepare_incremental_flush on a scratch clone, so the test predicate and the production gate answer the same question — single authority.
3. reset_recipient_to_base extracted, and both arms unified onto it. The incremental arm's reset became a named function, and the full pass in evaluate_layers — which open-coded the same five steps — now calls it too. Behavior-identical; see "Reset unification" below for why no signature change was needed.

Reset unification

reset_recipient_to_base is the single authority for the per-object "back to base" step, and both arms now go through it: the full pass in evaluate_layers applies it board-wide over the phased-in battlefield, the incremental arm applies it to recipients only. Previously the full pass open-coded the same five steps inline, which is what let the incremental arm's copy drift into existence in the first place.

The full pass keeps its one extra behavior — collecting face-down permanents so their CR 708.2 profile can be re-applied after Layer 1a — as a two-line tail after the call. No signature change was needed: none of the five reset steps touches face_down, so the collection reads the same value on either side of the reset. That is asserted from a grep of every face_down assignment in crates/engine/src; the writers all live in casting, conspiracy, deck-loading, planar-deck and filter-snapshot code, none of them on the reset path.

Unifying also restored a CR annotation the extraction had dropped: reset_recipient_to_base now carries the full pass's CR 701.60c note on derive_suspected_abilities explaining why the suspected designation's menace and "can't block" are re-derived onto live fields rather than base.

Review disposition

One /engine-implementer review round on the pre-final head returned NOT CLEAN, and it was right about the substantive point. Dispositions, all fixed in the head below rather than deferred:

• HIGH — the deferral rationale in an earlier draft of this PR body was false. That draft claimed unifying the two reset sites required a signature change because the full pass also collects face-down permanents. It does not: face_down is untouched by every reset step, so the hoist is two lines and behavior-identical. The review caught a fabricated technical constraint that had already survived being written down. Fixed by doing the unification, not by rewriting the excuse.
• MEDIUM — apply_layers_incremental's doc stated a guarantee this change had invalidated. It named incremental_flush_must_escalate (which is #[cfg(test)] and never on the production path — pre-existing staleness) and claimed the gate guarantees "no active effect's magnitude or affected set reads board population". The gate now has three disjuncts and what it actually provides is weaker: no detected perturbation. Doc rewritten to name prepare_incremental_flush, enumerate the disjuncts, and state the weaker guarantee explicitly.
• MEDIUM — the controller channel was unnamed. A population keyed on "creatures you control" is read pre-layer while a layer-2 ChangeController (CR 613.1b) can move an entrant between players' populations. This is not covered by the existing gap paragraph: CR 109.3 says an object's controller is not a characteristic, so "other characteristics" excludes it by construction. Now named as its own numbered channel, with the note that closing it is much cheaper than the full matrix.
• LOW — grant chains. GrantAbility / GrantStaticAbility / AddStaticMode / RemoveAllAbilities correctly classify false for "writes card types", but an effect that grants a type-writer is a second-order path the classifier cannot see. Now named in the same list, together with why the residual risk is small (entered_object_blocks_incremental already escalates for entrants carrying their own static or CDA) and the plain statement that it is not closed.
• NIT — module header of life_and_limb_sylvan_advocate.rs contradicted its own per-test comment. The header implied the board exercises the gate; the per-test comment said it does not. Reworded — and see round 2 below, which found the deeper problem with both.

A second review round on the fixed head returned NOT CLEAN again, with one MEDIUM that is worth stating plainly because it is the same defect class as the HIGH above:

• MEDIUM — two mutually contradictory mechanism claims for why the Life and Limb board doesn't reach the escalation gate. One comment said its entry routes through the replacement delivery path; another said the Advocate's condition reads battlefield zone membership and so marks a full flush. At most one could be true, and the two review rounds disagreed with each other about which. I traced it far enough to establish that round 2's supporting argument was itself wrong (inject_battlefield_presence does exist, at parser/oracle_nom/condition.rs:4690, and inject_controller calls it at :4725 — so the parser does stamp InZone { Battlefield } on that shape), but I did not establish the end-to-end answer by execution.

  Resolution: both mechanism claims are deleted rather than adjudicated. The test asserts a CR-correct end-to-end outcome that holds under either flush arm, and it now says exactly that and nothing more. Encoding an arm claim in prose that no assertion can keep honest is what produced three successive wrong comments here; the discriminating evidence for the condition channel is the synthetic fixture, which asserts escalation directly.

• LOW — apply_layers_incremental's doc enumerated four of five escalation disjuncts. The sibling doc added in the same commit says a second enumeration "is exactly what drifted last time" and then this one drifted. Truncated to point at prepare_incremental_flush as the single list rather than restating it.

• NIT ×2 — two stale locators (seed_live_characteristics_from_base and derive_suspected_abilities doc comments) still said the controller/combat-flag/suspected steps "stay inline in Step 1" and named apply_layers_incremental as the incremental caller. Both updated to name reset_recipient_to_base and prepare_incremental_flush.

A third round on the current head returned CLEAN — no HIGH, no MEDIUM. It mechanically confirmed the round-2→round-3 delta is comment-only (strip diff markers, filter comment and blank lines, zero lines remain), audited every comment in the diff against the code and found no surviving false mechanism claim, re-derived the 57-variant exhaustiveness by diffing the enum's variant names against the match arm names, verified all 18 CR citations by reading anchored line ranges, and checked both integration tests' Oracle constants against data/card-data.json rather than from memory.

One NIT is knowingly shipped unfixed: layers.rs:3489 says the escalation disjuncts are enumerated "at the prepare_incremental_flush call site", when the enumeration is inside that function's body. The substance is right — there is exactly one enumeration and it lives in code rather than prose — but "call site" is the wrong noun. It is left in deliberately: correcting one word would create a new head, and under this PR's own ordering rule that invalidates Gate A and all three review rounds, which is a worse trade than a wrong noun. Flagging it here so it is disclosed rather than discovered.

Round 2 also independently re-verified the core change: the hoist is behavior-identical (it checked every reset step for face_down writes rather than trusting the comment saying so), modification_writes_card_types is an exact wildcard-free match over the ContinuousModification variant set, both new tests carry real board-state assertions beyond the escalation boolean, and all CR citations check out against docs/MagicCompRules.txt.

The reviewer states plainly that it did not run cargo and derived its pass/fail claims by reading code paths. The suite evidence above is the answer to that: the two deliberately-pinned fast-path tests are green in the full run, not merely argued to be.

Validation Failures

None.

CI Failures

None from this change.

Disclosure for reviewers running the suite locally: nine tests in the mtgish-import crate are red, and they are red on upstream/main at this PR's base independently of this branch (six golden_structural fixtures, two convert:: unit tests, and manifest_coverage::every_list_field_is_in_ordering_manifest). This PR touches no file in crates/mtgish-import/, and its own diff is confined to crates/engine/.

Provenance

Cherry-picked onto current main from a development branch. One conflict in layers.rs: upstream's copy of the old incremental_flush_must_escalate tail (the Axis 2b comment and call) collided with this change's restructuring of that function into a #[cfg(test)] thin wrapper over prepare_incremental_flush. Resolved by taking the restructure; upstream's Axis 2b call survives inside prepare_incremental_flush, where the escalation disjuncts now live. Both stack.rs callers of the wrapper are inside mod tests.

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 during development of 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

  • Improved layered effect evaluation, including incremental updates and escalation handling.
  • Fixed edge cases where entering creatures change type, color, or controller and affect population-based abilities.
  • Corrected land-counting interactions involving transformed creatures and characteristic-defining abilities.

• Tests

  • Added regression coverage for layered evaluations, controller changes, and land-transformation interactions.
  • Added integration tests covering Ashaya, Life and Limb, and Sylvan Advocate interactions.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Incremental layer evaluation now tracks characteristic dependencies and modification writes. Full and incremental paths share reset logic. Entrant characteristic changes can trigger full evaluation when they affect population-sensitive effects or conditions. Regression tests cover engine and integration scenarios.

Changes

Incremental layer evaluation

Layer / File(s)	Summary
Characteristic dependency analysis crates/engine/src/game/filter.rs, crates/engine/src/game/quantity.rs, crates/engine/src/game/layers.rs	Filters, quantities, and static conditions classify characteristic reads with bounded recursion and conservative fallbacks.
Shared reset and entrant escalation crates/engine/src/game/layers.rs	Full and incremental evaluation share recipient reset logic. Incremental preparation classifies population reads and modification writes, probes entrant changes, and escalates when required.
Engine regression coverage crates/engine/src/game/stack.rs	Tests cover characteristic rewrites, affected filters, conditions, narrowing cases, and self-exclusion.
Integration regression coverage crates/engine/tests/integration/*	Tests cover Ashaya land counting and the Life and Limb interaction with Sylvan Advocate.

Estimated code review effort: 5 (Critical) | ~100 minutes

Sequence Diagram(s)

sequenceDiagram
  participant IncrementalEvaluation
  participant prepare_incremental_flush
  participant PopulationReadHelpers
  participant EntrantReachProbe
  IncrementalEvaluation->>prepare_incremental_flush: prepare layer flush
  prepare_incremental_flush->>PopulationReadHelpers: classify live population reads and writes
  prepare_incremental_flush->>EntrantReachProbe: probe entrant characteristic changes
  EntrantReachProbe-->>prepare_incremental_flush: return perturbation result
  prepare_incremental_flush-->>IncrementalEvaluation: escalate or continue incrementally

Loading

Possibly related PRs

• phase-rs/phase#6323: Both modify characteristic-dependency analysis in game/filter.rs and game/quantity.rs.
• phase-rs/phase#6626: Both modify characteristic and power-read evaluation in game/filter.rs.

Suggested reviewers: matthewevans, andriypolanski

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the incremental-flush escalation fix and matches a central change in the pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 3f57f80 and 042254d.

📒 Files selected for processing (5)

• crates/engine/src/game/layers.rs
• crates/engine/src/game/stack.rs
• crates/engine/tests/integration/ashaya_nontoken_lands.rs
• crates/engine/tests/integration/life_and_limb_sylvan_advocate.rs
• crates/engine/tests/integration/main.rs

[github-actions]

Generated for head e7f856cff57faa23e80083d242f0f7d11bdd33e9.

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

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.

[mcbradd]

Closed the controller channel rather than pinning it — b471740 is on the branch.

What changed

modification_writes_card_types(&ContinuousModification) -> bool became modification_population_key_write(&ContinuousModification) -> Option<PopulationKeyWrite>, still an exhaustive wildcard-free match over all 57 variants. ChangeController now yields Some(PopulationKeyWrite::Controller) (CR 613.1b, layer 2) alongside the 16 card-type writers yielding Some(PopulationKeyWrite::CardTypes) (CR 613.1d, layer 4), and the gate's write-side probe tests .is_none() instead of a bare false.

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 TargetFilter reads both when counting a population.

Discriminating test

controller_change_entry_escalates_when_population_is_controller_keyed (stack.rs). Board: two P0 2/2s, an anthem whose dynamic magnitude counts "creatures you control", and a control-theft enchantment whose affected filter is deliberately controller-FREE — so the point under test is the counted population, not the affected set. The entrant arrives under P1, so the pre-layer probe of "creatures you control" reports the count unperturbed; layer 2 then hands it to the anthem's controller and the count goes 2 → 3, moving the two PRE-EXISTING recipients.

It is discriminating in both directions:

• With ChangeController classified back as no key write, the test fails on the escalation assertion (layer 2 moves the entrant into the counted population — must escalate).
• With that assertion additionally bypassed, it fails on the board comparison: power mismatch for ObjectId(1) left: Some(4) right: Some(5) — the stale board really is 4/4 where a full pass derives 5/5.

The fixture also pins TheftBear0 at 5/5 on the forced-Full board, so it cannot pass vacuously if the theft ever stops reaching the entrant.

Cost

Nil in practice — battlefield ChangeController is rare, so the extra classification almost never fires. Both deliberately-pinned fast paths still take the incremental arm: count_anthem_nonmatching_entry_does_not_escalate_and_matches_full and devotion_gate_colorless_entry_does_not_escalate_and_matches_full.

Still open, still declared

GRANT CHAINS (an effect that grants a type-writing static rather than writing types itself) and the COLOR/KEYWORD/NAME/P/T matrix. known_gap_color_keyed_population_probes_entrant_pre_layer continues to pin the latter's current behavior and is expected to flip when the matrix lands.

Verification at b471740e12a5c1e22846f69295ae62d2847599fe

• cargo fmt --all --check clean.
• CI's exact clippy line (--workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings): exit 0.
• ./scripts/check-parser-combinators.sh: Gate G PASS, Gate A PASS head=b471740e12a5c1e22846f69295ae62d2847599fe.
• cargo test-all: 25296 run / 25287 passed / 29 skipped. The 9 failures are the pre-existing mtgish-import ones, unchanged from the base and outside crates/engine/. Base was 25295 tests; +1 is the new test.

Requesting re-review.

🤖 Generated with Claude Code

[coderabbitai]

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 lift

Remove the known color-rewrite under-escalation.

This test proves that the incremental path violates CR 613 layer ordering. The layer-5 AddColor effect 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

📥 Commits

Reviewing files that changed from the base of the PR and between 042254d and b471740.

📒 Files selected for processing (2)

• crates/engine/src/game/layers.rs
• crates/engine/src/game/stack.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/engine/src/game/layers.rs

[matthewevans]

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.

[mcbradd]

Blocker addressed in d8e11e957. You were right that the tripwire was not an acceptable resting state, and I have closed the channel rather than re-pinning it.

The classifier. modification_population_key_write gains a third variant, Color (CR 613.1e, layer 5), and SetColor / AddColor / AddChosenColor move out of the None arm into it. The match remains exhaustive and wildcard-free over every ContinuousModification, so a future key-rewriting variant still cannot be added without deciding this question at compile time. Parameterizing here rather than growing a sibling classifier keeps the axis inside CR 613 — card types (613.1d, layer 4), controller (613.1b, layer 2) and color (613.1e, layer 5) are all written by layers of the same rule section, and all three are read by TargetFilter when a population is counted.

The test. known_gap_color_keyed_population_probes_entrant_pre_layer is gone, replaced by color_change_entry_escalates_when_population_is_color_keyed. It asserts escalation, asserts the forced-full board derives 5/5, asserts normal-vs-forced equality, and closes with assert_pt_identical. The stale 4/4 expectation is deleted, not relocated.

Discrimination, both directions. Reverting only the classifier's Color arm to None and re-running:

FAIL [ 0.014s] (1/1) phase-engine game::stack::tests::batch_resolve::color_change_entry_escalates_when_population_is_color_keyed
panicked at crates\engine\src\game\stack.rs:10292:13:
a layer-5 color wash reaching the entrant moves a color-keyed count — the entry must escalate to a full re-evaluation

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 — count_anthem_nonmatching_entry_does_not_escalate_and_matches_full and devotion_gate_colorless_entry_does_not_escalate_and_matches_full — along with the other 31 fixtures in the escalation family: 33/33 green.

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 FilterProp-reads × ContinuousModification-writes matrix rather than a fourth per-key arm, and I would rather that arrive as its own reviewable change than as a fourth special case bolted onto this one. What holds the line in the meantime is the wildcard-free match, not corpus absence — I have removed the "no printed pairing is known" phrasing from the load-bearing position it previously occupied.

Verification at d8e11e9571fe688ff2544336653b6c013c715e8b: cargo fmt --all -- --check clean; CI's exact clippy line exits 0; cargo test-all 25296 run / 25287 passed / 9 failed, all nine pre-existing mtgish-import failures outside crates/engine/ and identical to the set on the parent commit; ./scripts/check-parser-combinators.sh Gate G PASS and Gate A PASS head=d8e11e9571fe688ff2544336653b6c013c715e8b. CR 613.1b / 613.1d / 613.1e / 613.1g each re-checked against docs/MagicCompRules.txt.

Requesting re-review.

🤖 Generated with Claude Code

[matthewevans]

Current-head maintainer hold for d8e11e9571fe688ff2544336653b6c013c715e8b: this new 5-file engine/layer head adds the previously requested color-key population-write channel and materially refactors the entry-flush test predicate/reset authority. Required Rust test shards are still running, and the final review-impl must cover the new current head before any merge action. The parser artifact remains no-change evidence only; no queue action during this hold.

[matthewevans]

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.

[mcbradd]

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 — count_anthem_nonmatching_entry_does_not_escalate_and_matches_full and devotion_gate_colorless_entry_does_not_escalate_and_matches_full — start escalating. That is why the P/T channel could not be closed by growing the enum. Typing both sides closes it while keeping the pins, and the pins now survive by typing rather than by exemption: their anthems write P/T, their boards read card types, controller and mana cost, and the intersection is empty.

The lattice

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 CR 707.9b and is read by devotion, so a copy effect can move a devotion-keyed population. The parameterisation axis stays inside CR 613's own taxonomy, so there is no cross-section unification: subtypes and supertypes fold into CardTypes because 613.1d is one sublayer.

modification_characteristic_writes covers all 57 ContinuousModification variants. target_filter_characteristic_reads covers all 98 FilterProp variants, with sibling walkers over QuantityRef and StaticCondition. All exhaustive and wildcard-free, so a new variant on either enum fails to compile until it is classified — the tripwire the old classifier had, now on both surfaces instead of one. Uncertain forms map to the full set, so a misclassification can only over-escalate.

The read set has to include affected filters unconditionally

I 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. name_rewrite_entry_escalates_through_affected_filter_reads is that board.

CR 613.6 carves out exactly one shape

A 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. incremental_entry_retains_multi_layer_effect_affected_set is the pre-existing board that proves it, and it regresses without the carve-out.

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 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 cross-group channel above is untouched by it: that fixture's rename and buff live in different definitions, so the buff's set is determined after the rename applies.

Ordering

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 token-storm board leaves there, since keyword grants write abilities while the board reads card types and controller, with zero matches_target_filter calls. That is stricter than the ordering it replaces, not looser.

Evidence

Eight 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.

reverted	fixture
AddStaticMode write → EMPTY	keyword_grant_entry_escalates_when_population_is_keyword_keyed
HasXInActivationCost read → EMPTY	same fixture, read side
SetPower/SetToughness writes → EMPTY	pt_change_entry_escalates_when_population_is_pt_keyed
SetChosenName write → EMPTY	name_change_entry_escalates_when_population_is_name_keyed
writer → ALL	keyword_grant_entry_stays_incremental_when_population_reads_are_disjoint
ChangeController → ALL	control_change_entry_stays_incremental_when_reads_are_pt_only
drop the affected-filter union term	name_rewrite_entry_escalates_through_affected_filter_reads
condition-filter recursion → EMPTY	keyword_grant_entry_escalates_through_condition_filter_reads
drop the CR 613.6 exclusion	pt_writer_entry_stays_incremental_when_only_its_own_affected_filter_reads

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: 25305 tests run: 25296 passed, 9 failed, 29 skipped. All nine failures are the pre-existing mtgish-import card-import set (convert::action, convert::replacement, six golden_structural, manifest_coverage); zero engine failures. cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings clean; fmt clean.

What I removed, and what I left

The remaining-gap block is deleted, because the matrix is what it was asking for. Grant chains close through RemoveAllAbilities mapping to the full set and GrantStaticAbility recursing into the granted definition's modifications. What is left is stated conservatism, not blindness, and it is written down at the gate.

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:

1. The matches! lists driving CR 613.8 dependency ordering have drifted from each other and from this classifier. Now that a single authority exists they could be replaced by it, but that changes ordering behaviour and needs its own fixtures.
2. The boolean membership classifiers feeding the Axis 2a population probe do not recurse nested reference filters in a couple of arms. That is Axis 2a's scope, and this PR keeps Axis 2a bit-identical deliberately, so I left it alone.

Ready for another look.

🤖 Generated with Claude Code

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

crates/engine/src/game/stack.rs (1)

11246-11251: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Update the stale fixture reference.

Replace controller_theft_count_anthem_board with controller_keyed_count_anthem_with_control_theft_board in 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 win

Remove the _ wildcard in the DistinctFrom arm.

The doc comment at Line 535-538 states this classifier is "EXHAUSTIVE, wildcard-free". The inner match on reference breaks that invariant. The inner enum is TargetFilter, which is known. A new TargetFilter variant that reads nothing will silently take the ALL branch, 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

📥 Commits

Reviewing files that changed from the base of the PR and between d8e11e9 and 2a3b55b.

📒 Files selected for processing (4)

• crates/engine/src/game/filter.rs
• crates/engine/src/game/layers.rs
• crates/engine/src/game/quantity.rs
• crates/engine/src/game/stack.rs

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Confirm how FilterProp::Owned resolves its ControllerRef at match time.
rg -nP -C10 'FilterProp::Owned' crates/engine/src/game/filter.rs | head -80

Repository: phase-rs/phase

Length of output: 3404

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- filter.rs structure ---'
ast-grep outline crates/engine/src/game/filter.rs | sed -n '1,180p'
printf '%s\n' '--- target scope classification ---'
sed -n '380,445p' crates/engine/src/game/filter.rs
printf '%s\n' '--- FilterProp classification ---'
sed -n '690,745p' crates/engine/src/game/filter.rs
printf '%s\n' '--- scope declarations and uses ---'
rg -n -C4 'enum TargetFilter|ControllerAndControlledPermanents|ScopedPlayer|TargetFilter::(Controller|Opponent|ScopedPlayer)|enum FilterProp|Owned \{' crates/engine/src crates/engine/tests

Repository: phase-rs/phase

Length of output: 50371

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- controller reference definitions ---'
rg -n -C6 'enum ControllerRef|ControllerRef::|resolve.*controller|controller.*source|source.*controller' crates/engine/src/game crates/engine/src | head -260
printf '%s\n' '--- Owned matching implementation ---'
rg -n -C12 'Owned\s*\{' crates/engine/src/game/filter.rs crates/engine/src/game | head -220
printf '%s\n' '--- characteristic-kind APIs and callers ---'
rg -n -C5 'CharacteristicKinds|read.*character|entry.incremental|incremental.*flush|flush' crates/engine/src/game/filter.rs crates/engine/src | head -320

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
p = Path("crates/engine/src/game/filter.rs")
text = p.read_text()
for needle in [
    "TargetFilter::Controller",
    "TargetFilter::Opponent",
    "TargetFilter::ScopedPlayer",
    "TargetFilter::StackAbility",
    "FilterProp::Owned",
]:
    print(f"--- {needle} ---")
    starts = [i for i in range(len(text)) if text.startswith(needle, i)]
    for i in starts[:12]:
        line = text.count("\n", 0, i) + 1
        print(f"line {line}")
        print("\n".join(text.splitlines()[max(0,line-4):line+8]))
PY

Repository: phase-rs/phase

Length of output: 24540

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- exact enum payloads ---'
rg -n -C8 'TargetFilter\s*::|pub enum TargetFilter|enum TargetFilter|FilterProp\s*::|pub enum FilterProp|enum FilterProp' crates/engine/src/game/filter.rs crates/engine/src/game | head -420
printf '%s\n' '--- all ControllerRef fields in filter types ---'
rg -n -C5 'ControllerRef' crates/engine/src/game/filter.rs crates/engine/src/game | head -360

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
# Read-only behavioral verifier: identify whether the relevant classification
# arms carry a ControllerRef payload and whether the matcher uses it.
python3 - <<'PY'
from pathlib import Path
p = Path("crates/engine/src/game/filter.rs")
lines = p.read_text().splitlines()
for lo, hi in [(1, 180), (250, 335), (380, 445), (680, 750), (900, 970), (4300, 4420)]:
    print(f"--- lines {lo}-{hi} ---")
    for n in range(lo, min(hi, len(lines)) + 1):
        print(f"{n}: {lines[n-1]}")
PY

Repository: phase-rs/phase

Length of output: 31759

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- relevant filter classification ---'
sed -n '380,445p' crates/engine/src/game/filter.rs
sed -n '690,745p' crates/engine/src/game/filter.rs
printf '%s\n' '--- declarations and resolution paths ---'
rg -n -C6 'enum TargetFilter|enum FilterProp|ControllerRef|TargetFilter::(Controller|Opponent|ScopedPlayer)|FilterProp::Owned' crates/engine/src/game crates/engine/src | head -500

Repository: phase-rs/phase

Length of output: 46198

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- complete FilterProp characteristic classifier ---'
sed -n '535,705p' crates/engine/src/game/filter.rs
printf '%s\n' '--- player matching and source-controller construction ---'
sed -n '1080,1155p' crates/engine/src/game/filter.rs
sed -n '1275,1475p' crates/engine/src/game/filter.rs
sed -n '6940,7045p' crates/engine/src/game/filter.rs
printf '%s\n' '--- classifier call sites ---'
rg -n -C8 'target_filter_characteristic_reads|filter_prop_characteristic_reads|CharacteristicKinds::CONTROLLER' crates/engine/src/game | head -260

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- TargetFilter and ControllerRef definitions ---'
rg -n -A95 -B8 'pub enum TargetFilter|pub enum ControllerRef|pub struct TypedFilter|pub enum FilterProp' crates/engine/src/types/ability.rs
printf '%s\n' '--- layer flush relation ---'
rg -n -A35 -B15 'target_filter_characteristic_reads|CharacteristicKinds|read_kinds|write_kinds|entry-incremental|incremental' crates/engine/src/game/layers.rs crates/engine/src/game/derived.rs crates/engine/src/game | head -420

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- usages of player-only TargetFilter arms ---'
rg -n -C5 'TargetFilter::Controller|TargetFilter::Opponent|TargetFilter::ScopedPlayer' crates/engine/src/game | head -360
printf '%s\n' '--- affected-filter callers ---'
rg -n -C10 'matches_filter|matches_player|target_filter_characteristic_reads|affected_filter_uses_object_population' crates/engine/src/game/layers.rs crates/engine/src/game/derived.rs crates/engine/src/game/filter.rs | head -420

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- focused classifier and matcher slices ---'
sed -n '535,705p' crates/engine/src/game/filter.rs
sed -n '1268,1318p' crates/engine/src/game/filter.rs
sed -n '1430,1475p' crates/engine/src/game/filter.rs
sed -n '6960,7035p' crates/engine/src/game/filter.rs
printf '%s\n' '--- source context and layer flush references ---'
rg -n -C12 'fn source_context_from_filter|source_controller:|target_filter_characteristic_reads\(' crates/engine/src/game/filter.rs crates/engine/src/game/layers.rs crates/engine/src/game/derived.rs

Repository: phase-rs/phase

Length of output: 50370

---

🏁 Script executed:

#!/bin/bash
set -e
# Read-only verifier for the disputed dependency:
# report the classifier arm and the executable match expressions for each scope.
python3 - <<'PY'
from pathlib import Path
s = Path("crates/engine/src/game/filter.rs").read_text()
checks = {
    "TargetFilter characteristic classifier": (
        "TargetFilter::Controller",
        "TargetFilter::Opponent",
        "TargetFilter::ScopedPlayer",
    ),
    "FilterProp characteristic classifier": (
        "FilterProp::Owned { .. }",
    ),
    "Owned live-object matcher": (
        "FilterProp::Owned { controller } => match controller",
        "source.controller == Some(obj.owner)",
        "scoped_player_or_controller(state, source.ability, source.controller, None)",
    ),
    "Owned zone-record matcher": (
        "FilterProp::Owned { controller } => match controller",
        "source.controller == Some(record.owner)",
        "scoped_player_or_controller(state, source.ability, source.controller, None)",
    ),
}
for title, needles in checks.items():
    print(f"--- {title} ---")
    for needle in needles:
        positions = []
        start = 0
        while True:
            i = s.find(needle, start)
            if i < 0:
                break
            positions.append(s.count("\n", 0, i) + 1)
            start = i + 1
        print(f"{needle!r}: lines {positions[:20]}{' ...' if len(positions) > 20 else ''}")
PY

Repository: phase-rs/phase

Length of output: 1132

---

🏁 Script executed:

#!/bin/bash
set -e
sed -n '535,705p' crates/engine/src/game/filter.rs
sed -n '1268,1318p' crates/engine/src/game/filter.rs
sed -n '1430,1475p' crates/engine/src/game/filter.rs
sed -n '6960,7035p' crates/engine/src/game/filter.rs

Repository: phase-rs/phase

Length of output: 17513

---

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- complete Owned match arms ---'
sed -n '5198,5248p' crates/engine/src/game/filter.rs
sed -n '5950,6000p' crates/engine/src/game/filter.rs

Repository: phase-rs/phase

Length of output: 5909

---

🏁 Script executed:

#!/bin/bash
set -e
sed -n '5198,5248p' crates/engine/src/game/filter.rs
sed -n '5950,6000p' crates/engine/src/game/filter.rs

Repository: phase-rs/phase

Length of output: 5875

---

Classify FilterProp::Owned as a CONTROLLER read. Both live-object and zone-change matching resolve ControllerRef::You, ControllerRef::Opponent, and ControllerRef::ScopedPlayer through source.controller, but the classifier at crates/engine/src/game/filter.rs#L721-L723 returns EMPTY. This can leave the entry-incremental flush with a stale verdict after a layer-2 control change.

📍 Affects 1 file

• crates/engine/src/game/filter.rs#L417-L424 (this comment)
• crates/engine/src/game/filter.rs#L721-L723

🤖 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 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.

[matthewevans]

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.

[mcbradd]

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 error[E0004]: non-exhaustive patterns: &StaticCondition::DuringOpponentsTurn not covered. Main had added a StaticCondition variant, CI builds the PR merged with main, and static_condition_characteristic_reads_at is wildcard-free — so the merge could not compile until somebody classified the new variant. That is the tripwire in the comment above doing exactly the thing I claimed for it, on the first new variant to arrive after I wrote it. It cost a red run and a rebase, which is the trade I would take again: the alternative is a _ => arm that silently classifies a new condition as reading nothing.

d802570 classifies it. It joins the DuringYourTurn arm — both consult the active player (CR 102.1), neither consults a layer-writable characteristic. The two are deliberately not Not of each other (in a team game a teammate can be the active player while the controller's team holds the turn, CR 102.3 + CR 805.4a), but that distinction is invisible to a classifier that only asks which characteristics get read.

The rebase itself was conflict-free across all four earlier commits — main's drift on the files this touches was 17 lines in layers.rs, 14 in quantity.rs, 98 in stack.rs, and nothing in filter.rs. The four commits are content-identical to what you reviewed; only their SHAs changed. DuringOpponentsTurn was the only variant the compiler rejected, on any of the four matched enums.

One question I raised against myself and then closed

Classifying a turn-relative condition as reading nothing invites the obvious objection: DuringYourTurn resolves "your" against the source's controller, and controller is layer-2 writable (CR 613.1b), so shouldn't it read Controller?

No, and the reason is the binding point rather than the CR. Both arms collect their effect set at the same moment — prepare_incremental_flush calls collect_shared_active_continuous_effects after resetting recipients to base and before any layer applies, and the full pass binds at that same point. A condition therefore sees the pre-layer-2 controller in both arms, so a control change cannot make them disagree, and this gate's contract is only that the two arms agree. Whether conditions ought to re-evaluate after layer 2 at all (CR 611.3a) is a real question, but it is the same question for both arms and not one a divergence gate can answer. Widening the classifier for it would have bought pure over-escalation. The feature-on differential sweep — comparator active on every flush, 22401 engine tests, zero divergence — is the standing evidence that the two arms do not in fact disagree here.

At this head

25673 tests run: 25664 passed, 9 failed, 29 skipped. The nine are the same pre-existing mtgish-import set as before (convert::action, convert::replacement, six golden_structural, manifest_coverage); zero engine failures, and this branch touches no file in that crate. cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings clean; cargo fmt --all --check clean. Gate A PASS head=d802570cdea5f1b3aa2275a5d8970496a4dbaed8 base=a95fb8776e965ffc898af894ae213f265abe753e, Gate G PASS. The push also ran the repo's own pre-push battery end to end — workspace clippy including phase-tauri, release check, parser gate, card-data generate/validate, coverage regression, frontend type-check — all green.

🤖 Generated with Claude Code

[matthewevans]

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]

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]

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.