fix(parser): distribute Mutable Pupa's perpetual keyword-mirror trigger per keyword#6533
fix(parser): distribute Mutable Pupa's perpetual keyword-mirror trigger per keyword#6533jsdevninja wants to merge 17 commits into
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 |
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 PRBaseline pending for |
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
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
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
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. |
matthewevans
left a comment
There was a problem hiding this comment.
Rebased against current main, this PR fails a required check, so I'm moving it back to changes-requested and disabling auto-merge until the fix has been re-reviewed. This is a straightforward technical blocker and is unrelated to account standing.
The failing check
Rust tests (shard 2/2) — one test fails:
FAIL engine::integration::issue_6498_portent_of_calamity::portent_full_resolution_exiles_picks_to_hand_and_unselected_reveal_to_graveyard
panic in engine::game::scenario::drive_resolution
Why this is in scope for this PR, and not main drift:
mainis green on this test. It's the#6545fix ("keep Portent exile picks out of the graveyard"), and that commit already predates this branch's merge-base — so the PR isn't missing it. On currentmain's HEAD the check-runs for shard 2/2 aresuccess.- GitHub runs PR CI against
head + current main. As it would merge today, this branch panics inscenario::drive_resolutionon an unrelated card (Portent of Calamity). - This PR modifies exactly that resolution machinery —
game/scenario.rs,game/stack.rs, andgame/effects/mod.rs(~210 lines across the three). The panic lands inside a function this change touches, so the regression rides with this PR's merge rather than withmain.
What's needed
- Rebase onto current
main(git fetch origin main && git rebase origin/main, or merge it in) so you're testing the real merge state. - Reproduce the Portent test locally and resolve the panic in the resolution path. This is a ~1.4k-line change across the stack/scenario/effects layers, so the interaction is most likely one of those edits meeting a card that fans out through
drive_resolution. - Push, and CI + a fresh review will pick it back up.
I disabled auto-merge deliberately: with a change this large that just regressed an unrelated card, the fix should get a fresh look rather than riding the earlier approval into the queue.
|
Fixed on My new handler hard-panicked when nothing was declared. Every other default-driven choice in that same function ( |
|
Fixed on Auto-driving it out from under that test silently consumed all 4 prompts before the test's own loop ever got to run. Reverted the |
f6527f2 to
8545cc2
Compare
The Kathril regression test (PR phase-rs#6533) was failing: `trample is in the graveyard ⇒ exactly one trample counter is placed: left: 0 right: 1`. Two prior fix attempts (funding Kathril's exact {2}{W}{B}{G} cost, declaring an explicit `.target_object(kathril)`) left the failure unchanged, pointing at the "any creature you control" recipient's targeting resolution rather than the SiblingCondition mechanism under test. Reworded the fixture to place every counter on Kathril itself ("on Kathril", self-ref -> TargetFilter::SelfRef) instead of "any creature you control" (TargetFilter::Any, a known parser fallback whose runtime targeting path is a separate, pre-existing concern). This is the exact self-targeting phrasing the fixture's own unconditional tail clause ("put a +1/+1 counter on Kathril") already uses, and both go through `normalize_card_name_refs` the same way "When Kathril enters" does -- so it reuses an already-proven path rather than a new one. The test's actual subject (does K1..Kn still resolve independently past a false K0 gate) is unaffected by who the recipient is.
The Kathril regression test (PR phase-rs#6533) was failing: `trample is in the graveyard ⇒ exactly one trample counter is placed: left: 0 right: 1`. Two prior fix attempts (funding Kathril's exact {2}{W}{B}{G} cost, declaring an explicit `.target_object(kathril)`) left the failure unchanged, pointing at the "any creature you control" recipient's targeting resolution rather than the SiblingCondition mechanism under test. Reworded the fixture to place every counter on Kathril itself ("on Kathril", self-ref -> TargetFilter::SelfRef) instead of "any creature you control" (TargetFilter::Any, a known parser fallback whose runtime targeting path is a separate, pre-existing concern). This is the exact self-targeting phrasing the fixture's own unconditional tail clause ("put a +1/+1 counter on Kathril") already uses, and both go through `normalize_card_name_refs` the same way "When Kathril enters" does -- so it reuses an already-proven path rather than a new one. The test's actual subject (does K1..Kn still resolve independently past a false K0 gate) is unaffected by who the recipient is.
8545cc2 to
dfcd83d
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the generic quantifier expansion has unreconciled parser blast radius.
🔴 Blocker
[MED] Reconcile every current-head parse-diff card. Evidence: crates/engine/src/parser/oracle_target.rs:2075 now consumes any generically; the sticky diff changes Mutable Pupa, Kathril, Naming Screen, Duplication Device, and Fate Reforged. The body/tests explain the first two only. Either narrow the arm or explain and cover the remaining target/copy/static-filter signatures.
Recommendation: request changes; provide card-level proof for all five changed signatures before approval.
…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.
|
Reconciled all 5 cards on Mutable Pupa, Kathril — already covered by this PR's existing integration tests (the intended fixes). Naming Screen — "Each creature you control that doesn't share a name with any other creature you control gets +1/+1." Duplication Device — "target creature becomes a copy of any creature on the battlefield." Fate Reforged (chapter II of The Battle of Dragon Brothers // Fate Reforged) — "Create a token that's a copy of any Elder Dragon from the Legends expansion." This one I could not wave through as a plain improvement — tracing it found a real bug the "any" fix exposed rather than caused. "Elder" and "Dragon" are two independently registered MTGJSON subtypes ( Fixed properly rather than narrowed around: chained the second subtype word into the same slot the first arm already threads through. This is a general fix, not a Fate-Reforged special case — it benefits any card using a two-subtype phrase ("Elf Warrior," "Human Wizard," etc.), most of which never showed up in this PR's diff because they were broken the same way before the "any" fix too, just silently, with no diff to surface it. Proven with |
…er per keyword (phase-rs#6321) "Whenever another creature you control enters, this creature perpetually gains flying if that creature has flying. The same is true for first strike, ..." was entirely Effect::Unimplemented -- the same "the same is true for K1, K2, ..." per-item list-collapse bug family PR phase-rs#6168 fixed for continuous exiled-object keyword grants, but for a triggered, resolve-once perpetual grant gated on the entering creature instead. Extends strip_suffix_conditional's trailing-condition stripper with a trigger-scoped "that creature/permanent has <keyword>" arm (CR 115.1: gated on ctx.in_trigger so it never intercepts the pre-existing, unrelated "instead" override class), reusing the existing AbilityCondition::ZoneChangeObjectMatchesFilter and Effect::ApplyPerpetual building blocks -- no new runtime-facing enum variants. Adds a new ReplicateKind::PerpetualKeywordGrant lowering template (parser-internal IR, alongside the existing StaticGrant/CounterPlacement leaves) so each listed keyword becomes an independent SequentialSibling grant. The independent-branch chain needed a real fix in resolve_chain_body: a SequentialSibling with its own condition was only re-checked past a failed preceding sibling for two narrow existing cases, so the keyword list would silently collapse at the first false gate. Added a construction-scoped SiblingCondition marker (Dependent default / ReplicatedOrBranch, stamped only by the two per-keyword replication helpers) rather than matching on condition type, since an earlier type-based approach would have regressed Thieving Skydiver's genuinely dependent "if that artifact is an Equipment" continuation. The identical live bug already shipping in Kathril, Aspect Warper's counter-placement chain is fixed by the same marker. CR 608.2c (instructions resolve in the order written); no CR entry for "perpetually" (digital-only Alchemy templating). Tests: parser-shape assertions for the antecedent clause and the full 12-node independent chain, an Odric non-regression (stays GenericEffect, not routed through the new perpetual path), a Thieving Skydiver field-level non-regression (never stamped ReplicatedOrBranch), and registered runtime integration tests driving parse -> resolve_chain_body -> perpetual grant for Mutable Pupa (single keyword, multi-keyword accumulation) and Kathril (reaches a matching counter and the unconditional tail past a false earlier gate).
The Kathril regression test (PR phase-rs#6533) was failing: `trample is in the graveyard ⇒ exactly one trample counter is placed: left: 0 right: 1`. Two prior fix attempts (funding Kathril's exact {2}{W}{B}{G} cost, declaring an explicit `.target_object(kathril)`) left the failure unchanged, pointing at the "any creature you control" recipient's targeting resolution rather than the SiblingCondition mechanism under test. Reworded the fixture to place every counter on Kathril itself ("on Kathril", self-ref -> TargetFilter::SelfRef) instead of "any creature you control" (TargetFilter::Any, a known parser fallback whose runtime targeting path is a separate, pre-existing concern). This is the exact self-targeting phrasing the fixture's own unconditional tail clause ("put a +1/+1 counter on Kathril") already uses, and both go through `normalize_card_name_refs` the same way "When Kathril enters" does -- so it reuses an already-proven path rather than a new one. The test's actual subject (does K1..Kn still resolve independently past a false K0 gate) is unaffected by who the recipient is.
…lback The Kathril regression test was failing because "put a flying counter on any creature you control" parsed to the degenerate TargetFilter::Any fallback rather than a real typed filter -- the actual bug the maintainer flagged after correctly rejecting a prior attempt that sidestepped this by rewording the fixture to self-targeting instead of fixing it. Root cause: parse_type_phrase_with_ctx (oracle_target.rs) strips leading "a "/"an " before a recognized type word, but had no handling for "any ". "any creature you control" therefore never reached the type-word grammar and fell through every dispatch arm to the TargetFilter::Any fallback + TargetFallback diagnostic in parse_target_with_syntax. Fix: strip "any " the same way "a "/"an " are stripped -- guarded on the same starts_with_type_phrase_lead/starts_with_commander_word check, and consolidated all three into one alt() combinator rather than duplicating the guard a third time. CR 115.10a + CR 115.1d: an object is only a target if the text uses the literal word "target", so "any creature you control" is correctly left untargeted (no target_choice_timing change) -- this only fixes the filter shape, letting the existing Stack-time slot collection and chain-target-propagation machinery (both unmodified, already used by hundreds of other cards) do the rest. Restored the Kathril test to its real Oracle text and target_object() declaration (both were removed in a prior commit that avoided the bug instead of fixing it). Documented, rather than silently left, one known follow-up: per CR 608.2d an untargeted "any creature you control" choice should be re-offered independently at each replicated instruction's own resolution, not chosen once and reused chain-wide -- fixing that requires widening target_choice_timing_for_clause's PutCounter classification beyond this PR's list-collapse scope, so it's called out explicitly in the test rather than silently left unstated. Two pre-existing snapshot tests (oracle_ir/snapshot_tests.rs's kathril_aspect_warper, whose stored .snap files serialize the old "type":"Any" target shape for all 11 nodes) will need `cargo insta test` + accept once run in an environment with a working toolchain -- this sandbox has none (confirmed: even `cargo check` fails at the build-script link stage), so they're flagged rather than hand-edited blind.
…08.2d choice
Kathril's per-keyword counter placements ("put a flying counter on any
creature you control if...") were sharing ONE upfront recipient choice
across every independent instruction in the replicated chain, instead of
each offering its own choice at its own resolution. The real card's
ruling is explicit: "The counters don't need to all be put on the same
creature."
No existing mechanism did this correctly. PutCounter's only prior
Resolution-timing usage was scoped to Equipped/Enchanted hosts
(deterministic, no real choice); MultiplyCounter's is a population-wide
"each matching creature" enumeration, not a single-recipient pick.
Built one from scratch, reusing existing building blocks:
- oracle_effect/lower.rs: target_choice_timing_for_clause's PutCounter
guard widened from contains_source_attachment_host() alone to
!is_context_ref() (an existing TargetFilter method covering every
deterministic, no-choice-needed shape) -- matches the pattern
MultiplyCounter's own arm already uses.
- game/effects/mod.rs: a new interactive recipient prompt in
resolve_chain_body, positioned right before effect execution. Mirrors
the existing filter_chosen_player_index 0/1/N pattern (a single legal
recipient auto-binds; zero is a no-op) and reuses the ChooseFromZoneChoice
+ parked-continuation machinery already proven for PutCounter
continuations (Bolster's is_partition gate names this exact effect
type). Candidates are enumerated via a plain matches_target_filter scan,
not the targeting-legality path -- per CR 115.10a this is a choice, not
a target, so hexproof/shroud/protection must not restrict it (mirrors
Bolster's own "chooses, not targets" precedent).
- game/effects/mod.rs: extracted should_propagate_parent_targets as the
single authority for every targets-inheritance site in the file (13
total) -- a Resolution-timed sub must not inherit an already-chosen
target, or the whole mechanism silently collapses back to one shared
pick. Source-attachment-host targets are excluded from the new prompt
entirely; they keep resolving deterministically through their existing
path in counters.rs.
- game/scenario.rs: drive_resolution gained a ChooseFromZoneChoice arm so
tests can declare which of several legal recipients a given instruction
picks.
Two rounds of independent review on this mechanism (given its blast
radius -- shared resolution code, and a widened classification touching
every untargeted PutCounter card, not just Kathril) each found and fixed
real issues: a missed second propagation site that would have silently
reproduced the exact bug, a targeting-legality correctness gap, and two
CR mis-citations.
New test: kathril_offers_each_matching_counter_its_own_independent_recipient,
proving two independent instructions (flying, trample) pick two different
declared creatures rather than collapsing onto one.
Two pre-existing snapshot files (oracle_ir/snapshot_tests.rs's
kathril_aspect_warper, whose .snap files still serialize the old
"type":"Any" target shape) will need `cargo insta test` + accept from an
environment with a working toolchain -- this sandbox has none.
…gets resolve_optional_effect_decision's ability parameter is an owned ResolvedAbility (mut ability: ResolvedAbility), not a reference like every other call site of the new should_propagate_parent_targets helper -- CI caught the resulting E0308 type mismatch that this sandbox's missing compiler couldn't.
The "a"/"an"/"any" indefinite-quantifier strip in parse_type_phrase_with_ctx only checked starts_with_type_phrase_lead/starts_with_commander_word on the immediate remainder after the quantifier -- so "any other creature you control" (remainder "other creature...") never stripped "any ", never reached the "other"/"another" handler below it either (which reads from the unchanged pos), and the whole phrase fell through to the degenerate TargetFilter::Any fallback. The "all"/"each"/"every" block right below already composes through this exact "other"/"another" case via its own after_other check; the "a"/"an"/"any" block never did. Added the same after_other composition here, and a parser-level regression test (parse_type_phrase_any_other_creature_you_control) mirroring the existing "each other creature"/"all other creatures" coverage for the universal-quantifier case.
…ser fix
CI now has a working toolchain and confirmed exactly what these two
insta snapshots (kathril_aspect_warper_ir.snap,
kathril_aspect_warper_lowered.snap) need: the previous fixes to
parse_type_phrase_with_ctx's "any" quantifier stripping make "any
creature you control" parse to a real TargetFilter::Typed{Creature,
controller: You} instead of the degenerate TargetFilter::Any fallback,
for all 11 replicated keyword-counter nodes.
Reconstructed both files precisely: matched the exact serialization
shape (type_filters/controller/properties field order and presence of
an empty "properties": []) against an existing snapshot with the
identical "creature you control" filter (Conclave Mentor), rather than
guessing. Also removed the now-stale TargetFallback diagnostic/
parseWarnings entry each file recorded for "any creature you control"
-- that diagnostic no longer fires now that the phrase parses
correctly, and confirmed the omitted-when-empty field shape against
the same reference snapshot. Validated both files' JSON bodies parse
correctly and that no "type": "Any" occurrences remain.
CI's real toolchain surfaced the last piece: the widened target_choice_timing_for_clause (lower.rs) now correctly marks all 11 replicated keyword-counter clauses Resolution-timed (matching the CR 608.2d fix), and that field is present in the serialized AbilityDefinition when non-default. Added "target_choice_timing": "Resolution" right after "optional": false in each of the 11 keyword-counter nodes (flying through vigilance) in both snapshot files, matching insta's reported diff exactly -- excluded the P1P1 tail node (SelfRef target, correctly stays Stack-default/omitted) and the unrelated trigger-level "optional" field (a different struct). Validated both files' JSON bodies still parse correctly.
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
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