feat(parser+engine): Imprisoned in the Moon type-swap-with-granted-ability aura (#4770)#5142
Conversation
![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 implements parsing for the "Imprisoned in the Moon" style of type-changing aura effects to resolve issue #4770, adding a new parser function and associated unit tests. The review feedback highlights several incorrect Comprehensive Rules (CR) annotations that violate rule R6, and identifies a sibling coverage gap for the card "Sugar Coat" under rule L2 due to the parser not handling optional subtypes before the core card type.
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.
| } | ||
| } | ||
|
|
||
| // CR 205.1a + CR 604.1: Imprisoned-in-the-Moon — "Enchanted <subject> is a |
There was a problem hiding this comment.
[MED] Incorrect CR annotation for Layer 6. Evidence:
crates/engine/src/parser/oracle_static/dispatch.rs:1051.
Why it matters: CR 604.1 defines static abilities generally, whereas Layer 6 (ability addition/removal) is governed by CR 613.1f.
Suggested fix: Update the comment to reference CR 613.1f instead of CR 604.1.
| // CR 205.1a + CR 604.1: Imprisoned-in-the-Moon — "Enchanted <subject> is a | |
| // CR 205.1a + CR 613.1f: Imprisoned-in-the-Moon — "Enchanted <subject> is a |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| /// | ||
| /// Handles type-changing aura effects like Ensoul Artifact, Imprisoned in the Moon, | ||
| /// and Darksteel Mutation. Reuses nom type-word and P/T combinators. | ||
| /// CR 205.1a + CR 613.1d (Layer 4) + CR 604.1 (Layer 6): Imprisoned-in-the-Moon |
There was a problem hiding this comment.
[MED] Incorrect CR annotation for Layer 6. Evidence:
crates/engine/src/parser/oracle_static/type_change.rs:555.
Why it matters: CR 604.1 defines static abilities generally rather than Layer 6 (CR 613.1f).
Suggested fix: Update the doc comment to reference CR 613.1f (Layer 6) instead of CR 604.1 (Layer 6).
| /// CR 205.1a + CR 613.1d (Layer 4) + CR 604.1 (Layer 6): Imprisoned-in-the-Moon | |
| /// CR 205.1a + CR 613.1d (Layer 4) + CR 613.1f (Layer 6): Imprisoned-in-the-Moon |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| /// `RemoveAllAbilities` (the permanent loses its own abilities), then the | ||
| /// `GrantAbility` produced by the shared `parse_quoted_ability_modifications` | ||
| /// authority. `RemoveAllAbilities` is emitted BEFORE the grant so the granted | ||
| /// ability SURVIVES the wipe (CR 613.7a intra-effect written order — the same |
There was a problem hiding this comment.
[MED] Incorrect CR annotation for intra-effect layer application. Evidence:
crates/engine/src/parser/oracle_static/type_change.rs:568.
Why it matters: CR 613.7a governs timestamps of static abilities, whereas CR 613.6 governs how parts of a single continuous effect apply across different layers.
Suggested fix: Update the comment to reference CR 613.6 instead of CR 613.7a.
| /// ability SURVIVES the wipe (CR 613.7a intra-effect written order — the same | |
| /// ability SURVIVES the wipe (CR 613.6 intra-effect layer application — the same |
References
- R6. CR annotations are mandatory and verified. Every rules-touching line of engine code must carry a comment of the form CR : . A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| .parse(r) | ||
| .ok()?; | ||
| let (r, _) = tag::<_, _, OracleError<'_>>("colorless ").parse(r).ok()?; | ||
| let (r, type_tf) = nom_target::parse_type_filter_word(r).ok()?; |
There was a problem hiding this comment.
[MED] Sibling coverage gap for Sugar Coat. Evidence:
crates/engine/src/parser/oracle_static/type_change.rs:593.
Why it matters: Sugar Coat is a sibling Aura in this class that turns a permanent into a "colorless Food artifact" with a granted ability, but it is not parsed because "Food" is a subtype prefix before the core type.
Suggested fix: Generalize the parser to handle optional subtypes before the core card type.
References
- L2. Sibling coverage: If a parser arm or string was extended, ensure sibling variants or related cards in the same class (such as Sugar Coat) are covered. (link)
- 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 to prevent combinatorial explosion and improve maintainability.
Parse changes introduced by this PR · 4 card(s), 7 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the fix direction here. I’m holding this one for the current-head review findings already posted inline.
Blocking items:
- The new parser handles
colorless <core type>but misses the same Aura family when a subtype precedes the core type, as with Sugar Coat. The local card data already models that sibling as colorless + artifact subtype/card type + loses abilities + adds ability, so this should be generalized before we mark the class covered. - The CR annotations cite
CR 604.1for Layer 6 andCR 613.7afor intra-effect ordering. Local CR text confirms Layer 6 isCR 613.1f, andCR 613.6is the relevant cross-layer continuation rule;CR 604.1andCR 613.7ado not say what the comments claim.
Please update the parser to cover the subtype+core-type sibling shape, add a parser test for it, and correct the CR comments.
…ility aura (phase-rs#4770) Imprisoned in the Moon ("Enchanted permanent is a colorless land with '{T}: Add {C}' and loses all other card types and abilities.") parsed to only [SetCardTypes {Land}, SetColor{[]}] — the `with "<ability>"` clause and the trailing ability strip were both silently dropped, so the enchanted permanent kept its own abilities (issue phase-rs#4770: an enchanted commander still had its abilities) and never gained the "{T}: Add {C}" mana ability. Add parse_enchanted_becomes_type_with_ability in oracle_static/type_change.rs, dispatched before parse_enchanted_is_type (whose base-P/T split does not model the with-quoted-ability clause). It composes SetCardTypes (CR 205.1a) + SetColor ([], CR 105.2) + RemoveAllAbilities + GrantAbility (the quoted ability, via the shared parse_quoted_ability_modifications authority). RemoveAllAbilities is emitted BEFORE the grant so the granted ability survives the wipe (CR 613.7a written order — same ordering the RemoveAllSubtypes -> AddChosenSubtype composition relies on). The plain enchanted-is-type family (Song of the Dryads, Darksteel Mutation) has no quoted-ability clause and falls through unchanged. No new variant, no new runtime — reuses existing Layer-4/Layer-6 modifications. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Generalize parse_enchanted_becomes_type_with_ability to accept optional
subtype(s) before the core card type, so the Imprisoned-in-the-Moon class also
covers Sugar Coat ("colorless Food artifact with \"...\" and loses all other
card types and abilities"). Each parsed subtype emits an AddSubtype (Layer 4)
placed with the other type-identity modifications, before the Layer-6 ability
wipe. Uses the case-insensitive parse_subtype building block; a core-type word
is never a subtype so the loop stops at the head noun. Adds the Sugar Coat
parser regression test.
Correct the CR annotations flagged in review (verified against
docs/MagicCompRules.txt):
- Layer 6 is CR 613.1f (ability add/remove), not CR 604.1 (static abilities
generally) — dispatch.rs and the type_change.rs doc comment.
- The 'granted ability survives the wipe' ordering is CR 613.6 (parts of one
continuous effect keep applying to the same object), not CR 613.7a
(timestamps).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed all three blocking items (pushed 1. Subtype-before-core-type sibling (Sugar Coat). Generalized 2. Parser test. Added 3. CR annotation corrections (verified against
Ready for another look. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the update. The Imprisoned in the Moon path is much closer, but the new Sugar Coat sibling still has a CR 205.1a subtype replacement issue.
[MED] Sugar Coat still preserves old artifact subtypes. Evidence: crates/engine/src/parser/oracle_static/type_change.rs:652 only appends AddSubtype { subtype: Food }, while crates/engine/src/game/layers.rs:4806 keeps all subtypes correlated with the new Artifact card type unless a RemoveAllSubtypes { Artifact } runs first. CR 205.1a says that when an effect sets one or more subtypes, the new subtypes replace any existing subtypes from the appropriate set. Why it matters: enchanting an artifact with another artifact subtype leaves it as, for example, Clue Food artifact instead of just Food artifact, so the Sugar Coat coverage is still rules-wrong. Suggested fix: insert RemoveAllSubtypes for each granted subtype set before the corresponding AddSubtype, and add a regression that an artifact subtype is wiped before AddSubtype(Food).
Setting a subtype replaces existing subtypes from the same set (CR 205.1a), but AddSubtype alone is additive: a host that already carries an artifact subtype (e.g. a Clue) enchanted by Sugar Coat would become 'Clue Food artifact' instead of 'Food artifact'. Emit RemoveAllSubtypes for the core type's subtype set before the AddSubtype(s) so the old subtypes are cleared first. The grammar guarantees each parsed subtype belongs to the core type's set, so one RemoveAllSubtypes covers them. Strengthen the Sugar Coat test to assert RemoveAllSubtypes(Artifact) precedes AddSubtype(Food). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed the CR 205.1a subtype-replacement issue (pushed |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sweep: reviewed current head after the follow-up fixes. The parser/static type-change seam is clean on review, parse-diff evidence is present, and merge-when-ready can wait for remaining checks.
2 participants
Closes #4770.
Bug: Imprisoned in the Moon ("Enchanted permanent is a colorless land with
"{T}: Add {C}"and loses all other card types and abilities.") parsed to only[SetCardTypes{Land}, SetColor{[]}]— thewith "<ability>"clause and the trailing ability-strip were both silently dropped. So the enchanted permanent kept its own abilities (the #4770 report: an enchanted commander still had its abilities) and never gained the{T}: Add {C}mana ability.Fix:
parse_enchanted_becomes_type_with_abilityin test-freeoracle_static/type_change.rs, dispatched beforeparse_enchanted_is_type(whose base-P/T split does not model awith "<ability>"clause). It composes:SetCardTypes(replace card types, CR 205.1a) +SetColor([])(colorless, CR 105.2)RemoveAllAbilities(strip the permanent's own abilities)GrantAbility(the quoted ability, via the sharedparse_quoted_ability_modificationsauthority — parses{T}: Add {C}to a mana ability)RemoveAllAbilitiesis emitted before the grant so the granted ability survives the wipe (CR 613.7a written order — the same ordering the existingRemoveAllSubtypes → AddChosenSubtypecomposition relies on). The plain enchanted-is-type family (Song of the Dryads, Darksteel Mutation) has no quoted-ability clause and falls through unchanged.No new variant, no new runtime — reuses existing Layer-4/Layer-6 modifications.
Verified: new
imprisoned_in_the_moon_becomes_colorless_land_with_granted_mana_abilityparser test (asserts the 4 modifications, RemoveAllAbilities before GrantAbility, EnchantedBy affected); live-parse confirmsmods=[SetCardTypes{Land}, SetColor{[]}, RemoveAllAbilities, GrantAbility{"{T}: Add {C}", is_mana_ability:true}]; full oracle_static suite 946/946 green; parser-combinator gate + fmt clean.Note for review: the modification order (RemoveAllAbilities before GrantAbility) is chosen so the granted ability survives the Layer-6 wipe; a cast-pipeline runtime test confirming the mooned permanent ends with exactly the
{T}: Add {C}ability would be a good addition (I verified the parser AST but could not run the full cast pipeline in my environment).🤖 Generated with Claude Code