fix(parser): distribute Mutable Pupa's perpetual keyword-mirror trigger per keyword - #6533
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ChangesPerpetual keyword mirroring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant OracleParser
participant EffectChainAssembler
participant GameResolver
participant IntegrationTests
OracleText->>OracleParser: parse perpetual keyword-grant trigger
OracleParser->>EffectChainAssembler: select per-keyword replication
EffectChainAssembler->>GameResolver: attach replicated sibling abilities
GameResolver->>GameResolver: evaluate independent sibling gates
IntegrationTests->>GameResolver: resolve Mutable Pupa or Kathril scenario
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
crates/engine/src/parser/oracle_effect/mod.rs (1)
21740-21745: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a shared per-keyword sibling-replication helper.
attach_perpetual_keyword_grantsis structurally identical to the existingattach_repeat_process_keywords(clone template → rewrite the effect-specific field → rewrite the condition viarewrite_ability_condition_keyword→ stampsub_link = SequentialSibling/sibling_condition = ReplicatedOrBranch/sub_ability = None→ push). The only difference is whichEffectfield gets the new keyword. This PR itself had to duplicate thesibling_conditionmarker addition into both functions — exactly the kind of drift risk a shared helper (parameterized by a small closure/mutator for the effect field) would eliminate.♻️ Sketch of a shared helper
-fn attach_repeat_process_keywords( - defs: &mut Vec<AbilityDefinition>, - template_index: usize, - keywords: &[Keyword], -) { - let template = defs[template_index].clone(); - for keyword in keywords { - let mut new_def = template.clone(); - if let Effect::PutCounter { counter_type, .. } = &mut *new_def.effect { - *counter_type = CounterType::Keyword(keyword.kind()); - } - if let Some(condition) = &mut new_def.condition { - rewrite_ability_condition_keyword(condition, keyword); - } - new_def.sub_link = SubAbilityLink::SequentialSibling; - new_def.sibling_condition = SiblingCondition::ReplicatedOrBranch; - new_def.sub_ability = None; - defs.push(new_def); - } -} - -fn attach_perpetual_keyword_grants( - defs: &mut Vec<AbilityDefinition>, - template_index: usize, - keywords: &[Keyword], -) { - let template = defs[template_index].clone(); - for keyword in keywords { - let mut new_def = template.clone(); - if let Effect::ApplyPerpetual { - modification: PerpetualModification::GrantKeywords { keywords: kws }, - .. - } = &mut *new_def.effect - { - *kws = vec![keyword.clone()]; - } - if let Some(condition) = &mut new_def.condition { - rewrite_ability_condition_keyword(condition, keyword); - } - new_def.sub_link = SubAbilityLink::SequentialSibling; - new_def.sibling_condition = SiblingCondition::ReplicatedOrBranch; - new_def.sub_ability = None; - defs.push(new_def); - } -} +fn replicate_per_keyword_sibling( + defs: &mut Vec<AbilityDefinition>, + template_index: usize, + keywords: &[Keyword], + mut rewrite_effect: impl FnMut(&mut Effect, &Keyword), +) { + let template = defs[template_index].clone(); + for keyword in keywords { + let mut new_def = template.clone(); + rewrite_effect(&mut new_def.effect, keyword); + if let Some(condition) = &mut new_def.condition { + rewrite_ability_condition_keyword(condition, keyword); + } + new_def.sub_link = SubAbilityLink::SequentialSibling; + new_def.sibling_condition = SiblingCondition::ReplicatedOrBranch; + new_def.sub_ability = None; + defs.push(new_def); + } +}As per coding guidelines, "Build reusable card-class building blocks rather than one-card special cases" and as per path instructions, avoid "any new helper that duplicates an existing building block."
Also applies to: 21763-21814
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 21740 - 21745, Extract the duplicated per-keyword sibling construction from attach_perpetual_keyword_grants and attach_repeat_process_keywords into one shared helper. Parameterize the helper with the effect-field mutator, while centralizing template cloning, rewrite_ability_condition_keyword, sibling metadata initialization (including ReplicatedOrBranch), sub_ability clearing, and pushing the result; update both callers to use it without changing their effect-specific behavior.Sources: Coding guidelines, Path instructions
crates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rs (1)
208-225: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCounter-count assertions use
>= 1instead of an exact count.
count(CounterType::Keyword(Keyword::Trample.kind())) >= 1andcount(CounterType::Plus1Plus1) >= 1should be exact (== 1) given the setup (one trample creature card in the graveyard ⇒ exactly one trample counter ⇒ exactly one +1/+1 counter).>=wouldn't catch a regression where aReplicatedOrBranchnode is visited more than once (e.g., a future change toresolve_chain_body's new disjunct double-resolving a sibling), which is precisely the class of bug this test suite is meant to guard against.♻️ Proposed fix
- assert!( - count(CounterType::Keyword(Keyword::Trample.kind())) >= 1, - "trample is in the graveyard ⇒ a trample counter is placed (chain reached past false flying/first-strike/... gates)", - ); + assert_eq!( + count(CounterType::Keyword(Keyword::Trample.kind())), + 1, + "trample is in the graveyard ⇒ exactly one trample counter is placed", + ); @@ - assert!( - count(CounterType::Plus1Plus1) >= 1, - "the unconditional +1/+1 tail must land (chain reaches the end)", - ); + assert_eq!( + count(CounterType::Plus1Plus1), + 1, + "exactly one +1/+1 counter (one counter placed on a creature this way)", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rs` around lines 208 - 225, Update the trample and +1/+1 counter assertions in this integration test to require exactly one counter instead of at least one. Change the checks for CounterType::Keyword(Keyword::Trample.kind()) and CounterType::Plus1Plus1 while preserving the existing zero-count flying assertion and diagnostic messages.crates/engine/src/game/effects/mod.rs (1)
8048-8055: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winIndependent-branch gate doesn't verify
sub_link, unlike its sibling disjunct.
sub_is_replicated_or_branchlets a sub resolve regardless of this node's failed condition purely becausesibling_condition == ReplicatedOrBranch. The disjunct right below it (sub.sub_link == SequentialSibling && sub.condition.is_none()) explicitly re-checks the link kind, but this new disjunct doesn't.SiblingConditionandSubAbilityLinkare independent fields, so nothing here stops aReplicatedOrBranchmarker on aContinuationStepsub from being treated as an independent OR-branch and resolved past a failed parent gate — the exact semantics this code exists to avoid for dependent continuations. Today's parser always pairs the two, but that invariant isn't enforced at this call site.🛡️ Proposed defensive guard
- let sub_is_replicated_or_branch = - sub.sibling_condition == SiblingCondition::ReplicatedOrBranch; + let sub_is_replicated_or_branch = sub.sibling_condition + == SiblingCondition::ReplicatedOrBranch + && sub.sub_link == SubAbilityLink::SequentialSibling;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/mod.rs` around lines 8048 - 8055, Update the `sub_is_replicated_or_branch` disjunct in the surrounding sub-resolution logic to require `sub.sub_link` to be the independent branch link type as well as `SiblingCondition::ReplicatedOrBranch`. Preserve the existing condition-dependent and independent-event-gate checks, and ensure dependent continuation links cannot bypass a failed parent condition solely because of their sibling condition marker.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 27072-27097: Update the kind selection for
ReplicateKind::PerpetualKeywordGrant in the current clause-building logic to
inspect the most recent clause whose disposition is not
ClauseDisposition::Continue, rather than using builder.clauses().last().
Preserve the existing ApplyPerpetual with GrantKeywords match and StaticGrant
fallback, using the established reverse-search pattern so absorbed rider clauses
are skipped.
In `@crates/engine/src/parser/oracle_ir/effect_chain.rs`:
- Around line 409-415: Correct the perpetual keyword replication documentation:
in crates/engine/src/parser/oracle_ir/effect_chain.rs lines 409-415, replace the
CR 608.2c citation with verified CR 702.1c; in
crates/engine/src/parser/oracle_effect/assembly.rs lines 641-647, add the
matching CR 702.1c citation and explicitly identify “perpetually” as a
digital-only extension outside the CR.
In `@crates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rs`:
- Around line 179-200: The Kathril integration test currently provides
insufficient mana and allows duplicate counters to pass. Update the mana setup
in kathril_reaches_matching_counter_and_tail_past_false_earlier_gates to include
the required generic, white, black, and green mana for {2}{W}{B}{G}, then
tighten the Trample and +1/+1 assertions to require exactly one counter each.
---
Nitpick comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 8048-8055: Update the `sub_is_replicated_or_branch` disjunct in
the surrounding sub-resolution logic to require `sub.sub_link` to be the
independent branch link type as well as `SiblingCondition::ReplicatedOrBranch`.
Preserve the existing condition-dependent and independent-event-gate checks, and
ensure dependent continuation links cannot bypass a failed parent condition
solely because of their sibling condition marker.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 21740-21745: Extract the duplicated per-keyword sibling
construction from attach_perpetual_keyword_grants and
attach_repeat_process_keywords into one shared helper. Parameterize the helper
with the effect-field mutator, while centralizing template cloning,
rewrite_ability_condition_keyword, sibling metadata initialization (including
ReplicatedOrBranch), sub_ability clearing, and pushing the result; update both
callers to use it without changing their effect-specific behavior.
In `@crates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rs`:
- Around line 208-225: Update the trample and +1/+1 counter assertions in this
integration test to require exactly one counter instead of at least one. Change
the checks for CounterType::Keyword(Keyword::Trample.kind()) and
CounterType::Plus1Plus1 while preserving the existing zero-count flying
assertion and diagnostic messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a0605a22-7ae0-42ae-809a-b9a4996dae5b
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__kathril_aspect_warper_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (23)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/effects/additional_phase.rscrates/engine/src/game/effects/double.rscrates/engine/src/game/effects/extra_turn.rscrates/engine/src/game/effects/grant_extra_loyalty_activations.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/player_counter.rscrates/engine/src/game/effects/reverse_turn_order.rscrates/engine/src/game/effects/skip_next_step.rscrates/engine/src/game/effects/skip_next_turn.rscrates/engine/src/game/effects/vote.rscrates/engine/src/game/stack.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/effect_chain.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rscrates/engine/tests/integration/the_chain_veil_loyalty_grants.rs
|
Maintainer follow-up on the current head:
|
Parse changes introduced by this PR · 10 card(s), 14 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current head c645fe0 is clean for merge-queue processing. The current parse diff is confined to Mutable Pupa; the typed replicated-branch path preserves dependent continuations; and the registered GameRunner regressions cover a late keyword and the Kathril false-prefix path. Required checks remain pending; auto-merge will wait for them.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after current-head maintainer review.
- The parser/engine seam and typed sibling-condition threading are correct.
- The parse-diff is limited to Mutable Pupa's conditional
ApplyPerpetualsupport. - The end-to-end regressions cover both late-keyword propagation and the Kathril false-prefix case; the final dereference correction is test-only.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head 9e91a31641 after the maintainer CI repair.
- The Kathril regression now declares Kathril as the ETB counter recipient through the existing
GameRunnertrigger-target selection path; it does not add an incorrectTargetFilter::Anysource fallback. - The IR expectation now includes
sibling_condition: ReplicatedOrBranchon all ten replicated Kathril keyword branches, matching the already-correct lowered snapshot.
cargo fmt --all and git diff --check passed. The worktree's Tilt wait returned exit 3 because Tilt watches the main checkout, so it cannot attest to this worktree; required GitHub checks are running and auto-merge remains enabled.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved: current head 3c1f1a9b5d contains only the maintainer test-fixture repair. The Kathril regression now funds its actual {2}{W}{B}{G} cost, so the existing cast → ETB → target-selection pipeline reaches the per-keyword-chain assertions it is meant to discriminate. Auto-merge remains enabled; required CI is running.
|
Maintainer hold: the current required Rust-tests failure is Kathril's graveyard keyword predicate (expected trample counter 1, got 0). The apparent repair is not a safe one-line test or filter fix: FilterProp::WithKeyword currently reads battlefield-only keywords, while off-zone queries have a kind-only authority; this predicate deliberately uses discriminant semantics, and entry/LKI/event-snapshot filter paths must retain their own snapshot semantics. Please do not patch this at the failing assertion. It needs a narrowly reviewed shared-characteristics boundary plus discriminating live off-zone and snapshot regression coverage before re-enabling auto-merge. |
The new drive_resolution handler for WaitingFor::ChooseFromZoneChoice hard-panicked when a test declared no objects for it, but this variant predates the Kathril work -- it's the existing CR 608.2d tracked-set choice mechanism (exiled-cards picks, etc.), and pre-existing tests using it (Portent of Calamity) never needed to declare anything, since drive_resolution previously had no handler at all for this variant and such tests must have relied on a path that didn't reach here, or on this exact prompt not existing pre-fix. Match the convention every other default-driven choice in this function already uses (ScryChoice, SurveilChoice, ArrangePlanarDeckTopChoice all auto-default rather than requiring explicit test declaration): try the declared-object pool first so a test can still pin a specific recipient (as the new Kathril discriminating test does), then fall back to the front of the legal set for anything undeclared, instead of panicking.
…drive_resolution Reverts the previous fix's approach entirely: adding an auto-driving arm for WaitingFor::ChooseFromZoneChoice to the shared drive_resolution was the wrong direction. That variant predates this PR (it's the existing CR 608.2d tracked-set choice mechanism) and Portent of Calamity's own test (issue_6498) deliberately relies on .resolve() STOPPING at the first such prompt so it can drive each per-type exile pick manually, one at a time, with its own selection logic -- exactly like the pre-existing Wick test does. Auto-driving it out from under that test silently consumed all 4 of its prompts before the test's own loop ever ran, dropping its exile count to 0. Removed the arm entirely, restoring the original `_ => break` handling for this variant. Updated the new Kathril discriminating test to match the same manual-driving convention Portent's and Wick's tests already use: call .resolve() (which now correctly stops at the first prompt), then loop over WaitingFor::ChooseFromZoneChoice states directly, answering each with the declared recipient in written order.
…ing it
Reconciling the coverage-parse-diff blast radius from the "any"
quantifier widening surfaced a real, pre-existing gap: the CR 205.3a
"[Subtype] [CoreType]" promotion (e.g. "Wizard creatures") only ever
consumed a SECOND word when it was a concrete core type. A second
consecutive SUBTYPE word (e.g. "Elder Dragon", "Elf Warrior", "Human
Wizard" -- two independently-registered subtypes stacked on one
permanent, not a compound word) was silently dropped, since the
`type_filters` builder discards it (the caller doesn't require an
empty remainder).
Fate Reforged chapter II ("a copy of any Elder Dragon from the Legends
expansion") is the concrete case the parse-diff surfaced: before the
"any" fix this collapsed all the way to TargetFilter::Any (matching
literally anything); after it, "any " correctly reaches the subtype
parser, which then only captured Subtype("Elder") and dropped
"Dragon" -- an over-broad filter matching any Elder-subtype creature,
not specifically Elder Dragons. Chained the second subtype word into
the same slot the first arm already threads through, matching the
Legends-era Elder Dragon type line (Nicol Bolas, Palladia-Mors, etc.)
correctly. This is a general fix, not a Fate-Reforged special case --
it benefits any card using a two-subtype phrase, most of which never
appeared in this PR's diff because they were broken the same way
before the "any" fix too, just silently.
Card-level reconciliation for the remaining coverage-parse-diff
entries (issue phase-rs#6321 / PR phase-rs#6533 review):
- Naming Screen: "doesn't share a name with any other creature you
control" -- parse_shared_quality_reference explicitly rejects a
TargetFilter::Any reference population, so before the "any"+"other"
composition fix this whole relative clause failed to parse and the
static ability fell through to an unstructured fallback. Proven via
parse_shared_quality_clause_naming_screen_reference using the card's
verbatim clause.
- Duplication Device: "becomes a copy of any creature on the
battlefield" -- Effect::BecomeCopy's target is parsed via the same
shared parse_target this whole fix touches (oracle_effect/subject.rs);
"any creature on the battlefield" now correctly reaches the
pre-existing, unmodified zone-suffix machinery instead of collapsing
to Any. Proven via parse_type_phrase_any_creature_on_the_battlefield.
- Kathril, Mutable Pupa: already covered by this PR's existing
integration tests.
0b74059 to
6b2d818
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head blocker — do not enqueue
[HIGH] The new consecutive-subtype branch misparses card names as a conjunction of subtype filters and fails required CI.
Evidence: current head f57544fbde85e342d36289cf9bc06112c31b6062; the branch at crates/engine/src/parser/oracle_target.rs:2537 consumes every second TypeFilter::Subtype as an AND-combined subtype. CI run 30113382940, Rust tests shard 1/2, now fails parser::oracle::tests::urzas_lands_share_delta_shape: "Urzas Mine" becomes Subtype("Urzas") + Subtype("Mine"), yielding a QuantityCheck instead of the expected two ControllerControlsMatching conditions. The same change makes legacy_misparses_are_now_honest_gaps parse the three Urza-land names as a nonempty QuantityComparison rather than leave them unsupported.
Why it matters: the generic rule cannot tell a card name from two independent creature subtypes. Reverting only the failing assertions would make previously honest parser gaps silently incorrect; retaining the branch breaks existing behavior. The branch also still cites CR 205.3a for this interpretation at lines 2537 and 15371, but that rule says only that a card may have one or more subtypes. The relevant support for separate creature subtypes is CR 205.3b + CR 205.3m, which must be verified and cited only if the redesigned grammar actually implements that distinction.
Required evidence is also unavailable: the current-head parse-diff artifact (parse-diff, created 2026-07-24T17:33:19Z in the same run) remains Baseline pending for 192897ca50a2d8daf3306f4d2a446b02bee64aa2.
Requested resolution: rework or remove this generic subtype-chain expansion at the type-phrase grammar seam. It needs a reviewed distinction for recognized creature-subtype sequences versus card names/other subtype categories, discriminating positive and negative parser coverage (including the existing Urza cases), corrected CR citations, and a current card-level parse-diff before re-review. A two-line citation change is insufficient.
|
Correction to the quoted CI evidence above: the exact card names are |
…aining
The two-consecutive-subtype-word chain added for Fate Reforged's
"any Elder Dragon" regressed the dedicated Urza-lands condition parser:
"Urza's" (LAND_SUBTYPES, card_type.rs) is the one possessive-suffixed
entry in the whole subtype vocabulary -- a name fragment meant to
attach to a following noun ("Urza's Mine"/"Tower"/"Power-Plant"), not
an independently AND-combinable subtype like "Elder"/"Elf"/"Human".
Chaining it fully consumed "an urza's mine" into one
Typed{Subtype("Urza's"), Subtype("Mine")} filter with an empty
remainder, which changed which downstream condition-builder claimed
the clause and broke urzas_lands_share_delta_shape (expected
ControllerControlsMatching{filter} with get_subtype() == "Mine", got
a QuantityCheck over the two-subtype AND filter instead) and
legacy_misparses_are_now_honest_gaps (expected None -- an
intentionally undocumented gap -- got a confidently wrong
QuantityComparison).
Guarded the chain on the first subtype word not ending in "'s"
(verified "Urza's" is the only such entry across LAND_SUBTYPES,
ARTIFACT_SUBTYPES, ENCHANTMENT_SUBTYPES, SPELL_SUBTYPES,
BATTLE_SUBTYPES, PLANESWALKER_SUBTYPES, and the MTGJSON-derived
creature vocabulary), restoring "urza's mine" to its original
Subtype("Urza's") + unconsumed "mine" remainder so the specialized
handler still sees it, while keeping the chain for genuine two-word
subtype pairs.
|
Fixed on Guarded the chain on the first subtype word not ending in |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested — do not enqueue
[HIGH] The Urza guard is a special case at the wrong parser boundary, not the required class-level distinction.
Evidence: head 7c48b74 replaces the prior generic subtype chain with s.ends_with("'s") at crates/engine/src/parser/oracle_target.rs:2537. That suppresses only one observed token, while the branch still asks the all-category parse_type_filter_word result to decide whether two consecutive words are independent creature subtypes. The existing authority in crates/engine/src/parser/oracle_util.rs:1271-1292, parse_creature_subtype, is explicitly restricted to the CR 205.3m creature-subtype vocabulary; it is the appropriate building block for the Elder Dragon class and naturally excludes noncreature subtype/card-name fragments such as Urza’s.
Why it matters: the new guard makes this particular regression green without establishing the grammar invariant for the class. Another noncreature subtype sequence or name fragment will still be consumed as an AND filter, and the parser continues to conflate subtype categories. Rework the consecutive-word branch through the creature-only authority (or an equivalent typed category boundary), with positive examples and negative noncreature/name-fragment coverage. Do not retain a data-specific possessive exception.
[MED] The two new chain annotations still cite CR 205.3a at oracle_target.rs:2555 and :15391. CR 205.3a says only that a card may have one or more subtypes; it does not describe how creature subtype words are separated. Verify and use CR 205.3b + CR 205.3m if the corrected grammar actually implements that behavior.
[HIGH] Required external evidence is not yet available. Current CI run 30121391095 still has Rust lint, both Rust-test shards, and Card data in progress; no parse-diff artifact exists for this head yet. The previous artifact was baseline-pending and cannot evidence the new head. Therefore this PR is not approval- or queue-eligible regardless of the dashboard recommendation.
Requested resolution: use the existing creature-only subtype parser at the type-phrase seam, correct the CR annotations, add discriminating class/negative tests, and wait for green current-head CI plus the current parse artifact before requesting re-review.
|
Parse-artifact update for current head 7c48b74: The current run 30121391095 has now published parse-diff artifact 8607708221 against main 438cad0. It reports 10 cards and 14 signatures, not a narrow Mutable Pupa/Kathril-only delta:
The PR body and focused tests do not reconcile or give card-level/discriminating coverage for Kor Castigator, Eldest Dragon Highlander, Hazezon Tamar, Push Your Luck, or Tovolar. These are exactly the broader two-subtype grammar class that the new possessive exception does not model. This is independent evidence that the parser change has unreviewed blast radius; do not approve or enqueue until every signature is explained and the grammar is corrected at the creature-only subtype seam. Current Rust lint and both Rust test shards remain pending. The earlier note that no artifact existed was accurate when filed; this published artifact strengthens the hold. |
Redesigns the previous "exclude Urza's specifically" patch into a principled, CR-grounded rule instead of an ad-hoc exception. CR 205.3b: subtypes of every card type except creature (and plane) are always single words on a printed type line -- each dash-separated word is its own subtype. Creature subtypes are the one category the rules let run one OR two words (CR 205.3m: the sole two-word creature type is "Time Lord"; every other listed type, including "Elder"/"Dragon"/ "Elf"/"Warrior"/"Human"/"Wizard", is one word). So when Oracle text names two consecutive creature-subtype words, that is genuinely ambiguous -- one two-word type, or two one-word types stacked -- in a way that never arises for other categories, where CR 205.3b already guarantees each word is separate. This generic chain exists to resolve exactly that creature-only ambiguity, so it is now scoped to fire ONLY when neither matched word is a registered noncreature subtype (fixed_noncreature_subtypes -- land/artifact/enchantment/spell/battle/planeswalker), replacing the narrower "does the first word end in 's" check. "Urza's" is a real land type (CR 205.3i) -- land subtypes genuinely CAN co-occur on one permanent (Urza's Mine has both the "Urza's" and "Mine" land subtypes) -- but the dedicated Urza-lands ControllerControlsMatching condition parser already owns that Oracle-text pattern and deliberately extracts only the discriminating second word, so this generic rule must stay out of its way, and out of every other noncreature category's way too, not just this one land cycle. Also corrects the CR citation on the chaining rule itself from 205.3a (which only establishes that a card may have subtypes at all) to 205.3b + 205.3m (which establish the actual one-vs-two-word creature distinction the rule implements) -- 205.3a stays correctly cited on the unrelated pre-existing "[Subtype] [CoreType]" promotion arm beside it, which is a different pattern.
|
Addressed on One clarification on the evidence first: CR citation, verified against "The generic rule cannot tell a card name from two independent creature subtypes" — this is the real finding, and the "Urza's" patch didn't actually address it (it excluded one instance, not the class). Reworked: the chain now fires only when neither matched word is a registered noncreature subtype ( Discriminating coverage: A fresh parse-diff will generate against this commit on the next CI run. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off on 50d1de7: current CI is green; the current parse artifact is reconciled; the subtype-chain boundary now excludes fixed noncreature subtype categories and retains the dedicated Urza-land path.
2 participants
Summary
Fixes #6321. Mutable Pupa's second ability was entirely
Effect::Unimplemented:(Oracle text verified against Scryfall
508be2d9-c896-4cbf-b574-2896769d7987.)This is the same "the same is true for K1, K2, …" per-item list-collapse family that #6168 fixed for continuous exiled-object keyword grants (Eater of Virtue, Urborg Scavengers) — but for a triggered, resolve-once perpetual keyword mirror gated on the entering creature, not a continuously re-evaluated
IsPresentboard search. Evolve (the first ability) was already correctly implemented and is untouched.Root cause & fix
No
AbilityConditionvariant checked "the trigger's event-bound object has keyword K" as a plain boolean gate. The fix reuses the existingAbilityCondition::ZoneChangeObjectMatchesFilter(readsstate.current_trigger_event, CR 603.2/603.6) andEffect::ApplyPerpetual/PerpetualModification::GrantKeywords— no new runtime-facing enum variants.strip_suffix_conditionalgets a new trailing "that creature/permanent has<keyword>" arm, explicitly scoped toctx.in_trigger(CR 115.1) so it can never intercept the pre-existing, unrelatedstrip_target_keyword_insteadclass. A newReplicateKind::PerpetualKeywordGrantlowering template (parser-internal IR, alongside the existingStaticGrant/CounterPlacementleaves) distributes the keyword list into independentSequentialSiblinggrants, reusingtry_parse_same_is_true_continuationexactly as #6168 did.The independent-branch chain needed a real runtime fix:
resolve_chain_bodyonly re-checked aSequentialSibling's own condition past a preceding sibling's failed gate for two narrow existing cases — so the keyword list would silently collapse at the first false gate. Fixed with a construction-scopedSiblingConditionmarker (Dependentdefault /ReplicatedOrBranch, stamped only by the two per-keyword replication helpers) rather than matching on condition type, since a type-based approach would have regressed Thieving Skydiver's genuinely dependent "if that artifact is an Equipment" continuation (confirmed during review). The identical live bug already shipping in Kathril, Aspect Warper's counter-placement chain is fixed by the same marker — same root cause, same fix, no extra casework.CR
Tests
GenericEffect(not routed through the new perpetual path); Thieving Skydiver's dependent continuation is never stampedReplicatedOrBranch.crates/engine/tests/integration/mutable_pupa_perpetual_keyword_mirror.rs) driving the real cast → trigger → resolve pipeline: Mutable Pupa + Affa Protector (single matching keyword), a multi-keyword accumulation case, and a Kathril regression proving the chain now reaches a matching counter and the unconditional tail past a false earlier gate.Verification
This environment could not run
cargo clippy/cargo test— no native (MSVC) linker is available in this sandbox, confirmed directly (evencargo check -p enginefails at the build-script link stage), and Tilt is not running.cargo fmt --allpasses. The implementation went through 4 rounds of independent review-and-fix (spawned as fresh, isolated review passes per this repo'sengine-implementerpipeline) against the actual current source, which caught and fixed 3 real issues along the way (a missingResolvedAbilityfield-propagation site that would have made the fix a no-op, dead code from an earlier design iteration, and a leaked keyword-predicate path that would have regressed several shipping "instead" cards) — but none of that substitutes for an actual compile. CI needs to be the first real compile/test signal for this PR; please treat that as load-bearing, not a formality, given the environment constraint above.Tier: Frontier
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests