fix(parser): Lignify-class subtype-only enchanted type-change with base P/T (#5300)#5305
Conversation
…se P/T (phase-rs#5300) "Enchanted creature is a <creature subtype> with base power and toughness N/N and loses all abilities." (Lignify: Treefolk 0/4) dropped the subtype: parse_enchanted_is_type parsed the base P/T and loses-abilities but the "is a Treefolk" copula names only a creature subtype, so granted_core_types stayed empty and the handler bailed — the enchanted creature never became the named type. A creature subtype makes the object a creature (CR 205.1a), and a base P/T is creature-only, so when the granted phrase is subtype-only AND carries a base P/T, imply the Creature core type and let the existing emission produce the full replacement (SetCardTypes[Creature] + RemoveAllSubtypes{Creature} + AddSubtype + SetPower/SetToughness + RemoveAllAbilities). The base-P/T gate keeps the basic-land subtype change ("Enchanted land is a Mountain", no P/T) on the SetBasicLandType handler. Reuses the tested Frogify/Darksteel emission path — no new engine variant, no new runtime.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses issue #5300 by implementing parsing support for Lignify-class subtype-only type-change auras that specify a base power and toughness. However, the current implementation incorrectly pushes CoreType::Creature to granted_core_types, which would emit SetCardTypes and strip other existing card types (such as Artifact or Land) from the enchanted creature, violating CR 205.1a. The feedback advises bypassing the empty check without adding CoreType::Creature to preserve other card types, and updating the corresponding test assertions to ensure SetCardTypes is not emitted.
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.
| if granted_core_types.is_empty() { | ||
| return None; | ||
| // CR 205.1a + CR 613.4b (issue #5300): the Lignify class names only a | ||
| // creature SUBTYPE in the copula — "Enchanted creature is a Treefolk | ||
| // with base power and toughness 0/4 and loses all abilities." No core | ||
| // card-type word appears, but a creature subtype makes the object a | ||
| // creature, so imply the Creature core type and let the emission below | ||
| // produce the full replacement (SetCardTypes + RemoveAllSubtypes + | ||
| // AddSubtype) instead of silently dropping the subtype. Gated on a base | ||
| // P/T (only creatures have one) so the basic-land subtype change | ||
| // ("Enchanted land is a Mountain", no P/T) still returns None here and | ||
| // defers to the SetBasicLandType handler. | ||
| if base_pt.is_some() && !granted_subtypes.is_empty() { | ||
| granted_core_types.push(CoreType::Creature); | ||
| } else { | ||
| return None; | ||
| } | ||
| } |
There was a problem hiding this comment.
[CRITICAL] Lignify-class auras (e.g., "Enchanted creature is a Treefolk...") only set the subtype of the enchanted creature; they do not set or overwrite its card types (CR 205.1a). Therefore, if the enchanted creature has other card types (such as Artifact or Land), it must retain them (e.g., Lignify on Gingerbrute results in an Artifact Creature — Treefolk Food, and on Dryad Arbor results in a Land Creature — Forest Treefolk).
By pushing CoreType::Creature to granted_core_types, the engine will emit SetCardTypes { core_types: vec![CoreType::Creature] }, which incorrectly strips all other card types. Instead, we should bypass the granted_core_types.is_empty() check when base_pt.is_some() && !granted_subtypes.is_empty() but leave granted_core_types empty so that SetCardTypes is not emitted.
if granted_core_types.is_empty() {
if base_pt.is_some() && !granted_subtypes.is_empty() {
// CR 205.1a: Lignify-class subtype-only type-change auras set the subtype but do not set or overwrite the card type.
// By leaving granted_core_types empty, we avoid emitting SetCardTypes, which would incorrectly strip other card types (like Artifact or Land).
} else {
return None;
}
}References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| assert!( | ||
| mods.contains(&ContinuousModification::SetCardTypes { | ||
| core_types: vec![CoreType::Creature], | ||
| }), | ||
| "creature subtype must imply SetCardTypes[Creature]: {mods:?}" | ||
| ); |
There was a problem hiding this comment.
[HIGH] Since Lignify-class auras only set the subtype and do not set or overwrite the card type, the test should assert that SetCardTypes is not emitted.
assert!(
!mods.iter().any(|m| matches!(m, ContinuousModification::SetCardTypes { .. })),
"Lignify must not emit SetCardTypes as it only sets the subtype and preserves other card types: {mods:?}"
);
Parse changes introduced by this PR · 7 card(s), 9 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Subtype-only Lignify effects replace all card types with Creature. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:1059 pushes CoreType::Creature, which flows into SetCardTypes at crates/engine/src/parser/oracle_static/type_change.rs:1097; SetCardTypes replaces obj.card_types.core_types in crates/engine/src/game/layers.rs:4885. Why it matters: "is a Treefolk" sets/replaces the creature subtype for an already enchanted creature; CR 205.1a says subtype changes replace subtypes from the appropriate set and that removing a subtype does not affect card types, so an enchanted artifact creature or land creature would incorrectly lose Artifact/Land and become only Creature. Suggested fix: do not synthesize CoreType::Creature for the subtype-only branch; preserve existing card types while emitting the creature subtype replacement (RemoveAllSubtypes { Creature } before AddSubtype(Treefolk)), and update crates/engine/src/parser/oracle_static/tests.rs:14150 to assert no SetCardTypes is emitted for Lignify.
…205.1a)
Address review (matthewevans, gemini): synthesizing CoreType::Creature
emitted SetCardTypes[Creature], which replaces the enchanted creature's
card types and strips Artifact/Land — wrong per CR 205.1a (a subtype
change does not affect card types). Lignify on Gingerbrute must stay an
Artifact Creature — Treefolk; on Dryad Arbor a Land Creature — Forest
Treefolk.
Leave granted_core_types empty for the subtype-only branch (a
`subtype_only_creature_change` flag gates it on base P/T + a granted
subtype), so no SetCardTypes is emitted; guard the SetCardTypes push on a
non-empty core-type set; and let the RemoveAllSubtypes{Creature} injection
fire for the flag so the subtype replacement (RemoveAllSubtypes before
AddSubtype) still runs. Test updated to assert no SetCardTypes is emitted.
|
Thanks — fixed the CR 205.1a violation. You're right: synthesizing Revised so the subtype-only branch leaves
Lignify now emits exactly Local: full engine lib suite 15,612 / 0, fmt + parser-combinator + engine-authorities gates pass, clippy clean. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head after the subtype-only fix. The change stays in the existing type-change parser seam, preserves card types per CR 205.1a, has focused parser coverage, and CI is green.
…type Follow-up to phase-rs#5305: Lignify coverage landed on main via parse_enchanted_is_type. Remove redundant dedicated dispatch arm; add parse_pt_mod_with_remainder and wire it into parse_enchanted_is_type base-P/T and inline-P/T paths, replacing find('/') byte-slicing. Co-authored-by: Cursor <cursoragent@cursor.com>
…type (phase-rs#5300 follow-up) (phase-rs#5302) * feat(parser): Lignify-class enchanted subtype-only type-change with base P/T (phase-rs#5300) Auras that name only a creature subtype before the base-P/T seam (Lignify) strict-failed because parse_enchanted_is_type requires a core card type word. Add parse_enchanted_is_creature_subtype_with_base_pt composing SetCardTypes, RemoveAllSubtypes, AddSubtype, SetPower/Toughness, and RemoveAllAbilities from existing layer modifications with no new runtime. Closes phase-rs#5300. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): address Lignify PR review — P/T trim, CR annotations, test Trim trailing dot on lowercased base-P/T fragment before remainder extraction (Gemini). Add inline CR 205.1a / CR 613.4b annotations on modification pushes. Add trailing-dot regression test via parse_static_line. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(parser): nom P/T remainder boundary for Lignify handler Add parse_pt_mod_with_remainder in grammar.rs composing signed and unsigned nom P/T parsers; replace hand-scanned find('/') tail extraction in the Lignify-class handler. Remove redundant inline CoreType/SubtypeSet import (already in prelude). Add combinator-level unit test. Co-authored-by: Cursor <cursoragent@cursor.com> * chore: rustfmt tests.rs Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): Lignify subtype-only must not emit SetCardTypes (CR 205.1a) Setting a creature subtype replaces creature subtypes only — card types stay (Artifact/Land creatures keep Artifact/Land). Align dedicated handler with parse_enchanted_is_type and lignify_subtype_only test. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(parser): nom P/T remainder combinator in parse_enchanted_is_type Follow-up to phase-rs#5305: Lignify coverage landed on main via parse_enchanted_is_type. Remove redundant dedicated dispatch arm; add parse_pt_mod_with_remainder and wire it into parse_enchanted_is_type base-P/T and inline-P/T paths, replacing find('/') byte-slicing. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Summary
Closes #5300.
Lignify-class auras —
"Enchanted creature is a <creature subtype> with base power and toughness N/N and loses all abilities."— silently dropped the subtype. On HEAD the line parses to a static withRemoveAllAbilities + SetPower + SetToughnessbut noSetCardTypes/AddSubtype, so the enchanted creature never becomes the named type (e.g. Lignify's enchanted creature never becomes a Treefolk).Root cause (per @jsdevninja's analysis):
parse_enchanted_is_typeroutes theis a <phrase>copula throughclassify_type, which pushes a bare creature subtype (Treefolk) togranted_subtypesbut leavesgranted_core_typesempty (no core card-type word). Thegranted_core_types.is_empty() → return Noneguard then bails, and the line falls through to a partial handler that keeps only the P/T + ability loss.Fix
A creature subtype makes the object a creature (CR 205.1a), and a base P/T is creature-only, so when the granted phrase is subtype-only and carries a base P/T, imply the
Creaturecore type. The existing, tested emission then produces the full replacement in written order:SetCardTypes[Creature]→RemoveAllSubtypes{Creature}→AddSubtype(Treefolk)→SetPower(0)→SetToughness(4)→RemoveAllAbilitiesI took the minimal in-place route rather than a separate
parse_enchanted_is_creature_subtype_with_base_pthandler (the issue's proposed structure): it reuses the already-tested Frogify/Darksteel emission (SetCardTypes / RemoveAllSubtypes-before-AddSubtype / trailing-clause) with zero duplication, and only adds a branch that fires whengranted_core_typesis empty — so the Frogify/Darksteel family (which name a core type) is never touched. The base-P/T gate keeps the basic-land subtype change ("Enchanted land is a Mountain", no P/T) returningNonehere and deferring to theSetBasicLandTypehandler. No new engine variant, no new runtime.Test plan (from the issue)
SetCardTypes(Creature) + RemoveAllSubtypes(Creature) + AddSubtype(Treefolk) + SetPower(0) + SetToughness(4) + RemoveAllAbilities(withRemoveAllSubtypesbeforeAddSubtype) — new testlignify_subtype_only_with_base_pt_type_change"... loses all abilities and is a Treefolk with base power and toughness 0/4.") parses (verified)darksteel_mutation_full_modification_set+ full suite unchanged)cargo fmt,scripts/check-parser-combinators.sh,scripts/check-engine-authorities.shpass; clippy clean; full engine lib suite 15,612 passed / 0 failed