Fix Diluvian Primordial - #6794
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughAdds parsing and execution support for per-opponent graveyard free casts, pairs each opponent with a typed graveyard card, preserves selected cards in a fixed free-cast pool, and carries structured graveyard replacement destinations through resolution. ChangesPer-opponent graveyard casting
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant CastFromZone
participant TargetSlots
participant FreeCastFromZones
OracleParser->>CastFromZone: parse per-opponent graveyard cast
CastFromZone->>TargetSlots: build paired opponent/card targets
TargetSlots-->>CastFromZone: return revalidated target IDs
CastFromZone->>FreeCastFromZones: open fixed member_pool
FreeCastFromZones-->>CastFromZone: cast selected cards with graveyard replacement
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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/game/effects/cast_from_zone.rs`:
- Around line 610-620: Replace the locally re-derived is_per_opponent_fanout
matches expression with the existing is_per_opponent_target_fanout predicate.
Reuse that canonical predicate for the router while leaving the surrounding
distribution and slot-building logic unchanged.
In `@crates/engine/tests/integration/diluvian_primordial_6754.rs`:
- Around line 165-170: Strengthen the assertions in the relevant Diluvian
Primordial integration tests, including
diluvian_primordial_skips_targetless_opponent_and_casts_other_selected_spell, to
verify the free-cast spell resolved by asserting P0’s hand or library count
changed as expected from its “Draw a card.” effect. Keep the existing zone
assertions, but ensure the positive effect distinguishes resolution from being
exiled without resolving.
🪄 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: 2b7d77bf-af65-4d0d-b84b-12bd414b6a74
📒 Files selected for processing (10)
crates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/diluvian_primordial_6754.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
💤 Files with no reviewable changes (1)
- docs/parser-misparse-backlog.md
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewing current head 145698640c8ab5cea95f9307c012fa91e0e3f348.
[HIGH] Free-cast routing leaves the sequential graveyard-exile rider attached to the cloned ability. Evidence: crates/engine/src/game/effects/cast_from_zone.rs:632-647 clones the rider-bearing ability and replaces only effect before opening the FreeCastWindow; crates/engine/src/game/effects/mod.rs:10165-10176 then stashes the sequential ParentTarget rider with the inherited targets; crates/engine/src/game/effects/change_zone.rs:465-467 resolves those targets. Why it matters: after a selected spell is free-cast, the continuation can resolve ChangeZone against that selected spell while it is on the stack, exiling it before it resolves. The present integration assertion at crates/engine/tests/integration/diluvian_primordial_6754.rs:167 only observes the final zone, so it cannot distinguish this failure from the required stack-to-graveyard exile replacement. Suggested fix: consume/suppress the parsed rider when routing to the free-cast window, and add a production-pipeline assertion that the selected spell's effect resolves before the stack-to-graveyard replacement puts it in exile.
[MED] The router duplicates only the max == PlayerCount(Opponent) portion of the canonical fanout predicate. Evidence: crates/engine/src/game/effects/cast_from_zone.rs:610-620 omits the canonical stack-timing and ControllerRef::TargetPlayer gates in crates/engine/src/game/ability_utils.rs:4763-4785. Why it matters: the routing condition can diverge from the existing target-slot/distribution authority and apply the free-cast route to a superficially similar, but semantically different, target shape. Suggested fix: reuse or move the canonical predicate so the router and target construction share one authority.
[MED] Required parser-surface evidence is absent for this engine/parser PR. Evidence: no current-head sticky comment containing <!-- coverage-parse-diff --> is present; the only current CodeRabbit review explicitly covers 240cfc8e701c7b55f676fe7ad9333a0b142aa1ae..145698640c8ab5cea95f9307c012fa91e0e3f348. Why it matters: without the card-level parse diff, this review cannot verify that the new parser route has no unintended supported/unsupported or AST-shape blast radius. Suggested fix: provide a current-head coverage parse-diff artifact and reconcile every changed card with this PR's intended scope.
The two current CodeRabbit threads are confirmed and folded into the findings above: canonical predicate reuse (thread) and the non-discriminating final-zone-only test (thread).
Parse changes introduced by this PR · 2 card(s), 6 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Review addendum for current head 145698640c8ab5cea95f9307c012fa91e0e3f348.
[MED] The newly published current-head parse-diff exposes an unexplained 34-card AST blast radius. The PR claims parse impact is only Diluvian Primordial, but the <!-- coverage-parse-diff --> artifact reports 34 cards with ability/ChangeZone.from changed from ∅ to stack (including Bösium Strip and Chandra, Acolyte of Flame), alongside the five Diluvian-specific signatures. This is explained by the shared lowering change in crates/engine/src/parser/oracle_effect/lower.rs:1060-1074: every SpellStackToGraveyardReplacement::Exile rider now lowers to ChangeZone { origin: Some(Zone::Stack), destination: Zone::Exile, target: ParentTarget, ... }. The PR provides no card-class review, behavior trace, or test coverage for those other 33 cards, so the claimed narrow scope is contradicted by the artifact. Please either (1) narrow/revert the unrelated shared AST change from this PR, or (2) explicitly justify and review the entire 34-card class, including representative runtime coverage and reconciled parse-impact documentation.
The existing current-head blockers remain unresolved:
- [HIGH] The free-cast routing clones the rider-bearing ability, replacing only
effect; the sequentialParentTargetChangeZonerider can therefore resolve against the selected spell while it is on the stack and exile it before it resolves. Suppress/consume that rider for the free-cast window and add a production-pipeline assertion that the selected spell actually resolves before the stack-to-graveyard exile replacement. - [MED]
cast_from_zone.rslocally re-derives only the opponent-count portion of the fanout condition and omits the canonical predicate's stack-timing andControllerRef::TargetPlayergates. Reuseis_per_opponent_target_fanout(or relocate it to a shared authority) so target construction and routing cannot diverge.
CodeRabbit's current-head findings confirm both retained blockers: canonical predicate reuse and the non-discriminating final-zone-only integration assertion.
|
Follow-up fixes are now in
The full local batch is green: fmt, clippy-strict, |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/cast_from_zone.rs (1)
612-635: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUnconditionally clearing
window.sub_abilitycan silently drop non-rider chain effects.
window.sub_ability = Nonefires for every per-opponent-fanout / during-resolution / without-payingCastFromZone, regardless of whethergraveyard_destination(i.e.cast_from_zone_graveyard_destination(ability)) actually found a rider to translate. If a future (or currently-unrecognized) producer of this shape carries asub_abilitythat isn't the graveyard-exile rider, this silently discards it with no diagnostic — the same class of bug this change fixes for the known rider, reintroduced for the unknown case.Scope the clear to only when a rider was actually consumed:
🐛 Proposed fix
window.targets = target_ids.drain(..).map(TargetRef::Object).collect(); - // The rider has been translated into the window's per-cast metadata; - // retaining it would run a second destination move after the window. - window.sub_ability = None; + // The rider has been translated into the window's per-cast metadata; + // retaining it would run a second destination move after the window. + // Only clear it when a rider was actually consumed — an unrecognized + // sub_ability shape must not be silently dropped. + if graveyard_destination.is_some() { + window.sub_ability = None; + }#!/bin/bash # Look for any other per-opponent fanout / free-cast producer whose sub_ability # might carry something other than the graveyard-destination rider. rg -n -C5 'graveyard_destination_rider' crates/engine/src🤖 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/effects/cast_from_zone.rs` around lines 612 - 635, Only clear window.sub_ability in the per-opponent fanout path when graveyard_destination confirms that cast_from_zone_graveyard_destination(ability) found and consumed the corresponding graveyard-exile rider. Preserve any unrelated or unrecognized sub_ability when no rider was translated, while keeping the existing rider translation behavior unchanged.
🤖 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/effects/cast_from_zone.rs`:
- Around line 612-635: Only clear window.sub_ability in the per-opponent fanout
path when graveyard_destination confirms that
cast_from_zone_graveyard_destination(ability) found and consumed the
corresponding graveyard-exile rider. Preserve any unrelated or unrecognized
sub_ability when no rider was translated, while keeping the existing rider
translation behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5bf1f7d3-278b-4365-acb9-5961852d157e
📒 Files selected for processing (18)
crates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/diluvian_primordial_6754.rscrates/engine/tests/integration/invoke_calamity_free_cast.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewing current head 9d390ef8805dc65459ce51b32d0a53652b6ce42b.
[HIGH] The per-opponent free-cast route unconditionally discards sequential continuations that are not the recognized graveyard-destination rider. crates/engine/src/game/effects/cast_from_zone.rs:610-633 enters the structural per-opponent, during-resolution free-cast branch and translates only cast_from_zone_graveyard_destination(ability) into FreeCastFromZones.graveyard_replacement. That extractor (:1039-1046) recognizes only the graveyard-destination rider, but window.sub_ability = None at :631 clears the entire sequential chain regardless. An unrelated current or future sequential continuation is therefore silently dropped instead of resolving after the free-cast window.
Please consume only the structurally recognized rider while preserving/executing any remaining continuation, and add a focused production-pipeline regression that places a non-rider sequential continuation after this fanout so the test fails if the continuation is cleared.
2 participants
Summary
Closes #6754.
Diluvian Primordial previously could not lower its per-opponent, opponent-graveyard cast clause: the fanout path only accepted battlefield-object targets, so the paired opponent/card selections were never represented. This PR adds the narrowly scoped graveyard exception, preserves each selected card as an exact free-cast pool, and routes the card's exile rider through the existing per-cast replacement path.
The fixed pool prevents a later offer from substituting a different card from an opponent's graveyard after a chosen target becomes illegal. The rider is installed once after a selected spell is cast, avoiding duplicate replacement choices while retaining established Invoke Calamity behavior.
Files changed
crates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/diluvian_primordial_6754.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.mdTrack
Developer
LLM
Model: gpt-5.6
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 115.1a, CR 108.3, CR 601.2a–c, CR 608.2g, CR 614.1a.
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.
cargo fmt --all— clean (EXIT_STATUS=0)cargo clippy-strict— clean (EXIT_STATUS=0)cargo test -p phase-engine— clean; 4,194 integration tests passed (EXIT_STATUS=0)./scripts/gen-card-data.sh— clean (EXIT_STATUS=0)cargo coverage— clean (EXIT_STATUS=0)cargo semantic-audit— clean (EXIT_STATUS=0)./scripts/check-parser-combinators.sh— Gate G PASS; Gate A PASSGate A
Gate A PASS head=145698640c8ab5cea95f9307c012fa91e0e3f348 base=d947f97469a5de1c332701f2062e1852d81ca011
Anchored on
crates/engine/tests/integration/riptide_gearhulk_5994.rs:1— established optional per-opponent ETB fanout coverage.crates/engine/src/game/casting_costs.rs:9314— establishedFreeCastOfferRemainingper-cast exile-rider authority.Final review-impl
Final review-impl PASS head=145698640c8ab5cea95f9307c012fa91e0e3f348
Claimed parse impact
Diluvian Primordial.
Scope Expansion
The exact-pool support is generalized within the existing
FreeCastFromZonespath so it can carry paired target IDs without changing its empty-pool behavior, which preserves Invoke Calamity's established controller-graveyard scan.Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
New Features
Bug Fixes
Tests