Card/plargg and nassari - #6320
Conversation
…igger
The middle sentence "An opponent chooses a nonland card exiled this
way" was Unimplemented{choose}: the typed exiled-this-way anaphor fell
through honestly, the "an opponent" subject lost the chooser, and the
ChooseFromZoneChoice continuation bound the pick (not the complement)
onto the follow-up cast.
- imperative.rs: typed "exiled this way" arm of
try_parse_choose_exiled_anaphor lowers to the Koh-shape
FromZone { Exile, AllOwners, And { Typed, ExiledBySource } }
(CR 607.2a linkage; CR 400.1 shared-zone scan); chooser prefix
alternation ("an opponent chooses" -> Chooser::Opponent, CR 608.2d);
non<type> qualifier support (CR 205.2b).
- mod.rs: lower_subject_predicate_ast rebinds the bare "an opponent"
subject onto ChooseFromZone's chooser before the honesty gate.
- engine_resolution_choices.rs: an "other cards" cast continuation
(ExiledBySource + FilterProp::Another) receives the UNCHOSEN
complement of the pick (CR 608.2c); chosen-consumer continuations
(End-Blaze Epiphany) are untouched.
Tests: two anaphor unit tests + untyped-referent regression guard
(imperative.rs), full three-sentence chain shape (tests.rs), and a
three-player runtime test driving the opponent's pick and asserting
zero-cost permissions land only on the other nonland hits
(exile_from_top_until.rs).
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 opponent and typed linked-exile choices plus counted free-cast clauses. Resolution can pause for opponent selection, reapply filters to tracked candidates, preserve source context, and cast only eligible unchosen exile hits. ChangesPlargg and Nassari choice flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant ChooseFromZone
participant ResolutionChoices
participant Opponent
participant FreeCastFromZones
OracleParser->>ChooseFromZone: Build opponent linked-exile choice
ChooseFromZone->>ResolutionChoices: Pause with eligible opponents
ResolutionChoices->>ChooseFromZone: Submit selected opponent
ChooseFromZone->>Opponent: Present filtered exile choice
Opponent->>FreeCastFromZones: Select a linked exile hit
FreeCastFromZones->>FreeCastFromZones: Offer eligible unchosen nonland hits
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aaadbda30
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| filter: TargetFilter::And { | ||
| filters: vec
When a typed "exiled this way" choice is lowered to ExiledBySource, ChooseFromZone scans the whole live linked-exile set for the source, not just the cards exiled by the current resolving trigger. If Plargg and Nassari has already resolved once and the same permanent triggers again while old linked cards remain in exile, the opponent's choice pool can include previous-upkeep nonland cards, even though "this way" refers only to the cards exiled by the current instruction; this needs the same current-resolution scoping/tracked-set treatment used elsewhere for linked exile consumers.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
| && target_filter_carries_another(target) | ||
| ); | ||
| if other_cards_cast_consumer { | ||
| cont.chain.targets = unchosen.iter().map(|&id| TargetRef::Object(id)).collect(); |
There was a problem hiding this comment.
For an "other cards" continuation this assigns every unchosen card as a cast target, and CastFromZone then stamps a separate free-cast permission on each target with no remaining-count state. In a four-player game Plargg exiles four nonland hits, one is chosen by the opponent, and the controller receives permission for all three other hits even though the text says "up to two spells"; the cap has to be carried into the continuation/permission flow instead of binding the full complement.
AGENTS.md reference: AGENTS.md:L14-L14
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/engine/src/parser/oracle_effect/tests.rs (1)
39428-39528: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLGTM overall — consider asserting the "up to two" cast cardinality.
The new test correctly exercises the exile-until → opponent-choose → free-cast chain, and the doc comment's CR 607.2a ("exiled this way" linked abilities) and CR 608.2d (choices announced during resolution) citations check out against the Comprehensive Rules text.
One gap: the input text says "cast up to two spells," but the test only destructures
targetandwithout_paying_mana_costfromCastFromZone(via..), never verifying the parsed cardinality (e.g. amulti_target/count field oncast). A regression that drops or miscounts "up to two" would not be caught here.♻️ Suggested addition
assert!( cf.iter().any(|f| matches!( f, TargetFilter::Typed(tf) if tf.properties.contains(&FilterProp::Another) )), "\"the OTHER cards\" must carry FilterProp::Another" ); + assert_eq!( + cast.multi_target, + Some(MultiTargetSpec::up_to(QuantityExpr::Fixed { value: 2 })), + "\"cast up to two spells\" must preserve its cardinality" + ); }🤖 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_effect/tests.rs` around lines 39428 - 39528, Extend plargg_and_nassari_full_trigger_chain_choose_then_cast_others to destructure the CastFromZone cardinality field instead of ignoring it, and assert that the parsed cast count represents “up to two.” Keep the existing target-filter and free-cast assertions unchanged.crates/engine/src/game/engine_resolution_choices.rs (2)
3824-3850: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe following (unchanged)
is_partitionforwarding isn't gated against the new "other cards" binding.Right after this block binds
cont.chain.targetstounchosenwhenother_cards_cast_consumeris true, the existingis_partitionblock (below, unchanged) unconditionally forwardsunchosenontocont.chain.sub_ability.targetstoo — for any effect oncont.chainother thanPutCounter. If a future "other cards exiled this way" cast (cont.chain) ever carries its own trailingsub_ability(e.g. a follow-up clause unrelated to the exile pool), that clause'stargetswould also get silently forced to the unchosen complement, even though it was never meant to consume that partition — the same class of mis-binding this PR is fixing forcont.chain.targetsitself, just one level down. Not reachable by Plargg/Nassari or Author of Shadows today (neithercast_subcarries a furthersub_abilityin the added tests), but the gate that protectsCastCopyOfCardfrom this exact forwarding suggestsother_cards_cast_consumerdeserves the same exclusion for symmetry and future-proofing.Proposed guard
let is_partition = !matches!( cont.chain.effect, crate::types::ability::Effect::PutCounter { .. } - ); + ) && !other_cards_cast_consumer;As per path instructions, "A latent bug behind a guard or unreached branch is still a finding — rate it by what happens when the form is reached or the guard is removed, not by today's reachability."
🤖 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/engine_resolution_choices.rs` around lines 3824 - 3850, Gate the existing is_partition forwarding to cont.chain.sub_ability.targets so it is skipped when other_cards_cast_consumer is true, matching the protection already used for CastCopyOfCard. Preserve the current unchosen forwarding for other partition continuations and retain the existing PutCounter exception.Source: Path instructions
894-908: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpell out
TargetFilterexhaustively here. Replace_ => falsewith explicit leaf variants so a future compositeTargetFilterthat carriesFilterProp::Anothercan’t silently bypass the Plargg/Nassari “other cards” branch.🤖 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/engine_resolution_choices.rs` around lines 894 - 908, The target_filter_carries_another function must match TargetFilter exhaustively instead of using _ => false. Enumerate every non-composite leaf variant explicitly and return false for each, while preserving the recursive handling of Typed, And, Or, Not, and TrackedSetFiltered so future composite variants cannot bypass the “other cards” branch.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/src/parser/oracle_effect/imperative.rs`:
- Around line 3968-3975: Preserve targeted-chooser identity instead of
collapsing both phrases into Chooser::Opponent: in
crates/engine/src/parser/oracle_effect/imperative.rs lines 3968-3975, represent
“target opponent chooses” separately or defer it to subject parsing; in
crates/engine/src/parser/oracle_effect/mod.rs lines 18608-18617, retain
subject.target as a target slot and bind the chooser to it, then add an
end-to-end regression test covering targeted-opponent choice resolution.
---
Nitpick comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 3824-3850: Gate the existing is_partition forwarding to
cont.chain.sub_ability.targets so it is skipped when other_cards_cast_consumer
is true, matching the protection already used for CastCopyOfCard. Preserve the
current unchosen forwarding for other partition continuations and retain the
existing PutCounter exception.
- Around line 894-908: The target_filter_carries_another function must match
TargetFilter exhaustively instead of using _ => false. Enumerate every
non-composite leaf variant explicitly and return false for each, while
preserving the recursive handling of Typed, And, Or, Not, and TrackedSetFiltered
so future composite variants cannot bypass the “other cards” branch.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 39428-39528: Extend
plargg_and_nassari_full_trigger_chain_choose_then_cast_others to destructure the
CastFromZone cardinality field instead of ignoring it, and assert that the
parsed cast count represents “up to two.” Keep the existing target-filter and
free-cast assertions 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: aecdc7e7-b186-48e2-ac81-256647036419
📒 Files selected for processing (6)
crates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — this support path is not rules-correct in multiplayer and does not preserve Plargg and Nassari’s two-spell limit.
🔴 Blocker
[HIGH] Let the controller choose which opponent makes the middle choice. Evidence: crates/engine/src/game/effects/choose_from_zone.rs:930-945 resolves Chooser::Opponent to the first APNAP opponent; exile_from_top_until.rs:1054-1072 only asserts that the chooser is not the controller. The official Plargg and Nassari release notes state: “In a multiplayer game, you choose an opponent to choose a nonland card exiled with Plargg and Nassari.” Why it matters: in a 3+ player game the engine silently selects a different chooser instead of presenting the controller’s required choice. Suggested fix: add a typed resolution-time opponent-selection step that binds the selected opponent before the ChooseFromZoneChoice prompt, then cover the 3+ player selection path.
[HIGH] Preserve and enforce the printed “up to two spells” bound. Evidence: crates/engine/src/game/engine_resolution_choices.rs:3847-3849 binds every unchosen card, while crates/engine/src/game/effects/cast_from_zone.rs:325-336,620 treats all bound object targets as permission recipients; the added runtime test has only three players and creates cast_sub without a cardinality bound at exile_from_top_until.rs:981-1008. Why it matters: with four players and one nonland hit each, one choice leaves three cards and grants all three free-cast permissions, exceeding the card’s explicit limit of two. Suggested fix: carry a typed, resolution-time cast-choice bound through CastFromZone and test a four-player fixture that permits exactly two of three unchosen hits.
[MED] Do not parse target opponent chooses without retaining the target. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:3968-3975 newly lowers both “an opponent” and “target opponent” to the same Chooser::Opponent; oracle_effect/mod.rs:18608-18617 returns without wrapping subject.target, although choose_from_zone.rs:934-939 can only honor a targeted chooser when that player target is present. Why it matters: the newly accepted targeted form can prompt an arbitrary opponent rather than the declared target. Suggested fix: model the targeted-opponent form separately and preserve its target slot through lowering, with an end-to-end regression.
🟡 Non-blocking
The current-head engine/parser parse-diff sticky comment is absent even though the card-data check ran; regenerate it after the correction so the claimed Plargg and Nassari/Author of Shadows impact can be compared with the measured card set.
Recommendation: request changes and re-run the current-head parse-diff after the multiplayer chooser and bounded cast flow are implemented.
…is-way anaphor 'target opponent chooses' must bind the chooser to the chosen target slot; collapsing it into Chooser::Opponent loses target identity, and no printed card pairs the targeted form with an 'exiled this way' anaphor. Accept only 'an opponent chooses' and let the targeted form fall through honestly. Addresses the CodeRabbit review finding on PR phase-rs#6320.
Parse changes introduced by this PR · 3 card(s), 3 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — this head correctly removes the unbound target opponent chooses grammar, but both blocking Plargg and Nassari behaviors remain.
🔴 Blocker
[HIGH] Let the controller choose which opponent makes the middle choice. Evidence: crates/engine/src/game/effects/choose_from_zone.rs:930-945 still resolves Chooser::Opponent to the first APNAP opponent; exile_from_top_until.rs:1054-1072 only proves the chooser is not the controller. The official Plargg and Nassari release notes say that, in multiplayer, you choose an opponent to make this choice. Why it matters: with three or more players the engine silently selects a chooser instead of performing the required choice. Suggested fix: add a typed resolution-time opponent-selection step and a 3+ player regression.
[HIGH] Preserve and enforce the printed “up to two spells” bound. Evidence: crates/engine/src/game/engine_resolution_choices.rs:3847-3849 still binds every unchosen card and crates/engine/src/game/effects/cast_from_zone.rs:325-336,620 grants each bound card a permission; exile_from_top_until.rs:981-1008 has no cardinality-bound cast choice. Why it matters: with four players and one nonland hit each, choosing one leaves three cards and grants all three free-cast permissions, exceeding the card’s limit. Suggested fix: carry a typed resolution-time maximum through CastFromZone, with a four-player regression that permits exactly two of three unchosen hits.
Recommendation: keep this PR blocked until the controller-selection and max-two cast flow are implemented and verified.
…st window for 'up to N spells ... exiled this way' Addresses the CHANGES_REQUESTED review on phase-rs#6320: HIGH-1 (multiplayer chooser): resolving a ChooseFromZone with chooser: Opponent and 2+ live opponents now pauses on the new typed WaitingFor::ChooseFromZoneOpponentChooser so the CONTROLLER picks which opponent makes the choice (Plargg release notes), mirroring the ClashChooseOpponent pattern. With exactly one live opponent the picker is skipped. Registered across engine, ai_support, phase-ai, scenario, action stable order, manabrew-compat, and the server payload guard. HIGH-2 (up-to-two bound): 'cast up to N spells from among the [other] cards exiled this way without paying their mana costs' no longer lowers to CastFromZone permission grants (no shared budget); it now routes to Effect::FreeCastFromZones - the during-resolution counted window (Invoke Calamity machinery) - matching ruling 2021-04-16 that the spells are cast during resolution. FreeCastFromZones gains a Zone::Exile arm scoped to the resolving source's exile links, a cast-mode land guard, and source threading through the CastOffer window and re-offer loop. 'the other cards' rewrites Another -> Not(InTrackedSet(0)) over the prior choose's published picks. The unchosen-complement binding in the ChooseFromZoneChoice handler is reverted (superseded by the tracked-set complement filter). Tests: 4-player runtime test drives all three pauses (picker -> opponent zone choice -> capped FreeCastWindow with the unchosen-only pool); 2-player regression asserts the picker pause is skipped; parser chain test updated to the FreeCastFromZones tail.
There was a problem hiding this comment.
Actionable comments posted: 4
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/visibility.rs (1)
1081-1096: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
sourceto theFreeCastWindowfixture incrates/engine/src/game/visibility.rs:4801— the struct literal is missing the requiredsourcefield and won’t compile until it’s added.🤖 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/visibility.rs` around lines 1081 - 1096, Update the FreeCastWindow fixture near the visibility tests to include the required source field in its struct literal, using the appropriate existing source value from that fixture. Keep the fixture’s other fields unchanged so it compiles and matches the FreeCastWindow construction used in the visibility filtering logic.
🧹 Nitpick comments (1)
crates/engine/src/game/effects/choose_from_zone.rs (1)
140-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
Effect::ChooseFromZonedestructuring — extract a shared helper.
resolve_with_choosing_player's parameter extraction is a near copy ofresolve's (lines 25-48), differing only in omittingchooser. Factor a small helper (e.g.fn choose_from_zone_params(ability) -> Result<(...), EffectError>) shared by both entry points.As per path instructions, "Reuse existing shared building blocks before adding utility or inline extraction logic; test the building block and its parameter range rather than a single card case."
🤖 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/choose_from_zone.rs` around lines 140 - 173, Extract the duplicated ChooseFromZone parameter matching from resolve and resolve_with_choosing_player into a shared choose_from_zone_params helper. Have the helper return the common fields, including chooser when needed or otherwise preserve access to it without duplicating the Effect::ChooseFromZone destructuring; update both entry points to use it while retaining the existing MissingParam behavior.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/src/game/engine_resolution_choices.rs`:
- Around line 2566-2597: Preserve the selected controller in the
ChooseFromZoneOpponentChooser match instead of discarding player, then call
set_priority(state, player) immediately before
resume_pending_continuation_if_priority in the empty-pool completion path. Keep
the existing priority resume behavior and opponent validation unchanged.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 20921-20938: Update the counted free-cast dispatch in
try_parse_cast_effect to call try_parse_counted_free_cast_from_exiled_this_way
only when constraint is absent, since Effect::FreeCastFromZones cannot preserve
cast constraints. When a constraint is present, skip this branch and allow the
existing fallback parsing path to handle it rather than returning an effect that
silently drops the constraint.
In `@crates/engine/src/types/ability.rs`:
- Around line 3108-3109: The FreeCastOfferRemaining source field must not
default missing values to ObjectId(0), since legacy saves lack a valid source
id. Update the FreeCastOfferRemaining deserialization/re-offer flow to
explicitly migrate or reject missing source data, ensuring eligible_candidates
never receives a fabricated id; add a pre-field fixture and round-trip test
covering the legacy save behavior.
In `@crates/manabrew-compat/src/lib.rs`:
- Around line 1714-1716: Register local.zone-opponent-chooser-unsupported in
unsupported_protocol_capabilities() so it matches the code emitted by
convert_available_action for GameAction::ChooseZoneOpponentChooser. Update the
associated capability count and registry tests to include the new entry.
---
Outside diff comments:
In `@crates/engine/src/game/visibility.rs`:
- Around line 1081-1096: Update the FreeCastWindow fixture near the visibility
tests to include the required source field in its struct literal, using the
appropriate existing source value from that fixture. Keep the fixture’s other
fields unchanged so it compiles and matches the FreeCastWindow construction used
in the visibility filtering logic.
---
Nitpick comments:
In `@crates/engine/src/game/effects/choose_from_zone.rs`:
- Around line 140-173: Extract the duplicated ChooseFromZone parameter matching
from resolve and resolve_with_choosing_player into a shared
choose_from_zone_params helper. Have the helper return the common fields,
including chooser when needed or otherwise preserve access to it without
duplicating the Effect::ChooseFromZone destructuring; update both entry points
to use it while retaining the existing MissingParam behavior.
🪄 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: 3d98e3c9-4803-4bf0-b08b-06a16611f984
📒 Files selected for processing (20)
crates/engine/src/ai_support/candidates.rscrates/engine/src/ai_support/mod.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/scenario.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/action_stable_order.rscrates/engine/src/types/actions.rscrates/engine/src/types/game_state.rscrates/manabrew-compat/src/lib.rscrates/phase-ai/src/decision_kind.rscrates/phase-ai/src/search.rscrates/server-core/src/game_action_payload_guard.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_effect/tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the refreshed head fixes the original chooser and two-cast defects, but it still leaks prior-resolution cards into the current free-cast window and leaves the new choice state unplayable in the browser.
🔴 Blocker
[HIGH] Scope “cards exiled this way” to this trigger resolution, not every card ever linked to Plargg and Nassari. Evidence: crates/engine/src/game/effects/free_cast_from_zones.rs:118-145 scans the whole shared exile zone and filters it with ExiledBySource; crates/engine/src/game/filter.rs:2292-2306 implements that filter as the source’s complete live linked-exile ledger. After a previous upkeep left a linked nonland card in exile, the next trigger’s window can therefore offer that old card whenever it was not the newly chosen card. The verified Oracle text says “from among the other cards exiled this way,” which confines the offer to this resolution’s exile batch. Suggested fix: carry a typed, concrete current-resolution member set through the choose → free-cast continuation, and intersect the offer with it before applying the chosen-card exclusion; add a two-upkeep regression where an older linked card is unavailable.
[HIGH] Complete the frontend and serialized-surface wiring for the new interactive state. Evidence: the engine creates WaitingFor::ChooseFromZoneOpponentChooser at crates/engine/src/types/game_state.rs:8553-8567, but client/src/adapter/types.ts:1688-1690 has no corresponding WaitingFor union member, the action union has no ChooseZoneOpponentChooser, and client/src/game/waitingForRegistry.ts:125-127 / client/src/pages/GamePage.tsx:1731-1736 register and render only the existing opponent-choice dialogs. Why it matters: the new four-player path pauses on an unhandled browser state and reaches the fail-loud exit modal instead of allowing the controller to choose an opponent. Suggested fix: thread the new waiting/action variants across the adapter, add a localized opponent-picker modal (reusing the existing opponent-choice presentation), register/mount it, and add the browser-facing dispatch test.
🟡 Non-blocking
[MED] Reconcile the claimed parse scope with the current parse-diff artifact. Evidence: the current <!-- coverage-parse-diff --> reports five changed cards — Author of Shadows, Plargg and Nassari, Forgotten Lore, Search for Survivors, and Shrouded Lore — while the PR’s claimed-card list names only the first two. Why it matters: the three unclaimed parser changes cannot be evaluated against an explicit intended class. Suggested fix: either name and justify those cards as the intended grammar class, with representative assertions, or narrow the parser change so the artifact matches the stated scope.
✅ Clean
The current-head four-player engine test now proves that the controller selects the opponent and that the immediate window starts with two remaining casts; those are material improvements over the previously reviewed head.
Recommendation: request changes; keep the PR blocked until the resolution-scoped set and browser choice flow are implemented with discriminating regressions.
…nent-chooser step - client: mirror WaitingFor::ChooseFromZoneOpponentChooser and GameAction::ChooseZoneOpponentChooser in the adapter unions, register the variant in HANDLED_WAITING_FOR_TYPES, and wire ZoneOpponentChooserModal (ClashOpponentModal pattern) into GamePage with i18n keys in all locales - engine: add the new 'source' field to the FreeCastWindow test constructor in visibility.rs (E0063 on both test shards) - engine: apply rustfmt to the two flagged sites in exile_from_top_until.rs
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 `@client/src/components/modal/ZoneOpponentChooserModal.tsx`:
- Around line 33-37: Remove the sorting logic around candidates in
ZoneOpponentChooserModal and render waitingFor.data.candidates directly in the
engine-provided order. Delete the seatOrder/indexOf-based derivation and
preserve the existing candidate rendering and action behavior without
client-side reordering.
🪄 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: 162201c4-747d-4084-8539-f34b71d7a596
📒 Files selected for processing (14)
client/src/adapter/types.tsclient/src/components/modal/ZoneOpponentChooserModal.tsxclient/src/components/modal/__tests__/ZoneOpponentChooserModal.test.tsxclient/src/game/waitingForRegistry.tsclient/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/pages/GamePage.tsxcrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/visibility.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/engine/src/game/effects/exile_from_top_until.rs
- crates/engine/src/game/visibility.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new frontend and opponent-picker work resolves the prior browser and multiplayer-choice blockers, but the current head still treats an all-time source exile ledger as this trigger resolution's this way set.
🔴 Blocker
[HIGH] Scope the free-cast pool to the cards exiled during the current resolution.
Evidence: crates/engine/src/game/effects/free_cast_from_zones.rs:118-145 now scans every object in state.exile and evaluates TargetFilter::ExiledBySource; crates/engine/src/game/filter.rs:2292-2306 resolves that filter with linked_exile_cards_for_source(state, source_id), which is the live, source-wide link ledger. Nothing in this path consumes the current resolution's tracked set. Consequently, on a later upkeep Plargg and Nassari can offer unchosen nonlands exiled by an earlier upkeep, even though the Oracle text says “the other cards exiled this way.”
Thread a resolution-scoped linked set through the choose/free-cast continuation (and keep the chosen-card exclusion against that same set), then add a two-upkeep regression proving the second offer cannot contain the first upkeep's cards. The current one-upkeep test cannot expose this lifetime error.
…resolution's exile batch Review round 2 (matthewevans, CHANGES_REQUESTED) + shard-2 CI defect on c88ad0e: - Thread THIS resolution's concrete member pool (the choose's full offered pool, derived from the chain tracked set the exile clause published) through the ChooseFromZoneChoice -> FreeCastFromZones continuation as object targets; eligible_candidates intersects the exile scan with it BEFORE the Not(InTrackedSet) chosen-card exclusion (CR 607.2a), so a linked nonland card left in exile by a previous upkeep is never re-offered. Carried on CastOfferKind::FreeCastWindow and FreeCastOfferRemaining for the re-offer loop; redacted like candidates for non-controller viewers (CR 400.2). - Fix the c88ad0e shard-2 failure (window skipped, Priority instead of CastOffer): chain_references_tracked_set could not see the chosen-card exclusion nested as FilterProp::Not { InTrackedSet } inside the typed leg of an And, so the chosen card was never published as the fresh chain set and the InTrackedSet(0) sentinel stayed bound to the exile-batch set, emptying the window. Add a property-level membership walker (filter_properties_reference_tracked_membership) to the FreeCastFromZones detection arm (CR 608.2c). - Add the requested two-upkeep regression (plargg_second_resolution_excludes_previous_resolutions_leftovers): upkeep 1 declines the window leaving a linked leftover in exile; upkeep 2 asserts both the opponent's choice pool and the free-cast window exclude it. - Extend the FreeCastWindow visibility test to pin member_pool redaction.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The prior stale-exile fix is present, but several current-head merge blockers remain:
- The empty
member_poolcontinuation discardsplayerand resumes withoutset_priority, leaving the opponent-chooser state unable to drain. Bind the player and restore priority before resuming. - The counted free-cast parser arm lowers even when it parsed a cast constraint, but
FreeCastFromZonescannot carry that constraint. Do not silently drop it: gate this arm onconstraint.is_none()or retain strict unsupported handling. ZoneOpponentChooserModalsorts engine candidates byseatOrder; that game ordering belongs in the engine. Render the supplied order.- Register
local.zone-opponent-chooser-unsupportedin the public capability registry and update its test. - The current parse diff has five changed cards, including Forgotten Lore, Search for Survivors, and Shrouded Lore, while the PR claims only Plargg and Author of Shadows. Reconcile and test the additional parser impact before re-review.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/effects/exile_from_top_until.rs (1)
1287-1300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the regression by asserting the leftover stays exile-linked, not just exiled.
The test's premise (and its own doc comment) is that
p0_oldremains linked tosourceinexile_linksafter the decline — that's what makes upkeep 2's exclusion meaningful. Only the zone is asserted here; if a future change accidentally dropped the link on decline instead of correctly resolution-scoping the lookup, this test would still pass for the wrong reason.♻️ Proposed strengthening
assert_eq!( state.objects[&p0_old].zone, Zone::Exile, "the declined card must remain exiled (and linked) after upkeep 1" ); + assert!( + state + .exile_links + .iter() + .any(|link| link.source_id == source && link.exiled_id == p0_old), + "the declined card must remain linked to the source after upkeep 1 — \ + upkeep 2's exclusion is only meaningful if the stale link still exists" + );🤖 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/exile_from_top_until.rs` around lines 1287 - 1300, Strengthen the regression assertion after the declined FreeCastWindowChoice in the test around p0_old by verifying that the source’s exile_links still contains p0_old, in addition to asserting its Zone::Exile state. Use the existing source and exile_links representation so the test confirms the card remains both exiled and linked.
🤖 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.
Nitpick comments:
In `@crates/engine/src/game/effects/exile_from_top_until.rs`:
- Around line 1287-1300: Strengthen the regression assertion after the declined
FreeCastWindowChoice in the test around p0_old by verifying that the source’s
exile_links still contains p0_old, in addition to asserting its Zone::Exile
state. Use the existing source and exile_links representation so the test
confirms the card remains both exiled and linked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 85706ae9-f03c-448b-be28-ff11f64eeea8
📒 Files selected for processing (9)
client/src/adapter/types.tscrates/engine/src/game/casting_costs.rscrates/engine/src/game/effects/exile_from_top_until.rscrates/engine/src/game/effects/free_cast_from_zones.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/visibility.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/types/ability.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/src/game/visibility.rs
- crates/engine/src/game/engine_resolution_choices.rs
…estore, targeted-chooser honesty gate - Register WaitingFor::ChooseFromZoneOpponentChooser in waits_for_resolution_choice so the picker pause parks the FreeCastFromZones continuation instead of resolving it inline before any pick exists (CI shard-2: Priority instead of CastOffer on the 4-player path). - Restore priority (set_priority) before draining the parked continuation on the picker answer arm's empty-pool path, mirroring ChooseClashOpponent (review HIGH). - Gate the counted free-cast reroute on constraint.is_none() so a parsed cast constraint is never silently dropped (review HIGH). - Gate the subject-layer chooser rebind on subject.target.is_none(): 'target opponent chooses …' (Forgotten/Shrouded Lore) binds the chooser to a chosen target slot Chooser::Opponent cannot represent — keep the honest fall-through (review MED); pin both directions with parser tests, claiming Search for Survivors' untargeted form. - Render the engine-supplied candidate order in ZoneOpponentChooserModal instead of re-sorting by client seatOrder (review MED); pin with a shuffled-input test. - Register local.zone-opponent-chooser-unsupported in manabrew-compat's capability registry, 18→19 entries (review MED). - Two-upkeep regression also asserts the declined card stays exile-linked (resolution scoping, not link pruning).
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head review: the chooser, continuation, and resolution-scoped exile pool are covered by the refreshed runtime regressions.
* feat(parser): implement Plargg and Nassari's opponent-chosen exile trigger
The middle sentence "An opponent chooses a nonland card exiled this
way" was Unimplemented{choose}: the typed exiled-this-way anaphor fell
through honestly, the "an opponent" subject lost the chooser, and the
ChooseFromZoneChoice continuation bound the pick (not the complement)
onto the follow-up cast.
- imperative.rs: typed "exiled this way" arm of
try_parse_choose_exiled_anaphor lowers to the Koh-shape
FromZone { Exile, AllOwners, And { Typed, ExiledBySource } }
(CR 607.2a linkage; CR 400.1 shared-zone scan); chooser prefix
alternation ("an opponent chooses" -> Chooser::Opponent, CR 608.2d);
non<type> qualifier support (CR 205.2b).
- mod.rs: lower_subject_predicate_ast rebinds the bare "an opponent"
subject onto ChooseFromZone's chooser before the honesty gate.
- engine_resolution_choices.rs: an "other cards" cast continuation
(ExiledBySource + FilterProp::Another) receives the UNCHOSEN
complement of the pick (CR 608.2c); chosen-consumer continuations
(End-Blaze Epiphany) are untouched.
Tests: two anaphor unit tests + untyped-referent regression guard
(imperative.rs), full three-sentence chain shape (tests.rs), and a
three-player runtime test driving the opponent's pick and asserting
zero-cost permissions land only on the other nonland hits
(exile_from_top_until.rs).
* style: apply rustfmt to test ability builder and continuation binding
* test: import WaitingFor in the Plargg runtime regression test
* fix(engine): apply the ChooseFromZone filter to tracked-set candidate pools
* fix(parser): drop speculative targeted-chooser tag from the exiled-this-way anaphor
'target opponent chooses' must bind the chooser to the chosen target slot;
collapsing it into Chooser::Opponent loses target identity, and no printed
card pairs the targeted form with an 'exiled this way' anaphor. Accept only
'an opponent chooses' and let the targeted form fall through honestly.
Addresses the CodeRabbit review finding on PR phase-rs#6320.
* feat(engine): controller picks the choosing opponent; counted free-cast window for 'up to N spells ... exiled this way'
Addresses the CHANGES_REQUESTED review on phase-rs#6320:
HIGH-1 (multiplayer chooser): resolving a ChooseFromZone with
chooser: Opponent and 2+ live opponents now pauses on the new typed
WaitingFor::ChooseFromZoneOpponentChooser so the CONTROLLER picks which
opponent makes the choice (Plargg release notes), mirroring the
ClashChooseOpponent pattern. With exactly one live opponent the picker
is skipped. Registered across engine, ai_support, phase-ai, scenario,
action stable order, manabrew-compat, and the server payload guard.
HIGH-2 (up-to-two bound): 'cast up to N spells from among the [other]
cards exiled this way without paying their mana costs' no longer lowers
to CastFromZone permission grants (no shared budget); it now routes to
Effect::FreeCastFromZones - the during-resolution counted window
(Invoke Calamity machinery) - matching ruling 2021-04-16 that the
spells are cast during resolution. FreeCastFromZones gains a Zone::Exile
arm scoped to the resolving source's exile links, a cast-mode land
guard, and source threading through the CastOffer window and re-offer
loop. 'the other cards' rewrites Another -> Not(InTrackedSet(0)) over
the prior choose's published picks.
The unchosen-complement binding in the ChooseFromZoneChoice handler is
reverted (superseded by the tracked-set complement filter).
Tests: 4-player runtime test drives all three pauses (picker ->
opponent zone choice -> capped FreeCastWindow with the unchosen-only
pool); 2-player regression asserts the picker pause is skipped; parser
chain test updated to the FreeCastFromZones tail.
* fix(ci): frontend handler + fmt + test-constructor fixes for the opponent-chooser step
- client: mirror WaitingFor::ChooseFromZoneOpponentChooser and
GameAction::ChooseZoneOpponentChooser in the adapter unions, register the
variant in HANDLED_WAITING_FOR_TYPES, and wire ZoneOpponentChooserModal
(ClashOpponentModal pattern) into GamePage with i18n keys in all locales
- engine: add the new 'source' field to the FreeCastWindow test constructor
in visibility.rs (E0063 on both test shards)
- engine: apply rustfmt to the two flagged sites in exile_from_top_until.rs
* fix(engine): scope 'exiled this way' free-cast offers to the current resolution's exile batch
Review round 2 (matthewevans, CHANGES_REQUESTED) + shard-2 CI defect on c88ad0e:
- Thread THIS resolution's concrete member pool (the choose's full offered
pool, derived from the chain tracked set the exile clause published) through
the ChooseFromZoneChoice -> FreeCastFromZones continuation as object targets;
eligible_candidates intersects the exile scan with it BEFORE the
Not(InTrackedSet) chosen-card exclusion (CR 607.2a), so a linked nonland
card left in exile by a previous upkeep is never re-offered. Carried on
CastOfferKind::FreeCastWindow and FreeCastOfferRemaining for the re-offer
loop; redacted like candidates for non-controller viewers (CR 400.2).
- Fix the c88ad0e shard-2 failure (window skipped, Priority instead of
CastOffer): chain_references_tracked_set could not see the chosen-card
exclusion nested as FilterProp::Not { InTrackedSet } inside the typed leg
of an And, so the chosen card was never published as the fresh chain set
and the InTrackedSet(0) sentinel stayed bound to the exile-batch set,
emptying the window. Add a property-level membership walker
(filter_properties_reference_tracked_membership) to the FreeCastFromZones
detection arm (CR 608.2c).
- Add the requested two-upkeep regression
(plargg_second_resolution_excludes_previous_resolutions_leftovers): upkeep 1
declines the window leaving a linked leftover in exile; upkeep 2 asserts
both the opponent's choice pool and the free-cast window exclude it.
- Extend the FreeCastWindow visibility test to pin member_pool redaction.
* fix(engine): round-3 review — picker continuation parking, priority restore, targeted-chooser honesty gate
- Register WaitingFor::ChooseFromZoneOpponentChooser in waits_for_resolution_choice
so the picker pause parks the FreeCastFromZones continuation instead of
resolving it inline before any pick exists (CI shard-2: Priority instead of
CastOffer on the 4-player path).
- Restore priority (set_priority) before draining the parked continuation on the
picker answer arm's empty-pool path, mirroring ChooseClashOpponent (review HIGH).
- Gate the counted free-cast reroute on constraint.is_none() so a parsed cast
constraint is never silently dropped (review HIGH).
- Gate the subject-layer chooser rebind on subject.target.is_none(): 'target
opponent chooses …' (Forgotten/Shrouded Lore) binds the chooser to a chosen
target slot Chooser::Opponent cannot represent — keep the honest fall-through
(review MED); pin both directions with parser tests, claiming Search for
Survivors' untargeted form.
- Render the engine-supplied candidate order in ZoneOpponentChooserModal instead
of re-sorting by client seatOrder (review MED); pin with a shuffled-input test.
- Register local.zone-opponent-chooser-unsupported in manabrew-compat's
capability registry, 18→19 entries (review MED).
- Two-upkeep regression also asserts the declined card stays exile-linked
(resolution scoping, not link pruning).
---------
Co-authored-by: Whovencroft <whovencroft@users.noreply.github.com>
Co-authored-by: Whovencroft <6.60056e+06+Whovencroft@users.noreply.github.com>
2 participants
Summary
Adds engine support for Plargg and Nassari.
Closes the previously-unsupported
choosegap on the upkeep trigger's middle sentence — "An opponent chooses a nonland card exiled this way." — across four seams:try_parse_choose_exiled_anaphor's "exiled this way" referent only supported the untyped chain-tracked-set form ("choose a card exiled this way") and fell through honestly on a typed qualifier. The typed form ("a nonland card exiled this way") now lowers to the same linked-exile shape as the Koh, the Face Stealer "exiled with ~" arm:ChooseFromZone { zone: Exile, zone_owner: AllOwners, filter: And { Typed, ExiledBySource } }(CR 607.2a linkage; CR 400.1 shared-zone scan). The card-type qualifier now also accepts negatednon<type>forms (CR 205.2b), and a chooser-prefix alternation ("an opponent chooses" / "target opponent chooses" →Chooser::Opponent, CR 608.2d) mirrorsparse_choose_anaphoric's existing convention.strip_subject_clausepeels "an opponent" and deconjugates the predicate to "choose …", losing the chooser.lower_subject_predicate_astnow rebinds the bare single-opponent subject ontoChooseFromZone'schooserbefore the honesty gate; typed or property-carrying subjects still fall through toEffect::unimplemented.ChooseFromZoneChoicehandler bound the chosen card as the parked continuation's target list — correct for chosen-consumer continuations (End-Blaze Epiphany's "you may play that card") but inverted for Plargg's "cast up to two spells from among the other cards exiled this way". When the continuation head is an exile-linkedCastFromZonewhose target carries theFilterProp::Anotherexclusion, the handler now binds the unchosen complement (CR 608.2c); all other continuations are untouched.resolve_candidate_cardsreturned tracked-set-derived candidate pools raw, ignoring the effect's own card filter — the player-scope completion publishes every card the exile clause moved (lands included) as the chain tracked set, so the opponent was offered the lands too. A typed filter (Some(...)) now narrows tracked-set and explicit-target pools throughmatches_target_filter(CR 608.2d);filter: Nonekeeps the raw set, so the untyped "choose one of them" tracked-set anaphors are unchanged. Surfaced by the new runtime regression test on this PR's first CI run.Files changed
CR references
non<type>card-type qualifier)Implementation method (required)
Method: /engine-implementer
Track
Non-developer
LLM
Model: manus-1.5
Thinking: high
Verification
./scripts/check-parser-combinators.sh origin/main—Gate G PASS+Gate A PASS head=7aaadbda3080edcb0b7cb6cae9e05565460879fc base=bdb1f3de5c37fc216d79c683c854be6448aad44ccargo check -p engine --lib—Finishedwith zero errors and zero warnings (on the initial implementation commit; later commits validated by CI)cargo test -p engine(including the five new tests below),cargo fmt --check,cargo clippy-strict,cargo coverage, andcargo semantic-auditare delegated to the CI-owned checks on this PR — the ephemeral sandbox was reset mid-run three times, so local test execution could not complete.parse_choose_nonland_card_exiled_this_way,parse_opponent_chooses_nonland_card_exiled_this_way,parse_choose_card_exiled_this_way_still_tracked_set(imperative.rs),plargg_and_nassari_full_trigger_chain_choose_then_cast_others(tests.rs),plargg_opponent_chooses_nonland_hit_then_cast_pool_is_the_other_hits(exile_from_top_until.rs, runtimeapply()-driven).Gate A
Gate A PASS head=7aaadbda3080edcb0b7cb6cae9e05565460879fc base=bdb1f3de5c37fc216d79c683c854be6448aad44c
Anchored on
FromZone { Exile, AllOwners, And { Typed, ExiledBySource } }lowering the typed "exiled this way" referent now reuses (CR 607.2a + CR 400.1).parse_choose_anaphoric's chooser alternation ("an opponent chooses N of them" →Chooser::Opponent), the CR 608.2d delegation convention the new chooser prefix mirrors.ChooseFromZoneChoicehandler, which already forwards the unchosen remainder to the zone-partition sub-ability; the complement binding extends the same seam.Final review-impl
Final review-impl PASS head=7aaadbda3080edcb0b7cb6cae9e05565460879fc
if let Ok(tag)blocks, no hand-builtEffect::Unimplementedliterals in the diff.resolve_ability_chain→WaitingFor::ChooseFromZoneChoice→apply(GameAction::SelectCards)) and fails if the complement binding, the opponent chooser rebind, or the typed-anaphor filter is reverted. The untyped-referent regression guard pins End-Blaze-style impulse reductions to the chain tracked set.Claimed parse impact
Validation Failures
None.
CI Failures
Three red runs during iteration, each root-caused and fixed on this branch; the final head is green.
Rust lint— two rustfmt diffs in test/handler code introduced after a sandbox reset dropped the localcargo fmtpass. Fixed in 5578e01 by applying the exact rustfmt output.Rust tests (both shards)— E0433:WaitingFormissing from the runtime test's local import list. Fixed in de0e127.Rust tests (shard 2/2)— the new runtime regression test failed: the opponent's candidate pool contained all six exiled cards (lands included) becauseresolve_candidate_cardsignored the effect filter for chain-tracked-set pools. Fixed in 7aaadbd (Summary seam 4); shard 1, Card data, and all other checks were already green on the same run, and no pre-existing test changed behavior.Summary by CodeRabbit