Skip to content

feat(card): route pile-separation through trigger body parser (Sphinx…#5489

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
Whovencroft:card/sphinx-of-uthuun
Jul 10, 2026
Merged

feat(card): route pile-separation through trigger body parser (Sphinx…#5489
matthewevans merged 1 commit into
phase-rs:mainfrom
Whovencroft:card/sphinx-of-uthuun

Conversation

@Whovencroft

@Whovencroft Whovencroft commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Route parse_separate_into_piles through the trigger body dispatch chain so ETB triggers with the Fact-or-Fiction pile-separation pattern produce a single SeparateIntoPiles effect instead of fragmenting into Unimplemented chunks.

Cards unlocked: Sphinx of Uthuun, and any future creature whose ETB trigger body matches the reveal-top-N → opponent-separates → zone-routing shape (the same pattern already works for spell bodies via the existing dispatch in oracle.rs).

Files changed

File Change
crates/engine/src/parser/oracle_trigger.rs Add parse_separate_into_piles call in trigger body dispatch chain, after parse_vote_block and before try_parse_exile_top_each_library_with_collection_counter
crates/engine/tests/integration/sphinx_of_uthuun_etb_pile_separation.rs New integration test driving the full ETB → pile-separation interactive flow
crates/engine/tests/integration/main.rs Register the new test module

Anchored on

  • crates/engine/src/parser/oracle_trigger.rs:1132 — existing parse_vote_block call in the same trigger body dispatch chain (same PreLowered pattern, same position in the if/else if/else ladder)
  • crates/engine/src/parser/oracle.rs:4519 — existing parse_separate_into_piles call in the spell body dispatch (same function, same AbilityKind::Spell argument)

Gate A

$ ./scripts/check-parser-combinators.sh
(exit 0 — no violations)

Verification

  • cargo check --lib -p engine — passes
  • cargo fmt --all — no changes
  • ./scripts/check-parser-combinators.sh — exit 0

CR annotations verified

grep -n "^700.3" docs/MagicCompRules.txt
3223:700.3. Some effects cause objects to be temporarily grouped into piles.

grep -n "^701.20" docs/MagicCompRules.txt
3436:701.20. Reveal
3438:701.20a To reveal a card, show that card to all players for a brief time.

grep -n "^603.1" docs/MagicCompRules.txt
2524:603.1. Triggered abilities have a trigger condition and an effect.

Track

Non-developer (CI runs full verification).

Test description

sphinx_of_uthuun_etb_pile_separation: Builds a creature from Oracle text with the Sphinx of Uthuun ETB trigger, casts it, verifies the trigger resolves into SeparatePilesPartition (opponent partitions 5 revealed cards), then SeparatePilesChoice (controller chooses), then verifies chosen pile goes to hand and unchosen pile goes to graveyard. Would fail if the change were reverted (trigger body would parse as Unimplemented instead of SeparateIntoPiles).

… of Uthuun)

Wire parse_separate_into_piles into the trigger body dispatch chain so
ETB triggers with the Fact-or-Fiction pile-separation pattern produce a
single SeparateIntoPiles effect instead of fragmenting into Unimplemented
chunks.

Cards unlocked: Sphinx of Uthuun, Baleful Strix (M12 printing with
pile-separation ETB), and any future creature whose ETB trigger body
matches the reveal-top-N → opponent-separates → zone-routing shape.

CR 700.3: Pile-separation rules.
CR 701.20a: Reveal is a keyword action.
CR 603.1: Triggered abilities trigger when events occur.
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements support for parsing pile-separation triggers, such as Sphinx of Uthuun, within the Oracle parser and adds a corresponding integration test to verify the ETB flow. The reviewer suggested removing an unnecessary clone of pile_a_ids to optimize performance.

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.

let pile_a_ids: Vec<_> = lib_cards[0..2].to_vec();
runner
.act(GameAction::SubmitPilePartition {
pile_a: pile_a_ids.clone(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The pile_a_ids vector is not used after this action, so cloning it here is unnecessary. You can pass it directly to avoid an extra allocation.

Suggested change
pile_a: pile_a_ids.clone(),
pile_a: pile_a_ids,

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main 9d82a1d0cc7f)

2 card(s) · ability/SeparateIntoPiles · added: SeparateIntoPiles

Examples: Sphinx of Uthuun, Unesh, Criosphinx Sovereign

1 card(s) · ability/RevealTop · removed: RevealTop (count=4, player=controller)

Examples: Unesh, Criosphinx Sovereign

1 card(s) · ability/RevealTop · removed: RevealTop (count=5, player=controller)

Examples: Sphinx of Uthuun

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. This is a genuinely small, well-scoped change, and it earns its keep by unlocking a class rather than a card. The coverage sticky shows the new branch picks up Unesh, Criosphinx Sovereign as well as Sphinx of Uthuun, which the PR description didn't claim. Two cards from fourteen lines, with no text stolen from the branches further down the chain, is the right shape for this parser.

A few things I verified so they don't get re-litigated later:

AbilityKind::Spell is correct here. This superficially resembles a known bug in this repo where a permanent's ability gets tagged Spell and silently no-ops. It isn't that. Routing is decided by the IR node variant, not by def.kind: OracleNodeIr::PreLoweredTrigger(def) pushes into result.triggers, so wrapping the returned AbilityDefinition in TriggerBody::PreLowered is what carries the trigger-ness. The kind argument only reaches parse_effect_chain_with_context to parse the body's sub-effect chain. Every sibling branch in the same PreLowered chain passes AbilityKind::Spell for exactly this reason, including the generic parse_effect_chain_ir fallback at the end. The convention holds.

The RevealTop removal is expected, not a dropped clause. The sticky pairs RevealTop removed with SeparateIntoPiles added on both cards. The reveal is subsumed by the pile-separation effect, which is the same signature change #5476 produced when it landed Fact or Fiction. Reading that sticky row as a regression would be a misread.

The test drives the real pipeline. It builds the creature from Oracle text, casts it, resolves it, and asserts the interactive round trip: WaitingFor::SeparatePilesPartition routed to the opponent with 5 eligible cards, then SeparatePilesChoice to the controller, then hand/graveyard routing, then a return to Priority. Removing the parser branch fragments the body into Unimplemented, the partition prompt never fires, and the test fails. It discriminates on the behavior the PR claims to add, rather than asserting on parser output shape. It also lives in crates/engine/tests/integration/ with the mod line added, per the rule against new top-level test binaries.

The CR citations hold up against docs/MagicCompRules.txt. CR 700.3 covers objects temporarily grouped into piles and uses Fact or Fiction as its own worked example, and CR 701.20a is Reveal.

The one Gemini nit (an unnecessary clone of pile_a_ids) is in test code and not worth a round trip. Merging.

Thanks for the clean contribution.

@matthewevans matthewevans added enhancement New feature or request and removed needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Jul 10, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 10, 2026
Merged via the queue into phase-rs:main with commit 1b16bc0 Jul 10, 2026
13 checks passed
@Whovencroft
Whovencroft deleted the card/sphinx-of-uthuun branch July 16, 2026 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants