Skip to content

fix(engine): transform "all <type>" as a mass effect, no target (Moonmist) - #6629

Merged
matthewevans merged 6 commits into
phase-rs:mainfrom
michiot05:fix/transform-all-mass-effect-6403
Jul 26, 2026
Merged

fix(engine): transform "all <type>" as a mass effect, no target (Moonmist)#6629
matthewevans merged 6 commits into
phase-rs:mainfrom
michiot05:fix/transform-all-mass-effect-6403

Conversation

@michiot05

@michiot05 michiot05 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Tier: Frontier
Model: claude-opus-4-8

Closes #6403.

Summary

Moonmist"Transform all Humans. Prevent all combat damage that would be dealt this turn by creatures other than Werewolves and Wolves." — forced a single-target prompt to pick one Human. The parser emitted a single-target Effect::Transform { target: Typed{Subtype Human} }, and the effect exposed a target slot, so a non-targeting mass effect was resolved as a targeted one (only the source transformed). "Transform all X" doesn't target (CR 115.10) — it should transform every Human on the battlefield with no prompt.

Rather than add a TransformAll sibling, this scope-parameterizes Transform to carry the single-vs-mass axis — the exact refactor the codebase already applied to Tap/TapAllSetTapState { scope } (the EffectScope doc-comment names it), so /add-engine-variant approves it as parameterization, not proliferation:

Effect::Transform { target } → Effect::Transform { target, scope: EffectScope }
  • EffectScope::Single (serde default): the legacy targeted/anaphoric transform ("transform target creature", "transform ~"), unchanged.
  • EffectScope::All: a non-targeting population filter enumerated at resolution. Effect::target_filter() returns None for All (so no target slot / prompt is built — mirroring SetTapState/DestroyAll), and a new resolve_all iterates the battlefield population applying the transform to each match, mirroring tap_untap::resolve_all.

Two rules-correctness details that make the mass path actually work:

  • target_filter() is the single target-exposure authority. Both production slot-builder paths route through it, so None for All fully removes the prompt (CR 115.10).
  • Non-transformable matches are a harmless no-op (CR 701.27c). resolve_all pre-filters the matched population to double-faced permanents (is_double_faced_permanent) so a matched single-faced creature is skipped rather than erroring — transform_permanent returns Err("no back face") for a non-DFC, so the mass loop must not propagate it (a naive mirror would abort the whole transform on the first single-faced Human).

The parser gains a "transform/convert all|each <filter>" mass branch (before the single branch), the loop-safety census classifiers scope-split Transform to a live-board census for All (mirroring DestroyAll — a wrong classification would defeat the LoopFirewall), and every SetTapState{Single}-gated site gets the parallel Transform gate. The serde default keeps old serialized Transform{target} deserializing to Single (byte-compatible).

Implementation method (required)

Method: /engine-implementer — plan (/engine-planner + /add-engine-effect, with a run-then-delete reproduction probe confirming the single-target prompt) + the /add-engine-variant gate (verdict: scope-parameterize per the SetTapState precedent; corrected the CR citations — transform is 701.27, not 701.28) → /review-engine-plan (found 5 blocking gaps: the plan compiled but did not fix the bug — the target_filter() linchpin was untouched, transform_permanent errors rather than no-ops on non-DFCs, the census classifiers were left scope-blind, ≥5 compile sites and the SetTapState-gate matrix were incomplete — all corrected) → implement → /review-impl (Maintainer-Simulation Gate PASS; independently confirmed all 5 blockers correct — target_filter() is the only target-exposure surface, census classification correct, Single-gate parity complete; one unreachable LOW noted below). Each step in a fresh agent context.

Gate A

./scripts/check-parser-combinators.shGate A PASS head=1613e601a (Gate G PASS). The mass parser branch is composed combinators (preceded/alt/tag/peek).

