Skip to content

feat(parser): reach graveyard cards with the chosen-creature-type static (Ashes of the Fallen)#5864

Merged
matthewevans merged 2 commits into
phase-rs:mainfrom
minion1227:minion_ashes
Jul 17, 2026
Merged

feat(parser): reach graveyard cards with the chosen-creature-type static (Ashes of the Fallen)#5864
matthewevans merged 2 commits into
phase-rs:mainfrom
minion1227:minion_ashes

Conversation

@minion1227

@minion1227 minion1227 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Closes #5863

Summary

"Each creature card in your graveyard has the chosen creature type in addition to its other types." (Ashes of the Fallen) strict-failed — zero statics, so the card did nothing after its ETB choice. The battlefield sibling (Arcane Adaptation) parses fine, so this was purely a zone-scope gap.

parse_arcane_adaptation_chosen_type_static dispatches on ChosenCreatureTypeStaticScope, whose subject matcher had fixed arms only for creatures you control are / each creature you control is / vehicle creatures you control are. The graveyard subject — which also reads "has" rather than "are" — had no arm, so the line fell through unparsed.

Files changed

  • crates/engine/src/parser/oracle_static/type_change.rs
  • crates/engine/src/parser/oracle_static/tests.rs

CR references

  • CR 109.2a — a description that names a card and a zone ("each creature card in your graveyard") means a card matching it in that stated zone; this is what scopes the grant to graveyard creature cards rather than battlefield permanents.
  • CR 613.1d — Layer 4, type-changing effects (the grant of the chosen creature type).
  • CR 205.1b — "in addition to its other types" retention (additive, no subtype wipe).

Implementation method (required)

Method: not-applicable — authored directly against the oracle-parser skill. Pure parser change; no card-data regeneration required (AST-shape fix).

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high

Verification

  • Required checks ran clean, or the exact CI-owned alternative is stated below.
  • Verification below is for the current committed head (019d0de20).
  • Final review below is clean for the current committed head.
  • Both anchors cite existing analogous code at the same seam.

Head 019d0de20 corrects the CR annotations flagged in review (CR 400.1 / CR 607.2d → CR 109.2a / CR 613.1d / CR 205.1b); it is a comment-only change over the reviewed head — no code, test, or AST change.

Results on head 019d0de20:

  • Full GitHub CI green on 019d0de20 — Rust lint (fmt, clippy, parser gate), Rust tests (shard 1/2 + 2/2), coverage-gate, card-data, frontend, Lobby worker, WASM, and Tauri compile all pass.
  • cargo clippy -p engine --all-targets --features proptest -- -D warningsFinished, 0 warnings locally on the committed content (--all-targets covers the doc-comment test targets) and confirmed green in CI.
  • cargo fmt --all — clean (no reformatting).
  • Parser suite unchanged from the reviewed head (8188 passed; comment-only diff), reconfirmed by the green CI test shards on 019d0de20.

Parse diff (Ashes of the Fallen)

Before:

statics = []          // card does nothing after the ETB choice

After:

affected = {Creature, controller: You, InAnyZone[Graveyard]}
mods     = [AddChosenSubtype { kind: CreatureType }]

Anchored on

  • crates/engine/src/parser/oracle_static/type_change.rs (parse_arcane_adaptation_chosen_type_static + ChosenCreatureTypeStaticScope) — the existing scope axis this extends. Same creature subject, same additive AddChosenSubtype{CreatureType}, same additive/SET application split; only the zone differs, so it becomes a fourth scope arm rather than a parallel handler.
  • crates/engine/src/parser/oracle_static/shared.rs (parse_hand_cards_have_derived_cost_keyword) — the established zone-fold precedent: a card-in-a-zone static carries its scope as FilterProp::InAnyZone { zones: [Hand] } on the same typed filter. This reuses that fold verbatim with [Graveyard].

Final review

Self-review against the review-impl lenses. No new engine primitive: AddChosenSubtype, FilterProp::InAnyZone, and the additive/SET split are pre-existing and already runtime-wired, so this is a parser-recognition change only. The new arm matches the full literal subject+verb ("each creature card in your graveyard has"), so it cannot claim any other line, and it binds "its" as the retention pronoun to match the card's "in addition to its other types" (the pronoun feeds the shared additive-suffix check). The graveyard scope is grouped with the other creature-card subjects in the SET/replace arm for semantic correctness; no SET printing exists today (Ashes is additive), so that arm is unreachable in practice and documented as such. Battlefield forms are byte-for-byte unchanged — regression-tested that Arcane Adaptation gains no zone restriction (arcane_adaptation_stays_unzoned_after_graveyard_arm), plus 8188 parser tests incl. snapshots unaffected.

