fix(parser): bare 'it's a <type>' replaces card types per CR 205.1a (Arixmethes, #5213)#5231
Conversation
…Arixmethes) Closes phase-rs#5213. A self type-change 'it's a <type>' with no CR 205.1b retention marker ('in addition to its other types' / 'still a') sets the object's card type, which REPLACES its existing card types (CR 205.1a). The parser emitted an additive AddType, so Arixmethes, Slumbering Isle stayed a 'Creature Land' while it had a slumber counter instead of becoming just a Land. parse_pronoun_becomes_type_static now converts a pure non-creature card-type change (only AddType mods, no Creature, no retention marker) to a replacing SetCardTypes via maybe_replace_card_types. Creature animations (Gideon Blackblade / Midnight Mangler / Circle of the Moon Druid) and 'in addition to its other types' forms keep the additive path. The existing SetCardTypes runtime drops the correlated creature subtype (Kraken), leaving a Legendary Land. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements card-type replacement logic for self type-change effects (CR 205.1a, e.g., Arixmethes, Slumbering Isle) by introducing maybe_replace_card_types and adding corresponding tests. Feedback highlights two style guide violations: a non-standard CR annotation format on line 1413 and the use of verbatim string scanning (scan_contains) instead of nom combinators on line 1433.
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.
| Some(def) | ||
| } | ||
|
|
||
| /// CR 205.1a / CR 205.1b: Decide whether a self type-change is a REPLACEMENT or |
There was a problem hiding this comment.
[MEDIUM] Non-standard CR annotation format. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:1413.
Why it matters: The CR annotation uses a combined slash format (CR 205.1a / CR 205.1b) which may fail automated regex validation requiring the strict CR <number>: <description> format.
Suggested fix: Split the CR annotations into separate, cleanly formatted lines.
| /// CR 205.1a / CR 205.1b: Decide whether a self type-change is a REPLACEMENT or | |
| /// CR 205.1a: An effect that sets an object's card type replaces its existing card types. | |
| /// CR 205.1b: Some effects specify that the object retains one or more of its prior card types. | |
| /// Decide whether a self type-change is a REPLACEMENT or |
References
- Every rules-touching line of engine code must carry a comment of the form CR : (regex CR \d{3}(.\d+[a-z]?)?). (link)
| if nom_primitives::scan_contains(body_lower, "in addition to") | ||
| || nom_primitives::scan_contains(body_lower, "still a") | ||
| { |
There was a problem hiding this comment.
[MEDIUM] Avoid verbatim string scanning for parsing Oracle phrases. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:1433.
Why it matters: Using scan_contains for phrases like 'still a' or 'in addition to' bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
| if nom_primitives::scan_contains(body_lower, "in addition to") | |
| || nom_primitives::scan_contains(body_lower, "still a") | |
| { | |
| let mut parser = alt(( | |
| tag("in addition to"), | |
| tag("still a"), | |
| tag("still an"), | |
| )); |
References
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review: right seam, CR 205.1a/205.1b checked, parse-diff is scoped to Arixmethes, and tests cover both parser shape and layer behavior.
…-land coverage phase-rs#5231 (merged) already covers Arixmethes land replacement, the slumber condition, additive retention, and artifact-creature retention. Swap the duplicate tests for a distinct untested sibling: the "it's a <type>" replacement and its "in addition to" retention exception are not land-specific — "~ is an artifact" REPLACES via SetCardTypes while "~ is an artifact in addition to its other types" stays additive.
…phase-rs#5218) * fix(parser): self-ref "it's a <type>" replaces card types (CR 205.1a) A self-referential type-change static of the form "it's a <core type>" (Arixmethes, Slumbering Isle: "As long as ~ has a slumber counter on it, it's a land.") went through the pronoun-animation path, which is unconditionally additive: it emitted `AddType(Land)` and left the permanent a "Legendary Creature Land — Kraken" — an impossible land creature. Per CR 205.1a, "is a <type>" WITHOUT "in addition to its other types" REPLACES the permanent's card types, so it must become a Land only (Creature removed → "Legendary Land — Kraken"). In `parse_pronoun_becomes_type_static`, when the body is a pure bare core-type set (no retained P/T, keywords, color, or subtype change) and carries no "in addition"/"still a <type>" retention clause, collapse the additive `AddType` list into one replacing `SetCardTypes` — mirroring the aura type-change path's `needs_set_card_types`, and reusing the existing `SetCardTypes` layer handler (which retains supertypes and creature subtypes, yielding "Legendary Land — Kraken"). The additive animations (Gideon Blackblade, manlands) are excluded because they carry a P/T, keywords, or an explicit retention clause. Closes phase-rs#5213 * Update crates/engine/src/parser/oracle_static/type_change.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * Update crates/engine/src/parser/oracle_static/type_change.rs Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> * fix(parser): use scan_contains in type-set guard; robust Arixmethes test CI fixes for the self-ref "it's a <type>" replacement: - The retention-clause guard used raw `str::contains`, which the parser combinator gate bans. Route through the approved `nom_primitives::scan_contains` helper (mirrors the existing "in addition to" scan in this module). - Make the Arixmethes regression assert `contains(SetCardTypes[Land])` + no `AddType` rather than an exact `==` on the modification vector, so it is not brittle to incidental additional modifications, and fully-qualify `CoreType` to drop the local import. * fix(parser): preserve CR 205.1b artifact-creature retention in type-set flip The self-ref "it's a <type>" → SetCardTypes replacement wrongly collapsed the artifact-creature animation class (Grond, the Gatebreaker; Midnight Mangler; Phoenix Fleet Airship) from additive `AddType artifact/creature` to a replacing `SetCardTypes`, which removes prior card types. Per CR 205.1b, animating a permanent INTO a creature ("~ is an artifact creature") GAINS the creature type while retaining its other card types — it must stay additive. Restrict the replacement to type-sets that do NOT add Creature: only a non-creature type-set like Arixmethes' "it's a land" (CR 205.1a / CR 305.7) replaces the card types. Add a regression asserting "During your turn / During turns other than yours, ~ is an artifact creature" stays `AddType` (no `SetCardTypes`). * docs(parser): drop incorrect CR 305.7 from it's-a-land type-replacement comment CR 305.7 governs setting a land's SUBTYPE (basic land types) and explicitly does not add or remove card types; card-type replacement for "it's a land" is CR 205.1a alone. * test(parser): replace duplicate phase-rs#5231 tests with distinct non-land coverage phase-rs#5231 (merged) already covers Arixmethes land replacement, the slumber condition, additive retention, and artifact-creature retention. Swap the duplicate tests for a distinct untested sibling: the "it's a <type>" replacement and its "in addition to" retention exception are not land-specific — "~ is an artifact" REPLACES via SetCardTypes while "~ is an artifact in addition to its other types" stays additive. --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Closes #5213
Summary
Fixes Arixmethes, Slumbering Isle (issue #5213): while it has a slumber counter it should become just a Land, but the engine showed the impossible "Legendary Creature Land — Kraken" — the Creature type was never removed.
Its static reads
As long as Arixmethes has a slumber counter on it, it's a land. (It's not a creature.). The parser emitted an additiveAddType{Land}, so the Kraken stayed a creature. Per CR 205.1a, an effect that sets a card type replaces the object's existing card types unless it retains them (CR 205.1b: "in addition to its other types" / "still a"). A bare "it's a land" therefore replaces.Fix
parse_pronoun_becomes_type_staticnow runs its computed modifications throughmaybe_replace_card_types, which converts them to a single replacingSetCardTypesonly when all of:AddType(a pure card-type change — no P/T, subtype, keyword, color, or supertype),Creature(creature forms retain via the CR 205.1b "artifact creature" rule and their P/T),in addition to/still a).So
it's a land→SetCardTypes{[Land]}(replaces, removing Creature), while creature animations (Gideon Blackblade, Midnight Mangler, Circle of the Moon Druid) andin addition to its other typesforms stay additive. No new runtime: the existingSetCardTypeslayer application (game/layers.rs) sets the core types and drops the correlatedKrakencreature subtype, leaving a Legendary Land.Files changed
crates/engine/src/parser/oracle_static/type_change.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/game/layers.rsAnchored on
crates/engine/src/parser/oracle_static/type_change.rsparse_attached_isnt_and_is_type/ the Imprisonedparse_enchanted_becomes_type_with_ability— existing handlers that already model type REPLACEMENT viaSetCardTypes/RemoveType; this reusesSetCardTypesfor the self "it's a [type]" form.crates/engine/src/game/layers.rs:4886ContinuousModification::SetCardTypes— the existing replacement runtime that sets core types and retains only correlated subtypes.CR references
CR 205.1a— an effect that sets an object's card type replaces its existing card types; correlated subtypes of a removed card type are also removed.CR 205.1b— retention only when the effect says "in addition to its other types", "still a [type]", or is an "artifact creature" form.Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Verification
cargo fmt --all— clean./scripts/check-parser-combinators.sh— clean (exit 0)./scripts/check-engine-authorities.sh— clean (exit 0)cargo test -p engine --lib parser::oracle_static— 1014 pass / 0 failarixmethes_bare_is_a_land_replaces_card_types(assertsSetCardTypes{[Land]}+ the slumber-counter condition, and thatin addition to/artifact creaturestay additive).set_card_types_land_strips_creature_and_creature_subtype(a Legendary Creature Kraken underSetCardTypes([Land])becomes a Legendary Land: Creature removed, Kraken subtype dropped, Legendary retained).Scope Expansion
None.
Validation Failures
None.
CI Failures
None.
🤖 Generated with Claude Code