Anchored on

  • Effect::SetTapState { target, scope, state } (types/ability.rs, the Tap/TapAll unification) — the end-to-end template: the EffectScope axis, the target_filter() scope-split (Single => Some(target), All => None), the census-classifier scope arms, and the Single-gate sites are all mirrored for Transform.
  • game/effects/tap_untap.rs::resolve_all — the mass-resolution shape (resolved_object_filter + matches_target_filter over state.battlefield, per-object, no targets) that transform_effect::resolve_all mirrors, with the DFC pre-filter added for CR 701.27c.
  • game/ability_scan.rs DestroyAll/Suspect{All} census arms — the correct loop-safety classification (LiveBoardCensus/Census) for a mass effect that iterates the growing battlefield; Transform{All} joins that group (explicitly not the SetTapState-only SnapshotOrEvent exception, which stays pinned by obligation_ii_census_exception_is_exactly_settapstate).

Verification

  • cargo fmt --all clean; cargo build -p engine -p mtgish-import -p phase-ai clean (every scope site threaded); cargo clippy -p engine --lib --tests -- -D warnings 0 warnings; cargo test -p engine --lib 17,706 passed / 0 failed (no drift from the shared Effect change, incl. the census/LoopFirewall tests); Gate A/G PASS.
  • Revert-sensitive tests (production path — real parse_oracle_text + real resolver, back_face DFC modeling):
    • moonmist_transforms_all_humans_without_a_prompt (primary): parse Moonmist verbatim, resolve over 2 DFC Humans + a Goblin + a Werewolf → both Humans transform, Goblin/Werewolf untouched, no WaitingFor prompt, target_filter().is_none(), PreventDamage sibling preserved.
    • mass_transform_exposes_no_target_slot (B1): Transform{All}.target_filter() == None, SingleSome(target).
    • mass_transform_skips_single_faced_human_without_error (B2): a single-faced Human (back_face:None) in the population → the DFC transforms, the single-faced Human is untouched, resolution does not error (removing the DFC pre-filter makes it error — revert-sensitive).
    • parse_transform_all_is_mass_scope / parse_transform_target_is_single_scope: the mass branch emits scope:All; "transform target creature" stays Single (control — mass branch didn't regress single).
  • CR annotations grep-verified against docs/MagicCompRules.txt: 701.27 / 701.27a (transform), 701.27c (a permanent that can't transform does nothing), 115.10 / 115.10a ("all X" is non-targeting).

Claimed parse impact

Serialized Effect::Transform gains a scope field: existing Transform cards mechanically gain "scope":"Single" (serde default → byte-compatible deserialization of old states), and Moonmist becomes "scope":"All". The parser mass branch fires only on literal "transform/convert all|each <filter>", so no other card's Transform parse changes shape. mtgish-import threads scope:Single on its existing TransformPermanent action (a forced compile-consequence of the shared reshape). Frontend unchanged (no client Effect::Transform handling). The exact card-data parse-diff (which cards carry a "transform all" line beyond Moonmist) is deferred to CI's coverage-parse-diff artifact — the scope parameter is the general building block for the whole mass-transform class.

Scope Expansion

None. Honest latent parity note (out of class, not a regression): the tracked-set-recording arm that special-cases SetTapState{All} for chained "those cards" sub-abilities has no Transform{All} sibling — unreachable today (Moonmist is a Typed population filter and no tracked-set mass-transform card exists; Transform{Single} had no such handling either). I left it rather than add a speculative guard; a Transform{All} arm can be added if such a card ever appears.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added end-to-end support for mass “transform/convert all” so qualifying double-faced permanents transform in a single resolution.
    • Transform instructions now explicitly distinguish single-target vs non-targeting mass behavior.
  • Bug Fixes

    • Corrected Transform target/coverage behavior to be scope-aware, preventing mix-ups between single and mass variants.
    • Updated parsing so unsupported front-face restrictions fail safely instead of degrading into incorrect behavior.
  • Tests

    • Added a Moonmist mass-transform regression test (including “no extra prompt” behavior) and expanded parser/resolution coverage for scope handling.

…mist)

Moonmist's "Transform all Humans" forced a single-target prompt to pick one
Human: the parser emitted single-target Effect::Transform{target} and the
effect exposed a target slot, so a non-targeting mass effect was resolved as
a targeted one (only the source transformed).

Scope-parameterize Transform to carry the single-vs-mass axis, mirroring the
Tap/TapAll -> SetTapState{scope} refactor (no new sibling variant):
  Effect::Transform { target } -> Transform { target, scope: EffectScope }
- EffectScope::Single (serde default): legacy targeted transform, unchanged.
- EffectScope::All: a non-targeting population filter enumerated at
  resolution. Effect::target_filter() returns None for All (no target slot /
  prompt built), mirroring SetTapState/DestroyAll (CR 115.10 — "all X" doesn't
  target). resolve_all iterates the battlefield population, pre-filtering to
  double-faced permanents so a matched single-faced creature is a harmless
  no-op instead of an error (CR 701.27c).

Parser gains a "transform/convert all|each <filter>" mass branch (before the
single branch); the census/loop-safety classifiers scope-split Transform to
LiveBoardCensus/Census for All (mirroring DestroyAll), and every
SetTapState{Single}-gated site gets the parallel Transform gate. Serde default
keeps old Transform{target} deserializing to Single. Frontend unchanged.

Closes phase-rs#6403

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@michiot05
michiot05 requested a review from matthewevans as a code owner July 25, 2026 11:37
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@matthewevans, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 36 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 87b290e9-31d4-4715-b1b0-76bf49bcd5e5

📥 Commits

Reviewing files that changed from the base of the PR and between 4ddb629 and 93892a6.

📒 Files selected for processing (1)
  • crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs
📝 Walkthrough

Walkthrough

Transform effects now carry explicit Single or All scope. Parsing, resolution, target analysis, coverage, import conversion, and tests distinguish targeted transforms from mass transforms that affect eligible double-faced permanents.

Changes

Transform scope support

Layer / File(s) Summary
Scope contracts and parser lowering
crates/engine/src/parser/..., crates/engine/src/types/ability.rs, crates/mtgish-import/src/convert/action.rs
Transform stores an explicit scope with legacy serde defaults; targeted and mass text lower to Single and All.
Single and mass resolution
crates/engine/src/game/effects/..., crates/engine/src/types/ability.rs, crates/engine/src/game/coverage.rs
Mass transforms filter battlefield objects, transform eligible double-faced permanents, emit one resolution event, and expose no target slot.
Target analysis and context routing
crates/engine/src/game/ability_scan.rs, crates/engine/src/game/ability_utils.rs, crates/engine/src/game/ability_rw.rs
Mass transforms use census and population-filter handling, while single transforms retain bounded target handling and granting-object concretization.
Regression coverage and fixtures
crates/engine/tests/integration/*, crates/engine/src/game/effects/transform_effect.rs, crates/engine/src/parser/...
Tests cover Moonmist parsing and resolution, strict unsupported restrictions, target-slot behavior, and explicit single-scope constructions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OracleParser
  participant EffectLowering
  participant TransformResolver
  participant Battlefield
  participant GameEvent
  OracleParser->>EffectLowering: produce Transform with Single or All scope
  EffectLowering->>TransformResolver: resolve scoped Transform
  TransformResolver->>Battlefield: evaluate mass filter when scope is All
  Battlefield-->>TransformResolver: matching permanents
  TransformResolver->>GameEvent: emit EffectResolved after transformations
Loading

Possibly related PRs

  • phase-rs/phase#6569: Updates adjacent effect-scanning and resolver-routing logic for Effect::FlipPermanent.

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 summarizes the main change: treating "all " transforms as a non-targeting mass effect.
Linked Issues check ✅ Passed The PR implements #6403 by making Moonmist-style "Transform all Humans" resolve as a mass effect with no target prompt.
Out of Scope Changes check ✅ Passed The changes are all tied to the scope-aware Transform refactor and Moonmist fix, with no obvious unrelated additions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@matthewevans matthewevans self-assigned this Jul 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/parser/oracle_trigger_tests.rs`:
- Line 5391: Update the Effect::Transform match in the relevant parser test to
destructure scope separately while retaining .. for other payload fields, then
assert that the parsed anaphoric “transform it” effect has EffectScope::Single.

In `@crates/mtgish-import/src/convert/action.rs`:
- Around line 3821-3822: Update the CR citation in the TransformPermanent
conversion block to reference CR 701.28/701.28a, keeping EffectScope::Single
unchanged.
🪄 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: 086549cc-2810-47af-9a7a-d2df8bac0bbe

📥 Commits

Reviewing files that changed from the base of the PR and between b9238c8 and 1613e60.

📒 Files selected for processing (14)
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/effects/token.rs
  • crates/engine/src/game/effects/transform_effect.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/parser/oracle_ir/ast.rs
  • crates/engine/src/parser/oracle_tests.rs
  • crates/engine/src/parser/oracle_trigger_tests.rs
  • crates/engine/src/types/ability.rs
  • crates/mtgish-import/src/convert/action.rs

Comment thread crates/engine/src/parser/oracle_trigger_tests.rs Outdated
Comment on lines +3821 to +3822
// CR 701.27a: mtgish `TransformPermanent` is a single targeted transform.
scope: EffectScope::Single,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

rg -n -A8 -B2 '^701\.28' docs/MagicCompRules.txt

Repository: phase-rs/phase

Length of output: 216


🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Repo root: '; pwd

# Find the rules document and the target source file.
fd -a 'MagicCompRules.txt' .
fd -a 'action.rs' crates/mtgish-import/src/convert

# Inspect the relevant area around the cited annotation.
if [ -f crates/mtgish-import/src/convert/action.rs ]; then
  sed -n '3808,3830p' crates/mtgish-import/src/convert/action.rs | cat -n
fi

# Search the rules document for transform-related sections if present.
rules_doc="$(fd -a 'MagicCompRules.txt' . | head -n 1 || true)"
if [ -n "$rules_doc" ]; then
  rg -n -A6 -B2 '701\.(27|28)' "$rules_doc"
fi

Repository: phase-rs/phase

Length of output: 1722


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the rules document anywhere in the repo.
fd -a 'MagicCompRules.txt' /home/jailuser/git || true
fd -a 'MagicCompRules.txt' . || true

# If found, show the transform sections.
rules_doc="$(fd -a 'MagicCompRules.txt' /home/jailuser/git | head -n 1 || true)"
if [ -n "$rules_doc" ]; then
  echo "RULES_DOC=$rules_doc"
  rg -n -A8 -B2 '701\.(27|28)' "$rules_doc"
fi

# Also inspect nearby comments in the source for the exact wording/context.
sed -n '3806,3826p' /home/jailuser/git/crates/mtgish-import/src/convert/action.rs | cat -n

Repository: phase-rs/phase

Length of output: 1525


Use CR 701.28/701.28a here. TransformPermanent is transform, so both citations should point to CR 701.28 instead of 701.27/701.27a.

🤖 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/mtgish-import/src/convert/action.rs` around lines 3821 - 3822, Update
the CR citation in the TransformPermanent conversion block to reference CR
701.28/701.28a, keeping EffectScope::Single unchanged.

Source: Path instructions

@matthewevans matthewevans added the bug Bug fix label Jul 25, 2026
@matthewevans matthewevans removed their assignment Jul 25, 2026
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 4 card(s), 6 signature(s) (baseline: main 054be34ef78a)

🟢 Added (1 signature)

  • 1 card · ➕ replacement/DamageDone · added: DamageDone (combat=CombatOnly, shield=Prevention { amount: All })
    • Affected (first 3): That's No Moonmist

🔴 Removed (1 signature)

  • 1 card · ➖ ability/Transform · removed: Transform (target=artifact or creature Phyrexian)
    • Affected (first 3): That's No Moonmist

🟡 Modified fields (4 signatures)

  • 2 cards · 🔄 ability/Transform · changed field filter: token you control Incubator
    • Affected (first 3): Glissa, Herald of Predation, The Argent Etchings
  • 2 cards · 🔄 ability/Transform · changed field target: token you control Incubator
    • Affected (first 3): Glissa, Herald of Predation, The Argent Etchings
  • 1 card · 🔄 ability/Transform · changed field filter: Human
    • Affected (first 3): Moonmist
  • 1 card · 🔄 ability/Transform · changed field target: Human
    • Affected (first 3): Moonmist

@matthewevans matthewevans self-assigned this Jul 25, 2026
@matthewevans matthewevans removed their assignment Jul 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/tests/integration/issue_6403_moonmist_mass_transform.rs`:
- Around line 67-97: Add a single-faced Human creature without BackFaceData to
the Moonmist scenario, then assert via its identifier that it remains
untransformed after casting. Keep the existing transformed Human assertions,
non-Human assertion, and outcome.final_waiting_for() WaitingFor::Priority check
unchanged so the matching non-transformable permanent is validated through the
cast path.
🪄 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: ca9d48d8-8299-4e82-8378-80403f3b9e32

📥 Commits

Reviewing files that changed from the base of the PR and between 1613e60 and 6400ed5.

📒 Files selected for processing (3)
  • crates/engine/src/parser/oracle_trigger_tests.rs
  • crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs
  • crates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/engine/src/parser/oracle_trigger_tests.rs

@matthewevans matthewevans self-assigned this Jul 26, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Mass transform drops the printed on their front face restriction for That's No Moonmist. Evidence: imperative.rs:5334 discards the remainder from parse_target_with_ctx and immediately emits EffectScope::All; the current parse-diff therefore records That's No Moonmist only as artifact or creature Phyrexian. Its current Oracle text is: “Transform all artifacts and Phyrexian creatures on their front face.” resolve_all then transforms every matched DFC, including a matching back-face permanent that the card must leave unchanged.

Please either model the front-face condition as a typed population predicate and add a regression that keeps a matching transformed permanent unchanged, or strict-fail/preserve Effect::unimplemented whenever this mass-transform branch leaves that semantic suffix unparsed. The latter is acceptable if the predicate is intentionally deferred; silently marking the card supported is not.

@matthewevans matthewevans removed their assignment Jul 26, 2026
@matthewevans matthewevans self-assigned this Jul 26, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[HIGH] Mass transform still silently drops the printed on their front face restriction for That's No Moonmist. imperative.rs:5334 discards the parsed suffix as (target, _) before emitting EffectScope::All; the PR's published parse-diff consequently records only the artifact or creature Phyrexian filter. The card's Oracle text also restricts the population to objects on their front face.

resolve_all currently filters only by that target filter and double-faced status, so it can transform a matching DFC already on its back face, although that object must be left unchanged.

The prior requested change at 6400ed502b78d239d407b4006c853957da5cc522 was not remediated by the merge-only current-head change. Please carry the front-face restriction as a typed population predicate and add a production cast-path regression containing a matching back-face permanent; otherwise strict-fail this rules-bearing suffix until it is represented.

@matthewevans matthewevans removed their assignment Jul 26, 2026
…s unparsed (phase-rs#6403 review)

Address matthewevans review on phase-rs#6629: the mass "transform all|each <filter>"
branch discarded the parse_target_with_ctx remainder, silently dropping the
"on their front face" restriction on That's No Moonmist ("Transform all
artifacts and Phyrexian creatures on their front face") — resolve_all could
then transform a matching back-face permanent CR 712.2 must leave unchanged.

The branch now captures the remainder: an empty remainder (Moonmist's fully
consumed "all Humans") still emits EffectScope::All; a surviving rules-bearing
suffix returns None so the line falls through to Effect::unimplemented (the
single authority) rather than shipping a silently-lossy Transform — and does
not fall through to the single "transform <target>" branch. Strict-failing the
suffix-bearing cards also means resolve_all only runs on fully-parsed
populations like "all Humans" (where the Human typing already implies front
face), so the mass path can never transform a matching back-face permanent.

Also thread the new Effect::Transform.scope field through two merge-drift sites
that main added after the branch point: cr733_resolved_transform.rs
(scope: Single) and the maintainer-added regression's BackFaceData
(printed_loyalty: None).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@michiot05

Copy link
Copy Markdown
Contributor Author

Thanks — fixed at head 4ddb629d0.

Front-face restriction (That's No Moonmist). Took the strict-fail option you offered: there's no parse_target_with_ctx support for consuming an "on their front face" postnominal into a typed population predicate today, and modeling it for essentially one card would be a special case rather than a building block — so I've made the mass branch strict-fail rather than silently drop the suffix.

The "transform/convert all|each <filter>" branch now captures the parse_target_with_ctx remainder (previously discarded as _):

  • Empty remainder (Moonmist's "all Humans", fully consumed) → EffectScope::All, unchanged.
  • A surviving rules-bearing suffix (" on their front face") → return None, so the line falls through to Effect::unimplemented (the single authority — not a hand-constructed literal), and critically does not fall through to the single "transform <target>" branch that would emit a targeted scope: Single transform still dropping the suffix.

This also closes the resolve_all back-face concern you raised: strict-failing the suffix-bearing cards means resolve_all only ever runs on fully-parsed populations like "all Humans", where the Human typing already implies front face (a transformed back-face Werewolf is a Werewolf, not a Human, so it never matches the population filter) — the mass path can't transform a matching back-face permanent.

Regressions (both pass): thats_no_moonmist_front_face_restriction_stays_unimplemented (verbatim Oracle → Effect::Unimplemented, with a Moonmist control that stays a supported scope: All), plus a parser-shape assertion that the suffixed phrase returns None. Your moonmist_cast_transforms_all_humans_without_target_selection still passes.

CI (was red). The auto-merge of main into the branch introduced two enum-reshape drift breaks in the integration crate (which my earlier local run hadn't compiled against merged-main): cr733_resolved_transform.rs constructing Effect::Transform without the new scope field, and the BackFaceData in your added issue_6403_moonmist_mass_transform.rs missing printed_loyalty (added on main after the branch point). Both threaded (scope: EffectScope::Single, printed_loyalty: None). Verified this round with cargo test -p engine --test integration --no-run (the crate that was failing): integration 77 passed, lib 17,747 passed / 0 failed, clippy 0, Gate A PASS. CR annotations grep-verified (701.27, 115.10a, 712.2).

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
crates/engine/tests/integration/cr733_resolved_transform.rs (1)

111-125: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert the replayed transformation count.

The test validates the journaled count against the live object, but replay only checks face state and timestamp. apply_resolved_transform could restore an incorrect count while this test still passes, despite claiming replay exactness. Add an assertion that replayed.transformation_count equals transform.resulting_transformation_count.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/tests/integration/cr733_resolved_transform.rs` around lines 111
- 125, Extend the replay assertions in the integration test after applying
apply_resolved_transform to verify that replayed.transformation_count equals
transform.resulting_transformation_count. Keep the existing face, name, and
timestamp assertions unchanged and use the same descriptive assertion style.
crates/engine/src/game/ability_rw.rs (2)

8181-8199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Exercise the ordering decision, not only the profile fields.

This test calls rw_quantity_ref and ability_rw_profile, but never reaches profiles_conflict or the production trigger-ordering path. A regression in how reads_event_live affects conflict classification would therefore still pass. Add a positive event-live-read × event-object-write case and controls for missing event objects or missing writes through the ordering predicate.

🤖 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/ability_rw.rs` around lines 8181 - 8199, Extend
triggering_scry_look_count_classifies_event_live to exercise the
trigger-ordering predicate, not just reads_event_live metadata: add a positive
conflict case combining an event-live read with an event-object write, plus
controls where the event object or write is absent. Route these cases through
profiles_conflict or the production trigger-ordering path and assert the
expected classifications, preserving the existing profile and twin-parity
checks.

Source: Path instructions


2953-2961: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add CR citations for both Effect::ExileFaceDownPile arms. They encode the face-down exile pile move and the HandLibrary/membership writes, but neither branch has a CR <number>: <description> annotation.

🤖 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/ability_rw.rs` around lines 2953 - 2961, Add CR
citations to both Effect::ExileFaceDownPile match arms, including descriptions
covering the face-down exile-pile move and the HandLibrary/membership writes.
Place the annotations directly on the relevant branches and preserve the
existing legacy_quantity_expr, legacy_target_filter, and return behavior.

Source: Path instructions

♻️ Duplicate comments (1)
crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs (1)

54-102: 🎯 Functional Correctness | 🟠 Major

Cover a matching single-faced Human.

Both Humans receive BackFaceData, so this test never exercises the CR 701.27c no-op for a matching but non-transformable permanent. Add a Human without BackFaceData, assert it remains untransformed, and retain the existing prompt-free resolution assertion.

As per path instructions, regression tests must cover the failure path through the production pipeline.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs` around
lines 54 - 102, Add a third Human permanent without BackFaceData in
moonmist_cast_transforms_all_humans_without_target_selection, and assert it
remains untransformed after resolution while the existing transformable Humans
and non-Human assertions remain intact. Keep the test using
runner.cast(moonmist).resolve() and retain the WaitingFor::Priority assertion to
cover the production pipeline without target selection.

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.

Inline comments:
In `@crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs`:
- Around line 119-141: The test
thats_no_moonmist_front_face_restriction_stays_unimplemented must add a positive
reach guard for the exact “all artifacts and Phyrexian creatures” population
without the “on their front face” suffix, asserting it parses as
Effect::Transform with EffectScope::All before checking the suffix variant is
Unimplemented. Keep the existing Moonmist control and ensure the negative
assertion verifies the unsupported restriction rather than total population
rejection.

---

Outside diff comments:
In `@crates/engine/src/game/ability_rw.rs`:
- Around line 8181-8199: Extend triggering_scry_look_count_classifies_event_live
to exercise the trigger-ordering predicate, not just reads_event_live metadata:
add a positive conflict case combining an event-live read with an event-object
write, plus controls where the event object or write is absent. Route these
cases through profiles_conflict or the production trigger-ordering path and
assert the expected classifications, preserving the existing profile and
twin-parity checks.
- Around line 2953-2961: Add CR citations to both Effect::ExileFaceDownPile
match arms, including descriptions covering the face-down exile-pile move and
the HandLibrary/membership writes. Place the annotations directly on the
relevant branches and preserve the existing legacy_quantity_expr,
legacy_target_filter, and return behavior.

In `@crates/engine/tests/integration/cr733_resolved_transform.rs`:
- Around line 111-125: Extend the replay assertions in the integration test
after applying apply_resolved_transform to verify that
replayed.transformation_count equals transform.resulting_transformation_count.
Keep the existing face, name, and timestamp assertions unchanged and use the
same descriptive assertion style.

---

Duplicate comments:
In `@crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs`:
- Around line 54-102: Add a third Human permanent without BackFaceData in
moonmist_cast_transforms_all_humans_without_target_selection, and assert it
remains untransformed after resolution while the existing transformable Humans
and non-Human assertions remain intact. Keep the test using
runner.cast(moonmist).resolve() and retain the WaitingFor::Priority assertion to
cover the production pipeline without target selection.
🪄 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: 186bcb2c-639d-4490-9110-4b1c605885e9

📥 Commits

Reviewing files that changed from the base of the PR and between 6400ed5 and 4ddb629.

📒 Files selected for processing (13)
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/effects/token.rs
  • crates/engine/src/game/effects/transform_effect.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/tests/integration/cr733_resolved_transform.rs
  • crates/engine/tests/integration/issue_6403_moonmist_mass_transform.rs
  • crates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (7)
  • crates/engine/tests/integration/main.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/token.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/game/effects/transform_effect.rs

@matthewevans matthewevans self-assigned this Jul 26, 2026
@matthewevans matthewevans removed their assignment Jul 26, 2026
@matthewevans

Copy link
Copy Markdown
Member

Current head 93892a6001 has required CI still running. The implementation review is complete; approval and merge-queue enrollment will resume after those checks settle. The parse-diff baseline is pending, but this branch already contains the maintainer merge-main remedy, so it will not receive another merge-main commit solely to retry that artifact.

@matthewevans matthewevans self-assigned this Jul 26, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved: current-head regression coverage now exercises the matching non-transformable Human and the strict-failed face-state suffix; all required checks are green.

@matthewevans
matthewevans added this pull request to the merge queue Jul 26, 2026
@matthewevans matthewevans removed their assignment Jul 26, 2026
Merged via the queue into phase-rs:main with commit 157b0e5 Jul 26, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Moonmist — Moonmist should not have to target a single human creature, should just transform all human.

2 participants