fix(engine): cast self-library peek spells during resolution (Kiora class) - #6302
Conversation
…lass) Self-library peek-and-cast cards (Kiora Sovereign of the Deep, Aetherworks Marvel, Construct a Cosmic Cube, Perception Bobblehead, Svella Ice Shaper, Velomachus Lorehold) previously routed "cast ... from among them" through the LingeringPermission driver: the looked-at cards were exiled, the free cast was never offered, and the rest were never bottomed. - Parser: seed chain_prior_self_library_peek from the raw bare-private-peek Dig shape via a single shared effect_is_bare_private_peek predicate (also used by the assembly pure-peek lowering, so the two sites cannot drift); flip the "from among them" fall-through to CastFromZoneDriver:: DuringResolution when the chain has a self-library peek and no exile producer (CR 608.2g). - Parser: parse the "with mana value N or less/greater" dig-peek suffix into a typed cast-permission constraint (fixes Perception Bobblehead's dropped MV<=3 cap; Founding the Third Path stays constraint-free). - Resolver: park an EffectZoneChoice on the Library for eligible candidates, freeze dynamic constraint refs (X, power) to Fixed at binding time (CR 608.2h), cast the chosen spell during resolution, and bottom the rest in a random order on accept, decline, and zero-eligible paths (CR 401.4). - Resolver: gate the pre-injected-target library route on references_exiled_by_source() so Planetarium of Wan Shi Tong's ParentTarget flow keeps its direct free cast. - Visibility: the prompt player sees the looked-at Library cards; opponents get redacted placeholders across serde round-trips (CR 701.20e, CR 400.2). Fixes the Discord bug report (thread 1529132485668245575).
📝 WalkthroughWalkthroughSelf-library peek parsing now routes eligible cast effects through during-resolution selection. Resolution supports private looked-at library choices, frozen mana-value constraints, bottoming behavior, cleanup, viewer-specific redaction, and integration coverage. ChangesSelf-library peek casting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant ParseContext
participant CastFromZone
participant EffectZoneChoice
participant GameState
OracleParser->>ParseContext: record self-library peek context
OracleParser->>CastFromZone: create during-resolution cast effect
CastFromZone->>GameState: collect looked-at library cards
CastFromZone->>EffectZoneChoice: open private eligible-card choice
EffectZoneChoice->>CastFromZone: return selected card or decline
CastFromZone->>GameState: cast selected card and bottom remaining cards
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.44.1)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
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)
213-248: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMissing
NeedsChoicecheck on the decline-shuffle inopen_private_zone_cast_selection.When
eligible.is_empty()andsource_zone == Zone::Library, this discardsshuffle_to_bottom's result (let _ = ...) and unconditionally pushesGameEvent::EffectResolved+ returnsOk(()). If the shuffle parks on a CR 616.1 ordering choice (BatchMoveResult::NeedsChoice),state.waiting_foris now the parked replacement prompt, yet this function still reports the effect as fully resolved — the exact asymmetry that the sibling decline path inengine_resolution_choices.rs(the "no decline fallback"zone == Zone::Librarybranch, lines ~4459-4478) explicitly guards against by checking forNeedsChoiceand returning early. Both code paths shuffle the same looked-at-library set to the bottom on a failed/declined self-library-peek cast, so they should share the same pause-safety.🐛 Proposed fix
if eligible.is_empty() { if source_zone == Zone::Library { let looked_at = looked_at_controller_library_cards(state, ability.controller); - let _ = crate::game::effects::cascade::shuffle_to_bottom( - state, - &looked_at, - ability.source_id, - None, - events, - ); + if matches!( + crate::game::effects::cascade::shuffle_to_bottom( + state, + &looked_at, + ability.source_id, + None, + events, + ), + crate::game::zone_pipeline::BatchMoveResult::NeedsChoice + ) { + return Ok(()); + } } events.push(GameEvent::EffectResolved { kind: EffectKind::CastFromZone, source_id: ability.source_id, subject: None, }); return Ok(()); }🤖 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 213 - 248, Handle the result of shuffle_to_bottom in open_private_zone_cast_selection when eligible.is_empty() and source_zone is Zone::Library: detect BatchMoveResult::NeedsChoice and return early without pushing EffectResolved. Preserve the existing shuffle and resolution behavior for completed results, matching the sibling decline path’s pause-safety handling.
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_ir/context.rs (1)
264-270: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment says "most recent producer" but the implementation checks ANY prior clause.
chain_prior_self_library_peekis populated inmod.rsviabuilder.clauses().iter().any(clause_ir_is_self_library_peek), which is true if any earlier clause in the chain is a self-library peek — not specifically the immediately preceding one. For a hypothetical chain with an intervening non-peek clause between the peek and the "cast from among them" clause, this flag would still betrueand route toDuringResolution, contradicting the "most recent producer" framing. Consider rewording the doc to matchchain_has_prior_exile_producer's "an EARLIER clause" phrasing, or tightening the predicate to only the immediately-preceding clause if "most recent" is the intended contract.🤖 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/parser/oracle_ir/context.rs` around lines 264 - 270, Align the documentation and behavior of chain_prior_self_library_peek with its existing any-earlier-clause predicate in the chain construction logic, using “an EARLIER clause” wording like chain_has_prior_exile_producer rather than “most recent producer.” Do not change the predicate unless the intended contract is explicitly to require the immediately preceding clause.crates/engine/src/game/effects/cast_from_zone.rs (1)
61-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated constraint-resolution match; unused hand lookup on the library branch.
The
constraintmatch at lines 90-96 (prefer the effect's explicitconstraint, else fall back toeffective_cast_from_zone_constraint) is byte-for-byte the same logic assnapshot_cast_from_zone_constraint_into_effect'seffectivecomputation (lines 746-751). Consider extracting a shared helper (e.g.explicit_or_effective_cast_from_zone_constraint) both call, so the two sites can't drift.Separately,
hand_owner/player(68-81) are computed unconditionally but only used by theZone::Handarm of thecardsmatch (83); theZone::Libraryarm ignores them and readsability.controllerdirectly. This is harmless today (sinceextract_controller_refon the library-onlyExiledBySource/library filters falls back toability.controlleranyway, matchinghand_owner), but it's dead work on the library path and slightly obscures that the two branches use different controller-resolution logic.🤖 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 61 - 96, Extract the shared explicit-or-effective constraint resolution into a helper and use it from both compute_hand_pick_eligible and snapshot_cast_from_zone_constraint_into_effect. Restructure compute_hand_pick_eligible so hand_owner and the player lookup occur only for the Zone::Hand branch, while the Zone::Library branch directly uses looked_at_controller_library_cards with ability.controller; preserve existing unsupported-zone behavior.
🤖 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/kiora_self_library_peek_cast.rs`:
- Around line 613-663: The test around
velomachus_power_constraint_is_frozen_before_the_library_choice currently checks
eligibility only when the choice opens, so it does not validate the frozen
constraint. Drive the EffectZoneChoice through its production resume path after
current_trigger_event is cleared, completing the selection and asserting the
mana-value-5 spell casts successfully while the mana-value-6 spell remains
excluded; use the existing runner action APIs and verify the resulting
cast/effect.
- Around line 236-249: Limit the constraint assertion in the loop to Founding
the Third Path’s chapter III rather than applying it to the first CastFromZone
returned by parsed_cast_from_zone. Update the test setup or helper usage around
parsed_cast_from_zone so it selects the chapter III copy-cast effect, then
assert that effect’s constraint is None while preserving the existing checks for
the other cards.
---
Outside diff comments:
In `@crates/engine/src/game/effects/cast_from_zone.rs`:
- Around line 213-248: Handle the result of shuffle_to_bottom in
open_private_zone_cast_selection when eligible.is_empty() and source_zone is
Zone::Library: detect BatchMoveResult::NeedsChoice and return early without
pushing EffectResolved. Preserve the existing shuffle and resolution behavior
for completed results, matching the sibling decline path’s pause-safety
handling.
---
Nitpick comments:
In `@crates/engine/src/game/effects/cast_from_zone.rs`:
- Around line 61-96: Extract the shared explicit-or-effective constraint
resolution into a helper and use it from both compute_hand_pick_eligible and
snapshot_cast_from_zone_constraint_into_effect. Restructure
compute_hand_pick_eligible so hand_owner and the player lookup occur only for
the Zone::Hand branch, while the Zone::Library branch directly uses
looked_at_controller_library_cards with ability.controller; preserve existing
unsupported-zone behavior.
In `@crates/engine/src/parser/oracle_ir/context.rs`:
- Around line 264-270: Align the documentation and behavior of
chain_prior_self_library_peek with its existing any-earlier-clause predicate in
the chain construction logic, using “an EARLIER clause” wording like
chain_has_prior_exile_producer rather than “most recent producer.” Do not change
the predicate unless the intended contract is explicitly to require the
immediately preceding clause.
🪄 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: d1294287-6bbd-402b-8963-0c0b76de9f13
📒 Files selected for processing (8)
crates/engine/src/game/effects/cast_from_zone.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/mod.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/tests/integration/kiora_self_library_peek_cast.rscrates/engine/tests/integration/main.rs
| for (name, oracle) in [ | ||
| ("Primeval Spawn", PRIMEVAL_SPAWN), | ||
| ("Improvisation Capstone", CAPSTONE), | ||
| ("Founding the Third Path", FOUNDING), | ||
| ] { | ||
| let parsed = parse(oracle, name, &["Sorcery"]); | ||
| let Effect::CastFromZone { constraint, .. } = parsed_cast_from_zone(&parsed) else { | ||
| unreachable!("helper returns CastFromZone") | ||
| }; | ||
| assert!( | ||
| constraint.is_none(), | ||
| "{name} must not gain this suffix constraint" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect what Founding the Third Path parses each ability/trigger CastFromZone to.
fd -t f 'oracle_effect' crates/engine/src/parser
rg -nP 'mana value 1 or 2|cast the copy' crates/engine/src -C2
# Confirm helper ambiguity: does any FOUNDING chapter produce CastFromZone with a constraint?
rg -nP 'parsed_cast_from_zone|cast_from_zone_in' crates/engine/tests/integration/kiora_self_library_peek_cast.rs -C2Repository: phase-rs/phase
Length of output: 14779
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,280p' crates/engine/tests/integration/kiora_self_library_peek_cast.rs | cat -n
printf '\n--- Founding parser references ---\n'
rg -n "Founding the Third Path|FOUNDING|cast the copy|mana value 1 or 2" crates/engine/src crates/engine/tests -C 2Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Oracle parser structure for any Founding-specific cast-from-zone handling.
rg -n "Founding the Third Path|chapter I|chapter III|CastFromZone|CastCopyOfCard|ManaValue" crates/engine/src/parser/oracle_effect crates/engine/src/parser/oracle_trigger_tests.rs -C 3Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant test section with surrounding helper definitions.
sed -n '1,280p' crates/engine/tests/integration/kiora_self_library_peek_cast.rs | cat -n | sed -n '1,280p'
printf '\n--- FOUNDING constant ---\n'
rg -n "FOUNDING|Founding the Third Path" crates/engine/tests/integration/kiora_self_library_peek_cast.rs -C 4Repository: phase-rs/phase
Length of output: 17874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the parser code that may emit the first CastFromZone for Founding.
rg -n "try_split_copy_cast_compound|fold_cast_copy_of_card_defs|CastCopyOfCard|CastFromZone" \
crates/engine/src/parser/oracle_effect/mod.rs \
crates/engine/src/parser/oracle_effect/assembly.rs -C 3Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test file around the Founding case ---'
sed -n '220,255p' crates/engine/tests/integration/kiora_self_library_peek_cast.rs | cat -n
printf '\n%s\n' '--- parser fold locations for copy->cast ---'
rg -n "try_split_copy_cast_compound|fold_cast_copy_of_card_defs|CastCopyOfCard|CastFromZone" \
crates/engine/src/parser/oracle_effect/mod.rs \
crates/engine/src/parser/oracle_effect/assembly.rs -C 2Repository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact Founding oracle text shape in parser fixtures/tests.
rg -n "Founding the Third Path|chapter I|chapter III|cast the copy" \
crates/engine/src/parser crates/engine/tests -C 4 | head -n 120Repository: phase-rs/phase
Length of output: 12423
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Exact Founding text/fixtures and whether it contains multiple cast-from-zone clauses.
rg -n "Founding the Third Path" crates/engine -C 6Repository: phase-rs/phase
Length of output: 1475
Scope this check to Founding’s chapter III. parsed_cast_from_zone will hit chapter I’s hand-cast grant first, so constraint.is_none() is exercising the wrong CastFromZone and never covers the copy-cast clause.
🤖 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/kiora_self_library_peek_cast.rs` around lines
236 - 249, Limit the constraint assertion in the loop to Founding the Third
Path’s chapter III rather than applying it to the first CastFromZone returned by
parsed_cast_from_zone. Update the test setup or helper usage around
parsed_cast_from_zone so it selects the chapter III copy-cast effect, then
assert that effect’s constraint is None while preserving the existing checks for
the other cards.
Source: Path instructions
| fn velomachus_power_constraint_is_frozen_before_the_library_choice() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::DeclareAttackers); | ||
| let velomachus = scenario | ||
| .add_creature(P0, "Velomachus Lorehold", 5, 5) | ||
| .from_oracle_text_with_keywords(&["flying", "vigilance", "haste"], VELOMACHUS) | ||
| .id(); | ||
| let mana_value_five = scenario | ||
| .add_spell_to_library_top(P0, "Velomachus MV5", false) | ||
| .with_mana_cost(ManaCost::generic(5)) | ||
| .from_oracle_text("You gain 1 life.") | ||
| .id(); | ||
| let mana_value_six = scenario | ||
| .add_spell_to_library_top(P0, "Velomachus MV6", false) | ||
| .with_mana_cost(ManaCost::generic(6)) | ||
| .from_oracle_text("You gain 1 life.") | ||
| .id(); | ||
| for index in 0..5 { | ||
| scenario | ||
| .add_spell_to_library_top(P0, &format!("Velomachus Filler {index}"), false) | ||
| .with_mana_cost(ManaCost::generic(6)) | ||
| .from_oracle_text("You gain 1 life."); | ||
| } | ||
|
|
||
| let mut runner = scenario.build(); | ||
| runner.state_mut().waiting_for = WaitingFor::DeclareAttackers { | ||
| player: P0, | ||
| valid_attacker_ids: vec![velomachus], | ||
| valid_attack_targets: vec![AttackTarget::Player(P1)], | ||
| valid_attack_targets_by_attacker: None, | ||
| attacker_constraints: Default::default(), | ||
| }; | ||
| runner | ||
| .declare_attackers(&[(velomachus, AttackTarget::Player(P1))]) | ||
| .expect("Velomachus must be able to attack"); | ||
| runner.resolve_top(); | ||
| runner | ||
| .act(GameAction::DecideOptionalEffect { accept: true }) | ||
| .expect("accepting Velomachus's optional cast must succeed"); | ||
|
|
||
| let WaitingFor::EffectZoneChoice { cards, zone, .. } = runner.state().waiting_for.clone() | ||
| else { | ||
| panic!("Velomachus's attack trigger must reach the library cast choice") | ||
| }; | ||
| assert_eq!(zone, Zone::Library); | ||
| assert!(cards.contains(&mana_value_five)); | ||
| assert!( | ||
| !cards.contains(&mana_value_six), | ||
| "Velomachus at power 5 must exclude a mana-value 6 spell" | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This test does not exercise the freeze it claims to verify.
Per the resolver (open_private_zone_cast_selection), the snapshot matters at the EffectZoneChoice resume, where current_trigger_event is cleared and a dynamic ref would read 0. This test only inspects the eligible set at choice-open time, while the attack trigger is still resolving and Velomachus's power (5) is live — so compute_hand_pick_eligible yields the same MV5-only set whether or not the constraint was frozen. Removing the snapshot would leave this test green.
To actually cover the failure path, drive the selection to completion after the trigger context is gone (or mutate/lose the source between open and resume) and assert the MV5 cast still succeeds under the frozen ceiling.
As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its 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/kiora_self_library_peek_cast.rs` around lines
613 - 663, The test around
velomachus_power_constraint_is_frozen_before_the_library_choice currently checks
eligibility only when the choice opens, so it does not validate the frozen
constraint. Drive the EffectZoneChoice through its production resume path after
current_trigger_event is cleared, completing the selection and asserting the
mana-value-5 spell casts successfully while the mana-value-6 spell remains
excluded; use the existing runner action APIs and verify the resulting
cast/effect.
Source: Path instructions
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
Self-library peek-and-cast cards (Kiora Sovereign of the Deep, Aetherworks
Marvel, Construct a Cosmic Cube, Perception Bobblehead, Svella Ice Shaper,
Velomachus Lorehold) previously routed "cast ... from among them" through the
LingeringPermission driver: the looked-at cards were exiled, the free cast was
never offered, and the rest were never bottomed.
Dig shape via a single shared effect_is_bare_private_peek predicate (also
used by the assembly pure-peek lowering, so the two sites cannot drift);
flip the "from among them" fall-through to CastFromZoneDriver::
DuringResolution when the chain has a self-library peek and no exile
producer (CR 608.2g).
a typed cast-permission constraint (fixes Perception Bobblehead's dropped
MV<=3 cap; Founding the Third Path stays constraint-free).
freeze dynamic constraint refs (X, power) to Fixed at binding time
(CR 608.2h), cast the chosen spell during resolution, and bottom the rest
in a random order on accept, decline, and zero-eligible paths (CR 401.4).
references_exiled_by_source() so Planetarium of Wan Shi Tong's
ParentTarget flow keeps its direct free cast.
get redacted placeholders across serde round-trips (CR 701.20e, CR 400.2).
Fixes the Discord bug report (thread 1529132485668245575).
Summary by CodeRabbit
New Features
Bug Fixes
Tests