Add deck-contribute workflow (deck triage → mechanic-clustered PRs)#1
Closed
ntindle wants to merge 1026 commits into
Closed
Add deck-contribute workflow (deck triage → mechanic-clustered PRs)#1ntindle wants to merge 1026 commits into
ntindle wants to merge 1026 commits into
Conversation
phase-rs#2772) * Fix Serpent's Soul-Jar cast-from-exile activated ability parsing Strip leading "Once each turn" as an activation restriction, recognize "this artifact" in exile-link cast targets, and allow persistent exile pools without a "this turn" suffix (issue phase-rs#1524). * fix(PR-2772): use nom prefix for once-each-turn activation * fix(PR-2772): update Soul-Jar static assertion --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
… (CR 702.112) (phase-rs#2782) Parameterize TriggerCondition::SourceIsRenowned into IsRenowned { subject: RenownSubject } so that 'if it's renowned' reads the event-subject creature's renowned designation (CR 702.112b) while 'if ~ is renowned' keeps reading the ability's own source (CR 702.112a). Previously both Oracle phrases collapsed onto SourceIsRenowned, which always read the trigger source -- wrong for any payoff that cares about a different (triggering/event) creature's renowned state. The EventSubject arm reuses the existing extract_source_from_event resolver (the same building block that resolves TargetFilter::TriggeringSource), with an .or(source_id) fallback for the SelfRef-shaped case. RenownSubject is a new 2-variant enum (no existing source-vs-event-subject selector to reuse). Updates all consumers: parser table, eval, synthesis (Renown's own intervening-if -> Source x3), and the mtgish-import converter (-> Source). Regenerates the valeron wardens fixture entry for the new serde shape.
…link (phase-rs#2783) Let self-hosters run phase-server for friends without port-forwarding or TLS setup, and surface a reachable join string to the host instead of the logs. Server (phase-server): - Advertise a public URL in ServerHello (additive Option<String>, omitted on the wire when None; lobby/server-core ServerHello stays byte-identical). - Source it from --public-url/PUBLIC_URL (validated at the boundary) or an optional embedded ngrok tunnel behind the `ngrok` cargo feature (NGROK_AUTHTOKEN). The tunnel forwards to the existing local listener via listen_and_forward, so the serve/teardown path is unchanged; a tunnel that fails to start never blocks local boot. Warns when the token is set but the feature was not built. Client: - Decode public_url into ServerInfo; HostControlTile copies a host-type-aware share string: `<code>@<host>` for a server-run host (gated on the advertised publicUrl), the bare code for a P2P host (joined via direct code). Clear stale serverInfo when starting a P2P host so an online publicUrl never leaks into a P2P share. - parseJoinCode honors an explicit ws://|wss:// scheme and scheme-aware default ports; formatJoinShare is its inverse. Block ws:// from an https page (mixed content) with an actionable error instead of a silent failure.
…ition authority (CR 719.3a) (phase-rs#2784) Route Case solve-condition parsing through parse_inner_condition (the single StaticCondition authority) and evaluate via layers::evaluate_condition, so every game-state condition the engine already understands -- life totals, hand size, control counts, event-history, quantity comparisons -- makes a Case auto-solve at the controller's end step. Previously only 'you control no <subtype>' (SolveCondition::ObjectCount) decomposed; every other solve condition fell to inert SolveCondition::Text and the Case never solved, leaving its 'Solved --' ability permanently dead. Adds SolveCondition::Condition { condition: StaticCondition } (parameterized, not per-phrasing siblings), delegates parsing to parse_inner_condition via nom_on_lower, and threads the Case's source_id into evaluate_solve_condition so SelfRef/ source-relative conditions resolve against the Case. Keeps the working ObjectCount arm (the generic combinator does not reproduce its Suspected+subtype filter) and the Text fallback for genuinely unparseable residue.
… gate (phase-rs#1525) (phase-rs#2769) * fix(parser): split Gathering Stone ETB/upkeep trigger and chosen-type gate (phase-rs#1525) * fix(PR-2769): cover chosen permanent gate * fix(parser): satisfy clippy unneeded_struct_pattern in Gathering Stone test Use unit-variant matches for TriggerMode::ChangesZone and TriggerMode::Phase. --------- Co-authored-by: phase-bot <bot@local> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…lf (phase-rs#2764) * fix(engine): prefer chosen targets over ETB event context for StackSpell Aven Interrupter's ETB was exiling itself because resolved_targets consulted the ZoneChanged trigger event before the player-chosen stack spell target. Fixes phase-rs#2351 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-2764): scope chosen target precedence * fix(PR-2764): format Aven integration helper * fix(engine): keep ParentTargetSlot and StackSpell on propagated targets (phase-rs#2351) Always prefer the full propagated target list for ParentTarget, ParentTargetSlot, and StackSpell before the filter-matching gate so Goblin Welder-style slot anaphors and Aven Interrupter ETB exile both resolve correctly. * fix(engine): resolve fanout and event-context targets in resolved_targets (phase-rs#2351) Resolve pure event-context filters before chosen-target precedence, ignore player slots in per-opponent fanout filter matching, and restore the ability.targets fallback so Haytham Kenway exile works alongside Aven Interrupter and ParentTargetSlot regressions. * style(engine): cargo fmt targeting.rs for CI rustfmt gate --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…#2793) Forks inherit a workflow's schedule, workflow_run, and tag-push triggers but not the upstream's secrets, so these jobs fire daily in forks where they can only fail noisily (emailing fork owners) or, where GITHUB_TOKEN suffices, push a server image into the fork owner's own ghcr namespace. Add 'if: github.repository == "phase-rs/phase"' to the choke-point job of each auto-firing workflow; downstream jobs skip via needs propagation: - nightly-release.yml: cut-nightly-release (dispatch-recovery follows) - release.yml: resolve-release-ref (root of the whole graph) - deploy.yml: card-data, build-wasm, build-server-binary roots - refresh-feeds.yml: refresh-feeds - coverage.yml: tarpaulin ci.yml is intentionally left open so fork contributors still get CI.
…rollbar and layout adjustments (phase-rs#2786) - Added a new CSS class for the deck builder shell to improve layout without causing scrollbars. - Updated various components (DeckBuilder, DeckStack, ModalPanelShell) to utilize the thin scrollbar styling for a more modern UI. - Adjusted layout properties in DeckBuilder to ensure proper height and overflow handling across different viewports. - Removed thin scrollbar from PreferencesModal to maintain consistency with other components. Co-authored-by: carlh171112 <carlh171112@gmail.com>
…mitive (CR 702.165a) (phase-rs#2796) * fix(engine): add 'becomes the target of a backup ability' trigger primitive (CR 702.165a) Adds the reusable source-identification primitive so a card like Huge Truck ('Whenever another creature you control becomes the target of a backup ability, ...') can fire when one of its creatures is targeted by a Backup ability specifically. The trigger mode (BecomesTarget), event, and matcher all already exist and fire for triggered-ability target locks (CR 603.3d); the missing pieces were source identification: - AbilityTag::Backup marks the synthesized Backup ETB ability so a stack-source filter can recognize it on the stack (CR 702.165a: Backup is a triggered ability). - TargetFilter::StackAbility is parameterized with an optional keyword tag (CR 113.7a: an ability on the stack exists independently of its source); stack_ability_matches_filter honors it. Existing StackAbility constructions default tag: None (serde-default keeps the {"type":"StackAbility"} wire format). - The parser recognizes 'becomes the target of a backup ability' (+ plural) and wires valid_source = StackAbility { tag: Some(Backup) }. Huge Truck's 'gains all abilities shared this way' payoff (CR 702.165c) is a single-card effect, intentionally scoped OUT as a follow-up; this delivers only the reusable trigger primitive. Covered by parser, matcher, and integration tests. * fix(engine): complete StackAbility tag ripple in parser + louisoix test CI on the prior commit failed to compile: the new tag field on TargetFilter::StackAbility was missing from construction/pattern sites in oracle_nom/target.rs, oracle_static/static_helpers.rs, oracle_static/tests.rs, and the louisoix_sacrifice_counter integration test. Add tag: None to each (pure mechanical ripple from the field addition). * test(engine): fix backup becomes-target test to target ANOTHER creature The positive test had the backup ETB target the payoff creature itself, but the trigger fires on 'ANOTHER creature you control becomes the target' — the source (Huge Truck) targeting itself is excluded, so 0 cards were drawn. Target the Backup Bear instead (a distinct P0 creature), so 'another creature you control' is satisfied and the trigger draws exactly one card. * test(engine): fix backup ETB trigger never firing — name collision + keyword hint The backup-becomes-target integration test could never reach the feature it exercised. Two test-fixture bugs, both fixed here (mirroring the passing Renown scenario test): 1. Name collision with self-reference normalization. The creature was named "Backup Bear". CR 201.4 self-reference normalization rewrites a card's own name in its Oracle text to `~`, so the keyword line "Backup 1" was rewritten to "~ 1" — which parses to no keyword. `synthesize_backup` then saw an empty keyword set, emitted no ETB trigger, nothing was targeted, no BecomesTarget event fired, and the payoff drew 0 cards. Renamed to "Valeron Sentry". (Real Backup cards never embed "Backup" in their name, so production is unaffected.) 2. Missing keyword hint. Bare Oracle inference cannot recognize the space-separated numeric keyword form ("Backup 1") on its own, so `Keyword::Backup(1)` never reached `face.keywords`. Build the creature via `from_oracle_text_with_keywords(&["Backup"], ..)` so the line routes through `parse_keyword_from_oracle` (mirroring how MTGJSON supplies the keyword array in production). With both fixed the full chain runs end-to-end: keyword → synthesized ETB → target-lock → GameEvent::BecomesTarget → StackAbility{tag:Backup} match → draw. Both the positive and negative tests pass locally.
* refactor(engine): route single-authority bypasses through canonical entries
- explore.rs: replace hand-rolled library->hand zone mutation with
zones::move_to_zone (emits ZoneChanged + runs zone-exit cleanup)
- layers: drop the unused evaluate_layers re-export; migrate the two
production direct callers (become_copy, engine_replacement) to
flush_layers / mark_layers_full + flush_layers; doc-reserve direct
evaluate_layers calls for tests
- ability_utils: funnel validate_selected_targets{,_for_ability} count
validation through one shared inner body (targeting.rs pattern)
- engine_resolution_choices: route the two synthetic-ability resolver
bypasses through resolve_ability_chain; doc resolve_effect's contract
- keywords: document the object-scoped vs state-scoped query tiers
* feat(parser): single authority for expressing unparsed text
Effect::unimplemented(name, fragment) is the one way to construct the
Unimplemented effect; name is a stable pattern-class key consumed by
coverage gap categorization. Delete the dead parse_or_unimplemented /
option_to_nom boundary fns (zero callers) and fix the stale CLAUDE.md
oracle_nom/error.rs description.
* ci: extend parser gate + add engine-authority and skill-doc gates
- check-parser-combinators.sh: close .rfind/.split/.splitn blind spots;
new family (E) verbatim-sentence equality (== "25+ chars"); new
family (F) hand-constructed Effect::Unimplemented literals
- check-engine-authorities.sh (new): diff gate forbidding raw
.keywords.contains/.iter queries outside the keyword authorities
- check-skill-doc.sh (new): asserts oracle-parser SKILL.md matches the
parser tree (paths, symbols, priority-table row parity); SKILL.md
regenerated against the code (38-slot priority table, IR layer,
clause_shell, oracle_static/ split, single-authority doctrine)
* refactor(parser): make condition bridges exhaustive over their source enums
static_condition_to_trigger_condition, static_condition_to_ability_condition,
and ability_condition_to_static_condition no longer hide unbridged variants
behind wildcards: adding a StaticCondition/AbilityCondition variant is now a
compile error at each bridge, forcing a conscious bridging decision. All
expanded arms map to None exactly as the wildcards did (no behavior change).
* refactor(engine): cost-payment Phase 0 — sorcery-speed delegation + pay_from_pool rename
Phase 0 of the cost-payment unification plan (.planning/cost-payment-unification):
- casting.rs: CastingProhibitionCondition::NotSorcerySpeed now delegates to
restrictions::is_sorcery_speed_window instead of re-deriving the CR 307.1
timing predicate inline — restrictions.rs is the single timing authority.
- mana_payment.rs: pay_cost → pay_from_pool (pool-level arithmetic only),
freeing the pay_cost name for the future game/costs.rs ability-cost
payment authority. Callers + re-export updated.
* refactor(parser): consolidate three parallel duration grammars into oracle_nom/duration.rs
Single authority for Oracle duration phrases (CR 611.2 / CR 611.2b /
CR 514.2): the prefix-nested nom grammar in oracle_nom/duration.rs now owns
every phrase→Duration mapping. strip_leading_duration /
strip_trailing_duration in oracle_effect/lower.rs are positional-only
wrappers (word-boundary scan + quantity-clause guard; the two phrase tables
are deleted), and the three inline re-encodings in oracle_effect/subject.rs
delegate to the grammar.
Conflict between the old grammars resolved in the strip table's (test-pinned)
favor: 'for as long as you control ~' maps to UntilHostLeavesPlay. That is a
pre-existing imprecision vs CR 611.2b (control loss without leaving the
battlefield should end it) — now visible in exactly one place.
* refactor(parser): replace verbatim sentence clusters with nom single authorities
- 'Your speed can increase beyond 4' (CR 702.179d-e) was string-equality
encoded at three sites (two oracle.rs routing predicates + the semantic
parse in oracle_static/dispatch.rs); now one is_speed_unlock_sentence
combinator in dispatch.rs, called by all three.
- The ChooseFromZone dig continuation 'put the rest on the bottom of your
library [in a random order | in any order]' (CR 401.4) was three full-line
equalities in sequence.rs; now one all_consuming combinator with the order
suffix as an opt(alt(...)) axis.
* refactor(engine): zone-change pipeline Phase A — carve out game/zone_pipeline.rs
Zero-behavior-change carve-out per .planning/zone-change-pipeline/PLAN.md:
execute_zone_move / deliver_replaced_zone_change / apply_zone_delivery_tail
and friends move verbatim from effects/change_zone.rs into the new
game/zone_pipeline.rs module (one mechanical super::→crate:: path fix); a
pub(crate) use shim keeps every existing caller compiling unchanged.
Seeds the Phase B+ vocabulary (not yet consumed, scoped #[allow(dead_code)]):
ZoneMoveRequest + builders, ZoneChangeCause (CR-annotated exemption split),
EntryMods (EtbTapState, not bool), ExileLinkSpec, DeliveryCtx, and the
ApprovedZoneChange proof token (no Serialize/Deserialize/Clone/Default;
private event field; pub(crate) approve_post_replacement mint path).
Independently reviewed: APPROVED. Phase B watch-item: move_object must seed
EtbTapState onto the ProposedEvent directly instead of round-tripping
through execute_zone_move's legacy bool boundary.
On push-to-main the regression check can only do harm: the merge has already happened, and a failing check blocks the R2 baseline republish that runs later in the same job — wedging main red until someone manually uploads a baseline (the recurring PR phase-rs#2339 / PR phase-rs#2802 swallowed-clause ratchet deadlock). continue-on-error on main pushes lets the baseline self-heal after an intentional ratchet absorption, while PR and merge_group runs remain fully blocking.
…ase-rs#2827) * docs(skills): add pr-review-loop continuous PR review sweep skill Orchestration loop that selects open contributor PRs, dispatches an isolated agent to review each via review-impl against the seam/idiom/value lenses, posts one verdict comment per PR, and polls for new PRs and real content commits on an interval until stopped. Runner-agnostic (resolves acting login at runtime), GitHub comment history as the dedup ledger, merge-vs-content commit detection via parent count, and read-only by contract (never checks out or rewrites PRs — that is pr-contribution-handler). * docs(skills): tighten pr-review-loop selection + dedup per review - fold author!=ACTING_LOGIN into the candidate jq so the number list is pre-filtered (was prose-only) - add a concrete defer_to skip snippet checking both comments and reviews - dedup ledger now takes max timestamp across comments AND formal reviews (runner-agnostic: a human runner may leave a formal review, not a comment)
…se-rs#2824) * refactor(engine): route single-authority bypasses through canonical entries - explore.rs: replace hand-rolled library->hand zone mutation with zones::move_to_zone (emits ZoneChanged + runs zone-exit cleanup) - layers: drop the unused evaluate_layers re-export; migrate the two production direct callers (become_copy, engine_replacement) to flush_layers / mark_layers_full + flush_layers; doc-reserve direct evaluate_layers calls for tests - ability_utils: funnel validate_selected_targets{,_for_ability} count validation through one shared inner body (targeting.rs pattern) - engine_resolution_choices: route the two synthetic-ability resolver bypasses through resolve_ability_chain; doc resolve_effect's contract - keywords: document the object-scoped vs state-scoped query tiers * feat(parser): single authority for expressing unparsed text Effect::unimplemented(name, fragment) is the one way to construct the Unimplemented effect; name is a stable pattern-class key consumed by coverage gap categorization. Delete the dead parse_or_unimplemented / option_to_nom boundary fns (zero callers) and fix the stale CLAUDE.md oracle_nom/error.rs description. * ci: extend parser gate + add engine-authority and skill-doc gates - check-parser-combinators.sh: close .rfind/.split/.splitn blind spots; new family (E) verbatim-sentence equality (== "25+ chars"); new family (F) hand-constructed Effect::Unimplemented literals - check-engine-authorities.sh (new): diff gate forbidding raw .keywords.contains/.iter queries outside the keyword authorities - check-skill-doc.sh (new): asserts oracle-parser SKILL.md matches the parser tree (paths, symbols, priority-table row parity); SKILL.md regenerated against the code (38-slot priority table, IR layer, clause_shell, oracle_static/ split, single-authority doctrine) * refactor(parser): make condition bridges exhaustive over their source enums static_condition_to_trigger_condition, static_condition_to_ability_condition, and ability_condition_to_static_condition no longer hide unbridged variants behind wildcards: adding a StaticCondition/AbilityCondition variant is now a compile error at each bridge, forcing a conscious bridging decision. All expanded arms map to None exactly as the wildcards did (no behavior change). * refactor(engine): cost-payment Phase 0 — sorcery-speed delegation + pay_from_pool rename Phase 0 of the cost-payment unification plan (.planning/cost-payment-unification): - casting.rs: CastingProhibitionCondition::NotSorcerySpeed now delegates to restrictions::is_sorcery_speed_window instead of re-deriving the CR 307.1 timing predicate inline — restrictions.rs is the single timing authority. - mana_payment.rs: pay_cost → pay_from_pool (pool-level arithmetic only), freeing the pay_cost name for the future game/costs.rs ability-cost payment authority. Callers + re-export updated. * refactor(parser): consolidate three parallel duration grammars into oracle_nom/duration.rs Single authority for Oracle duration phrases (CR 611.2 / CR 611.2b / CR 514.2): the prefix-nested nom grammar in oracle_nom/duration.rs now owns every phrase→Duration mapping. strip_leading_duration / strip_trailing_duration in oracle_effect/lower.rs are positional-only wrappers (word-boundary scan + quantity-clause guard; the two phrase tables are deleted), and the three inline re-encodings in oracle_effect/subject.rs delegate to the grammar. Conflict between the old grammars resolved in the strip table's (test-pinned) favor: 'for as long as you control ~' maps to UntilHostLeavesPlay. That is a pre-existing imprecision vs CR 611.2b (control loss without leaving the battlefield should end it) — now visible in exactly one place. * refactor(parser): replace verbatim sentence clusters with nom single authorities - 'Your speed can increase beyond 4' (CR 702.179d-e) was string-equality encoded at three sites (two oracle.rs routing predicates + the semantic parse in oracle_static/dispatch.rs); now one is_speed_unlock_sentence combinator in dispatch.rs, called by all three. - The ChooseFromZone dig continuation 'put the rest on the bottom of your library [in a random order | in any order]' (CR 401.4) was three full-line equalities in sequence.rs; now one all_consuming combinator with the order suffix as an opt(alt(...)) axis. * refactor(engine): zone-change pipeline Phase A — carve out game/zone_pipeline.rs Zero-behavior-change carve-out per .planning/zone-change-pipeline/PLAN.md: execute_zone_move / deliver_replaced_zone_change / apply_zone_delivery_tail and friends move verbatim from effects/change_zone.rs into the new game/zone_pipeline.rs module (one mechanical super::→crate:: path fix); a pub(crate) use shim keeps every existing caller compiling unchanged. Seeds the Phase B+ vocabulary (not yet consumed, scoped #[allow(dead_code)]): ZoneMoveRequest + builders, ZoneChangeCause (CR-annotated exemption split), EntryMods (EtbTapState, not bool), ExileLinkSpec, DeliveryCtx, and the ApprovedZoneChange proof token (no Serialize/Deserialize/Clone/Default; private event field; pub(crate) approve_post_replacement mint path). Independently reviewed: APPROVED. Phase B watch-item: move_object must seed EtbTapState onto the ProposedEvent directly instead of round-tripping through execute_zone_move's legacy bool boundary. * refactor(types): unify Effect::AddCounter into Effect::PutCounter (CR 122.1) Effect::AddCounter and Effect::PutCounter expressed the same operation (place counters on an object) through two variants, forcing every match site to handle both — and several AI policy sites silently handled only one (cast_facts, redundancy_avoidance had AddCounter-only legs with PutCounter receiving different treatment). One variant, one meaning: PutCounter is the single authority, with #[serde(alias = "AddCounter")] accepting legacy serialized data. All ~70 match/construction sites across engine, phase-ai, and mtgish-import migrated; duplicate arms produced by the rename were collapsed (compiler-verified via -D unreachable-patterns). Engine inventory regenerated; the two remaining AddCounter entries are unrelated enums (ReplacementEvent / game event). * refactor(parser): rewrite parse_restriction_modes as negation x verb-phrase list grammar Replace ~14 verbatim equality clusters ('can't attack or block', 'can't block or be blocked', ...) with a composed nom grammar: alt(("can't", "cannot")) + separated_list1 over restriction atoms (attack / block / be blocked / be sacrificed / be enchanted / be equipped / be countered / transform / crew). Any combination of atoms now parses — including orders no current card uses — instead of only the enumerated permutations. Elided-'be' English ellipsis ('can't be equipped or enchanted') handled via a bare-atom fallback inside the list parser. Also freezes oracle_quantity.rs for new grammar: new quantity recognition belongs in oracle_nom/quantity.rs (module doc). * refactor(parser): delete zero-caller parse_target_phrase wrapper oracle_target::parse_target is the single authority for 'target <type phrase>' extraction; the unused nom wrapper was a second grammar waiting to drift. Its tests retargeted to parse_type_phrase, the building block production actually calls. * ci: make coverage ratchet informational on push-to-main On push-to-main the regression check can only do harm: the merge has already happened, and a failing check blocks the R2 baseline republish that runs later in the same job — wedging main red until someone manually uploads a baseline (the recurring PR #2339 / PR #2802 swallowed-clause ratchet deadlock). continue-on-error on main pushes lets the baseline self-heal after an intentional ratchet absorption, while PR and merge_group runs remain fully blocking. * refactor(types): replace sorcery_speed bool with ActivationRestriction::AsSorcery single authority CR 602.5d: 'Activate only as a sorcery' timing is now represented solely by ActivationRestriction::AsSorcery in AbilityDefinition.activation_restrictions. The parallel sorcery_speed display bool is deleted; is_sorcery_speed() queries the restriction. A hand-written Deserialize (AbilityDefinitionDe mirror) tolerates the legacy serialized field and migrates sorcery_speed:true into AsSorcery (dedup). Serialized exports no longer emit the key; snapshots and the integration card fixture follow. * refactor(types): fold ActivationCadence into ActivationRestriction (CR 602.5b) CR 602.5b: once-each-turn activation cadence on keyword actions (Crew) is now expressed with the existing ActivationRestriction::OnlyOnceEachTurn instead of the parallel ActivationCadence enum, which is deleted. Keyword::Crew's once_per_turn becomes Option<Box<ActivationRestriction>> (None = unrestricted; boxed to break the Keyword -> ActivationRestriction -> ParsedCondition -> Keyword size cycle). The custom Crew deserializer accepts both the legacy ActivationCadence tagged shape ({"type":"Unlimited"}/{"type":"OncePerTurn"}) and the new Option<ActivationRestriction> shape. Regenerated integration card fixture carries the new export shape (and drops the legacy sorcery_speed key). * docs(engine): fix stale sorcery_speed display-flag comment (review follow-up) * refactor(engine): seed three-state EtbTapState directly onto ZoneChange proposal (Phase B watch-item) execute_zone_move took `effect_enter_tapped: bool` and could only ever set EtbTapState::Tapped, collapsing the Unspecified-vs-Untapped distinction the pipeline carrier (ProposedEvent::ZoneChange.enter_tapped: EtbTapState) exists to preserve. move_object further round-tripped its req.mods.enter_tapped (EtbTapState) through .is_tapped() before passing it in. Thread EtbTapState end-to-end: execute_zone_move now accepts EtbTapState and seeds it onto the proposal whenever it is not Unspecified (CR 614.1). All callers that already hold an EtbTapState (change_zone resolver paths, seek) pass it through unchanged; bool-literal callers pass EtbTapState::Unspecified. move_object passes req.mods.enter_tapped directly. No bool round-trip remains. This is the recorded Phase A review condition for proceeding with Phase B. * refactor(engine): route destroy.rs delivery through zone pipeline proof token (Phase B) apply_destroy_after_replacement delivered its inner (post-replacement) ZoneChange with a bare zones::move_to_zone in both arms (the post-Destroy inner move and the outer-replacement-redirected-to-ZoneChange move). A destruction redirected to the battlefield (CR 614.6) therefore dropped the entire delivery tail: CR 614.1c enters-with-additional-counter statics, enter_tapped / enter_with_counters, exile-link tracking, and the post-replacement-continuation drain. Seal each already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement (preserving its `applied` set and re-validating it is a ZoneChange) and deliver through zone_pipeline::deliver — the single delivery tail. A NeedsChoice from the tail propagates as `false` (state.waiting_for already set) so the caller does not advance. Adds a discriminating integration test (destroy_redirect_to_battlefield_delivery_tail) that casts "Destroy target creature" at a victim with a Moved->Battlefield redirect under a CR 614.1c additional-+1/+1-counter static, and asserts the redirected creature receives the counter — fails on the old raw move (0 counters), passes through the tail. CR 701.8a (line 3315), CR 614 / 614.6 (line 3072), CR 614.1c (line 3056). * refactor(engine): route sacrifice.rs delivery through zone pipeline proof token (Phase B) apply_sacrifice_after_replacement delivered its inner (post-replacement) graveyard ZoneChange — and the outer-redirect ZoneChange — with a bare zones::move_to_zone. A sacrifice redirected to the battlefield (CR 614.6) therefore dropped the delivery tail (CR 614.1c enters-with-additional-counter statics, enter_tapped / enter_with_counters, post-replacement-continuation). Seal each already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement and deliver through zone_pipeline::deliver. The Sacrifice proposal carries no source, so no exile-link context is attributed. A NeedsChoice from the tail returns SacrificeApply::NeedsChoice (state.waiting_for already set) so the caller pauses. Adds a discriminating test (sacrifice_redirected_to_battlefield_applies_enters_with_counters_tail) driving the real sacrifice_permanent pipeline with a Moved->Battlefield redirect under a CR 614.1c additional-+1/+1-counter static — fails on the old raw move (0 counters), passes through the tail. CR 701.21a (line 3445), CR 614 / 614.6 (line 3072), CR 614.1c (line 3056). * refactor(engine): route sba.rs lethal-damage delivery through zone pipeline proof token (Phase B) The lethal-damage state-based-action destruction loop (check_lethal_damage) delivered its inner (post-replacement) ZoneChange with a bare zones::move_to_zone. A lethal-damage death redirected to the battlefield (CR 614.6, Rest in Peace / "would die -> return" class) therefore dropped the delivery tail (CR 614.1c enters-with-additional-counter statics, enter_tapped / enter_with_counters, post-replacement-continuation). Seal the already-replaced ZoneChange via ApprovedZoneChange::approve_post_replacement (source carried from the Destroy event) and deliver through zone_pipeline::deliver. Per CR 704.3, completing all SBAs may require a replacement choice surfaced by the delivery tail (CR 614.12a Devour as-enters); the NeedsChoice arm pauses exactly as the existing regeneration NeedsChoice arm does (state.waiting_for already set by the tail). Adds a discriminating test (sba_lethal_damage_redirected_to_battlefield_applies_enters_with_counters_tail) driving check_lethal_damage directly with a Moved->Battlefield redirect under a CR 614.1c additional-+1/+1-counter static — fails on the old raw move (0 counters), passes through the tail (exactly 1). CR 701.19b (line 3426), CR 704.3 (line 5457), CR 704.5g (line 5476), CR 614 / 614.6 (line 3072), CR 614.1c (line 3056), CR 614.12a (line 3099). * docs(engine): correct SBA delivery CR annotation to 704.5g + 614.6 (review follow-up) 701.19b is regeneration-via-static; the delivered event here is a lethal-damage destruction (CR 704.5g) whose redirect is a replacement (CR 614.6). * refactor(engine): hoist token + B->B no-op guards into move_object preflight (Phase C0a, Risk #11) The unified pipeline runs replace_event ahead of zones::move_to_zone's delivery-time guards. A one-shot replacement could therefore be consumed (last_effect_count, CR 616.1 choices) on a move the primitive then rejects as a no-op. Hoist the two cheap, side-effect-free object-level guards (CR 111.8 token-outside-battlefield cease-to-exist, CR 603.2g + CR 603.6a Battlefield->Battlefield no-op) into move_object's preflight, before the execute_zone_move replacement consult. Behavior-neutral in Phase C0a (move_object has no production callers yet); the guards take effect as the bucket-B effect sites migrate in C1+. CR 111.8 (line 663), CR 603.2g (line 2573), CR 603.6a (line 2595). * fix(engine): clear damage on lethal-damage death redirected to battlefield (Phase C0b, CR 614.5) A lethal-damage SBA destruction redirected by a Moved replacement back to the battlefield delivers a Battlefield->Battlefield ZoneChange. zones::move_to_zone's CR 603.2g no-op guard returns before reset_for_battlefield_entry, so the creature keeps its marked damage. The SBA fixpoint then re-derives lethal damage and re-fires the destruction-replacement on every iteration (counter / event stacking, capped at MAX_SBA_ITERATIONS=9) — a pre-existing CR 614.5 violation (a replacement gets only one opportunity), surfaced visibly by the Phase B delivery-tail routing because the enters-with-additional-counter static now applies on each re-entry. Root cause: the no-op guard is correct for a spurious self-loop (Coiling Oracle) but the redirected death is a genuine leave-and-re-enter — a new object per CR 400.7 whose damage is gone. After delivering the redirect, clear the marked damage the no-op delivery left behind so the fixpoint sees no lethal damage and the one-shot replacement is not re-applied. Adds a discriminating test (sba_lethal_damage_redirect_to_battlefield_applies_counter_exactly_once) driving check_state_based_actions with a Moved->Battlefield redirect under a CR 614.1c additional-+1/+1-counter static: pre-fix the creature ends with Some(9) counters (9 re-fires); post-fix exactly Some(1). CR 614.5 (line 3069), CR 400.7 (line 1948), CR 603.2g (line 2573), CR 704.5g. * fix(engine): route mill delivery through zone pipeline so Moved redirects fire (Phase C1) mill::apply_mill_after_replacement delivered each milled card with a bare zones::move_to_zone, which never proposed a per-card ZoneChange. Moved-level redirects ("if a card would be put into a graveyard from anywhere, exile it instead" — Rest in Peace / Leyline of the Void class) were therefore silently dropped for milled cards: a milled card reached the graveyard even with Rest in Peace in play (PLAN §8 Risk #1; confirmed bug). Route each milled card through zone_pipeline::move_object, which proposes the inner ZoneChange and consults the Moved replacements before delivery. The milled card itself anchors the Effect cause (mill to a graveyard creates no exile-link and a Moved replacement's valid_card is evaluated against the moved card, so this matches the pre-pipeline raw attribution while enabling the consult). A per-card NeedsChoice (CR 616.1 multi-replacement race on one milled card) is parked (state.waiting_for already set) and stops the batch; no real card produces this on a library->graveyard mill, so the resumable batch-continuation (PLAN §5a / Risk #10) is deferred. Discriminating test asserts non-interactivity via the mandatory RIP redirect. Adds mill_honors_rest_in_peace_graveyard_to_exile_redirect: drives the real Mill pipeline with a global graveyard->exile Moved replacement and asserts milled cards land in EXILE (graveyard empty) — fails on the old raw move (graveyard). Also removes the now-live #[allow(dead_code)] on zone_pipeline::move_object. CR 701.17a (line 3402), CR 614.6 (line 3072), CR 616.1 (line 3169). * fix(engine): resume mill batch across per-card replacement choices instead of stranding (Phase C1 review fix) The C1 mill migration bailed with 'return Ok(())' when a per-card Moved replacement surfaced a choice, on the false claim that no real card produces one: the CR 616.1 materiality classifier treats ANY destination-redirecting Effect::ChangeZone as Unconditional-material, so two simultaneously-applicable graveyard->exile redirects (Rest in Peace + Leyline of the Void — a real, common combination) prompt for ordering on EVERY milled card. Pre-fix the first card's prompt never even surfaced (the pause set pending_replacement but no caller parked waiting_for) and cards 2..N stranded in the library. Implement the PLAN §5a batch continuation (option a): - mill's per-card delivery loop now parks the prompt via replacement::park_waiting_for and stashes the undelivered tail in the new state.pending_mill_deliveries (PendingMillDeliveries { remaining, destination }). - handle_replacement_choice drains the parked tail after the chosen event delivers (Execute arm, following the established pending-queue drain pattern; Prevented arm analogous) via mill::drain_pending_mill_deliveries, re-parking when the next card surfaces its own prompt. Discriminating test mill_under_two_graveyard_redirects_delivers_every_card_through_ordering_choices: mills 3 cards under RIP + Leyline, answers each CR 616.1 ordering prompt, and asserts ALL milled cards leave the library and end in exile with the tail fully drained. Pre-fix it fails (no prompt surfaced; cards stranded). CR 701.17a (line 3402), CR 616.1 (line 3169), CR 614.6 (line 3072). * docs(engine): scope C0b damage scrub as degenerate self-redirect guard, not CR 400.7 re-entry (review fix) The C0b comment justified clearing damage_marked via the CR 400.7 new-object rule, contradicting the chosen delivery: the Battlefield->Battlefield no-op means reset_for_battlefield_entry never ran, so incarnation epoch, summoning sickness, counters, and entered_battlefield_turn are all stale — claiming a new object while delivering a stale one is inconsistent. Reword to the narrow reading: a 'remains on the battlefield instead of dying' replacement is regeneration-shaped — CR 701.19a/b replaces destruction with 'remove all damage marked on it' while the permanent STAYS the same object — so the two-field damage scrub matches that semantics without claiming a new-object re-entry. Add a TODO at the site recording the staleness (and the delivery tail's CR 614.1c counter re-application) as the open question for when a real would-die->battlefield redirect card class appears: no card currently parses to one (parser builds die->exile / shuffle-back; Persist/Undying are dies-triggers), so the semantics decision is deliberately not baked in on zero real cards. Test doc header updated to match. Comment-only change; behavior unchanged (test still passes: exactly one CR 614.1c counter per SBA fixpoint). CR 701.19a/b (lines 3424/3426), CR 603.2g (line 2573). * docs(engine): correct mill NeedsAuraAttachmentChoice stash comment (round-2 review P2) The stash comment claimed reaching the aura-attachment-choice arm 'fails loudly (undrained tail)'. It does not: an aura-host choice surfaces as WaitingFor::ReturnAsAuraTarget, not the replacement-choice path, so drain_pending_mill_deliveries (only invoked from handle_replacement_choice) never fires for it. A stale pending_mill_deliveries would instead be silently drained by the NEXT unrelated replacement-choice resume. The arm is dead code for every parsed Mill today (Battlefield is not a Mill destination); document the real behavior so a future battlefield-mill variant is understood as a bug to surface rather than a self-healing path. CR 303.4f (line 822). * fix(engine): propagate mill per-card pause through apply_mill_after_replacement (round-2 review P1) The nested Mill-event resume path clobbered a per-card mill park. When a Mill-event replacement (e.g. a mill-doubler) resolves through a CR 616.1 ordering choice, handle_replacement_choice's Mill arm applied the accepted event with 'let _ = apply_mill_after_replacement(...)', discarding the helper's pause signal, then unconditionally reset state.waiting_for to Priority (~:392). If deliver_mill_cards parked a per-card prompt inside that arm (two simultaneously-applicable graveyard->exile redirects make every milled card prompt for CR 616.1 ordering), the reset stranded the first paused card. Plumb deliver_mill_cards's existing pause bool out through apply_mill_after_replacement (now returns Result<bool, EffectError>; true = fully delivered, false = parked) and early-return from the Mill arm on a pause, mirroring the apply_etb_counters early-return precedent. EffectError has no EngineError conversion at that arm, so the error is mapped to 'delivered' (preserving the prior let _ swallow) and only the pause is acted on. mill::resolve also bails before EffectResolved on a per-card pause so it doesn't emit a resolution event over a parked prompt. Reachable only with two simultaneous Mill-event replacements that both prompt, which no parsed card produces, so a full runtime repro is impossible. The unit test apply_mill_after_replacement_reports_per_card_pause_to_caller drives the shared seam directly: under two graveyard->exile redirects the first milled card surfaces a CR 616.1 prompt, asserting the helper returns false, leaves waiting_for set to that prompt, and parks the tail — the exact contract the Mill arm's early-return now depends on. CR 701.17a (line 3402), CR 616.1 (line 3169), CR 614.6 (line 3072). * docs(engine): confirm rad-counter mill read is park-safe (round-2 review P3) rad_counters assesses CR 728.1 life loss from library_before (a pre-mill snapshot) immediately after apply_mill_after_replacement. Document that this read remains correct even if the mill parks mid-batch on a per-card CR 616.1 ordering choice: milled_ids is fixed by the post-replacement count against the snapshot (identifying the correct top-N cards regardless of delivery routing), and the per-card life-loss loop reads card type from state.objects, which is zone-independent. The parked tail is delivered by the resume path; the snapshot predates any delivery. CR 728.1 (line 6239). * fix(engine): route countered spell to graveyard/exile through zone pipeline (Phase C3) counter::resolve and resolve_all moved a countered spell off the stack with a raw zones::move_to_zone, which never proposed a per-card ZoneChange. Moved-level graveyard redirects (Rest in Peace / Leyline of the Void: 'if a card would be put into a graveyard from anywhere, exile it instead') were therefore silently dropped for countered spells: a countered spell reached the graveyard even with Rest in Peace in play (PLAN §8 Risk #3 — graveyard-redirect class, confirmed bug). Route the stack -> graveyard/exile move through zone_pipeline::move_object so the inner ZoneChange is proposed and Moved replacements are consulted before delivery. The exile-on-counter destination (CR 702.34a/127a/180a Flashback / Aftermath / Harmonize) is a static destination rule, not a replacement, so it is still selected before the consult. SpellCountered now fires immediately on stack removal (the counter itself) rather than after the consequent move, so a CR 616.1 ordering pause during delivery does not drop it. A single applicable redirect never prompts; only two simultaneous redirects produce a CR 616.1 choice — that mass multi-card continuation is the Phase C4 class (bail on pause; no parsed card combines mass counter with a double graveyard redirect today). Discriminating test countered_spell_honors_rest_in_peace_graveyard_to_exile_redirect: counters a spell with a global graveyard->exile Moved redirect on the battlefield and asserts the spell ends in EXILE (graveyard empty). FAILS on the pre-C3 raw move (spell reaches the graveyard, redirect dropped). CR 608.2b (line 2807), CR 614.6 (line 3072), CR 616.1 (line 3169), CR 701.6a (line 3309). * fix(engine): route mass bounce through zone pipeline + generalize batch continuation (Phase C4) bounce::resolve_all delivered each mass-bounced permanent with a raw zones::move_to_zone under a comment claiming 'no replacement-pipeline detour is needed because mass-bounce events are not destruction events (CR 614.6 doesn't apply here)'. That justification was wrong by citation: CR 614.6 governs replacement semantics generally, and CR 614.1 replacements watch zone-change *events*, not only destruction. The raw move never proposed a per-object ZoneChange, so Moved redirects watching the bounce destination ('if a permanent would be returned to a hand, exile it instead' class) silently never fired (PLAN §8 Risk #4). The single-target bounce raw moves (battlefield/graveyard/ stack -> destination) had the same gap. Route every bounce move through the pipeline. mass bounce uses a new shared batch entry zone_pipeline::move_objects_simultaneously; the single-target and non-targeted-single paths use move_object. The stale 614.6 comment is deleted. Generalize the Phase C1 mill continuation rather than add a second parallel struct (CLAUDE.md parameterize-don't-proliferate applies to state types too): - PendingMillDeliveries -> PendingBatchDeliveries (identical { remaining, destination } shape; serialized as a plain struct so the type rename is wire-transparent; the GameState field pending_mill_deliveries -> pending_batch_deliveries carries a serde field-name alias for save compat). - New zone_pipeline::move_objects_simultaneously runs each request through move_object, parks + stashes the undelivered tail on a CR 616.1 pause, and stamps CR 603.10a co-departure over the departed subset on completion (a no-op for non-battlefield origins, so mill reuses it unchanged). - zone_pipeline::drain_pending_batch_deliveries replaces mill::drain_pending_mill_deliveries; mill::apply_mill_after_replacement now delegates to the shared batch entry, deleting its bespoke deliver_mill_cards. - engine_replacement.rs drains pending_batch_deliveries from both the Execute and Prevented resume arms. A single applicable redirect never prompts (the realistic path), so the common mass bounce never pauses. Only two simultaneous redirects on one object split a batch; the co-departure stamp is then per delivered segment rather than threaded across the pause boundary (no parsed card hits this — documented). Two discriminating tests (bounce_destination_redirect.rs): - mass_bounce_honors_to_hand_redirect: a single to-hand -> exile redirect sends every mass-bounced creature to EXILE, not the hand. FAILS on the old raw move. - mass_bounce_under_two_redirects_delivers_every_permanent_through_choices: two simultaneous redirects make every bounced creature prompt a CR 616.1 ordering choice; answering each delivers ALL of them (none stranded) and fully drains the parked batch tail. CR 614.6 (line 3072), CR 616.1 (line 3169), CR 603.10a (line 2634), CR 400.7 (line 1948). * fix(engine): centralize replacement-choice park in move_object so paused single moves can't ghost (round-3 review Fix 1+2) zone_pipeline::move_object did not park state.waiting_for on NeedsChoice: replace_event sets only pending_replacement, and the wait-state was each caller's to set. The C3/C4 single-move migrations (counter.rs x2, bounce.rs x3) bailed with 'return Ok(())' under comments falsely claiming the pipeline parks. Under two simultaneously-applicable graveyard->exile redirects (Rest in Peace + Leyline of the Void, or two RIP copies — RIP is not legendary), a countered spell was: removed from the stack, SpellCountered emitted, its move parked in pending_replacement, and the prompt NEVER surfaced (the engine gates ChooseReplacement on the wait state) — permanently ghosting the spell (off state.stack but obj.zone == Stack). Centralize the park at the single unparked origin: execute_zone_move's replace_event NeedsChoice arm now calls replacement::park_waiting_for before returning, making counter.rs, bounce.rs, seek.rs (latent same-class ghost — it discards the result entirely), and every future C5-C9 single-move migration safe by construction. Idempotence verified: - park_waiting_for keeps the CR 614.12a devour guard (never clobbers an already-surfaced EffectZoneChoice) and recomputes the identical ReplacementChoice from the same pending_replacement, so callers that still self-park (change_zone's park_waiting_for arms; end_phase / exile_from_top_until's replacement_choice_waiting_for) double-set the same value. - sba.rs's self-parks are on its OWN replace_event calls (Destroy / inner ZoneChange proposals), not downstream of move_object — unaffected today; redundant-but-idempotent when C7 migrates it. - deliver_batch's explicit park is removed (now redundant); the delivery-tail NeedsChoice path is deliberately NOT parked here — its wait state is already set by the counter-pause/devour machinery. The false 'parks via the pipeline' comments are now true and updated to credit the centralized park; two stale CR 608.2b citations corrected to CR 701.6a. Discriminating test countered_spell_under_two_redirects_surfaces_prompt_and_exiles (fail-first verified): counters a spell under RIP + Leyline, asserts the CR 616.1 ordering prompt SURFACES, answers it via a real GameAction::ChooseReplacement dispatch, and asserts the spell ends in EXILE with no ghost (zone == Exile, exile container holds it, graveyard empty, pending_replacement clear). Pre-fix run: FAIL with 'waiting_for = Priority { player: PlayerId(0) }, spell zone = Stack' — the exact unparked-pause ghost. CR 616.1 (line 3169), CR 614.6 (line 3072), CR 701.6a (line 3301). * fix(engine): batch-continuation comment accuracy + rad-counter park bail + discard delivery re-bucket (round-3 review Fix 3) (a) deliver_batch's aura-arm comment claimed a stale tail 'surfaces a bug' (undrained). It does not: the engine_replacement drain gate keys on pending_batch_deliveries.is_some(), not provenance, so a stale tail would be silently drained by the NEXT unrelated replacement-choice resume — the same mischaracterization the P2 review fix corrected in mill.rs. State the real behavior. (b) The mark_simultaneous_departures no-op-for-mill reasoning was wrong: departed_subset (zones.rs:609) DOES include milled cards — it filters on current zone != Battlefield, and a card now in a graveyard passes. The actual no-op comes from the EVENT gate: mark_simultaneous_departures (zones.rs:580) only stamps ZoneChanged events with from: Some(Zone::Battlefield), and a library-origin move emits none. Both comments corrected. (c) rad_counters now bails on a parked mill (apply_mill_after_replacement -> false) like mill::resolve does: continuing into the CR 728.1 life-loss loop would propose LifeLoss replacement events while the parked CR 616.1 choice is pending and could overwrite pending_replacement, ghosting the paused milled card. The life-loss/rad-removal tail is dropped on that parked path (reachable only with two simultaneous graveyard redirects active while rad counters trigger) — accepted and documented rather than threaded through the batch resume. (d) discard.rs complete_discard_to_graveyard re-bucketed with a comment: it is a Bucket A post-replacement DELIVERY site (eventual Phase E shape: approve_post_replacement + deliver on the lowered ZoneChange carrying applied), not a move_object site — re-proposing would double-apply Discard-level definitions. Deferred from this round because deliver's tail drains post_replacement_continuation at delivery time with Moved context, and timing equivalence for the Discard-execute chain class (madness / Abundance) needs its own discriminating tests. Audit correction recorded at the site: the discard_applier Discard->ZoneChange lowering runs only when a Discard-level definition (madness class) applies — moved_matcher accepts only ZoneChange proposals — so a PLAIN discard never consults Moved redirects and Rest in Peace does not exile it today. That graveyard-redirect gap (same class as mill C1 / counter C3 / bounce C4) needs the inner-ZoneChange-with-applied lowering plus a discriminating RIP-on-discard test as its own commit. CR 616.1 (line 3169), CR 728.1 (line 6239), CR 603.10a (line 2634), CR 701.9a (line 3334 area — see docs), CR 303.4f (line 1652). * fix(engine): plain discard consults Moved redirects (CR 614.6 / 701.9a) A plain discard moved hand -> graveyard via raw `move_to_zone`, never consulting `Moved` replacements, so Rest in Peace / Leyline of the Void did not exile a discarded card. Lower the accepted Discard into an inner hand -> graveyard `ZoneChange` carrying the outer pass's `applied` set and run it through the replacement pipeline, mirroring `discard_applier`'s lowering. The `applied` set guards against re-running Discard-level (madness) definitions; the lowered ZoneChange already re-loops through the pipeline (CR 616.1f) for the madness path, so only the unmodified Execute(Discard) arm needed the fix. - complete_discard_to_graveyard now takes (source_id, applied), re-proposes through replace_event, and returns DiscardOutcome so a Moved-redirect CR 616.1 choice can propagate. - Mayhem marker (CR 702.187b, record_card_discarded) stamped only when the card actually reaches the graveyard (CR 701.9c) — redirects leave it elsewhere, matching the Madness -> exile path. - All four callers (resolve specific/non-specific, discard_as_cost, handle_replacement_choice resume, ward-discard) updated to surface the pause. CR grep (docs/MagicCompRules.txt): 701.9a To discard a card, move it from its owner's hand to that player's graveyard. 701.9c If a card is discarded, but an effect causes it to be put into a hidden zone instead ... 614.6. If an event is replaced, it never happens. A modified event occurs instead ... Discriminating tests (effects/discard.rs): plain_discard_consults_rest_in_peace_and_exiles — RIP on board, plain discard ends in exile not graveyard (failed on the old raw-move path). madness_class_discard_still_works_without_double_consult — madness redirect to exile fires exactly once (no double-consult). * fix(engine): seek bail-on-pause + route non-battlefield move through pipeline (CR 616.1 / 614.6) Seek ignored `execute_zone_move`'s result and always pushed `EffectResolved`, so a per-card replacement pause was clobbered, and a multi-card seek with consecutive pausing replacements overwrote `pending_replacement` — ghosting earlier picks. It also moved non-battlefield destinations via raw `zones::move_to_zone`, never proposing a per-card ZoneChange (C9 entry), so `Moved` redirects never fired. Replace the per-card loop with the shared batch entry `zone_pipeline::move_objects_simultaneously` (mill::resolve pattern): both destinations now route through the pipeline, attribution stays `ability.source_id` (so battlefield entries record `entered_via_ability_source` and exile-link tracking keys off the seek source), and a mid-batch CR 616.1 pause parks `state.waiting_for` + stashes the tail in `pending_batch_deliveries`. Bail before `EffectResolved` on NeedsChoice so the prompt is not clobbered. CR grep (docs/MagicCompRules.txt): 614.6. If an event is replaced, it never happens. A modified event occurs instead ... (616.1 ordering among applicable replacements — pipeline_loop authority) Discriminating test (effects/seek.rs): seek_parks_on_per_card_replacement_choice_and_stashes_tail — two simultaneous graveyard->exile redirects make the first seeked-to-graveyard card prompt for CR 616.1 ordering; the batch parks, stashes the tail, and EffectResolved is NOT emitted (old loop ran to completion over the parked prompt). * fix(engine): reveal_until battlefield entry routes through zone pipeline (CR 614.1c / 306.5b) The kept-card battlefield entry used a raw `zones::move_to_zone`, skipping the CR 614.1c delivery tail — so a revealed planeswalker/battle entered with 0 loyalty/defense counters and was immediately put into the graveyard by CR 704.5i. Route the entry through `zone_pipeline::move_object`: the delivery tail seeds intrinsic enters-with counters, enters-with-counters statics, and applies the CR 614.1 tap-state from the seeded EntryMods. The previous manual `obj.tapped = true` is dropped (the tail does it — double- application check). The `enters_attacking` combat placement (CR 508.4) stays after delivery (not part of the zone tail). Bail on NeedsChoice / NeedsAuraAttachmentChoice (centralized park in move_object). The `move_to_library_position` rest-pile site (shuffle_to_bottom) is a library-placement SIBLING raw mover, DEFERRED to Phase D with a comment: move_object's placement arm is still a Phase-A stub that skips the replacement consult, and a library→bottom reposition has no Moved-redirect class to consult, so routing through it gains nothing now. CR grep (docs/MagicCompRules.txt): 306.5b A planeswalker has the intrinsic ability "This permanent enters with a ..." 310.4b A battle has the intrinsic ability "This permanent enters with a number ..." 614.1c Effects that read "[This permanent] enters with . . . ," ... 704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard. 508.4. If a creature is put onto the battlefield attacking, its controller ch... 303.4f If an Aura is entering the battlefield under a player's control by any... 616.1. If two or more replacement and/or prevention effects are attempting to... Discriminating test (effects/reveal_until.rs): reveal_until_planeswalker_enters_with_intrinsic_loyalty — a loyalty-4 planeswalker revealed to the battlefield enters with 4 loyalty counters (old raw path: 0 counters, dead by CR 704.5i). * fix(engine): resolution-choice battlefield entries route through zone pipeline (CR 614.1c) The RevealUntilKeptChoice-accept and DigChoice-kept handlers moved battlefield-entry cards via raw `zones::move_to_zone` + a manual `obj.tapped`, skipping the CR 614.1c delivery tail — so a kept/dug planeswalker or battle entered with 0 loyalty/defense and died to CR 704.5i. Route the battlefield branches through `zone_pipeline::move_object` (consistent with the synchronous reveal_until path migrated in C5): the tail seeds intrinsic enters-with counters and applies the CR 614.1 tap-state from the seeded EntryMods, so the manual tap is dropped. Bail on NeedsChoice / NeedsAuraAttachmentChoice (centralized park; realistically unreachable for the dig classes). `enters_attacking` (CR 508.4) combat placement stays post-delivery. DigChoice now binds `source_id` for CR 400.7 attribution (falls back to the moved object when None, matching the pre-pipeline raw move). Non-battlefield resolution-choice moves (manifest/surveil/dig rest to graveyard, route_rest_partition) are left raw this round — they sit inside multi-card loops with post-loop cleanup (revealed-marker clearing, continuation drain) where a per-card Moved-redirect pause cannot simply `return` without stranding the rest; migrating them needs the batch-entry + continuation restructure and is deferred. Library-placement sibling sites (move_to_library_position / move_to_library_at_index) stay deferred to Phase D. CR grep (docs/MagicCompRules.txt): 306.5b A planeswalker has the intrinsic ability "This permanent enters with a ..." 310.4b A battle has the intrinsic ability "This permanent enters with a number ..." 614.1c Effects that read "[This permanent] enters with . . . ," ... 704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard. 508.4. If a creature is put onto the battlefield attacking, its controller ch... 400.7. An object that moves from one zone to another becomes a new object wit... 616.1. If two or more replacement and/or prevention effects are attempting to... 303.4f If an Aura is entering the battlefield under a player's control by any... Discriminating test (effects/reveal_until.rs): reveal_until_kept_choice_planeswalker_enters_with_loyalty — drives the RevealUntilKeptChoice accept handler; a loyalty-5 planeswalker enters with 5 loyalty counters (old raw handler: 0, dead by CR 704.5i). * fix(engine): non-destroy SBA deaths consult Moved redirects (CR 704.5 / 614.6) The non-destroy state-based-action graveyard moves (zero toughness, zero loyalty, zero defense, legend-rule loser, unattached aura, battle without a protector, final-chapter Saga sacrifice) used a bare `zones::move_to_zone`, skipping the CR 614.6 replacement consult. These are "leaves the battlefield" / "dies" events (CR 603.6c + CR 700.4), so a `Moved` graveyard->exile redirect (Rest in Peace / Leyline of the Void) must apply — it did not. Add a shared `move_to_graveyard_via_pipeline` helper that routes each SBA-departing permanent through `zone_pipeline::move_object` with `ZoneChangeCause::StateBasedAction`. It returns `true` (and the caller bails) on a CR 616.1 ordering pause, mirroring the established `check_lethal_damage` regeneration-pause arm; the CR 704.3 fixpoint re-runs after the choice resolves and re-derives any undelivered SBA deaths, so bailing strands nothing. The self-parks at the destroy loop remain redundant-but-idempotent with the centralized park in move_object. CR grep (docs/MagicCompRules.txt): 704.5f If a creature has toughness 0 or less, it's put into its owner's graveyard. 704.5i If a planeswalker has loyalty 0, it's put into its owner's graveyard. 704.5j If two or more legendary permanents with the same name are controlled ... 704.5m If an Aura is attached to an illegal object or player, or is not attac... 704.5s If the number of lore counters on a Saga permanent ... 704.5v If a battle has defense 0 ... 704.5w If a battle has no player in the game designated as its protector ... 603.6c Leaves-the-battlefield abilities trigger when a permanent moves from t... 700.4. The term dies means "is put into a graveyard from the battlefield." 614.6. If an event is replaced, it never happens. A modified event occurs instead ... 616.1. If two or more replacement and/or prevention effects are attempting to... Discriminating test (sba.rs): sba_zero_toughness_death_consults_rest_in_peace_and_exiles — a zero-toughness creature with RIP on the battlefield is exiled, not put into the graveyard (old bare-move path: graveyard). * docs(engine): document C8/C9 zone-pipeline deferral — no Moved class targets Hand/Exile/Stack (PLAN Risk #5/#8) C8 (casting_costs/engine.rs cost sites) and C9 (Hand/Exile 1-site tail) are analyzed and deferred: the parser and synthesis only ever emit `Moved` redirects with `destination_zone` Graveyard (Rest in Peace / Leyline class) or Battlefield (ETB modifiers) — verified by grep of oracle_replacement.rs `destination_zone(Zone::...)`. There is NO `Moved` redirect class targeting a Hand, Exile, or Stack destination, so routing those moves through `move_object` would consult nothing and only add an unresumed-pause path to cost-payment / draw / dig flows that do not model a mid-flow replacement-choice resume. Draw-level replacements already apply at `ReplacementEvent::Draw` upstream. Records the finding inline at draw.rs:209 (the PLAN Risk #5 audit anchor), covering the whole Hand/Exile C9 tail (gift_delivery, connive, explore, turns return-to-hand; haunt, discover, exile_top, cascade, collect_evidence, ripple). seek.rs:97 and mill.rs:108 were migrated in earlier rounds (D2 / C1); discard.rs:38 stays Bucket A (Phase E). CR grep (docs/MagicCompRules.txt): 614.6. If an event is replaced, it never happens. A modified event occurs instead ... No behavioral change; comment-only. No discriminating test (the deferral is the absence of a behavioral change to test). * fix(engine): scope self-ETB Moved replacements to battlefield entry (CR 614.1c) Self-scoped as-enters replacements ("~ enters with N counters", enters tapped/prepared, as-enters choices, enter-as-copy, Karoo/shock/reveal lands; plus the Fading/Vanishing, Modular, Sunburst, Graft, Bloodthirst, Devour, Amplify keyword synthesizers) parsed/synthesized as ReplacementEvent::Moved with valid_card(SelfRef) and NO destination_zone. moved_matcher skips the destination gate when destination_zone is None, and a battlefield permanent is in-scan for its OWN departure — so the def matched the permanent's own battlefield EXIT: phantom counters + CounterAdded events on corpses (SBA deaths, bounce, destroy/sacrifice), a spurious CR 616.1 ordering prompt under a single Rest in Peace, and Optional clone defs forcing an "enter as a copy?" prompt on death. Fix is data-shape, not matcher special-casing: stamp .destination_zone(Zone::Battlefield) at construction on every self-ETB Moved def — CR 614.1c defs ("[This permanent] enters with...") are definitionally battlefield-entry-scoped. Parser sites: enters-tapped (unconditional/unless/if-controls), Karoo pay-cost, enters-prepared, reveal-land, shock land, as-enters-choose, clone, counter-choice, and the enters-with-counters builder (previously stamped only the is_external ChangeZone branch). Synthesis sites: Fading/Vanishing, Modular, Sunburst, Graft, Bloodthirst, Devour, Amplify (Riot/Unleash/Siege/Tribute/Saga were already stamped). The unearth / "would leave the battlefield" departure watchers stay destination-agnostic by design. CR grep (docs/MagicCompRules.txt): 614.1c Effects that read "[This permanent] enters with . . . ," "As [this per... 614.6. If an event is replaced, it never happens. A modified event occurs ins... 616.1. If two or more replacement and/or prevention effects are attempting to... Discriminating tests (fail-first evidence captured pre-fix): sba.rs::sba_death_does_not_apply_own_enters_with_counter_replacement — FAILED pre-fix: phantom counter on corpse (left: 1, right: 0) sba.rs::sba_death_under_single_rip_exiles_directly_no_prompt_no_counters — FAILED pre-fix: spurious CR 616.1 ordering prompt bounce.rs::bounce_does_not_apply_own_enters_with_counter_replacement — FAILED pre-fix: phantom counter on bounced card (left: 1, right: 0) All three drive parse_replacement_line (real parser output), then the SBA / bounce pipeline. * docs(engine): correct C8/C9 deferral rationale at draw.rs (destination-None Moved class) The recorded rationale ("no Moved class targets Hand/Exile") was incomplete: self-ETB Moved constructors carried NO destination_zone, and None matches every destination — so pre-Fix-1 such defs DID match Hand/Exile deliveries. The deferral conclusion stands (and was reinforced: migrating C8/C9 before the destination stamps landed would have widened the phantom-application defect). Post-stamp, explicit destinations are Graveyard/Battlefield only and the remaining destination-None defs are deliberate battlefield-departure watchers (unearth class), so the original claim is now true. Comment-only. * fix(engine): prevented discard records nothing; document paused-path bookkeeping gap (CR 614.6 / 701.9a) Prevented arm: a prevented inner ZoneChange means the card never left the hand — per CR 701.9a (to discard = move hand -> graveyard) NO discard occurred, so skip record_discard / the CR 702.187b Mayhem stamp / the Discarded event. Previously the arm fell through and recorded+emitted a discard that never happened, incoherent with the NeedsChoice early-return. Distinct from a REDIRECTED discard (CR 701.9c: still discarded — the Execute and madness arms keep recording+emitting). NeedsChoice arm: documented gap (counter.rs resolve_all style) instead of a resume-side continuation — a CR 616.1 ordering pause on the inner move (TWO materially-different Moved redirects on one discard, e.g. RIP + Wheel of Sun and Moon) delivers the card via the generic ZoneChange resume with no discard context, skipping "whenever you discard" bookkeeping for that card and abandoning a multi-card remainder. A proper fix needs a new serialized state slot + resume wiring; not built for a board no parsed deck assembles. Single-redirect boards (RIP alone) never prompt and are fully correct. CR grep (docs/MagicCompRules.txt): 614.6. If an event is replaced, it never happens. A modified event occurs ins... 701.9a To discard a card, move it from its owner's hand to that player's grav... 701.9c If a card is discarded, but an effect causes it to be put into a hidde... * fix(engine): batch-tail re-stash preserves request mods + attribution (CR 400.7 / 614.1c) PendingBatchDeliveries rebuilt paused-batch tails as ZoneMoveRequest::effect(obj, dest, obj), dropping the seek flow's enter_tapped mod and ability-source attribution across the pause boundary (seek is the first batch caller passing non-default EntryMods + a shared source). Extend the stash with serde-default fields (source_id, enter_tapped, exile_tracking) carrying the batch-uniform request context: captured from the first tail request in the new stash_batch_tail helper (source equal to the request's own object_id is the mill self-anchor idiom and stashes None), re-applied per request in drain_pending_batch_deliveries. Batch-uniform rather than per-request, mirroring the single-destination batch design; per-card heterogeneity remains a flagged design extension. Test: seek_parks_on_per_card_replacement_choice_and_stashes_tail extended to assert the stashed tail preserves the seek's ability-source attribution. * feat(engine): scan stack-resident object's own Moved redirect on stack exit (CR 608.2n / 614.12) find_applicable_replacements scanned only [Battlefield, Command] plus the entering-object (to:Battlefield) and discard exceptions, so a spell's own self-scoped `Moved` replacement was never discovered for its stack -> graveyard move. Add a stack-self-move exception mirroring the entering- object exception: when the proposed event is a ZoneChange whose `from` is Stack, include the moving object itself as a candidate source so its own SelfRef-scoped Moved def can fire as it leaves the stack. Hot-path cost: a single per-event Option match on the event's `object_id` when `from == Stack` (no extra zone sweep); the loop iterates the same `active_replacements` set as before, this only lets that one object pass the zone gate, and the `is_stack_self_move && !in_scanned_zone` SelfRef guard keeps it scoped to that object's own definitions. Inert until a stack -> graveyard self-redirect def is installed (next commit wires the Invoke Calamity rider through it): no parsed self-scoped Moved def today carries destination_zone: Graveyard, and the existing enters-with-counters self-defs are scoped to destination_zone: Battlefield. CR grep (docs/MagicCompRules.txt): 608.2n As the final part of an instant or sorcery spell's resolution, the spell is put into its owner's graveyard. 614.12. Some replacement effects modify how a permanent enters the battlefield. * fix(engine): route stack resolution defaults through pipeline; replace exile-rider flag with synthetic Moved def (CR 608.2n / 614.6) The stack resolution-default moves (resolved instant/sorcery → graveyard, fizzled/countered-on-resolution spell, prevented permanent → graveyard) delivered via raw `move_to_zone`, never proposing the inner ZoneChange, so board-wide `Moved` graveyard→exile redirects (Rest in Peace / Leyline of the Void) silently dropped on resolved/countered/prevented spells (PLAN §8 Risk #2 — confirmed bug). Route all three through `zone_pipeline::move_object` (new `ZoneMoveRequest::spell_resolution_default`, Cause::SpellResolutionDefault). The Invoke Calamity free-cast "if this spell would be put into your graveyard, exile it instead" rider was a bespoke per-object boolean (`exile_from_stack_instead_of_graveyard`) read by hand at the two dest computations. Replace it with a synthetic self-scoped `Moved` replacement (valid_card: SelfRef, destination_zone: Graveyard, execute: ChangeZone → Exile) installed at the same attach point — the same class as RIP/Leyline, just scoped to one spell — so the pipeline applies it like any other Moved redirect. Delete the flag, its game_object field, and the `object_exiles_instead_of_graveyard` reader. The flag may appear in saved games; GameObject has no deny_unknown_fields, so serde ignores the legacy field on deserialize (an old mid-cast save lacks the synthetic def — the def travels with the object via replacement_definitions; acceptable). Flashback/aftermath/harmonize exile-on-stack-exit stays a STATIC destination rule selected pre-pipeline (dest = Exile), so its proposed move is Stack→Exile; the Graveyard-scoped redirect class never matches it — no double-apply. CR 616.1 ordering pauses (two simultaneous redirects) are parked by move_object; each site emits the standard StackResolved + trigger-context-clear epilogue and bails for the replacement-choice resume. CR grep (docs/MagicCompRules.txt): 608.2n As the final part of an instant or sorcery spell's resolution, the spell is put into its owner's graveyard. 608.3e If a permanent spell resolves but its controller can't put it onto the battlefield, that player puts it into its owner's graveyard. 614.1a Effects that use the word "instead" are replacement effects. 614.6. If an event is replaced, it never happens. A modified event occurs instead. Discriminating tests: stack.rs::rest_in_peace_exiles_resolved_instant — RIP redirects a resolved instant's graveyard move to exile (fails on old raw delivery). stack.rs::flashback_spell_exiles_once_with_rest_in_peace_present — flashback exiles exactly once via its static rule; RIP (graveyard-scoped) does not double-apply on the stack→exile move. casting_costs.rs::invoke_calamity_rider_exiles_free_cast_spell_on_resolution — the rider's synthetic Moved def redirects a free-cast spell to exile on resolution through the deleted-flag's replacement path. * fix(engine): paused fizzle runs the resolution epilogue before bailing (CR 608.2b / 616.1) The fizzle/countered-on-resolution arm bailed on a replacement-ordering pause with a bare `return`, skipping the epilogue directly below it (StackResolved emission + current_trigger_event / current_trigger_events / current_trigger_match_count / die_result_this_resolution clears) — leaking stale cross-resolution context across the pause and never emitting StackResolved. The other two C2 sites (instant→graveyard else-branch, prevented-permanent arm) inline that epilogue before their pause-returns; the fizzle arm now falls through to its shared epilogue instead (the pause needs nothing else from the fizzle path — `move_object` parked the prompt and the move, and the resume path delivers it). Reachable: an Invoke Calamity free-cast spell fizzling under a single Rest in Peace = rider + RIP = two simultaneous graveyard→exile redirect candidates = CR 616.1 ordering prompt = this arm. Also documents the rider's known scope gap at the install site: the synthetic def carries no "this turn" duration (ReplacementDefinition has no duration field; revert_layered_characteristics_to_base only runs for battlefield exits), behavior-preserving vs the deleted flag — a Duration field on ReplacementDefinition is the eventual fix. CR grep (docs/MagicCompRules.txt): 608.2b If the spell or ability specifies targets, it checks whether the targets are still legal ... 616.1. If two or more replacement and/or prevention effects are attempting to modify the way an event affects an object or player ... Discriminating test (fail-first verified): casting_costs.rs::invoke_calamity_rider_fizzle_under_rip_parks_choice_with_clean_epilogue — free-cast spell with the rider fizzles under one RIP → CR 616.1 prompt parks → StackResolved emitted + all four context fields cleared on the paused path → GameAction::ChooseReplacement resumes → spell exiled. Pre-fix failure: panicked at "paused fizzle must still emit StackResolved" (prompt assertion passed, proving reachability). * fix(engine): route surveil + manifest-dread rest piles through zone pipeline (Phase C6) The non-battlefield rest-pile loops in engine_resolution_choices.rs delivered every unkept card with a bare zones::move_to_zone(.., Zone::Graveyard, ..), proposing no per-card ZoneChange — so each card's own Moved redirects (Rest in Peace / Leyline of the Void: 'would be put into a graveyard from anywhere -> exile instead') silently never fired (PLAN Risk #1 class). The post-loop cleanup (surveil kept-on-top reorder; manifest-dread reveal-marker removal) ran inline at the end of the loop, which is why C6 was deferred twice: a per-card CR 616.1 pause mid-pile would run that cleanup before the paused tail finished, then never again. Route both piles through a new zone_pipeline::move_objects_simultaneously_then, which carries a typed BatchCompletion (NOT a closure) describing the post-loop work. The batch runs the cleanup exactly once on true completion: inline on the synchronous (single-redirect) path, and via drain_pending_batch_deliveries when a CR 616.1 ordering choice pauses the pile. BatchCompletion rides on the parked PendingBatchDeliveries tail (mirroring PendingCounterPostAction), and the drain re-attaches it across each re-park so it can never run early or twice. The paused-on-last-card empty-tail case is handled by ensure_batch_record so the completion still fires after the final card's redirect resolves. CR grep (docs/MagicCompRules.txt): 701.25a To 'surveil N' means to look at the top N cards of your library, then put any number of them into your graveyard and the rest on top of your library in any order. 614.6. If an event is replaced, it never happens. A modified event occurs instead ... 616.1. If two or more replacement and/or prevention effects are attempting to modify ... 603.10a Some zone-change triggers look back in time. These are leaves-the-battlefield abilities ... Discriminating tests (surveil_rest_pile_redirect_continuation.rs): surveil_rest_pile_honors_graveyard_exile_redirect — single redirect: unkept cards land in EXILE (not graveyard), kept card stays on top once. FAILS on the old raw move (cards reached the graveyard). surveil_rest_pile_under_two_redirects_runs_keep_on_top_cleanup_once — two simultaneous redirects pause every unkept card on a CR 616.1 prompt; answering each delivers ALL to exile (none stranded), drains the parked tail, and runs the kept-on-top reorder EXACTLY once (kept card on top, present once — not duplicated, not missing). * refactor(engine): route exempt-cause moves through zone pipeline (Phase D) Migrate the four CR-exempt zone-change classes off raw zones::move_to_zone onto the single pipeline entry under explicit exempt ZoneChangeCause variants, so Phase E can flip enforcement knowing every production caller goes through the pipeline. The exempt path seals the proposed event directly and delivers it WITHOUT the replace_event consult (the 'would'-semantics layer); the unconditional primitive guards (CR 111.8 token, CR 614.1d ETB block, CR 400.7 cleanup) still run in zones.rs delivery for every cause (PLAN section 2/3). Sites: - mulligan.rs (CR 103.5 PregameProcedure): Serum Powder hand->exile, mulligan hand->library shuffle, pregame draw, and the two opening-hand/mulligan bottoming loops (via the library-placement arm). - elimination.rs (CR 800.4a PlayerLeftGame): owner's objects -> exile. 'This is not a state-based action' and no replacement applies to a player leaving. - casting_costs.rs::finalize_cast (CR 601.2a CastingToStack): the committed Hand/GY/Exile/Command -> Stack transition. Part of the casting process, not a replaceable event. - engine_debug.rs (DebugCommand): MoveToZone (incl. library top/bottom/Nth via placement), Mill, and the no-ETB battlefield staging path. Operator intent is 'force the state'; no intrinsic enters-with-counter seeding, matching the prior raw placement. ZoneChangeCause::is_exempt() is the single authority for the consult skip; each exempt arm carries its CR citation. New ZoneMoveRequest constructors (casting_to_stack / pregame / player_left_game / debug) keep call sites short. Removed now-unused 'use super::zones' in mulligan.rs. CR grep (docs/MagicCompRules.txt): 103.5. Each player draws a number of cards equal to their starting hand size ... A player who is dissatisfied with their initial hand may take a mulligan ... shuffles the cards in their hand back into their library ... puts a number of those cards ... on the bottom of their library 601.2a To propose the casting of a spell, a player first moves that card (or that copy of a card) from where it is to the stack. 800.4a When a player leaves the game, all objects (see rule 109) owned by that player leave the game ... This is not a state-based action. No behavior change: exempt causes already bypassed the consult via raw moves; this routes them through the pipeline with identical delivery. Existing mulligan / elimination / debug / casting tests stay green (test-engine ok). * docs(engine): document W3 library-placement consult deferral with evidence (Phase D) The placement-aware pipeline arm exists as a stub (move_object delivers a Some(placement) request directly via move_to_library_at_index, skipping the replacement consult). Phase D's exempt pregame/debug library callers already route through it. Completing the stub so it RUNS the consult on a library placement (PLAN section 3.5 'the consult should still run for future-proofing') is DEFERRED with evidence: - Zero value today: no Moved replacement in the card pool targets destination_zone(Library). Verified destination distribution: 25 destination_zone(Zone::Battlefield) 17 destination_zone(Zone::Graveyard) 2 destination_zone(Zone::Exile) 0 Library so the consult is a guaranteed no-op on every library placement. - Real risk for that no value: a correct completion must gate the CR 701.24a delivery-tail auto-shuffle on placement-absence across the shared deliver / deliver_replaced_zone_change / DeliveryCtx signatures (a library *placement* must NOT shuffle; a plain library-destination ZoneChange MUST). That is a cross-cutting change to every bucket-A/B/C delivery caller with a silent-randomization landmine (a wrong gate randomizes put-on-top / scry / cascade placements with no crash or test failure). The raw move_to_library_position / _at_index sibling production callers (put_on_top, cascade, discover, reveal_until, drawn_this_turn_choice, engine_resolution_choices) stay on the raw movers until that completion: a library reposition is not 'put into a graveyard/exile/hand', so nothing is skipped by staying raw. Migrating them onto the stub arm ahead of the consult completion is value-neutral churn that does not advance Phase E (which can only demote the library movers once ALL callers AND the consult are in place) and carries per-site top-order-reversal preservat…
* fix missing art for reversible cards * fix(PR-2682): cover reversible Scryfall generation --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…2789) Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…cks (phase-rs#2787) Co-authored-by: Cursor <cursoragent@cursor.com>
…ests (Fixes phase-rs#575) (phase-rs#2795) Co-authored-by: Cursor <cursoragent@cursor.com>
…hase-rs#567) (phase-rs#2797) Co-authored-by: Cursor <cursoragent@cursor.com>
phase-rs#564) (phase-rs#2790) * fix(engine): transfer Wishclaw Talisman to chosen opponent after search (Fixes phase-rs#564) Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-2790): route gain_control target extraction through shared effect_object_targets helper Addresses the MED review finding: the inlined filter_map dropped TargetFilter::ParentTargetSlot handling (CR 608.2c) that the shared helper provides, and duplicated what sibling give_control_object_targets already delegates. --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…tField alignment (phase-rs#2794) * style(client): enhance CardSearch and SelectField components with consistent styling Updated CardSearch component to utilize shared CSS classes for dropdown styling, improving UI consistency. Adjusted SelectField component to ensure full-width appearance and better integration with the updated styles. This change enhances the overall user experience across the application. * style(client): simplify SelectField component styling Refactored the className property in the SelectField component to remove unnecessary full-width styling, enhancing the component's appearance while maintaining its functionality. This change contributes to a cleaner and more consistent UI across the application. --------- Co-authored-by: carlh171112 <carlh171112@gmail.com>
…rs#2812) * style(client): enhance CSS for dark mode and number input steppers - Added a dark color scheme to the HTML element for improved accessibility. - Customized number input steppers to replace default styles with a more modern design, enhancing user experience. - Adjusted hover and focus states for number input steppers to improve interactivity and visual feedback. * style(client): refine number input steppers for fine pointer devices - Introduced media query for fine pointer devices to customize number input steppers. - Enhanced hover and focus states for improved interactivity and visual feedback. - Updated comments for clarity on design choices and accessibility considerations. * style(client): update number input steppers for consistent app-wide design - Enhanced number input steppers to ensure a uniform appearance across the application, replacing default Chrome/Edge styles with a hairline-dark design. - Updated comments for clarity on the global application of styles and the intentional removal of native spinners in Firefox. - Maintained accessibility considerations for touch devices and fine pointer interactions.
…ase-rs#565) (phase-rs#2800) * test(engine): pin Birthing Ritual end-step trigger behavior (Fixes phase-rs#565) Add integration coverage that Birthing Ritual triggers exactly once at the beginning of the controller's end step when another creature is controlled, and stays suppressed when the intervening-if is unmet. Co-authored-by: Cursor <cursoragent@cursor.com> * test(PR-2800): make intervening-if skip test discriminating Both reviews flagged the MED: asserting an empty stack after advance_until_stack_empty() is vacuous — the stack drains whether or not the trigger fired. Now observes at the end-step priority window without draining (mirroring the fire test) and guards the phase. Mutation-verified: adding a creature to the scenario makes this assertion fail (CR 603.4). --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…ing (phase-rs#2826) * refactor: update FormatPicker and ModalPanelShell for improved layout and accessibility - Adjusted spacing and layout in FormatPicker for better visual consistency. - Enhanced ModalPanelShell with accessibility features, including aria attributes and improved structure. - Updated body class in GameSetupPage to align with new layout standards. * feat: enhance ModalPanelShell with open prop for conditional rendering - Added an `open` prop to ModalPanelShell to control visibility and manage exit animations. - Updated GameSetupPage to utilize the new `open` prop, improving the modal's rendering logic. - Refactored modal structure for better accessibility and layout consistency.
…e-safe battlefield entries (phase-rs#2829) * fix(engine): route declined-cipher, declined-madness, and legend-rule losers to graveyard through zone pipeline (CR 608.2n / 702.35a / 704.5j / 614.6) These three graveyard-destination moves delivered via raw move_to_zone, never proposing the inner ZoneChange, so board-wide Moved graveyard->exile redirects (Rest in Peace / Leyline of the Void) silently dropped: - cipher decline (CR 608.2n): the resolving cipher card -> owner's graveyard now routes through SpellResolutionDefault; handle_encode_choice returns the ZoneMoveResult so the caller surfaces a parked CR 616.1 prompt instead of clobbering it with Priority. - madness decline (CR 702.35a): the exiled card -> owner's graveyard now routes through move_object; the arm evaluates to the parked WaitingFor on pause so the post-action pipeline is skipped. - legend rule (CR 704.5j): the losing legends -> graveyards now route through move_objects_simultaneously (CR 603.10a co-departure stamp); a mid-batch CR 616.1 choice parks the prompt and stashes the tail. CR grep (docs/MagicCompRules.txt): 608.2n spell goes to graveyard on resolution; 702.35a madness; 704.5j legend rule; 614.6 replaced event never happens; 603.10a simultaneous leaves-battlefield. * fix(engine): route Attraction-open and ninjutsu battlefield entries through zone pipeline (CR 614.1c) Both battlefield entries delivered via raw move_to_zone, skipping the pipeline delivery tail that applies enters-with-counters statics (StaticMode::EntersWithAdditionalCounters — Hardened Scales / Conclave Mentor 'creatures you control enter with an additional +1/+1 counter' class). The raw mover applies those statics nowhere, so an opened Attraction or a ninja entering via ninjutsu silently missed them. Route both through zone_pipeline::move_object. A battlefield-entry pause (CR 616.1 multi-redirect ordering / CR 303.4f aura host / counter- replacement) is not reachable for these object classes in the supported pool: an Attraction is a non-Aura artifact, a ninja is a non-Aura creature, and no supported entry surfaces a choice. The bail is a safety guard, not a resume path — ninjutsu's post-entry combat placement (CR 702.49c) cannot resume across a pause, so on the unreachable pause we stop with the prompt parked rather than act over parked state. Attribution self-anchors (CR 400.7; the raw move recorded no source). CR grep (docs/MagicCompRules.txt): 614.1c enters-with statics; 702.49c ninjutsu combat placement; 616.1 multi-replacement ordering. * docs(engine): correct stale aura-arm drain note + document dig multi-kept pause limitation Two comment-only corrections from the d5a12b8c6 review verdict: 1. zone_pipeline.rs deliver_batch aura arm: the note claiming a tail stashed on NeedsAuraAttachmentChoice would be 'silently drained by the NEXT unrelated replacement-choice resume' is stale. As of d5a12b8c6 the ReturnAsAuraTarget handler (engine.rs:3608-3611) and its chain-resume sibling (engine.rs:3572) both drain pending_batch_deliveries, so the aura-attachment resume finishes the parked batch correctly. 2. engine_resolution_choices.rs DigChoice kept-loop pause: document the multi-kept limitation — if kept card #1 pauses, kept phase-rs#2+ stay in the library (the for-loop return exits before they move), and publish_tracked_set: Some(kept) publishes ALL kept including the unmoved ones, so a downstream sub-ability can be wired to cards still in the library. Pre-existing, strictly no-worse-than the old raw path; revisit with a kept-loop continuation if a 2+-kept-to-battlefield dig that pauses on the first is ever added. No behavior change. * fix(engine): route reveal-until and dig graveyard rest piles through zone pipeline so Moved redirects fire (CR 614.6 / 701.20a / 603.10a) The reveal-until and dig 'put the rest into your graveyard' piles delivered via raw move_to_zone, never proposing the inner ZoneChange, so a board-wide Moved graveyard->exile redirect (Rest in Peace / Leyline of the Void) silently dropped on the rest cards. 12 card-data cards carry RevealUntil with rest_destination: Graveyard (Mind Funeral class), plus dig's no-kept-zone 'rest to graveyard' branch — all affected. move_rest is now the single authority: a Graveyard (or any non-library) rest pile routes through move_objects_simultaneously (CR 603.10a co-departure stamp), so each rest card consults the redirect; a Library rest pile keeps the random-order shuffle_to_bottom (no Moved-redirect class targets Library, and the placement arm would lose the shuffle). The new move_rest_then carries an optional BatchCompletion for callers that defer cleanup across a pause. Synchronous completion runs the resolver's own marker-clear + EffectResolved inline (the dispatching chain processor still owns priority/continuation). On a mid-pile CR 616.1 ordering pause, the prompt is parked and the cleanup is deferred onto a cleanup-only RevealRestPile completion (empty rest_cards — the pile IS the batch) so the drain runs it once and EffectResolved never lands over the parked prompt. The dig unkept->graveyard branch mirrors this, deferring finish_with_continuation via the same completion. Discriminating test reveal_until_graveyard_rest_redirected_to_exile_by_rest_in_peace: a graveyard-rest reveal-until with a RIP graveyard->exile Moved redirect on the battlefield now exiles the rest pile (old raw path: graveyard'd). CR grep (docs/MagicCompRules.txt): 614.6 replaced event never happens; 701.20a reveal until / rest pile; 603.10a simultaneous leaves-battlefield. * fix(engine): route morph and manifest face-down entries through zone pipeline (CR 708.3 / 614.1c) Both face-down battlefield entries (cast face-down via morph/disguise, and manifest) delivered via raw move_to_zone followed by a manual apply_face_down_creature_characteristics + back_face snapshot — bypassing the pipeline delivery tail, so a face-down 2/2 never received enters-with-counters statics ('creatures you control enter with an additional +1/+1 counter' — Hardened Scales / Conclave Mentor class). Route both through zone_pipeline::move_object with the new ZoneMoveRequest::face_down(profile) mod. The delivery tail is already the canonical face-down authority (it snapshots the real face into back_face and applies the vanilla-2/2 profile BEFORE the entry per CR 708.3, identical to change_zone's face-down path) AND seeds enters-with-counters statics, so the manual post-move override is dropped — morph/manifest now match the rest of the engine's face-down entries. A battlefield-entry pause is unreachable for a vanilla 2/2 (not an Aura, no Moved redirect / counter-replacement choice); the bail keeps the helpers safe by construction. CR grep (docs/MagicCompRules.txt): 708.2a face-down characteristics; 708.3 turned face down before it enters; 614.1c enters-with statics; 701.40a manifest. * fix(engine): route exile-until-leaves returns through zone pipeline (CR 610.3a / 614.1c) check_exile_returns (Banisher Priest / Fiend Hunter / Oblivion Ring class — 'exile until ~ leaves') returned each exiled card via raw move_to_zone, skipping the pipeline delivery tail: a battlefield return missed enters-with- counters statics (Hardened Scales class), and a non-battlefield return dropped any Moved redirect. Group the returns by destination zone (first-seen order for determinism — Zone isn't Ord) and route each group through move_objects_simultaneously_then (CR 603.10a co-departure). A returned creature can pause on an as-enters / aura-host choice (CR 303.4f / 616.1), so the spent UntilSourceLeaves link cleanup rides a new BatchCompletion::RemoveExileLinks per group, drained once the group's pile lands (synchronously or via the replacement-choice / aura-attachment resume) — never before a paused card finished returning. Links for cards that already left exile by other means are dropped immediately; only the in-flight group ids ride their completion. CR grep (docs/MagicCompRules.txt): 610.3a return to previous zone; 614.1c enters-with statics; 603.10a simultaneous; 303.4f aura host on entry. * fix(engine): route reveal-until kept-to-graveyard cards through zone pipeline; document dig rest-partition gap (CR 614.6 / 701.20a) A reveal-until KEPT card sent to the graveyard (4 cards, kept_destination: Graveyard — Mind Funeral-style 'put it into your graveyard') was delivered raw, skipping a Moved graveyard->exile redirect (Rest in Peace / Leyline of the Void). Route it through move_object on both the synchronous resolve path (the non-Hand/Battlefield kept branch; Library stays raw as a placement) and the RevealUntilKeptChoice handler (accept_zone / decline_zone via the new route_kept_card_or_defer helper). On a CR 616.1 pause the rest-pile move + marker clear defer onto a RevealRestPile completion; the kept-choice handler's rest pile now flows through move_rest_then so its completion (marker clear + finish_with_continuation) runs once on either path — closing the latent unhandled-pause the move_rest migration introduced here. Documents the remaining route_rest_partition gap (dig 'rest into graveyard', 65 Dig cards + the null->Graveyard default): it has a caller INSIDE run_batch_completion, so migrating it needs a re-pause-from-completion contract plus pause handling in both synchronous callers — a cross-cutting change tracked for a follow-up rather than a partial migration. CR grep (docs/MagicCompRules.txt): 614.6 replaced event never happens; 701.20a reveal until / rest pile. * fix(engine): paused face-down entry resumes face down; prevented-ETB fallback consults Moved redirects (CR 708.3 / 608.3e / 614.6) Round-1 review findings 1 (HIGH) + 3 (MEDIUM) on the zone-pipeline tail migration — both in handle_replacement_choice: 1. The ZoneChange resume arm destructured the approved event with '..', DISCARDING face_down_profile, and delivered via the raw mover — a morph / manifest entry parked on a CR 616.1 ordering prompt resumed FACE UP, violating CR 708.3 and leaking the morpher's hidden card. The prompt is REACHABLE: two co-played external enter-tapped Moved effects (Authority of the Consuls + Imposing Sovereign class) collide on the entry's tap field (no same-value dedupe), surfacing the ordering choice — empirically proven by the new test's parked-prompt assertion. Fix: extract the tail's CR 708.3 block into zone_pipeline::apply_face_down_entry_profile (single authority, not a mirrored copy) and call it from the resume arm immediately after the move, before the tap/controller/counter blocks (tail ordering). Full tail-routing via approve_post_replacement + deliver was assessed and deferred to the Phase-B token migration with a flagged TODO: the arm's epilogue drains post_replacement_continuation with spell-resolution ctx + post_replacement_source clearing, orders pending_spell_resolution differently, and carries the bespoke played_from_zone preservation (PLAN Open Question phase-rs#3) — three behavioral divergences that need their own reconciliation, not a drive-by. Morph/manifest 'pause unreachable' comments replaced with honest reachability documentation (the bail is complete: the profile rides the parked event; the resume applies it). 2. The CR 608.3e prevented-ETB graveyard fallback delivered raw. The consulted (prevented) event was the battlefield ENTRY; the fallback is a fresh, never-consulted event, so routing it through move_object (SpellResolutionDefault, mirroring stack.rs's C2 prevented-permanent site) cannot double-apply — the prevention def is Battlefield-scoped and cannot re-match a Graveyard move. RIP/Leyline redirects now fire on the discarded spell. The dead pending_continuation is cleared before the move so a CR 616.1 pause cannot leave it for the next resume's epilogue. Fail-first evidence (both red before the fix, green after): - paused_face_down_morph_entry_resumes_face_down: panicked 'resumed morph entry must be FACE DOWN (CR 708.3)' - prevented_etb_graveyard_fallback_consults_moved_redirects: left: Graveyard, right: Exile (staging note in-test: no ZoneChange applier can yield Prevented, so the parked choice is staged as a regeneration-shield Destroy prevention; the resume is driven through the real GameAction entry) CR grep (docs/MagicCompRules.txt): 708.2a face-down characteristics; 708.3 turned face down before it enters; 608.3e prevented ETB to graveyard; 614.6 replaced event never happens; 616.1 multiple replacement ordering. * fix(engine): ninjutsu and Attraction-open pauses resume via continuations instead of dropping post-entry work (CR 702.49 / 701.51 / 616.1) Round-1 review finding 2 (MEDIUM): both 3d1497411 bails were wrong because the battlefield-entry prompt IS reachable — two co-played external enter-tapped Moved effects (Authority of the Consuls + Imposing Sovereign for creatures; Kismet / Frozen Aether class for artifacts) produce a material same-field collision on the entry's tap state (no same-value dedupe) and surface a CR 616.1 ordering prompt. On that board: - ninjutsu: the bail skipped the cast-variant provenance tag AND the CR 702.49c tapped-and-attacking combat placement — the resumed ninja entered untagged and non-attacking. - Attraction open: the bail left in_attraction_deck set, never emitted AttractionOpened, and dropped every remaining open of the instruction. Adjudication: continuations, not documented limitation — the BatchCompletion infrastructure already exists (RevealRestPile / RemoveExileLinks shape). Two new variants: NinjutsuPlacement (defers finish_ninjutsu_entry: tag + combat placement + NinjutsuActivated + layers) and AttractionOpenRemainder (defers finish_attraction_open + the remaining opens, which may themselves re-park and re-defer through the same completion — the drain takes the old record before running it, so a fresh park is preserved). Both finish helpers are shared single authorities between the synchronous and resumed paths. The 'pause unreachable' comments are replaced with honest reachability documentation. Bonus CR-correctness in the shared helper: AttractionOpened now fires only when the card actually entered the battlefield (CR 701.51c — prevented or replaced entries must not trigger 'opens an Attraction'; Done also covers prevented/redirected deliveries). Also corrects the CR 610.3a cite to CR 610.3 proper on the RemoveExileLinks doc + completion arm (610.3a is the already-occurred-event timing subpart; 610.3 is the return rule). Fail-first evidence (both red before, green after): - paused_ninjutsu_entry_resumes_with_combat_placement_and_tag: panicked 'resumed ninja must be placed attacking (CR 702.49c)' - paused_attraction_open_resumes_bookkeeping_and_remaining_opens: panicked 'open bookkeeping must run on the resumed Attraction (old bail left the flag set)' Both tests passed their parked-prompt assertions pre-fix, empirically confirming the reviewer's reachability claim for both object classes. CR grep (docs/MagicCompRules.txt): 702.49/49a/49c ninjutsu + placement; 701.51/51b/51c open an Attraction + trigger gate; 616.1 ordering; 610.3 return-to-previous-zone. * docs(engine): flag CR 614.12 face-down consult gap; correct exile-return cite to CR 610.3 (review finding 4 + nit) Round-1 review finding 4 (MEDIUM-LOW, comment-only): execute_zone_move's replacement consult runs the matcher pass against the object's PRINTED characteristics, but CR 614.12 (docs/MagicCompRules.txt:3094) requires checking 'the characteristics of the permanent as it would exist on the battlefield' — for a face-down (morph/manifest) entry that is the 2/2 with no name/types/subtypes (CR 708.2a), so a type- or name-keyed entry replacement wrongly matches a face-down printed Wizard. Narrow class today (the common enter-tapped/counter statics are type-agnostic or creature-scoped, which the face-down 2/2 satisfies); the fix is profile-projected characteristics in the matcher pass when face_down_profile is present. Documented at the consult with the CR cite. Also corrects check_exile_returns' CR 610.3a cite to CR 610.3 proper (610.3a is the already-occurred-event timing subpart; 610.3 is the rule that creates the return one-shot). CR grep: 614.12 'check the characteristics of the permanent as it would exist on the battlefield'; 610.3 'A second one-shot effect ... returns the object to its previous zone.' No behavior change. * fix(engine): arrival-gate ninjutsu post-entry work, twin of the Attraction CR 701.51c gate (CR 702.49c) Round-2 review LOW: finish_ninjutsu_entry ran the cast-variant tag and the CR 702.49c combat placement unconditionally — ZoneMoveResult::Done also covers prevented/redirected deliveries, so a redirected resumed entry would tag a non-battlefield object and place it into combat.attackers. Gate both behind the same zone == Battlefield arrival check as finish_attraction_open. Unreachable today (no supported Moved redirect retargets a battlefield entry away from the battlefield), but the gate makes the helper correct by construction rather than by census, matching the twin's standard. NinjutsuActivated stays deliberately OUTSIDE the gate, unlike the Attraction twin's AttractionOpened: CR 701.51c explicitly suppresses the 'opens an Attraction' trigger when the entry is prevented/replaced, but ninjutsu's activation event occurred when the ability was activated (cost paid, attacker returned) — a redirected entry does not un-activate it. Asymmetry documented inline. No new test per the review (unreachable class, identical behavior for every reachable entry — existing paused_ninjutsu_entry_resumes_with_combat_placement _and_tag still green). CR grep (docs/MagicCompRules.txt): 702.49c enters attacking; 701.51c trigger suppression on prevented/replaced entry.
…phase-rs#566) (phase-rs#2798) * fix(engine): honor effective Dash keyword for alternative cast (Fixes phase-rs#566) Read Dash through effective_spell_keywords in candidate generation, cost preparation, and AlternativeCastChoice dispatch so granted dash costs match Blitz/Evoke. Add Ragavan-shaped integration regression tests. Co-authored-by: Cursor <cursoragent@cursor.com> * test(PR-2798): discriminating granted-Dash coverage + convert Spectacle to effective_spell_keywords Both reviews flagged the MED gap: the Ragavan tests inject printed Dash into obj.keywords, so they pass on origin/main and never exercise the granted-keyword path this PR fixes. New test grants Dash via a StaticMode::CastWithKeyword battlefield static (CR 604.1) with no printed Dash — verified to FAIL against origin/main casting.rs and pass with the fix. Also converts the two remaining Spectacle keyword reads (candidates + cost lookup) to effective_spell_keywords, completing the alternative- cost keyword class this PR brought Dash into (CR 702.137a). * style(PR-2798): cargo fmt (pinned-toolchain formatting) --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…ause (CR 508.4 + CR 107.3) (phase-rs#2807) Tokens created with "that are tapped and attacking, where X is …" (e.g. Anim Pakal, Thousandth Moon) were silently ignoring the entering-tapped and entering-attacking flags. Root cause: `parse_token_description`'s `attacking_clause` combinator used `eof` as the only allowed terminator after the attacking phrase. When a variable-count binding (", where X is …") followed, the eof check failed at every scan position and `tapped`/`enters_attacking` both stayed false. Fix: accept `, where ` as a second valid terminator. The where- expression is also saved before the text is truncated at the clause boundary so the X-binding step can still resolve the variable count. Covers the full class of token effects that combine an entry modifier with a variable-count rebind.
…0.2a) (phase-rs#2817) resolve_enters_under_player only mapped ControllerRef::You to a concrete PlayerId and returned Err("not yet supported") for every other variant, so "put ... onto the battlefield under their/that/target player's control" (ScopedPlayer, TargetPlayer, ParentTargetController, …) failed mid-resolution for an entire class of cards (each-player / target-player mass put-into-play and reanimation — Tempting Wurm, Show and Tell, Hypergenesis, Warp World). Delegate to the canonical ControllerRef -> PlayerId resolver (filter::controller_ref_player, now pub(crate)), threading state + ability. None still means the owner's control; a ControllerRef that genuinely cannot resolve to a single player in this context (e.g. bare Opponent) keeps the explicit Err rather than silently guessing.
…ase-rs#2823) * feat(engine): conjure a duplicate of a referenced card (CR 707.2) The Conjure runtime existed but the parser only recognized "conjure a card named X"; "conjure a duplicate of <reference>" (it / that card / target … card exiled with ~) fell to Effect::Unimplemented — the largest single unimplemented subset (~47 cards). Parameterize ConjureCard's identity into a typed `ConjureSource`: - Named { name } — the existing named-card form. - Duplicate { duplicate_of: TargetFilter } — copy a referenced card. `#[serde(flatten)]` + an untagged `ConjureSource` keep the wire shape backward compatible: legacy `{"name": "X"}` still deserializes to `Named` and re-serializes identically. Add a parser arm for the duplicate form (anaphoric "it"/"that card" -> ParentTarget; "target … card" -> parsed target) and resolve it by copying the referenced card's identity by name through the existing named-conjure card-creation path (CR 707.2). An unresolved reference conjures nothing. * fix(parser): only claim fully-modeled duplicate-conjure (no swallowed clause) The duplicate-conjure arm returned Effect::Conjure even when it did not fully model the reference (e.g. parse_target left "exiled with ~" unconsumed) or left trailing text after the zone, silently dropping that text — a swallowed clause the coverage-honesty gate rejects. Now claim the clause only when it is fully consumed: a "target …" reference must be parsed in full by parse_target, a bare anaphor must be exactly "it" / "that card" / "this card", and no text may remain after the zone (+optional "tapped"). Anything else is left a loud Effect::Unimplemented rather than silently swallowing the reference. * fix(engine): duplicate-conjure copies copiable values, not just the name (CR 707.2) Review feedback (natefinch): resolving the referenced card's name and re-looking it up in `card_face_registry` produced a name-only, characteristic- less card for the general case — the registry only preloads statically-named conjures (and `walk_effect` excludes Duplicate), so a duplicate of a normal deck card found no face and applied nothing. Copy the referenced card's current copiable values directly (CR 707.2) via `compute_current_copiable_values` + `apply_copiable_values` (the same building blocks `token_copy` uses), so the conjured real card carries full characteristics (types, mana cost, P/T, abilities) regardless of the scoped registry. Works for non-battlefield references (exile/graveyard) since the computation starts from the object's intrinsic copiable values. Also resolves the transformed-DFC / modified-copiable-value cases. The resolver test now gives the referenced card real characteristics (Creature 2/2) and asserts the conjured duplicate copies them, so it actually discriminates the previous name-only bug. * fix(engine): box ConjuredIdentity::Named face to satisfy clippy::large_enum_variant
…hase-rs#3137) * Seed loyalty counters before mutating planeswalker loyalty. Planeswalkers that carried a loyalty characteristic without a counter-map entry were not counter-tracked, so combat damage and loyalty costs had no lasting effect after layer evaluation. Co-authored-by: Cursor <cursoragent@cursor.com> * Seed intrinsic loyalty counters at ETB and rehydrate repair. Route battlefield placement through the zone-change pipeline so planeswalkers receive CR 614.1c loyalty counters on entry, and repair missing counter-map entries during card-db rehydration via apply_etb_counters. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-3137): remove rehydrate counter repair shim * Fix Frostcliff no-choice fixture after battlefield entry --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
Map "exile a card from the top of your library" to ExileTop with face-down and per-opponent count so the trigger resolves without a library-wide prompt. Fixes phase-rs#2397 Co-authored-by: Cursor <cursoragent@cursor.com>
Guard Obuun's begin-combat trigger so animated lands become X/X from the source's power instead of dying as 0/0 creatures. Fixes phase-rs#2398 Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(parser): parse flying on tokens before equal-to count Strip "equal to …" before token keyword parsing so Broodspinner-style "flying equal to the number of …" insects keep the Flying keyword. Fixes phase-rs#2854 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): use nom for equal-to token keyword suffix strip Replace forbidden .split(" equal to ") dispatch with take_until-based helpers so the parser combinator gate passes on PR phase-rs#3151. Co-authored-by: Cursor <cursoragent@cursor.com> * test(PR-3151): drive flying token regression through parser * fix(PR-3151): keep token keywords before quoted statics --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…s#3149) * fix(parser): restrict Lulu stun target to attacking creatures Parse the "creature attacking you" suffix as Attacking { defender: You } so Stern Guardian's trigger only legalizes attackers, not every creature. Fixes phase-rs#2386 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): do not swallow target if-clause after attacking you Reject postnominal "attacking you" when followed by "if/unless" so Stalking Leonin-style target gates are not split incorrectly, which regressed swallowed-clause coverage on PR phase-rs#3149. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-3149): cover defender-scoped attacking planeswalker phrases * fix(parser): resolve Boarded Window swallowed-clause regression Parse "creatures attacking you" statics and "you were dealt N or more damage this turn" intervening-if conditions so Lulu PR phase-rs#3149 no longer surfaces hidden swallow warnings on Boarded Window after the attacking- you suffix parser landed. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-3149): parse opponent damage-threshold conditions --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…o consumer anaphors stay chain-local (phase-rs#2350) (phase-rs#3156)
…hase-rs#1134) (phase-rs#3157) Inspirit, Flagship Vessel's hexproof/indestructible grant is gated on HasCounters{charge >= 8}; assert it tears down when counters drop below 8. Station mechanic shipped in 67bf02e; this locks in the teardown behavior. CR 611.3a: continuous effects from static abilities are not locked in; they apply only while the condition holds. CR 122.1: counters on objects.
…ill a planeswalker' animation (phase-rs#1155) (phase-rs#3159) Parse 'During your turn, ~ is a [P/T] [types] creature with [keywords] that's still a planeswalker' as a DuringYourTurn type-change static. Adds the 'is'-copula arm to parse_pronoun_becomes_type_static and generalizes the still-a-<type> rider stripper (was land-only). CR 205.1b: 'still a [type]' retains prior card types (AddType is additive). CR 613.7: continuous type-change from a static ability.
…kept-to-top destination (phase-rs#2349) (phase-rs#3155)
… as ChangeZoneAll (phase-rs#656) (phase-rs#3160) * test(engine): regression for Station counter-gated static teardown (phase-rs#1134) Inspirit, Flagship Vessel's hexproof/indestructible grant is gated on HasCounters{charge >= 8}; assert it tears down when counters drop below 8. Station mechanic shipped in 67bf02e; this locks in the teardown behavior. CR 611.3a: continuous effects from static abilities are not locked in; they apply only while the condition holds. CR 122.1: counters on objects. * fix(parser): return each/all to battlefield preserves enter-with-counters as ChangeZoneAll (phase-rs#656) 'Return each creature card from your graveyard to the battlefield' with a finality counter collapsed to single-target ChangeZone because the mass branch was gated on enter_with_counters being empty. Thread the counters through ReturnAllToZone -> ChangeZoneAll so mass returns apply them. CR 122.1h: a permanent with a finality counter that would go to graveyard is exiled instead. CR 400.7: zone change creates a new object.
* fix(parser): absorb 'the tokens are goaded [duration]' after token creation * test(PR-3158): cover saga-scoped token goad continuation --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…, not the source (phase-rs#1146) (phase-rs#3161) * test(engine): regression for Station counter-gated static teardown (phase-rs#1134) Inspirit, Flagship Vessel's hexproof/indestructible grant is gated on HasCounters{charge >= 8}; assert it tears down when counters drop below 8. Station mechanic shipped in 67bf02e; this locks in the teardown behavior. CR 611.3a: continuous effects from static abilities are not locked in; they apply only while the condition holds. CR 122.1: counters on objects. * fix(parser): Gonti-class look-and-exile-face-down exiles the dug card, not the source (phase-rs#1146) 'Look at top N, exile one of them face down, then play it' lowered to a keep_count:0 peek + sibling ChangeZone{ParentTarget}, which short-circuited the dig choice and exiled the trigger source instead of the chosen card. Fuse it into Dig{keep_count:1, destination:Exile} (Hideaway model) so the player-selected card is the one exiled. CR 701.20e: looking is private. CR 406.3/708.2: face-down exile. CR 702.75a: Hideaway structural analog.
* refactor(client): remove dead components, orphaned tests, and unused banding helpers
Delete confirmed-orphan frontend components (zero import sites) plus their
colocated tests, and prune the now-unused banding helpers they were the sole
consumers of.
Components removed:
- combat/{CombatOverlay,AttackerControls,BlockerControls,BlockerArrow} — live
combat UI is ActionButton; these were superseded and unmounted.
- log/GameLog — superseded by GameLogPanel.
- menu/{AiDifficultyDropdown,BackButton,GamePresets,MainMenuActionCard,MenuLogo}
- lobby/LobbyDraftRooms (+ CreateServerDraftForm export, also unused)
- controls/PhaseTracker, multiplayer/ServerOfflineDialog, targeting/TargetArrow,
zone/ZoneIndicator
utils/combat.ts: removed isLegalBand/hasBanding (CR 702.22c band-legality), whose
only consumer was the deleted CombatOverlay. This also removes the last instance
of MTG rules logic computed in the frontend — band legality is the engine's
responsibility (it re-validates declared bands on submit). buildAttacks,
getDefaultAttackTarget, hasMultipleAttackTargets, getValidAttackTargets retained.
Verified: tsc clean, eslint 0 errors, 1336/1336 vitest pass.
* perf(client): cut redundant per-action work on the board hot path
Four isolated frontend perf fixes, no behavior change:
- BattlefieldBackground: the full-deck dominant-color scan (library + hand +
battlefield) ran inside a useMemo keyed on the whole gameState, so it
recomputed on every action — yet its result is consumed only by the
'auto-wubrg' background and only until a color-matched image is locked in.
Short-circuit the scan when boardBackground != 'auto-wubrg' or already locked.
- battlefieldProps.groupKey: replace JSON.stringify(obj.counters) (a serialize
allocation per permanent on every board rebuild) with a sorted-entries concat.
Also insertion-order-stable, so identical permanents group regardless of the
order counters were applied (matches the sorted keyword key alongside it).
- LogEntry: wrap in memo. The log panel re-renders on every search keystroke /
filter toggle / verbosity change; entry objects and the inspect callback are
stable refs, so memo lets unchanged rows skip those re-renders.
- CardCoverageDashboard: four views each independently fetched and parsed the
same large coverage JSON. Add a module-cached useCoverageData() hook so the
fetch + parse happens once and is shared across views/tab switches.
Verified: tsc clean, eslint clean, 1336/1336 vitest pass.
* fix(client): tighten async-hook lifecycle and effect dependencies
- useCardImage: the image-load effect listed the raw tokenImageRef object in its
deps, so a caller passing a fresh inline {scryfall_id,...} object each render
re-fired the effect (release + refetch the cached image src) for an unchanged
image. Stabilize the ref's identity to tokenImageRefKey via useMemo — the key
captures exactly the fields fetchTokenImageByRef reads. The lone exhaustive-deps
disable is scoped to the one-line memo, keeping the effect's dep check honest.
- useResumables: the async loadP2PSession/loadGame .then() callbacks called
setMatch/setMatchSummary with no unmount guard. Add the cancelled-flag pattern
used by the other async hooks so we never setState on an unmounted component.
- usePrintingsLoaded: depend on [] (the module-level "resolved" flag is the
load-once guard) instead of [loaded], which re-subscribed once after
setLoaded(true) only to immediately bail; add an unmount guard on the async
setState.
Verified: tsc clean, eslint clean, 1336/1336 vitest pass.
* fix(client): wire long-press card preview in modal/zone card lists on mobile
Card lists rendered in a loop (choice modals, zone viewers, search/crew/etc.)
use useInspectHoverProps -- which skipped mouse handlers on touch (a synthesized
mouseenter would open the dismiss-looping preview and block selection) but never
provided a long-press fallback. So board/hand cards got the long-press preview on
mobile and modal cards did not. Not a regression -- never implemented for the
list-render path (the hook was added for mobile selection in Apr 2026 with
long-press deferred).
Wire long-press centrally in useInspectHoverProps so all ~17 consumers get it
with no per-call-site changes: a single shared long-press timer (only one pointer
is active at a time) records the pressed id via onPointerDown and shows the
sticky preview on fire, mirroring useCardHover. The click a browser synthesizes
after the press is swallowed in the capture phase (runs before the caller's
bubble-phase onClick), so the gesture previews without also toggling selection --
callers keep their plain onClick. CardPreview renders at z-100 over modals at
z-50, so no portal/z-index change is needed.
Removed the now-redundant explicit data-card-hover attribute from the three
callers that hand-set it (the hook now always provides it).
Verified: tsc clean, eslint clean, 1336/1336 vitest pass.
* refactor(client): add shared assertNever exhaustiveness guard
Add a shared assertNever(value: never) helper and apply it where a switch over a
closed engine union is meant to be exhaustive:
- LogEntry.renderSegment switched over all 7 LogSegment variants with no default,
so a new engine segment type would silently render nothing. A default calling
assertNever now makes that a compile error instead.
- Consolidate AlternativeCostModal's local assertNeverKeyword onto the shared
helper.
Scoped deliberately to already-exhaustive switches. UI switches that
intentionally handle a subset of an engine union (most WaitingFor/GameAction
dispatchers) are left as-is — forcing assertNever onto a partial switch would
mean handling variants the display layer correctly ignores.
Verified: tsc clean, eslint clean, 1336/1336 vitest pass.
* fix(client): address review feedback on coverage cache + groupKey comment
- CardCoverageDashboard.useCoverageData: the module-level coveragePromise cached
rejections permanently, so a transient fetch/HTTP/JSON failure would stick
across all view mounts with no retry until a full page reload (a regression vs
the old per-view fetch, which retried on each mount). Clear the cached slot in
.catch so only successful results stay cached and the next mount re-fetches.
- battlefieldProps.groupKey: reword the comment from insertion-order-stable to
order-independent (the sorted key is order-independent, a correctness
improvement over the old insertion-order JSON.stringify). Comment only.
Verified: tsc clean, eslint clean, 1336/1336 vitest pass.
…ed into a QuantityExpr on an ef (phase-rs#3163) * fix(parser): Trailing 'for each ...' multiplier clause is not converted into a QuantityExpr on an ef Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(PR-3163): harden for-each multiplier parsing --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…hase-rs#3164) * fix(client): address Gemini review feedback from phase-rs#3162 Three follow-ups to the frontend cleanup pass (phase-rs#3162), from Gemini code-assist review: - BattlefieldBackground: resolveBackground locks the chosen image in lockedRef for the "random" and "auto-wubrg" modes, but the lock was never cleared when the background mode or viewed player changed, so switching modes mid-session (or switching seats) kept showing the stale locked image. Reset lockedRef when boardBackground or playerId changes (compare-prior-value-during-render idiom). - CardCoverageDashboard.useCoverageData: switching dashboard tabs remounts the view, and the hook initialized loading:true every mount, flashing a spinner even when the doc was already cached. Keep the resolved document in a module-level cachedCoverage and initialize state synchronously from it. - battlefieldProps.groupKey: guard Object.entries(obj.counters ?? {}) so a missing counters map (partial mocks / defensive) can't throw a TypeError on the board-rebuild hot path — restoring the robustness the old JSON.stringify(obj.counters) happened to have. Verified: tsc clean, eslint clean, 1336/1336 vitest pass. * feat(engine): expose is_mana_ability on serialized abilities (CR 605.1a) The frontend's isManaObjectAction (viewmodel/cardActionChoice.ts) decided whether an ActivateAbility routes through the mana-tap ring by introspecting the serialized ability AST (`effect.type === "Mana"`) — engine-owned structural knowledge leaking into the display layer, and a shallower check than the engine's own CR 605.1a classifier (it ignored the no-target guard). Expose the engine's authoritative classification instead: AbilityDefinition's Serialize impl now emits a derived `is_mana_ability` UI key (skip_serializing_if false), computed via the single-source-of-truth mana_abilities::is_mana_ability classifier — mirroring the existing `consumes_source` derived key. The frontend reads the flag; it never inspects the effect tree. - engine: AbilityDefinition Serialize emits is_mana_ability (types/ability.rs); serialization test in mana_abilities.rs. - client: SerializedAbility.is_mana_ability (types.ts); isManaObjectAction reads it (cardActionChoice.ts). - snapshots: 11 oracle_ir / integration snapshots gain the additive key for their mana-source abilities (Birds of Paradise, Boseiju, Llanowar Elves, Manamorphose, Mox Pearl, Harold and Bob). Regenerated via cargo insta; purely additive. Verified: Tilt clippy + wasm green; cargo insta full engine run green (incl. new test); frontend tsc + eslint clean. * test(client): update isManaObjectAction fixture to engine is_mana_ability flag Phase 3.1 inverted isManaObjectAction from effect-AST introspection to reading the engine-provided is_mana_ability flag (CR 605.1a), but the test fixture still built a mana ability without the flag, so the "recognizes only engine-provided mana actions" case asserted true and now received false. Add is_mana_ability: true to the mana-ability fixture to match the contract the test name already describes. * fix(client): reset BattlefieldBackground lock via remount key, not render-phase ref writes Gemini flagged the lastModeRef/lastPlayerRef prev-value tracking added in the prior follow-up: mutating refs during render is unsafe under concurrent / StrictMode rendering. Replace it with a remount key — GamePage keys BattlefieldBackground on `${boardBackground}-${playerId}`, so a mode or seat change unmounts/remounts the component with a fresh null lockedRef. Same reset semantics (returning to "random" re-rolls the image) with a pure render. The remaining lockedRef writes inside resolveBackground are the lazy-init ref pattern React explicitly blesses, so they stay. Refutes Gemini's CardCoverageDashboard comment: no test imports the module, so the module-cache "test leakage" has nothing to leak into; an exported test-only resetCoverageCache() would be building for a hypothetical (CLAUDE.md). The cache is purely beneficial in production (skips the tab-switch loading flash).
…ion so ETB observers fire (phase-rs#830) (phase-rs#3167)
…yard for milled cards (phase-rs#751) (phase-rs#3168)
…+ unbounded reviews) Maintainer feedback on PR phase-rs#3163: non-trivial engine/parser PRs must go through the /engine-implementer skill so the architectural seam and CR validation are reviewed before code is generated. Wire deck-contribute (mechanic clusters) and contribute-card (per card) to embody that contract: - implement + fix steps run as the `engine-implementation-executor` agent - plan-review and impl-review loop until a full round is CLEAN (caps are runaway-loop safeguards, not a two-rounds-and-ship gate); a non-clean plan now also marks the unit partial - review-impl must confirm the card actually parses correctly (discriminating runtime check), not just that the diff looks clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Author
|
Closing — has already landed in phase-rs/phase at . This PR is superseded. |
AgilErck
pushed a commit
that referenced
this pull request
Jun 17, 2026
…phase-rs#2672) Derived from a week-long retrospective of the AI-contribution pipeline (Gemini, /review-impl, /pr-contribution-handler) over ~2,000 PR comments. Stops the most-flagged author-side defects at the source. - engine-implementation-executor: blocking discriminating-test gate (the #1 finding across every reviewer was AST-shape-only tests that pass when the fix is reverted) and CR-annotation diff gate (catches hallucinated subparts like CR 702.808); extend the parser-diff-gate regex to rfind/split/split_once (check-parser-combinators.sh blind spots). - engine-implementer: Step 4 confirms both new gates before commit. - pr-contribution-handler: duplicate-PR + scope-contamination intake gate and enqueue item; post comments via --body-file (shell backtick mangling) and batch comment-fetch (rate-limit). - review-impl: codify CR +// compound-annotation convention so Gemini's format pedantry is refuted, not echoed. - AI-CONTRIBUTOR: contributor-side duplicate-PR check (3.1), branch off current upstream/main (4), and test-discrimination + rfind/split in the 5 cross-check.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
16 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
deck-contribute.jsbesidecontribute-card.js. It's a deck-level triage front-end that reusescontribute-cardfor the per-card units.Pipeline: Parse → Classify → Audit → Cluster → Implement
cargo semantic-audit; buckets each card as unsupported / supported-flagged / supported-clean.Add Suspend mechanic (N cards)), and skips mechanics that already have an open upstream PR.limit > 0) — one independent fork PR per unit offupstream/main: mechanic clusters via an inline class-level plan→impl→review→cross-check→verify→PR pipeline; one-off + misparsed cards via nestedcontribute-card.Design notes
limit: 0= triage-only — a bare run just reports the buckets, opens no PRs.main(fork → upstream); stacked PRs aren't possible without upstream write access.contribute-card.jspath is relative).🤖 Generated with Claude Code