fix(engine): keep Portent exile picks out of the graveyard (#6498)#6545
Conversation
…6498) Model put-the-rest after ForEachCategory as LastRevealed still in the library, and preserve dynamic X on reveal Digs instead of collapsing to RevealTop{1}. Co-authored-by: Cursor <cursoragent@cursor.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
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:
📝 WalkthroughWalkthroughOracle parsing and runtime resolution now preserve dynamic reveal counts, expose reveal-only Dig cards for selection, track exile causes, and move remaining revealed or exiled cards to the correct zones. Integration tests cover Portent of Calamity parsing and resolution. ChangesPortent of Calamity behavior
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Spell
participant Dig
participant CategorySelection
participant ZoneState
Spell->>Dig: resolve dynamic reveal
Dig->>ZoneState: record LastRevealed and CardsRevealed
ZoneState->>CategorySelection: expose revealed pool
CategorySelection->>ZoneState: exile selected cards with causes
ZoneState->>ZoneState: move remaining revealed cards
ZoneState->>ZoneState: move remaining exiled cards to hand
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Blocked — the first Put the rest cleanup is separated correctly, but the card's final Put the rest of the exiled cards into your hand still has no stable binding to only the selected exile set.
🔴 Blocker
[HIGH] The final cleanup is routed through the generic the rest tracked-set path after the new Library→Graveyard cleanup has added the rejected reveal cards to that same chain set. Evidence: choose_from_zone.rs:284 creates the fresh per-category set and choose_from_zone.rs:524 publishes the exile picks; the new sequence.rs:3995 adds a LastRevealed Library→Graveyard move; and effects/mod.rs:5028 deliberately extends, rather than replaces, that chain set. The generic target parser then consumes only the the rest prefix as TrackedSet (oracle_target.rs:1030), losing the of the exiled cards qualifier. Why it matters: Portent's actual Oracle text requires the final instruction to move only the uncast cards exiled by the preceding category choices; it must not also select cards that were just moved to the graveyard. This violates the card's written sequence (CR 608.2c) and reveal-zone behavior (CR 701.20b). Suggested fix: model the two distinct remainder sets explicitly—revealed cards still in the library for the first cleanup, and the category-exiled set (minus a cast card) for the final cleanup—rather than reusing a chain-wide TrackedSet. Add a runtime regression with five revealed cards, four selected/exiled and one left for the graveyard, then decline or cast the free spell and assert that only the remaining exiled cards go to hand.
[MEDIUM] The new runtime test cannot expose that full-card failure: it reveals exactly four differently typed cards and selects all four, then stops as soon as the free-cast prompt appears. Evidence: issue_6498_portent_of_calamity.rs:119 creates only four cards, and issue_6498_portent_of_calamity.rs:144 exits after the category choices. Why it matters: neither remainder instruction nor the post-cast/decline tail is asserted. Suggested fix: make the runtime case drive the entire spell and verify the three final zones above. Also use Portent's actual {X}{U} cost in the scenario rather than {X}{U}{U}.
🟡 Scope
[LOW] PR_6498.md duplicates the PR narrative as a new repository file. Evidence: PR_6498.md. Suggested fix: remove it; the issue/PR description is the appropriate home for this context.
Recommendation
Please address the stable-set and end-to-end coverage blockers on a new head, then request re-review. CI and CodeRabbit are still running, so they cannot validate the missing tail behavior yet.
Parse changes introduced by this PR · 19 card(s), 26 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: 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/parser/oracle_effect/assembly.rs (1)
2305-2347: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
rest_destinationfor dynamic reveal Digs
crates/engine/src/parser/oracle_effect/assembly.rs:2305-2347/crates/engine/src/game/effects/dig.rsThe dynamic-count branch clears
rest_destination, butdig.rsreturns early onkeep_count == 0before applying it. Any reveal-Dig that still needs a plain “put the rest …” move will silently drop that routing.🤖 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/assembly.rs` around lines 2305 - 2347, Preserve the original rest_destination when handling dynamic-count reveal Digs in the assembly loop. In the non-Fixed branch, keep keep_count at zero and clear only destination as needed, but do not clear rest_destination, so dig.rs can apply the “put the rest” routing despite the keep_count == 0 early return.
🧹 Nitpick comments (1)
crates/engine/src/game/effects/dig.rs (1)
144-175: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate public-reveal marking logic vs. lines 186-200.
The
is_revealbranch here (insert intorevealed_cards, buildcard_names, pushCardsRevealed) is byte-for-byte the same sequence already present later in this function for the non-zerokeep_countpath. Extract a small helper (e.g.mark_publicly_revealed(state, events, ability, &cards)) to avoid the two copies diverging later.♻️ Suggested extraction
+fn mark_publicly_revealed( + state: &mut GameState, + events: &mut Vec<GameEvent>, + ability: &ResolvedAbility, + cards: &[CardId], +) { + for &card_id in cards { + state.revealed_cards.insert(card_id); + } + let card_names: Vec<String> = cards + .iter() + .filter_map(|id| state.objects.get(id).map(|o| o.name.clone())) + .collect(); + events.push(GameEvent::CardsRevealed { + player: ability.controller, + card_ids: cards.to_vec(), + card_names, + }); +}Then call it from both the
keep_count == 0branch and the existingis_revealblock below.🤖 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/dig.rs` around lines 144 - 175, Extract the duplicated public-reveal handling from the raw_keep_count == 0 branch and the non-zero keep_count is_reveal path into a shared helper, such as mark_publicly_revealed, that updates revealed_cards, builds card_names, and emits CardsRevealed. Replace both inline sequences with calls to the helper while preserving their existing controller and cards values.
🤖 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/sequence.rs`:
- Around line 6655-6683: Update the default branch of the destination selection
in the ForEachCategory ExileFromPool “put the rest” arm to use Zone::Library,
matching the sibling plain-Dig “put the rest” arm while preserving all explicit
phrase mappings.
In `@crates/engine/tests/integration/issue_6498_portent_of_calamity.rs`:
- Around line 107-177: Update
portent_exiled_picks_stay_exiled_not_dumped_to_graveyard so the revealed set
includes at least one additional card beyond the four selected type picks, such
as adding a fifth library card with a duplicate type or increasing X
accordingly. Track the unselected revealed card and assert it ends in
Zone::Graveyard, while retaining the existing assertions that selected cards
remain in Zone::Exile and outside the graveyard.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/assembly.rs`:
- Around line 2305-2347: Preserve the original rest_destination when handling
dynamic-count reveal Digs in the assembly loop. In the non-Fixed branch, keep
keep_count at zero and clear only destination as needed, but do not clear
rest_destination, so dig.rs can apply the “put the rest” routing despite the
keep_count == 0 early return.
---
Nitpick comments:
In `@crates/engine/src/game/effects/dig.rs`:
- Around line 144-175: Extract the duplicated public-reveal handling from the
raw_keep_count == 0 branch and the non-zero keep_count is_reveal path into a
shared helper, such as mark_publicly_revealed, that updates revealed_cards,
builds card_names, and emits CardsRevealed. Replace both inline sequences with
calls to the helper while preserving their existing controller and cards values.
🪄 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: 106336a3-2947-4eae-83c1-299acedff565
📒 Files selected for processing (8)
PR_6498.mdcrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/dig.rscrates/engine/src/game/effects/mod.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_6498_portent_of_calamity.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: Blocked — the duplicate root narrative is removed on this head, but the selected-exile subset still has no stable binding through Portent’s final instruction.
🔴 Blocker
[HIGH] The two “rest” instructions require different, explicitly bound sets, but the new continuation matches any earlier per-category exile. Evidence: crates/engine/src/parser/oracle_effect/sequence.rs:3974-4010 searches all assembled effects for ForEachCategory and always emits Library + LastRevealed. That is correct only for the first “Put the rest into your graveyard”; Portent’s final “Then put the rest of the exiled cards into your hand” must operate on the category-exiled subset, excluding a free-cast card. The generic anaphor path also recognizes only “the rest” as TrackedSet at crates/engine/src/parser/oracle_target.rs:1030, so the “of the exiled cards” qualifier is lost. Why it matters: the final action can move no cards from Library after the first cleanup, or can draw from a chain-wide set rather than exactly the still-exiled picks. CR 608.2c requires the written instructions and their anaphora to be applied in order. Suggested fix: represent the revealed-library remainder and the category-exiled remainder as separate explicitly bound sets, then consume each at its corresponding instruction.
[HIGH] The dynamic-reveal demotion broadens behavior to nine cards without preserving the ordinary rest-disposition path. Evidence: crates/engine/src/parser/oracle_effect/assembly.rs:2310-2343 changes every non-fixed reveal Dig in this shape to keep_count 0 and clears rest_destination, while crates/engine/src/game/effects/dig.rs:149-181 returns before any rest move. The current parse-diff artifact records nine changed cards, including Sunbird’s Invocation and Sphinx of Clear Skies, but the new test covers only Portent. Why it matters: a dynamic reveal that still needs a non-category “put the rest” instruction now has no resolver path for it. Suggested fix: limit the Portent-specific remainder binding to its demonstrated grammar/IR class, or extend the general dynamic Dig representation and runtime resolution with card-class tests for every claimed changed signature.
[MED] The runtime test stops before either final remainder instruction and has no unselected revealed card. Evidence: crates/engine/tests/integration/issue_6498_portent_of_calamity.rs:119-177 reveals four distinct types, chooses all four, and exits at the free-cast prompt. Why it matters: it cannot fail if an unselected reveal is not put into the graveyard or if the remaining exiled cards are not put into hand. Suggested fix: reveal five cards with a duplicate type, complete both the decline/cast branch and final cleanup, and assert the library remainder is in the graveyard while only the remaining exile subset reaches hand. Use the card’s verified {X}{U} cost in that scenario.
✅ Clean
The focused removal of PR_6498.md leaves the issue and PR descriptions as the single narrative source.
Recommendation: request-changes — retain the cleanup, then rework the two remainder sets and add an end-to-end discriminating regression before re-review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer review
The current head f0235059 only removes the duplicate root-level PR notes; the engine/parser changes remain otherwise identical to the previously blocked head.
[HIGH] The final Put the rest of the exiled cards into your hand still has no stable binding to the cards selected by the per-category exile. Evidence: choose_from_zone.rs:284 creates the fresh category set, choose_from_zone.rs:524 publishes its picks, sequence.rs:3975 inserts the revealed-card Library→Graveyard cleanup, and oracle_target.rs:1030 consumes only the the rest prefix as an unqualified tracked set. Why it matters: Portent must move only the uncast cards exiled by the earlier choices; it must not rely on the chain-wide set after the rejected revealed cards have also been processed. Suggested fix: model the final exiled subset explicitly, reusing the existing provenance-filtered tracked-set authority, and cover its decline/cast tail.
[HIGH] The runtime test still stops at the free-cast prompt and reveals exactly four different card types. Evidence: issue_6498_portent_of_calamity.rs:119-177. Why it matters: it neither proves a revealed-but-unselected card reaches the graveyard nor proves the final uncast exile cards reach hand. Suggested fix: reveal a fifth duplicate-type card, drive the optional cast choice to completion, and assert all three resulting zones.
The fresh parse-diff for the unchanged engine head affects nine dynamic-reveal cards. Its shared Dig { keep_count: 0 } path is plausible because it publishes CardsRevealed into the existing chain set, but the PR supplies no sibling runtime coverage; complete the audited class rather than treating Portent's partial scenario as proof.
CodeRabbit reconciliation: its missing-rest test finding is confirmed. Its rest_destination preservation suggestion is not sufficient: dig.rs returns early for keep_count == 0, so the correct route is the explicit chained disposition, not a retained unused field. The default-zone comment is a low-risk follow-up and does not resolve the blockers.
Requested before re-review: run the engine-implementer plan/review cycle for the typed final-exile binding, then provide discriminating full-pipeline tests for Portent and the affected dynamic-reveal siblings.
…e-rs#6498) Separate revealed-library rest from exiled-card cleanup via LastRevealed and TrackedSetFiltered(Exiled), emit explicit LastRevealed siblings for dynamic reveal Digs, and lower the hand tail as ChangeZoneAll. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — the Portent fix still ships an unproven parser-class expansion outside its stated scope.
🔴 Blocker
crates/engine/src/parser/oracle_effect/sequence.rs:4032 adds an explicit LastRevealed cleanup for every reveal-only Dig { keep_count: Some(0) }, while the current parse-diff reports nine affected cards: Enshrined Memories, Epiphany at the Drownyard, Glimpse of Tomorrow, Glint Raker, Green Sun's Twilight, Hew the Entwood, Sphinx of Clear Skies, Sunbird's Invocation, and Portent. The PR only has a Portent runtime fixture. This shared continuation branch can change where each sibling class sends its revealed remainder, but there is no class-level behavior map or discriminating coverage for those affected cards.
Suggested fix: either narrow the Portent-specific route to the proven ForEachCategory / exiled-card relation, or explicitly justify the generalized Dig semantics and add representative runtime tests for the affected count sources (X, event-context, tracked-set, and source-mana-value).
✅ Clean
crates/engine/src/parser/oracle_target.rs:1026 uses the existing typed TrackedSetFiltered { caused_by: Exiled } vocabulary for Portent’s final exile-set tail; the new Portent test exercises the optional-decline path.
Recommendation: request changes for the unexplained nine-card parser blast radius before approval.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_target.rs (1)
1029-1046: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCorrectly disambiguates the two distinct "the rest" clauses on Portent of Calamity.
Verified against the card's actual Oracle text ("Put the rest into your graveyard. ... Then put the rest of the exiled cards into your hand.") — this arm targets only the second, more specific anaphor (exiled-minus-cast), and is correctly placed ahead of the generic
TRACKED_SET_PHRASEStable so the bare"the rest"tag doesn't prefix-match and strand" of the exiled cards"as an unconsumed remainder.One nice-to-have: every other arm in this file has a dedicated unit test in the
testsmodule; this new arm doesn't, relying solely on the separate integration test file for coverage.🧪 Suggested unit test
#[test] fn the_rest_of_the_exiled_cards_is_tracked_set_filtered_by_exile_cause() { let (f, rest) = parse_target("the rest of the exiled cards"); assert_eq!(rest, ""); assert_eq!( f, TargetFilter::TrackedSetFiltered { id: TrackedSetId(0), filter: Box::new(TargetFilter::Any), caused_by: Some(ThisWayCause::Exiled), } ); }🤖 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_target.rs` around lines 1029 - 1046, Add a dedicated unit test in the parser’s existing tests module for the new “the rest of the exiled cards” arm. Use parse_target, assert the remaining text is empty, and verify the result equals TrackedSetFiltered with TrackedSetId(0), TargetFilter::Any, and ThisWayCause::Exiled.
🤖 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 6530-6541: The mass-filter handling around
TargetFilter::TrackedSetFiltered currently defaults origin to Exile for every
cause; restrict that fallback to caused_by == Some(ThisWayCause::Exiled). Match
the ReturnAllToZone arm’s cause-sensitive behavior, preserving the existing
origin when the effect was caused by milling, discarding, returning,
sacrificing, destroying, or no specific cause.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_target.rs`:
- Around line 1029-1046: Add a dedicated unit test in the parser’s existing
tests module for the new “the rest of the exiled cards” arm. Use parse_target,
assert the remaining text is empty, and verify the result equals
TrackedSetFiltered with TrackedSetId(0), TargetFilter::Any, and
ThisWayCause::Exiled.
🪄 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: 84ae3fce-e563-47a4-acf1-deeaa29ad131
📒 Files selected for processing (5)
crates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/issue_6498_portent_of_calamity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/issue_6498_portent_of_calamity.rs
…se-rs#6498) Fix clippy if-same-then-else, emit LastRevealed siblings for dynamic reveal Digs before assembly demotion, and add parse coverage for the shared put-the-rest grammar class Matthew requested. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head 55fe7a3f.
The new helper broadens the parser from Portent to dynamic reveal-only Dig continuations, but the added coverage is AST-only. The prior blocker remains: this changes the shared class without representative runtime behavior proving the rest pile is moved correctly for a second affected card/class. Please add a discriminating runtime regression for a distinct dynamic reveal-only Dig with a trailing rest clause (and retain Portent), then re-request review.
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/tests/integration/issue_6498_portent_of_calamity.rs`:
- Around line 164-208: Add a verified Comprehensive Rules citation annotation to
the test function dynamic_reveal_put_rest_emits_last_revealed_sibling, using the
required “CR <number>: <description>” format and covering reveal/remainder
instruction ordering and movement of the remainder to the specified zone. Keep
the test logic unchanged and ensure the cited rule numbers and descriptions are
verified.
- Around line 164-208: Extend
dynamic_reveal_put_rest_emits_last_revealed_sibling with a runtime scenario that
casts the parsed “Reveal the top X cards… Put the rest on the bottom… in any
order” effect using a concrete X value, resolves the library-ordering prompt,
and asserts that exactly the revealed cards are moved to the library bottom
while unrevealed cards remain in their original relevant positions. Reuse the
existing integration-test setup and resolution helpers, and retain the AST
assertion as a complementary check.
🪄 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: ff9b146f-e489-4189-bdb5-6a50d66b993e
📒 Files selected for processing (2)
crates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_6498_portent_of_calamity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_effect/sequence.rs
…-rs#6498) Add a discriminating cast-pipeline test for the shared dynamic-reveal + put-the-rest grammar class, stamp library-bottom placement on the explicit LastRevealed sibling, and narrow TrackedSetFiltered mass-move origin defaults. Co-authored-by: Cursor <cursoragent@cursor.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/issue_6498_portent_of_calamity.rs`:
- Around line 250-276: Update the test around runner.cast(spell).x(3).resolve()
to provide a player-selected, non-default ordering through the production
pipeline, exercising the ordering choice and failure path as required. Replace
the position-only assertions with an exact post-resolution library sequence that
verifies the unrevealed card remains on top and the revealed cards follow in the
selected order.
🪄 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: 852d937c-00ae-4fd8-9d08-abf3a7e4ffe7
📒 Files selected for processing (3)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/issue_6498_portent_of_calamity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_effect/sequence.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Held on current head 8024cbb0. The new dynamic_reveal_put_rest_moves_revealed_cards_to_library_bottom scenario supplies the requested production-path sibling coverage beyond Portent. Required Rust verification remains in progress and Tauri currently reports a failed job, so no approval or queue action yet.
…sion The runtime sibling test for reveal-only Dig + put-the-rest was failing CI because it assumed reveal order rather than the engine's deterministic batch placement order. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head 02ecfff3.
The new regression hard-codes a deterministic bottom-tail order and explicitly asserts that resolution reaches Priority with no prompt. That is rules-incorrect for this Oracle text: "Put the rest on the bottom of your library in any order" moves three cards simultaneously, so their owner may arrange them in any order (CR 401.4). The parser currently records this continuation with reorder_all: false in oracle_effect/sequence.rs, and the test consequently proves the missing choice rather than the requested behavior.
Please preserve the existing selected-exile/remainder fix, but model this library-bottom ordering at the continuation/resolution seam and add a production-path regression that submits a non-default order and verifies it becomes the bottom tail. Do not approve or enqueue while the choice is absent.
…e-rs#6498) CR 401.4: when multiple cards are placed at the same library position with random_order false, route through EffectZoneChoice instead of a silent batch order. Update the dynamic-reveal sibling regression to submit a non-default bottom tail and verify it sticks. Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new order prompt gives the order to the wrong player.
🔴 Blocker
change_zone.rs:1669-1704 states the CR 401.4 owner-order rule, but creates one EffectZoneChoice with player: filter_controller for all matching cards. engine_resolution_choices.rs:6722-6733 then applies that one submitted order directly to each selected card’s owner library. A spell that returns/reorders multiple cards in a targeted opponent's library therefore lets the spell controller choose their order; if one mass effect covers multiple libraries, it also combines independent owner choices. CR 401.4 says the owner of those cards may arrange cards placed simultaneously in that library.
Split the matching cards by owner and surface one ordered choice per owner (or carry an owner-scoped continuation through the existing EffectZoneChoice path). Add a targeted-opponent or multi-owner runtime regression; the current self-library Portent scenario cannot exercise this authority boundary.
Recommendation: request changes before approval or enqueue.
The TrackedSetFiltered mass-move promotion must not absorb bounded "up to N" picks like "put up to one land discarded this way onto the battlefield". Co-authored-by: Cursor <cursoragent@cursor.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the current head still gives a library-order choice to the wrong player.
🔴 Blocker
change_zone.rs:1669-1704 still creates one EffectZoneChoice for all matching cards with player: filter_controller. For cards put into the same library location in any order, CR 401.4 gives that ordering decision to each card’s owner. The matching set can include opponent-owned cards, so the current implementation lets the spell controller order them and collapses independent owner choices.
The new !up_to parser lowering is separate from this unresolved resolver defect and does not provide the needed targeted-opponent/multi-owner ordering regression.
Split the pending ordering work by owner (or otherwise prompt the proper owner for its cards), and add a runtime regression covering an opponent’s library.
Recommendation: request changes before approval or enqueue.
|
Blocked on current head The previous blocker (17:48, head 🔴 Blocker[HIGH] /// CR 401.4: Group object ids by owner in APNAP order for per-owner library
/// arrangement prompts.
fn group_object_ids_by_owner_apnap(
state: &GameState,
object_ids: &[ObjectId],
) -> Vec<(PlayerId, Vec<ObjectId>)> {
use std::collections::BTreeMap;
let mut by_owner: BTreeMap<PlayerId, Vec<ObjectId>> = BTreeMap::new();
...
by_owner.into_iter().collect()
}
Seat-id order equals APNAP order only when the active player is The engine already owns this. /// CR 101.4: APNAP (Active Player, Non-Active Player) ordering.
pub fn apnap_order(state: &GameState) -> Vec<PlayerId> {
apnap_order_from(state, None, state.active_player)
}and
Suggested fix — same intersect-the-apnap-sequence shape, no new vocabulary: let mut by_owner: HashMap<PlayerId, Vec<ObjectId>> = HashMap::new();
for &id in object_ids {
by_owner.entry(state.objects[&id].owner).or_default().push(id);
}
crate::game::players::apnap_order(state)
.into_iter()
.filter_map(|pid| by_owner.remove(&pid).map(|cards| (pid, cards)))
.collect()[HIGH] That makes this assertion in assert_eq!(*player, PlayerId(0), "APNAP: active player's batch first");It passes identically whether the production code is APNAP-correct or plain seat-ordered. A test that cannot fail on revert of the behavior it names is not coverage for that behavior. Suggested fix: set 🟡 Non-blocking[MED] [LOW] [LOW] Parse blast radius is real but under-covered at the edges. The ✅ Clean
Refuting one external findingCodeRabbit's } else {
// "on the bottom", "on top of", and other library rest piles.
Zone::Library
};
CINot contributor-caused. The only non-green check is Recommendation: fix the two HIGH findings together — they are one change. Replace the |
…6545) CR 101.4: group mass library-order batches via apnap_order instead of seat id, make the multi-owner unit test active on P1's turn, and scope scenario auto-answers to PutAtLibraryPosition only. Co-authored-by: Cursor <cursoragent@cursor.com>
Remove unnecessary usize casts in scenario auto-answer path and bump phase-tauri to 0.35.2 in client/src-tauri/Cargo.lock after the main merge. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Re-reviewed at ✅ The APNAP fix is real, at the right seam
crate::game::players::apnap_order(state)
.into_iter()
.filter_map(|pid| by_owner.remove(&pid).map(|cards| (pid, cards)))
And the test now discriminates, which is what the last round was really asking for. Setting Also verified clean this round:
On the 🟡 One non-blocking item
📋 Housekeeping on the lockfile commit
Recommendation: approve once CI reports green on |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approving on 451908c4. Re-verified live: all required checks green (Rust lint/tests shards, Card data, Frontend, Tauri, WASM), head unchanged since the 23:08Z re-review that cleared the APNAP blocker and its discriminating test. The only red is Contributor trust (Superagent bot flag, not a required check).
The .expect() message accuracy note on change_zone.rs:1758 remains a non-blocking follow-up — not worth another round-trip.
Enqueuing via merge-when-ready.
Resolves the deterministic crates/engine/tests/integration/main.rs mod-line conflict by keeping both test modules in rustfmt-sorted order. Maintainer-side port: main advanced under this PR tonight (phase-rs#6538 phase-rs#6543 phase-rs#6545 phase-rs#6554 phase-rs#6563 phase-rs#6579). The phase-tauri 0.35.1->0.35.2 client/src-tauri/Cargo.lock bump is now a no-op, since main landed the identical bump in phase-rs#6568.
…6498) (phase-rs#6545) * fix(engine): keep Portent exile picks out of the graveyard (phase-rs#6498) Model put-the-rest after ForEachCategory as LastRevealed still in the library, and preserve dynamic X on reveal Digs instead of collapsing to RevealTop{1}. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-6545): remove duplicate PR narrative * fix(engine): resolve Portent's two remainder sets and hand tail (phase-rs#6498) Separate revealed-library rest from exiled-card cleanup via LastRevealed and TrackedSetFiltered(Exiled), emit explicit LastRevealed siblings for dynamic reveal Digs, and lower the hand tail as ChangeZoneAll. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): unblock PR 6545 clippy and dynamic-reveal rest path (phase-rs#6498) Fix clippy if-same-then-else, emit LastRevealed siblings for dynamic reveal Digs before assembly demotion, and add parse coverage for the shared put-the-rest grammar class Matthew requested. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): add runtime coverage for dynamic reveal rest path (phase-rs#6498) Add a discriminating cast-pipeline test for the shared dynamic-reveal + put-the-rest grammar class, stamp library-bottom placement on the explicit LastRevealed sibling, and narrow TrackedSetFiltered mass-move origin defaults. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(test): assert exact library-bottom order in dynamic reveal regression The runtime sibling test for reveal-only Dig + put-the-rest was failing CI because it assumed reveal order rather than the engine's deterministic batch placement order. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): prompt library order for ChangeZoneAll bottom rest (phase-rs#6498) CR 401.4: when multiple cards are placed at the same library position with random_order false, route through EffectZoneChoice instead of a silent batch order. Update the dynamic-reveal sibling regression to submit a non-default bottom tail and verify it sticks. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): keep up-to tracked-set puts on ChangeZone (phase-rs#6498) The TrackedSetFiltered mass-move promotion must not absorb bounded "up to N" picks like "put up to one land discarded this way onto the battlefield". Co-authored-by: Cursor <cursoragent@cursor.com> * fix * fix * fix * fix(engine): use APNAP order for per-owner library prompts (phase-rs#6545) CR 101.4: group mass library-order batches via apnap_order instead of seat id, make the multi-owner unit test active on P1's turn, and scope scenario auto-answers to PutAtLibraryPosition only. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): unblock PR 6545 clippy and Tauri lock sync Remove unnecessary usize casts in scenario auto-answer path and bump phase-tauri to 0.35.2 in client/src-tauri/Cargo.lock after the main merge. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
3 participants
Closes #6498
Summary
Discord report: Portent of Calamity did not let you keep selected revealed cards — they went to the graveyard.
Oracle:
Reveal the top X cards … For each card type, you may exile a card of that type from among them. Put the rest into your graveyard. …Root cause
ChangeZoneAll { Exile → Graveyard, TrackedSet }. AfterForEachCategoryexile, the tracked set is the exiled picks — so selecting a card immediately dumped it to the graveyard.RevealTopdemotion collapsed non-fixed counts (X) to1.Changes
keep_count: 0) instead of collapsing X toRevealTop { count: 1 }.ForEachCategoryexile, "put the rest" emitsChangeZoneAll { Library → Graveyard, LastRevealed }so only non-exiled revealed cards go to the graveyard.CardsRevealedfrom reveal-only Digs into the chain tracked-set path;ForEachCategorypool falls back tolast_revealed_ids.Test plan
cargo test -p engine --lib -- portentcargo test -p engine --test integration -- portentSummary by CodeRabbit