Claimed parse impact

  • Ashes of the Fallen

The CI coverage-parse-diff sticky enumerates the full affected set for the current head.

Validation Failures

None.

CI Failures

None.

@minion1227
minion1227 requested a review from matthewevans as a code owner July 15, 2026 14:54

@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 introduces support for graveyard-scoped creature card type-change effects (such as Ashes of the Fallen) by adding the GraveyardCreatureCards variant to ChosenCreatureTypeStaticScope and implementing its target filtering, parsing, and application logic. The review feedback correctly identifies that the comments citing CR 400.1 should instead cite CR 109.2a to accurately reflect the distinction between cards in non-battlefield zones and permanents on the battlefield, in accordance with Rule R6.

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 +70 to +72
/// CR 400.1 + CR 607.2d: the grant reaches creature CARDS in the controller's
/// graveyard rather than permanents on the battlefield (Ashes of the Fallen).
GraveyardCreatureCards,

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

[MED] Incorrect CR citation for zone-based card vs permanent distinction. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:70.
Why it matters: CR 400.1 merely defines what an object is, whereas CR 109.2a specifically explains why an object with a card type in a non-battlefield zone is a card rather than a permanent.
Suggested fix: Change CR 400.1 to CR 109.2a.

Suggested change
/// CR 400.1 + CR 607.2d: the grant reaches creature CARDS in the controller's
/// graveyard rather than permanents on the battlefield (Ashes of the Fallen).
GraveyardCreatureCards,
/// CR 109.2a + CR 607.2d: the grant reaches creature CARDS in the controller's
/// graveyard rather than permanents on the battlefield (Ashes of the Fallen).
GraveyardCreatureCards,
References
  1. Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)

