Fix Grave Sifter per-player creature-type graveyard return (#5279)

[andriypolanski]

Closes #5279

Summary

Discord report title "Grace Sifter" maps to Grave Sifter (C14). Oracle: each player chooses a creature type, then returns any number of matching cards from their graveyard to their hand.

Three bugs blocked the ETB:

1. Wrong chosen-type filter — after a CreatureType choice, "cards of that type" parsed as IsChosenCardType (card-typed base). Runtime stores the choice on ChosenAttribute::CreatureType, so no graveyard cards matched and the return step was skipped.
2. Missing up-to — "returns any number of" did not set up_to: true on the graveyard ChangeZone.
3. Player-scope chain split — parser stamps player_scope: All on both Choose and ChangeZone. Keeping the return inside the scoped template without clearing the child's redundant player_scope re-entered the fan-out driver mid-instruction, skipping P0's graveyard return and jumping to P1's creature-type choice.

Files changed

• crates/engine/src/parser/oracle_target.rs — IsChosenCreatureType when pending choice is CreatureType
• crates/engine/src/parser/oracle_effect/imperative.rs — strip "any number of ", thread up_to
• crates/engine/src/parser/oracle_ir/ast.rs — up_to on ReturnToZone
• crates/engine/src/parser/oracle_effect/mod.rs — ReturnToZone lowering sites
• crates/engine/src/game/effects/mod.rs — is_player_scope_local_continuation + clear redundant child player_scope
• crates/engine/tests/integration/issue_5279_grave_sifter.rs — regression tests
• crates/engine/tests/integration/main.rs — mod registration

Test plan

• grave_sifter_etb_parses_choose_creature_type_per_player — Choose(CreatureType) + player_scope: All, sub ChangeZone with IsChosenCreatureType, origin: Graveyard, up_to: true
• grave_sifter_etb_prompts_creature_type_choice — APNAP NamedChoice with Bear in options
• grave_sifter_each_player_creature_type_return_from_graveyard — P0 chooses Bear → EffectZoneChoice for Grizzly Bears → P1 chooses Giant → bear returns to hand
• split_player_scope_keeps_reveal_anaphora_chain_but_detaches_aggregate — Choose → ChangeZone stays inside scoped template; child player_scope cleared

Verification

cargo fmt --all
cargo test -p engine --lib split_player_scope_keeps_reveal
cargo test -p engine issue_5279 -- --nocapture

• cargo fmt --all
• cargo test -p engine --lib split_player_scope_keeps_reveal
• cargo test -p engine issue_5279 -- --nocapture
• tilt logs clippy / tilt logs test-engine (when Tilt is up)

[gemini-code-assist]

Code Review

This pull request implements support for Grave Sifter (issue #5279) by allowing players to choose a creature type and return any number of matching cards from their graveyard to their hand. It updates the effect resolution logic to treat the choice and the zone change as a single per-player instruction, clears redundant player scopes on co-scoped continuations, parses the 'any number of' quantifier, and correctly resolves 'cards of that type' to the chosen creature subtype. The review feedback suggests optimizing the parser in imperative.rs by using nom::bytes::complete::tag_no_case to avoid unnecessary string allocations on every target parsing pass.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

Avoid allocating a new String via to_ascii_lowercase() on every target parsing pass. Since "any number of" is always lowercase in this context (or can be matched case-insensitively), you can use nom::bytes::complete::tag_no_case directly on target_text to avoid unnecessary allocations on a hot parser path.

        let any_number_prefix = nom::bytes::complete::tag_no_case::<_, _, OracleError<'_>>("any number of ")
            .parse(target_text)
            .ok()
            .map(|(rest, _)| target_text.len() - rest.len());
        let (target_text, return_up_to) = match any_number_prefix {
            Some(consumed) => (&target_text[consumed..], true),
            None => (target_text, false),
        };

[matthewevans]

Verdict: architecturally sound — three real bugs, all fixed at the right seam. Holding only for CI (Rust gates still pending, branch is BEHIND); approve as bug on settled green.

I verified the premise against the printed card before reviewing the design. Grave Sifter's Oracle text (Scryfall, verbatim):

"When this creature enters, each player chooses a creature type and returns any number of cards of that type from their graveyard to their hand."

Each of the three bugs in the description maps to a clause of that sentence, so this is a genuine defect trio, not a speculative refactor.

✅ Clean

1. Chosen-attribute axis (oracle_target.rs:2986). "cards of that type" has a card-typed head noun, so the old code unconditionally emitted IsChosenCardType — reading the wrong ChosenAttribute axis, matching nothing, and silently skipping the return. Keying the override on ctx.pending_choice_type == Some(ChoiceType::CreatureType) is the right discriminator: it resolves the anaphor against what was actually chosen rather than against the head noun. IsChosenCreatureType is a pre-existing FilterProp (ability.rs:3573) — no new variant.

2. up_to threading (imperative.rs:1778, ast.rs:971). Effect::ChangeZone.up_to already existed and was hardcoded false at the lowering site; the parser simply never carried the quantifier. Stripping "any number of " with tag() and threading the flag onto the AST node fixes the classic parsed-but-not-consumed shape. CR 107.1c grep-verified verbatim: "If a rule or ability instructs a player to choose 'any number,' that player may choose any positive number or zero." — exactly the zero-or-more semantics up_to: true encodes.

3. Player-scope chain split (effects/mod.rs:2478). The load-bearing fix, and it's at the correct seam: a new arm on the existing is_player_scope_local_continuation rather than a bespoke branch in the driver. Keying on the effect shape (Choose { persist: true } → ChangeZone { origin: Some(_) }) rather than on a card or a specific zone makes this cover the whole per-player choose-then-move class, not just Grave Sifter. CR 101.4 (APNAP) grep-verified — each player's return must run inside their own iteration, not after all choices resolve.

Tests are discriminating and registered. mod issue_5279_grave_sifter is wired into tests/integration/main.rs (not inert), and it lives under tests/integration/ rather than as a top-level binary. grave_sifter_each_player_creature_type_return_from_graveyard drives the real runtime path end-to-end (P0 chooses Bear → EffectZoneChoice → P1 chooses Giant → bear returns to hand), which is what actually discriminates bug #1 — a parse-only assertion would not have caught the ChosenAttribute axis mismatch. The unit test asserting the kept ChangeZone's player_scope is cleared pins bug #3 precisely.

🟡 Non-blocking

Redundant disjunct in the new guard (effects/mod.rs:2660). next_is_co_scoped_anaphoric_consumer already clears next.player_scope unconditionally at effects/mod.rs:2634-2636, before the outer if is even evaluated. Re-testing it inside the new inner if is a no-op. The condition can collapse to just:

if is_player_scope_local_continuation(&node.effect, &next.effect) {
    next.player_scope = None;
}

Same behavior, and it makes the new clearing rule read as the single thing it actually is.

CR 205.2a is the imprecise citation (oracle_target.rs:2986). 205.2a enumerates card types (artifact, creature, land, …). The rule that actually justifies the override is CR 205.3m — creature types are subtypes, which is precisely why a chosen "Bear" cannot be a card type and the head noun must not drive the axis. Suggest swapping the annotation to CR 205.3m (+ 608.2c for the anaphor), so a future grep lands on the rule the code depends on.

Widened origin relative to its siblings (effects/mod.rs:2481). Every existing ChangeZone arm in this matcher pins origin: Some(Zone::Library); the new arm accepts origin: Some(_). I think that's correct — Choose { persist: true } is the load-bearing guard, and a persisting per-player choice followed by a zone move that consumes it is per-player by construction regardless of origin zone — but it is the broadest arm in the function, so it's worth being deliberate that this is intended to cover the class (graveyard, hand, exile returns alike) rather than an unintended widening.

Recommendation

Approve as bug once the Rust gates settle green and the branch is rebased (currently BEHIND, all four Rust checks still pending). The two annotation/simplification notes above are non-blocking and can ride along or land separately — neither changes behavior.

[github-actions]

Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main 8a6806e39c13)

1 card(s) · ability/ChangeZone · field target: chosen card type scoped player controls in graveyard card → chosen creature type scoped player controls in graveyard card

Examples: Grave Sifter

1 card(s) · ability/ChangeZone · field up_to: ∅ → true

Examples: Grave Sifter

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Approving — the two nits are cleared and all Rust gates are green on this head.

✅ Clean

• Chosen-attribute axis (oracle_target.rs:2984) — keying on ctx.pending_choice_type resolves "cards of that type" against the chosen creature type rather than the card-typed head noun, reusing the pre-existing IsChosenCreatureType. CR 205.3m (creature types are subtypes) is now the correct citation.
• up_to threading — Effect::ChangeZone.up_to was a parsed-but-not-consumed field hardcoded false at the lowering site; "returns any number of cards" now honors CR 107.1c.
• Player-scope chain split (effects/mod.rs:2478) — new arm on the existing is_player_scope_local_continuation, keyed on effect shape (Choose{persist} → ChangeZone{origin}) rather than on a card. CR 101.4 (APNAP) verified.
• Redundant disjunct removal (effects/mod.rs:2661) — verified safe: next_is_co_scoped_anaphoric_consumer already clears next.player_scope upstream, and the outer retention disjunction still reads the flag, so retention behavior is preserved.
• mod issue_5279_grave_sifter is registered in tests/integration/main.rs and drives the real runtime path end-to-end — a parse-only assertion would not have caught the axis mismatch.

Approved and enqueuing as bug.