Skip to content

fix(engine): parse and resolve Nether Spirit's dropped self-reanimation intervening-if#5884

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
rykerwilliams:fix/nether-spirit-self-reanimation-condition
Jul 15, 2026
Merged

fix(engine): parse and resolve Nether Spirit's dropped self-reanimation intervening-if#5884
matthewevans merged 2 commits into
phase-rs:mainfrom
rykerwilliams:fix/nether-spirit-self-reanimation-condition

Conversation

@rykerwilliams

Copy link
Copy Markdown
Contributor

Summary

Nether Spirit ({1}{B}{B}, Tempest): "At the beginning of your upkeep, if this card is the only creature card in your graveyard, you may return this card to the battlefield."

The intervening-if was dropped entirely by the parser (condition: null in card-data.json). split_trigger splits Oracle text at the first unrecognized comma; since "At the beginning of your upkeep" has no comma, the whole "if this card is the only creature card in your graveyard, you may return this card to the battlefield" fell through to the effect side untouched — there was no combinator in parse_remaining_state_presence_conditions recognizing "~ is the only <type> [card] in <zone>". That also left trigger_zones stuck at the structural default ["Battlefield"] (making the trigger undetectable while the card sits in the graveyard) and ChangeZone.origin null.

  • Adds parse_source_is_only_type_in_zone (oracle_nom/condition.rs), composed entirely from existing building blocks (parse_source_self_token, parse_type_phrase, an ObjectCount == 1 QuantityComparison). When the type-phrase filter carries a non-battlefield zone, wraps the condition in StaticCondition::And{[SourceInZone{zone}, quantity]} so the existing, unmodified trigger_condition_source_zones walker auto-derives trigger_zones/ChangeZone.originzero runtime code changed. Precedent: Jocasta, Automaton Avenger already drives the identical And{SourceInZone, ...} derivation end-to-end.
  • Removes a stale game/coverage.rs exclusion that was silencing the semantic auditor for this exact bug (confirmed via card-data.json scan: the excluded substring matches exactly this one card).

CR 603.4 (intervening-if, checked at trigger time and re-checked at resolution), CR 113.6 / CR 113.6b (ability functions off-battlefield only from the zone it states).

Test plan

  • 3 new parser unit tests (condition.rs): graveyard form → And[SourceInZone(Graveyard), ObjectCount==1]; battlefield form → bare QuantityComparison (no spurious SourceInZone); rejection guard (no "only" → parse error)
  • Parser-shape test (oracle_trigger_tests.rs): asserts condition.is_some(), trigger_zones == [Graveyard], ChangeZone.origin == Some(Graveyard) for Nether Spirit's verbatim Oracle text
  • 6 integration tests (nether_spirit_only_creature_card_intervening_if.rs) driving the real process_triggers/stack::resolve_top pipeline: does-not-fire with 2 creature cards, positive reach-guard, fires-and-returns when sole, decline leaves it in graveyard, CR 603.4 resolution-time recheck fizzles when a 2nd creature card enters in response, two-copies-neither-fires
  • Live-revert-and-rerun proof: disabling the new combinator's registration reproduces failure in the parser-shape test and 4/6 integration tests (2 negative-guard tests pass vacuously as expected, guarded by the positive reach-guard test)
  • cargo fmt --all, parser combinator gate, CR-annotation gate all pass
  • cargo clippy -p engine --lib --tests -- -D warnings clean
  • Full cargo test -p engine --lib (16687+ tests) and --test integration green after rebase onto current main

rykerwilliams added a commit to rykerwilliams/phase that referenced this pull request Jul 15, 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 parsing and runtime support for Nether Spirit's intervening-if condition ("~ is the only [card] in "). It introduces the parse_source_is_only_type_in_zone parser, which correctly hoists off-battlefield zones into a SourceInZone conjunct to allow proper trigger detection and zone origin derivation. Comprehensive unit and integration tests are added to verify the behavior. The reviewer suggests simplifying the parser's return statement by returning the parsed remainder subslice directly instead of calculating a consumed offset via pointer arithmetic.

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.

Comment on lines +4178 to +4179
let consumed = remainder.as_ptr() as usize - input.as_ptr() as usize;
Ok((&input[consumed..], condition))

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

Calculating the consumed offset via pointer arithmetic is unnecessary here because remainder is already a valid subslice of input with the correct lifetime. Returning remainder directly is safer, cleaner, and more idiomatic.

    Ok((remainder, condition))

@matthewevans matthewevans self-assigned this Jul 15, 2026
rykerwilliams and others added 2 commits July 15, 2026 13:50
…on intervening-if

CR 603.4: Nether Spirit's "if this card is the only creature card in your
graveyard" clause was never hoisted into TriggerDefinition.condition -
split_trigger's first-comma split had nothing to land on since "At the
beginning of your upkeep" has no comma, so the entire intervening-if +
effect fell through to the effect side untouched, leaving condition: null.
That also left trigger_zones stuck at the structural default
["Battlefield"], making the trigger undetectable from the graveyard, and
ChangeZone.origin null.

Adds parse_source_is_only_type_in_zone (oracle_nom/condition.rs), a new
combinator for "~ is the only <type> [card] in <zone>" composed from
existing building blocks (parse_source_self_token, parse_type_phrase,
ObjectCount == 1 QuantityComparison). When the type-phrase filter carries
a non-battlefield zone, wraps the condition in
StaticCondition::And{[SourceInZone{zone}, quantity]} so the existing,
unmodified trigger_condition_source_zones walker (CR 113.6 / CR 113.6b)
auto-derives trigger_zones and ChangeZone.origin - zero runtime code
changed. Precedent: Jocasta, Automaton Avenger already drives the same
And{SourceInZone, ...} derivation end-to-end.

Also removes a stale game/coverage.rs exclusion that was silencing the
semantic auditor for this exact bug.
@matthewevans
matthewevans force-pushed the fix/nether-spirit-self-reanimation-condition branch from 8fbcc4a to 7b16bc6 Compare July 15, 2026 20:51
@matthewevans matthewevans added the bug Bug fix label Jul 15, 2026
@matthewevans matthewevans removed their assignment Jul 15, 2026
@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 1 card(s), 3 signature(s) (baseline: main 674af5650599)

1 card(s) · ability/ChangeZone · field from: graveyard

Examples: Nether Spirit

1 card(s) · trigger/Phase · field active in: battlefieldgraveyard

Examples: Nether Spirit

1 card(s) · trigger/Phase · field condition: source is in graveyard and # of in graveyard you control creature = 1

Examples: Nether Spirit

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

@matthewevans matthewevans self-assigned this Jul 15, 2026

@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.

Verdict: Approved

🔴 Blocker

None.

🟡 Non-blocking

None.

✅ Clean

  • The condition is parsed at the existing state-condition authority and uses the established ObjectCount == 1 plus SourceInZone representation, allowing existing trigger-zone and return-origin derivation to do the runtime work.
  • The current parse diff is exactly one card, Nether Spirit: condition, active trigger zone, and ChangeZone origin all move from absent/default state to the intended graveyard semantics.
  • The registered integration suite reaches trigger collection, the intervening-if fire-time and resolution-time checks, the optional choice, and the graveyard-to-battlefield move. The reviewer-requested direct remainder return is included in the current head.

Recommendation: approved and queued for merge.

@matthewevans
matthewevans added this pull request to the merge queue Jul 15, 2026
@matthewevans matthewevans removed their assignment Jul 15, 2026
Merged via the queue into phase-rs:main with commit e3667d0 Jul 15, 2026
13 checks passed
rykerwilliams added a commit to rykerwilliams/phase that referenced this pull request Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants