feat(parser): support Me, the Immortal + The Master, Mesmerist - #3509
Conversation
…ne via GrantCastingPermission —
…-unsupported-cluster-play-cast
There was a problem hiding this comment.
Code Review
This pull request implements counter persistence across zone changes (CR 122.2 and CR 113.6b) to support cards like Me, the Immortal and Skullbriar, the Walking Grave, including parser support, state tracking, and corresponding unit tests. It also fixes a parser issue where possessive apostrophes could prevent sentence-boundary splitting. Feedback on this PR highlights a critical rules-fidelity issue in zones.rs where querying the raw GameObject directly for active static definitions bypasses Layer 6 ability-removal effects (such as Humility or Yixlid Jailer), meaning counters would incorrectly persist even if the object has lost its abilities in the source zone.
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.
| fn counters_persist_on_move(state: &GameState, object_id: ObjectId, to: Zone) -> bool { | ||
| let Some(obj) = state.objects.get(&object_id) else { | ||
| return false; | ||
| }; | ||
| super::functioning_abilities::active_static_definitions(state, obj).any(|def| { | ||
| matches!( | ||
| &def.mode, | ||
| StaticMode::CountersPersistAcrossZones { excluded_zones } | ||
| if !excluded_zones.contains(&to) | ||
| ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
[HIGH] Bypassing the layers pipeline in counters_persist_on_move violates CR 113.6b and official rulings when ability-removal effects (such as Humility or Yixlid Jailer) are active in the from-zone.
Why it matters
According to official rulings for Skullbriar, if it loses its abilities (e.g., due to Humility on the battlefield or Yixlid Jailer in the graveyard), its counter-persistence ability does not function when it changes zones, and its counters must be cleared. By querying active_static_definitions directly on the raw GameObject from state.objects, Layer 6 ability-removal effects are completely bypassed, leading to incorrect rules behavior where counters are incorrectly preserved.
Suggested Fix
Instead of reading static definitions directly from the raw object, query the layered/effective characteristics of the object in the from-zone. If the engine has an effective_object or similar layered state accessor, use that to check for the presence of the CountersPersistAcrossZones static ability.
Additionally, please add a unit test in zones.rs verifying that if the object loses its abilities (simulating Humility/Yixlid Jailer), its counters are correctly cleared upon zone transition.
References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Convenience shortcuts that get rules wrong are not simpler; they are wrong. (link)
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review — CHANGES REQUESTED (one rules-fidelity gap + rebase)
Reviewed at head. The architecture here is genuinely class-based, not two name-matched special cases, and most of it is excellent. One open rules-fidelity gap (raised by Gemini, confirmed below) needs addressing before enqueue, and the branch is BEHIND main.
What's right (keep all of this)
StaticMode::CountersPersistAcrossZones { excluded_zones: Vec<Zone> }parameterizes the exclusion set as a typed zone list — covers the whole "Counters remain on ~ as it moves to any zone other than [zones]" class. Parser anchors on the normalized~glyph (name-agnostic; Me, the Immortal and Skullbriar share the verbatim phrase), and the zone-list tail is analtcombinator (parse_excluded_zone_list) with a word-order variant arm — sibling phrasings slot in without a new variant. Tests assert on the~form and exercise the full destination matrix, so the parameter is verified, not one card.- The clause-splitter fix in
sequence.rs(a~'spossessive apostrophe no longer opens a phantom single-quote that swallows a.sentence boundary) is a real building-block fix for the whole "[filter referencing ~'s P/T] gains [kw] until EOT. [imperative on it]" class, with a discriminating splitter-level test. Good reasoning that Oracle quoted-ability text always uses double quotes. - All new exhaustive-match sites (
Hash,is_resettable/grouping,Display,is_data_carrying_static) updated. - CR annotations verified against docs/MagicCompRules.txt: CR 122.2 (counters cease on zone change), CR 113.6b (ability functions only from stated zones), CR 701.15a (goad) — all confirmed.
The gap (confirmed against head — must address)
counters_persist_on_move reads functioning_abilities::active_static_definitions, which iterates the object's raw obj.static_definitions (gated only by phasing / command-zone / condition). It does not reflect Layer-6 ability-removal (Humility / Yixlid Jailer's "Cards in graveyards lose all abilities"):
evaluate_layers(layers.rs:1244) only processes battlefield + hand objects; graveyard/exile objects never haveRemoveAllAbilitiesapplied to theirstatic_definitions.- So with Yixlid Jailer out and Me/Skullbriar in a graveyard bearing counters, a graveyard→exile move would incorrectly persist the counters — the from-zone ability is rules-meant to be stripped (CR 113.6b read from the from-zone state). The doc-comment in
zones.rsanticipates theactive_zonesaxis but is silent on this ability-removal axis.
This is narrow, and graveyard ability-removal appears generally unmodeled engine-wide, so it is not a regression of existing behavior — but it is a known-wrong interaction introduced on a new path, and the PR's own doc comment gives a false sense of completeness by addressing only the active_zones caveat.
Requested changes
- Either guard the helper against from-zone ability-removal, or — given graveyard layer application is a broader engine limitation — extend the
zones.rsdoc-comment caveat to explicitly name the Humility/Yixlid (Layer-6 ability-removal) axis as the documented limitation, matching how the active_zones caveat is already called out, and add a// TODO-class follow-up note. Make the deferral honest rather than implicit. - Rebase onto
origin/main(mergeStateStatus: BEHIND).
Everything else is enqueue-quality. Once the deferral is documented honestly (or the guard added) and the branch is current, this is good to land.
Address maintainer review: the counters_persist_on_move doc-comment implied that "any layer-applied ability removal (such as Yixlid Jailer in graveyards) [is] evaluated from the correct zone", which overstated completeness. The helper reads obj.static_definitions directly via active_static_definitions, and evaluate_layers only applies layers to battlefield + hand objects — so a graveyard/exile object never has its abilities stripped, and with Yixlid Jailer in play the counters would INCORRECTLY persist on a graveyard->exile move. Restructure the doc-comment into two explicitly-named documented limitations (the existing active_zones caveat plus the Layer-6 ability-removal axis), make the CR 113.6b paragraph claim only the condition gate (not ability removal), and add a TODO to re-check persistence against the layer-resolved view once evaluate_layers applies Layer-6 ability removal to non-battlefield zones. This makes the deferral honest rather than implicit; graveyard ability-removal is unmodeled engine-wide, so this is not a regression. Verification: clippy -p engine --all-targets -D warnings clean; cargo test -p engine green (0 failed).
Maintainer follow-up — deferral documented honestly & enqueuingAddressed the one open rules-fidelity gap by making the deferral explicit rather than implicit, and rebased onto current The previous
The CR 113.6b paragraph now claims only the Verification: |
matthewevans
left a comment
There was a problem hiding this comment.
Deferral documented honestly (Layer-6 ability-removal axis named explicitly + TODO), rebased onto main. Engine suite green.
…-unsupported-cluster-play-cast
Summary
Implements full engine support for two previously-unsupported Doctor Who precon cards.
Cards supported
CountersPersistAcrossZones, excluded Hand/Library; runtime guard inzones.rs::counters_persist_on_move).~'s powerpossessive no longer swallows the trailing period, so the activated ability grants Skulk (until EOT) to an opponent creature filtered byPtComparison Power LE Ref{Power,Source}, and the trailingGoad it.chains correctly.Note
These were two cards from a WHO triage cluster that turned out heterogeneous; the other three (The Fourth/Eighth/Second Doctor) need distinct play-from-zone / attack-restriction primitives and are handled in separate follow-up PRs.
Verification
cargo fmtclean ·cargo clippy -p engine --all-targets -D warningsclean ·cargo test -p engine12,176 pass / 0 fail · both cards re-parse with zero Unimplemented in the fixed clauses.CR refs verified against docs/MagicCompRules.txt.
🤖 Generated with Claude Code