Comment on lines +93 to +98
// CR 400.1 + CR 607.2d: Ashes of the Fallen grants the chosen type to
// creature CARDS in the controller's graveyard. Same creature subject,
// one axis different — the zone — so the filter carries the restriction
// via the identical `InAnyZone` fold the hand-scoped derived-cost grant
// (`parse_hand_cards_have_derived_cost_keyword`) already applies.
Self::GraveyardCreatureCards => TargetFilter::Typed(

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

[MED] Incorrect CR citation for zone-based card vs permanent distinction. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:93.
Why it matters: CR 400.1 merely defines what an object is, whereas CR 109.2a specifically explains why an object with a card type in a non-battlefield zone is a card rather than a permanent.
Suggested fix: Change CR 400.1 to CR 109.2a.

Suggested change
// CR 400.1 + CR 607.2d: Ashes of the Fallen grants the chosen type to
// creature CARDS in the controller's graveyard. Same creature subject,
// one axis different — the zone — so the filter carries the restriction
// via the identical `InAnyZone` fold the hand-scoped derived-cost grant
// (`parse_hand_cards_have_derived_cost_keyword`) already applies.
Self::GraveyardCreatureCards => TargetFilter::Typed(
// CR 109.2a + CR 607.2d: Ashes of the Fallen grants the chosen type to
// creature CARDS in the controller's graveyard. Same creature subject,
// one axis different — the zone — so the filter carries the restriction
// via the identical `InAnyZone` fold the hand-scoped derived-cost grant
// (`parse_hand_cards_have_derived_cost_keyword`) already applies.
Self::GraveyardCreatureCards => TargetFilter::Typed(
References
  1. Rule R6: Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)

@github-actions

github-actions Bot commented Jul 15, 2026

Copy link
Copy Markdown

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

🟢 Added (1 signature)

  • 1 card · ➕ static/Continuous · added: Continuous (affects=you control in graveyard creature, mods=add chosen subtype)
    • Affected (first 3): Ashes of the Fallen

🔴 Removed (1 signature)

  • 1 card · ➖ ability/static_structure · removed: static_structure
    • Affected (first 3): Ashes of the Fallen

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

@matthewevans matthewevans self-assigned this Jul 16, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 16, 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.

Request changes — correct both type-change CR annotations.

🟡 Finding

[MED] The new annotations cite unrelated CR 607.2d and CR 400.1. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:70,93. Why it matters: the comments claim rule support they do not have. Suggested fix: use the verified relevant citations CR 109.2a + CR 613.1d / CR 205.1b with accurate descriptions.

Recommendation: request-changes.

@matthewevans matthewevans removed their assignment Jul 16, 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.

Blocked — the new graveyard scope uses controller semantics where Ashes of the Fallen requires ownership.

🔴 Blocker

[HIGH] The parsed “your graveyard” filter becomes ControllerRef::You at crates/engine/src/parser/oracle_static/type_change.rs:100, and off-zone continuous effects use ordinary target matching in crates/engine/src/game/layers.rs:4855. That matcher consults effective controller via last-known information for off-zone objects (crates/engine/src/game/filter.rs:775 and :1696). Why it matters: a creature owned by Ashes' controller but controlled by an opponent when it dies can be excluded from its owner's graveyard, while an opponent-owned card can be included in the wrong graveyard. The engine already has the owner-zone authority at crates/engine/src/game/filter.rs:1217, and its existing layer regression at layers.rs:10338 demonstrates this owner/controller divergence. Suggested fix: route owner-scoped off-zone continuous-effect candidates through the owner-zone matcher at the layer authority, then add runtime stolen-card coverage for both directions.

The amended head corrected CR annotations but did not address this semantic path. The parse-diff is limited to Ashes of the Fallen; existing parser AST tests do not exercise layer application.

Recommendation: request changes; fix ownership at the shared off-zone continuous-layer seam and add discriminating runtime tests.

@minion1227

Copy link
Copy Markdown
Contributor Author

@matthewevans addressed in 019d0de20 — comment-only, no code/test/AST change.

Both flagged annotations now cite the verified rules; each was grepped against docs/MagicCompRules.txt before landing:

  • CR 109.2a — "If a spell or ability uses a description of an object that includes the word 'card' and the name of a zone, it means a card matching that description in the stated zone." That is exactly what scopes "each creature card in your graveyard" to graveyard cards rather than battlefield permanents.
  • CR 613.1d — Layer 4, type-changing effects.
  • CR 205.1b — "in addition to its other types" retention.

Fixed at three sites, not the two cited: the parser subject arm (type_change.rs:282) carried the same CR 400.1, and the commit trailer + test doc comment were corrected to match, so no wrong citation is left behind.

I left the pre-existing CR 607.2d on the enum doc and the Vehicle arm untouched — there it's the linked-ability rule (the ETB "choose a creature type" ↔ "the chosen creature type"), which is correct and not part of this change; only my graveyard-scope lines misused it.

CI is green on 019d0de20. Ready for re-review.

@minion1227

Copy link
Copy Markdown
Contributor Author

@matthewevans Addressed the ownership blocker.

Fix: The graveyard scope no longer uses .controller(ControllerRef::You). It now scopes by ownershipFilterProp::Owned { controller: ControllerRef::You } + the existing FilterProp::InAnyZone { Graveyard } fold (type_change.rs):

  • CR 400.3: a card only ever rests in its owner's graveyard, so a graveyard is an owner-defined zone.
  • CR 109.5: "your" on a card that has no controller resolves to its owner — exactly the graveyard case.

As you noted, the continuous-effect matcher (layers.rs) evaluates a Typed filter's controller against the effective-controller field, which isn't meaningful off the battlefield. FilterProp::Owned matches obj.owner directly (filter.rs object path), so no layers.rs change is needed — the fix stays entirely in the parser.

The prior CR-annotation request is already reflected (CR 109.2a + CR 613.1d + CR 205.1b), and I added CR 400.3 + CR 109.5 for the ownership scoping.

Verified: oracle_static parser suite green (1150 passed), clippy -p engine --all-targets --features proptest clean (0 warnings). The battlefield forms (Arcane Adaptation) are untouched.

@matthewevans matthewevans self-assigned this Jul 16, 2026
@matthewevans

Copy link
Copy Markdown
Member

Review held for current-head evidence. The dbcdb247 ownership change is newer than the required parse-diff artifact (the sticky report is from 2026-07-15), and the current CI has a terminal Rust-lint failure while Rust tests/card-data are still running. The diff's added coverage is parser-shape-only, so it does not yet demonstrate the requested stolen-creature owner/controller runtime behavior. I will re-review once current-head CI and parse-diff evidence are available.

@matthewevans matthewevans removed their assignment Jul 16, 2026
…tic (Ashes of the Fallen)

"Each creature card in your graveyard has the chosen creature type in
addition to its other types." (Ashes of the Fallen) strict-failed — the
line produced zero statics, so the card did nothing after its ETB choice.

This is the ZONE-axis sibling of Arcane Adaptation: the same creature
subject and the same additive AddChosenSubtype{CreatureType} grant, one
axis different — the cards live in the OWNER's graveyard rather than on the
battlefield. So it extends the existing scope axis
(ChosenCreatureTypeStaticScope) that parse_arcane_adaptation_chosen_type_
static already dispatches on.

Scope by OWNERSHIP, not control: a graveyard is an owner-defined zone
(CR 400.3 — a card only ever rests in its owner's graveyard) and "your" on
a card that has no controller resolves to its OWNER (CR 109.5). The new
scope's target_filter therefore carries FilterProp::Owned{You} plus the
FilterProp::InAnyZone{Graveyard} zone fold, NOT .controller(You). This
matters because the continuous-effect matcher (layers.rs) evaluates a Typed
filter's `controller` against a card's effective-controller field, which is
not meaningful off the battlefield; FilterProp::Owned matches obj.owner
directly and is already runtime-wired by the object matcher.

No new engine primitive: AddChosenSubtype, Owned, InAnyZone and the
additive/SET application split are all pre-existing and already runtime-
wired. The battlefield forms are untouched — regression-tested that Arcane
Adaptation gains no zone restriction.

CR 109.2a (a description naming a card plus a zone means a card in that
zone) + CR 400.3 + CR 109.5 (owner-defined graveyard scope) + CR 613.1d
(Layer 4 type-changing) + CR 205.1b (in-addition retention).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d grant

The prior review requested runtime coverage proving Ashes of the Fallen's "creature
cards in your graveyard" grant is scoped by OWNERSHIP, not stale last-known control.
The current head already encodes this via `FilterProp::Owned { You }` (matches
`obj.owner` directly); this adds the layer-level proof the parser-only AST tests
lacked (CR 400.3 + CR 109.5).

Drives `evaluate_layers` with Ashes (chosen type Zombie) over three graveyard
creature cards:
- a card you own -> gains Zombie (additive, keeps its own subtype),
- a card you OWN but an opponent last CONTROLLED (stale controller) -> still gains
  Zombie (ownership, not control),
- an opponent-OWNED card -> not reached.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227

Copy link
Copy Markdown
Contributor Author

Ownership blocker addressed — filter-level owner scope + runtime coverage

Thanks for the review. The HIGH ownership finding was resolved on head 140d60e6d (which landed ~15h after the CHANGES_REQUESTED review, so the review predates it), and this push adds the runtime coverage you asked for.

The semantic path is now owner-scoped at the filter, not stale control. The graveyard arm no longer uses .controller(ControllerRef::You); it uses FilterProp::Owned { controller: You } + InAnyZone[Graveyard]. FilterProp::Owned { You } evaluates as source.controller == Some(obj.owner) (crates/engine/src/game/filter.rs:3929), i.e. it reads obj.owner directly — so the layer matcher at layers.rs:4863 selects graveyard cards by ownership regardless of the object's stale last-known controller. This achieves the owner-scoping you recommended without depending on control-change LKI.

Runtime stolen-card coverage (both directions) — new commit 02f7bc31e, crates/engine/tests/integration/ashes_of_the_fallen_owner_scope_5864.rs, driving the production evaluate_layers:

  • a creature you own in your graveyard → gains the chosen type (additive);
  • a creature you own but an opponent last controlled (stale controller P1) → still gains it (CR 109.5: ownership, not control) — the exclusion direction;
  • an opponent-owned graveyard card → not reached — the wrong-inclusion direction.

Verification on head 02f7bc31e: the new integration test passes; cargo clippy -p engine --test integration --features proptest -- -D warnings is clean.

Re-requesting review.

@matthewevans matthewevans self-assigned this Jul 17, 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.

Approved — current head fixes the owner/controller scope and adds a discriminating layer-runtime regression.

The current parse-diff is for 02f7bc31 and contains only Ashes of the Fallen. The test parses the actual static, evaluates the production layer authority, proves an owner-P0/stale-controller-P1 graveyard card is affected, preserves its existing subtype, and excludes P1-owned graveyard cards.

@matthewevans matthewevans added enhancement New feature or request quality For high-quality minimal to no-churn PRs and removed bug Bug fix labels Jul 17, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 17, 2026
@matthewevans matthewevans removed their assignment Jul 17, 2026
Merged via the queue into phase-rs:main with commit 18e8dd2 Jul 17, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_ashes branch July 18, 2026 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request quality For high-quality minimal to no-churn PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parser: chosen-creature-type static strict-fails when scoped to the graveyard (Ashes of the Fallen)

2 participants