Partial: Ultimate Nullification - #6940
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe parser now supports typed multi-zone exile and heterogeneous mass-exile expressions. It emits typed exile AST nodes and adds parser, synthesis, and integration tests for Thought Distortion and Ultimate Nullification. ChangesMass exile support
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Generated for head Parse changes introduced by this PR · 2 card(s), 6 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_effect/tests.rs`:
- Around line 24395-24405: Extend the existing filter assertion in the
multi-zone parser test to also require a Typed `TargetFilter` containing
`TypeFilter::Creature` and `FilterProp::InZone { zone: Zone::Battlefield }`.
Keep the current Card plus InAnyZone assertion, and ensure both expected legs
are validated from the same `filters` result.
- Around line 24245-24251: Correct the rules annotation above the
ultimate-nullification test: remove CR 608.2f and retain only verified
Comprehensive Rules citations whose rule text directly supports the mass exile
of creatures and graveyards and the self-return to the bottom of its owner’s
library, or remove unsupported citations. Keep the documented AST-shape
expectations unchanged.
In `@crates/engine/tests/integration/ultimate_nullification.rs`:
- Around line 177-204: Update the Err branch of the cast match in the
integration test to validate that the announcement failed specifically because
the mandatory legendary-creature sacrifice is unsatisfiable, rather than
accepting any error. Match or inspect the cast error using the concrete error
symbols available in the announcement flow, and assert the expected
sacrifice-related reason so unrelated parser, mana, or implementation failures
cannot satisfy the test.
- Around line 178-180: Update the explanatory comment in the announcement error
branch to cite CR 601.2h instead of CR 601.2f, keeping the existing description
of the engine rejecting an announcement whose total cost cannot be paid.
🪄 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: 1cb01115-e67a-40a7-a13f-3c1a443f9e11
📒 Files selected for processing (6)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/ultimate_nullification.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The heterogeneous mass-exile recognizer claims type-qualified, owner-scoped origins that it cannot represent. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:2755-2788 accepts any leading type phrase, unconditionally scopes that leg to InZone(Battlefield), then appends an unscoped Card + InAnyZone leg. The current-head parse-diff reports Thought Distortion changing from in hand card non-creature non-land to in hand in battlefield card non-creature non-land or in graveyard card; Thought Distortion's authoritative Scryfall Oracle text is “Target opponent reveals their hand. Exile all noncreature, nonland cards from that player's hand and graveyard.” Why it matters: the hand leg becomes impossible after the injected battlefield constraint, while the graveyard leg loses both the target-player owner scope and the noncreature/nonland restriction, so resolving this spell exiles the wrong cards. Green CI does not refute this card-level semantic regression.
Please decline this form to the existing owner-scoped multi-zone parser until it can preserve both the parsed type restriction and that player's ownership across every origin, or generalize the ownership/type-origin representation at the correct seam. Add an exact Thought Distortion parser regression and a production-pipeline runtime regression that proves only the targeted opponent's noncreature, nonland cards in hand and graveyard are exiled (with opponent/other-player and creature/land controls). The Ultimate Nullification implementation can remain a separate bare-zone path.
The heterogeneous "exile all <permanent types> and <zone(s)>" recognizer
(try_parse_mass_exile_permanents_and_zones) over-matched cards whose leading
leg already carries its own source-zone / owner scope, then injected
InZone(Battlefield) and rebuilt the zone leg as all-owners / all-cards. This
regressed two supported cards:
- Thought Distortion ("exile all noncreature, nonland cards from that player's
hand and graveyard") gained an impossible hand-and-battlefield leg and lost
both the "that player's" owner scope and the noncreature/nonland restriction
on the graveyard leg.
- Worldfire ("exile all cards from all hands and graveyards") gained the same
impossible battlefield injection on its hand leg.
Guard the recognizer with is_bare_battlefield_permanent_leg: only fire when the
leading permanent leg has no controller scope and no InZone/InAnyZone property
(a true battlefield permanent-type union per CR 109.2). Ultimate Nullification
("creatures and graveyards") is unaffected; Thought Distortion / Worldfire fall
back to their existing owner-scoped multi-zone parse.
Add a Thought Distortion parser regression pinning the fix.
Review nit cleanup:
- Remove unverified CR 608.2f from the parser-test annotation (a resolution-time
simultaneity rule, not a parse-shape rule).
- Strengthen the multi-zone generalization test to also validate the
Creature+InZone(Battlefield) leg from the same filters result.
- Tighten the integration Err branch to assert the specific
ActionNotAllowed("...required additional cost") rejection so an unrelated
parser/mana failure cannot satisfy it.
- Fix CR 601.2f -> CR 601.2h in the announcement-rejection comment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/tests/integration/ultimate_nullification.rs (1)
41-43: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winModel the printed mana cost in both fixtures.
Both tests currently cast a free spell with no seeded mana. The positive test does not verify mana payment. The negative test can report the sacrifice-specific failure only because the mana component is missing. Under CR 601.2f–h, mana and additional costs are both part of the total cost. (media.wizards.com)
crates/engine/tests/integration/ultimate_nullification.rs#L41-L43: set the printed cost and seed P0 with{4}{W}for the cast-and-resolve test.crates/engine/tests/integration/ultimate_nullification.rs#L149-L151: set the same cost and mana pool before testing the unavailable legendary sacrifice.Based on learnings, Oracle-text fixtures need an explicit
CardBuilder::with_mana_costcall.🤖 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/ultimate_nullification.rs` around lines 41 - 43, Update both fixtures in crates/engine/tests/integration/ultimate_nullification.rs at lines 41-43 and 149-151: add an explicit CardBuilder::with_mana_cost for {4}{W} and seed P0’s mana pool with {4}{W} before casting. Apply the same setup to both the cast-and-resolve test and the unavailable legendary-sacrifice test so mana payment is included alongside the additional cost.Sources: Learnings, MCP tools
🤖 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/tests/integration/ultimate_nullification.rs`:
- Around line 41-43: Update both fixtures in
crates/engine/tests/integration/ultimate_nullification.rs at lines 41-43 and
149-151: add an explicit CardBuilder::with_mana_cost for {4}{W} and seed P0’s
mana pool with {4}{W} before casting. Apply the same setup to both the
cast-and-resolve test and the unavailable legendary-sacrifice test so mana
payment is included alongside the additional cost.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c2d3c821-416a-4b08-bacf-6a36b3ae0f07
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/ultimate_nullification.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/parser/oracle_effect/tests.rs
- crates/engine/src/parser/oracle_effect/imperative.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The Thought Distortion correction is not regression-proven at the production boundary. Evidence: crates/engine/src/parser/oracle_effect/tests.rs:24394-24470 parses only the isolated exile sentence and asserts only that some typed leg retains Hand plus the noncreature/nonland types; it never asserts the that player's owner/TargetPlayer binding, tests the complete “Target opponent reveals their hand. …” Oracle text, or resolves that spell against targeted-opponent, other-player, creature, and land controls. Why it matters: the prior defect was precisely a lost ownership restriction, so this test can pass if the fallback exiles qualifying cards from every player rather than only the targeted opponent. Please add an exact Thought Distortion parser assertion for the owner scope and a discriminating cast/resolve regression that proves only the targeted opponent's noncreature, nonland cards in hand and graveyard move.
[HIGH] The required current-head parser evidence is absent. Evidence: the comprehensive PR-comment scan contains the <!-- coverage-parse-diff --> artifact only for old head 3dedb2f35e6d5b7ea5c1f98a8aa79afd9b90ce30, while this review is for f6342d2d04337489515b08bcd6536d5220528d74; engine/parser source changed on this head. Why it matters: that artifact is the gate for confirming that the intended Thought Distortion/Worldfire reversion did not create further card-level parser regressions. Please publish a parse-diff bound to this head before re-review.
[MED] The Ultimate Nullification runtime fixtures cast a free spell instead of the printed {4}{W} spell. Evidence: crates/engine/tests/integration/ultimate_nullification.rs:41-44,151-154 call add_spell_to_hand_from_oracle(...).id() without with_mana_cost, whereas that scenario builder only creates and parses the object (crates/engine/src/game/scenario.rs:701-728) and existing spell-cast tests set a cost explicitly. Why it matters: the positive fixture therefore does not exercise payment of the real total cost, and the negative fixture can fail before the intended sacrifice-cost branch for the wrong reason. Set {4}{W} and seed matching mana in both fixtures.
Addresses the second review round on PR phase-rs#6940. Thought Distortion regression at the production boundary (was: isolated `parse_effect_chain` on one sentence with weak assertions). The new test runs the card's COMPLETE Oracle text through `build_oracle_face` — the same entry the card-data pipeline uses — and pins the exact baseline shape my recognizer guard restores: - `RevealHand` targeting the Opponent (the owner binding), - its `ChangeZoneAll` keeps `origin: Some(Hand)` — the ownership linkage the prior defect destroyed by rewriting it to `origin: None`, - the exile filter keeps `Non(Creature)`/`Non(Land)` + `InZone(Hand)` with NO injected `InZone(Battlefield)`, as a single `Typed` (never an `Or`). It also documents the boundary: the trailing "and graveyard" leg is a pre-existing `Unimplemented` gap on main (`Effect:graveyard`) that this PR neither closes nor worsens. Ultimate Nullification runtime fixtures now cast the printed {4}{W} spell: both fixtures set the real mana cost and seed matching mana. The positive fixture therefore pays the true total cost (mana + the sacrifice additional cost); the negative fixture has mana fully payable so the ONLY unpayable component is the legendary sacrifice, which the tightened error assertion confirms. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/database/synthesis.rs`:
- Around line 10828-10829: Correct the comment near the reveal/exile logic by
removing the unsupported CR 601.2c citation and target-linked scope claim, or
replace it with a verified rule citation whose body explicitly describes the
written instructions resolving in order, using CR 608.2c only if that sequence
is what the code documents.
🪄 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: b128fbd1-161b-4434-84cf-ced499a2547b
📒 Files selected for processing (3)
crates/engine/src/database/synthesis.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/ultimate_nullification.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/parser/oracle_effect/tests.rs
- crates/engine/tests/integration/ultimate_nullification.rs
|
Re-review is held for head |
…t Distortion) Closes the last open item on PR phase-rs#6940's review: implement — not just decline — the owner-scoped, type-restricted, two-zone exile so Thought Distortion fully resolves, with a discriminating runtime regression. Previously `try_parse_multi_zone_player_exile` claimed only the BARE "cards from <owner> <zone> and <zone>" form (Identity Crisis, CR 108.2). A type-qualified variant — Thought Distortion's "exile all noncreature, nonland cards from that player's hand and graveyard" — fell through: the hand leg parsed as a single-zone exile and "and graveyard" was orphaned into an `Unimplemented { "graveyard" }` no-op (card marked unsupported, gap `Effect:graveyard`). Generalize the recognizer to carry an optional leading type restriction (CR 205.2a/205.3a): it now parses "[<types>] cards from <owner-possessive> <zone-union>" and returns the card-type filters alongside the owner axis and the zone union. The bare form is tried first and keeps empty type_filters, so its representation (and Identity Crisis's parse) is byte-identical. Both dual-site call sites (parse_exile_ast + the compound-splitter probe) build a single `ChangeZoneAll { origin: None, Typed { <types>, controller: TargetPlayer, InAnyZone([Hand, Graveyard]) } }`. Runtime resolution is inherited from the existing multi-zone owner-exile path (Identity Crisis), so `TargetPlayer` binds to the revealed opponent and the exile is confined to that player's zones. Thought Distortion is now fully supported (no coverage gap). Tests: - runtime regression (tests/integration/thought_distortion.rs): casting the real card exiles ONLY the targeted opponent's noncreature/nonland cards, from BOTH hand and graveyard — with owner-scope controls (the caster's own cards stay) and type controls (the target's creature/land cards stay). The graveyard assertions are the revert-failing authority. - production-boundary parse test (synthesis): asserts the owner-scoped, type-restricted, InAnyZone([Hand, Graveyard]) shape and zero gaps. - matcher unit test: bare (empty type_filters) and type-qualified cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/tests/integration/thought_distortion.rs`:
- Around line 52-57: Add a P1-owned land card to the graveyard setup alongside
opp_gy_noncreature and opp_gy_creature, then extend the assertions in the
affected thought-distortion test to verify that this card remains in
Zone::Graveyard. Ensure the assertion exercises the Graveyard leg’s nonland
filtering rather than only validating creature and instant entries.
🪄 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: 242010f2-d3e5-4ae6-b42f-de1455efd156
📒 Files selected for processing (6)
crates/engine/src/database/synthesis.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/thought_distortion.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/tests/integration/main.rs
- crates/engine/src/database/synthesis.rs
- crates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The Thought Distortion runtime regression does not prove the nonland restriction for its graveyard origin. Evidence: crates/engine/tests/integration/thought_distortion.rs:52-57 creates only an instant and a creature in P1's graveyard, while :86-102 asserts only that the instant moves and the creature remains; the sole land control is in P1's hand at :49-50. Why it matters: a regression that preserves the type restriction for Hand but drops Non(Land) while collecting the Graveyard leg would still pass, even though this PR newly claims one filter over both origins. Suggested fix: add a P1-owned land card in the graveyard and assert that it remains there after the real cast/resolve pipeline.
[HIGH] The required parser coverage artifact is stale for this implementation head. Evidence: the only <!-- coverage-parse-diff --> PR comment is generated for 008c609d37a691aa22b3b2c634fcb145d79b6715, while this review covers parser changes at 9793b5feadd774df68c9ffa48ced360ae680e542; crates/engine/src/parser/oracle_effect/imperative.rs:2682-2772 changes the parser surface. Why it matters: without a head-bound card-level diff, this review cannot establish that the new type-qualified multi-zone recognizer has no unintended parser blast radius. Suggested fix: after the regression update, publish a completed coverage-parse-diff artifact bound to the resulting head and re-request review.
|
[MED] The new Thought Distortion synthesis test documents the target linkage with an unverified |
The head-noun guard used .strip_suffix(" cards")/(" card") for parse dispatch,
which the nom-combinator mandate gate (Gate A) rejects. Drop it: parse_type_phrase
must FULLY consume the delimited head, and the owner-possessive + 2-zone-union
structure is already the discriminator, so the manual suffix guard was redundant.
Behavior and all type_filters are unchanged (parse_type_phrase output is identical).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Per review: the runtime regression only proved 'nonland' for the hand origin (the sole land control was in P1's hand); a regression dropping Non(Land) for just the graveyard leg would still pass. Add a P1-owned land card in the graveyard and assert it stays, so the nonland restriction is proven on BOTH origins of the single owner-scoped multi-zone filter. Adds a reusable add_land_to_graveyard scenario helper mirroring add_creature_to_graveyard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The revised runtime fixture now covers the target opponent’s graveyard land and the current parse-diff artifact reports the intended two signature additions. I found one remaining current-head issue before this can be approved.
MED — crates/engine/src/database/synthesis.rs:10826-10827: remove or correct the CR citation. The assertion is a useful check of the parser’s anaphoric target binding, but CR 601.2c governs choosing targets when casting; it does not establish the subsequent that player's binding claimed by this comment. Please either use a directly verified rule citation that supports the stated relationship or remove the rule annotation and describe this solely as parser-chain behavior. The same finding remains unresolved in the current review thread: #6940 (comment).
Evidence reviewed at 6c0cf885743fff6ca072bc89a0d7908cf1e7fea8: the exact-head coverage-parse-diff artifact, the new Thought Distortion runtime fixture, source diff, and all required CI checks (green). No local build was run.
Per review: CR 601.2c governs choosing targets during casting, not the subsequent "that player's" anaphoric binding the comment attached it to. Remove the citation from both the doc comment and the inline reveal-assertion comment; describe the reveal->exile owner linkage as parser-chain/anaphora resolution (template behavior, not a numbered rule). Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@matthewevans thanks — addressed the remaining MED. CR 601.2c citation ( While there I re-verified the remaining CR citations in the new code against This is a comment-only change; parser output, the parse-diff, and all tests are unaffected. Ready for re-review. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved for the current head 919094646178ec7c90e3a4d3809a3eba5d669387.
The current parse-diff is bound to this head and matches the reviewed Ultimate Nullification and Thought Distortion changes. Required Rust and frontend checks are green; the prior owner/type/zone, graveyard-land, real-cost, and CR-annotation findings are resolved with production-boundary regressions.
2 participants
Summary
Adds engine support for Ultimate Nullification.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass./scripts/check-parser-combinators.sh (Gate A)— pass (required env fix: default PATH resolved python3 to a broken WindowsApps stub returning Permission denied exit 126, which the gate mis-reported as 'cross-product detector test suite RED'; re-ran with /c/msys64/mingw64/bin prepended so python3=Python 3.9.7, detector self-test 10/10 OK, Gate G PASS + Gate A PASS)cargo clippy-strict— incomplete (still compiling in background id biwckxqdo when StructuredOutput was force-required; last log: Compiling phase-engine + server-core, no errors yet but not finished)cargo test -p phase-engine— not run (blocked behind clippy in the && chain)./scripts/gen-card-data.sh— not runcargo coverage— not run (current data/card-data.json regenerated Aug 2 15:27 already shows the card is not fully supported)cargo semantic-audit— not runScope Expansion
Added the recognizer at a second site (try_parse_verb_and_target compound-splitter probe) in addition to parse_exile_ast — the plan missed that the compound splitter intercepts "creatures and graveyards" first; both mirror the established try_parse_multi_zone_player_exile dual-site pattern. card-data.json left for the pipeline to regenerate.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
New Features
Bug Fixes