feat(engine): Susan Foreman planeswalk replacement arranges planar deck top#6070
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
matthewevans
left a comment
There was a problem hiding this comment.
Review — changes requested
Verdict: blocker
The new Effect::ArrangePlanarDeckTop and WaitingFor::ArrangePlanarDeckTopChoice variants are not integrated with phase-ai, so the PR cannot compile the workspace.
crates/phase-ai/src/policies/effect_classify.rs:52does not matchArrangePlanarDeckTop.crates/phase-ai/src/policies/redundancy_avoidance.rs:279does not matchArrangePlanarDeckTop.crates/phase-ai/src/search.rs:751does not matchArrangePlanarDeckTopChoice.
Evidence: the required Decision-cost perf gate failed with E0004 for all three exhaustive matches (GitHub Actions run 29555228669, job 87805802188). Please add rules-appropriate AI classification and choice generation/handling, then rerun CI.
Clean: isolated diff review found no unsafe command/network or prompt-injection pattern; the failing contributor-trust comment is advisory only.
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
There was a problem hiding this comment.
Verdict: changes requested — the phase-AI exhaustiveness fix is present, but the new replacement is still restricted to the wrong planeswalk source class.
🔴 Blocker
-
crates/engine/src/game/effects/planeswalk.rs:27-30invokesplanechase::planeswalkdirectly for every non-sentinelEffect::Planeswalk, so none of those events reaches the newReplacementEvent::Planeswalkmatcher. This contradicts Susan Foreman's Oracle text: “If you would planeswalk, instead look at the top two cards of your planar deck, put one on the bottom of your planar deck and the other on top, then planeswalk.” The effect has no “as a result of rolling the planar die” restriction. The same source file labels card-instruction and state-based planeswalks as nonreplaceable, whilecrates/engine/src/game/planechase.rs:426-430and:463-466identify direct phenomenon/SBA planeswalk paths.The new integration test only constructs the synthetic planar-die sentinel, so it cannot fail when a non-sentinel planeswalk bypasses the replacement. Route every planeswalk event through one replacement-aware authority, carrying the applied-replacement state so Susan's trailing “then planeswalk” does not reapply Susan while other eligible replacement effects still compose. Add runtime coverage for a non-sentinel
Effect::Planeswalkand the existing planar-die path.
✅ Clean
2dcc152adds the three previously missingphase-aiexhaustive-match arms, addressing the earlier compile blocker.
Recommendation: request changes and route this through the shared planeswalk/replacement authority before re-review.
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: changes requested — the new head fixes Effect::Planeswalk, but the shared replacement authority still excludes two rules-defined planeswalk causes.
🔴 Blocker
-
crates/engine/src/game/planechase.rs:433(encounter) and:469(check_phenomenon_planeswalk_sba) still callplaneswalk(...)directly.crates/engine/src/game/effects/planeswalk.rs:27-37only consultsplaneswalk_through_replacementsforEffect::Planeswalk; therefore Susan Foreman never applies when a phenomenon is encountered or when its state-based-action planeswalk occurs. The verified Oracle text is: “If you would planeswalk, instead look at the top two cards of your planar deck, put one on the bottom of your planar deck and the other on top, then planeswalk.” It has no planar-die or ability-instruction restriction.docs/MagicCompRules.txt:3574(CR 701.31c) expressly lists the planeswalking ability, phenomenon/SBA paths, leaving-game path, and ability instructions as planeswalk causes;:3054-3056(CR 614.1/614.1a) makes this an event replacement. The currentPlaneswalkCausecan distinguish onlyPlanarDieandInstruction, so the direct paths cannot preserve Fixed Point in Time's narrower scope while applying Susan, and they have no choice/continuation handling for Susan's arrangement prompt.Route every CR 701.31c cause through one replacement-aware planeswalk authority with an explicit non-planar-die cause, preserving the applied-replacement set and correctly pausing/resuming the phenomenon/SBA path. Keep Fixed Point limited to
PlanarDie. Add runtime tests that Susan replaces encounter and the phenomenon SBA, alongside the existing planar-die and card-instruction coverage.
✅ Clean
824540277correctly routes non-sentinelEffect::Planeswalkthrough the replacement pipeline, carries the applied set into Susan's trailing planeswalk, and adds a card-instruction regression test; that resolves the prior review finding but does not cover the direct CR 701.31c callers.
Recommendation: request changes — complete the shared authority for every planeswalk cause, then re-request review.
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: changes requested — #6070 is still rules-incomplete and the current head does not compile the workspace.
🔴 Blocker
-
crates/engine/src/game/planechase.rs:433(encounter) and:469(check_phenomenon_planeswalk_sba) call rawplaneswalk(...), whereascrates/engine/src/game/effects/planeswalk.rs:30-36is the only caller ofplaneswalk_through_replacements. Susan Foreman's verified text is “If you would planeswalk, instead … then planeswalk,” with no planar-die/instruction restriction.docs/MagicCompRules.txt:3574(CR 701.31c) lists phenomenon and state-based paths among planeswalk causes, and CR 614.1a makes this a replacement event. Therefore Susan does not replace those two legal planeswalk causes. Route every CR 701.31c cause through the shared replacement-aware authority, preserve the applied set, and add runtime coverage for encounter and the phenomenon SBA while keeping Fixed Point restricted to planar-die causes. -
CI run
29557534949failed its Rust lint and Rust-tests shards:crates/mtgish-import/src/convert/replacement.rshas nineReplacementDefinitioninitializers missing the new requiredplaneswalk_scopefield (for example lines 156, 222, 599, 737, 988, 1079, 1264, and 1383). Thread the new field through every construction or provide a constructor/default at the type authority; the current head cannot be enqueued.
✅ Clean
- The head correctly changed ordinary
Effect::Planeswalkto callplaneswalk_through_replacementsand retains the applied-replacement state for Susan's chained planeswalk.
Recommendation: request changes — complete the shared replacement authority and all ReplacementDefinition construction paths, add the two runtime regressions, then re-request review.
c434de0 to
944af17
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved: the current head routes planar-die, card-instruction, encounter, and phenomenon-SBA planeswalks through the replacement-aware authority; current runtime coverage exercises each previously blocked path.
Fixes #5820
Summary
Susan Foreman (Doctor Who):
This adds a building-block planeswalk replacement path: look at the top N cards of the planar deck, keep exactly
keep_on_topon top (exact-count choice, unlike scry’s any-subset), then complete the chainedEffect::Planeswalkviapending_continuationafter the player submitsSelectCards.Changes
Engine types & state
Effect::ArrangePlanarDeckTop { count, keep_on_top }— typed arrange primitive for planar deck topsWaitingFor::ArrangePlanarDeckTopChoice { player, cards, keep_on_top }— interactive exact-keep selectionability.rs,game_state.rs,ability_rw.rs,ability_scan.rs,coverage.rs,printed_cards.rs,trigger_index.rs,analysis/ability_graph.rs,oracle_effect/sequence.rs,oracle_ir/doc.rs,scenario.rsResolver & wiring
crates/engine/src/game/effects/arrange_planar_deck.rs— setsArrangePlanarDeckTopChoicewaiting stateengine_resolution_choices.rs—validate_exact_keep_on_top_selection, reorderplanar_deck,finish_with_continuationdrainreplacement.rs—ProposedEvent::Planeswalkin player-scope gate (alongside Draw/Scry)effects/planeswalk.rs— planar-die sentinel routes throughreplace_event; chainedPlaneswalkuses non-sentinel source to avoid infinite replacement loopParser
oracle_replacement.rsparse_would_planeswalk_replacement()—"if you would planeswalk, …"parse_arrange_planar_deck_planeswalk_substitute()→[ArrangePlanarDeckTop(2,1), Planeswalk]would_planeswalk_replacement_parses_susan_foremanAI & frontend
ai_support/candidates.rs—select_cards_variants(..., Some(keep_on_top))for exact keep countphase-ai/decision_kind.rs—ArrangePlanarDeckTopChoicedecision kindclient/src/adapter/types.ts,waitingForRegistry.ts,CardChoiceModal.tsx,libraryModals.tsx—ArrangePlanarDeckTopModalwithexactKeepCountonReorderableTopChoiceTests
crates/engine/tests/integration/issue_5820_susan_foreman.rs— end-to-end: planar-diePlaneswalk→ replacement → arrange pause →SelectCards→ planeswalk to chosen top cardengine_resolution_choicesunit tests — reorder handler + stashedPlaneswalkcontinuation drainarrange_planar_deckunit test — waiting-for setupTest setup note: planar deck objects must be inserted into
objectswithoutadd_to_zone(Zone::Command)— only the active plane belongs incommand_zone. Deck cards live only inplanar_deck(face down), mirroringplanechase_tests::create_planar_object.Test plan
Tilt (when up):
clippy,test-engine,check-frontendafter merge.Notes for reviewers
keep_on_topis enforced byvalidate_exact_keep_on_top_selection(not scry’s any-subset semantics).Effect::Planeswalksub-ability uses Susan’s object source (non-sentinel), so it planeswalks directly after arrange without re-entering the replacement pipeline.planar_ability_sentinel_idplaneswalking ability is replaceable (Fixed Point in Time precedent); card/SBA planeswalks bypass replacement per CR 701.31c discrimination inplaneswalk.rs.