Skip to content

Fix Witherbloom Affinity, Sprout Swarm convoke, Crafty Cutpurse - #405

Merged
matthewevans merged 8 commits into
phase-rs:mainfrom
AgilErck:fix/card-bugs
May 15, 2026
Merged

Fix Witherbloom Affinity, Sprout Swarm convoke, Crafty Cutpurse#405
matthewevans merged 8 commits into
phase-rs:mainfrom
AgilErck:fix/card-bugs

Conversation

@AgilErck

Copy link
Copy Markdown
Contributor

Summary

Three independent card-correctness fixes, each covering a class beyond the named card. Review-cycle corrections folded in (CR-annotation fixes + alt-ordering bug + spec.controller redirect under CR 111.2).

1. Witherbloom, the Balancer — Affinity display and granted-keyword parsing

  • Display cost for un-castable spells. legal_actions_full only populated spell_costs for spells with an active CastSpell action, so a commander you couldn't yet afford fell back to the printed {5}{B}{G} instead of the Affinity-reduced cost. Added display_spell_cost, an engine-authoritative path that runs the full cost-modification pipeline (Affinity, ReduceCost/RaiseCost, commander tax, pending one-shot reductions) while suppressing situational checks (timing, mana, can't-cast statics). Threaded a for_display flag through prepare_spell_cast_with_variant_override_inner.
  • Granted keyword parser. Keyword::from_str(\"affinity for creatures\") fell through to Keyword::Unknown(...), so the granted-keyword path on Witherbloom's static silently no-op'd at cast time (CR 702.41a). Added an \"affinity for \" prefix-strip branch routing through the same parse_affinity_type helper used by the colon-separated form.
  • Affected filter scope. parse_spells_have_keyword collapsed compound type prefixes ("instant and sorcery") to TypedFilter::card(), broadening the granted keyword to every spell type — latent under bug chore: update coverage stats and badges #2, real otherwise. Resolved by using main's apply_spell_keyword_subject_constraints recursive helper, which preserves the TargetFilter::Or disjunction across both branches.

Class coverage: every "[type] spells you cast have [keyword]" card with a compound type prefix or a granted Affinity (Auriok Sunchaser-class artifact statics, Crucible-class graveyard land statics, etc.).

2. Sprout Swarm — Convoke creatures get the mana-tap ring

isManaObjectAction only matched TapLandForMana and Mana-typed ActivateAbility. During WaitingFor::ManaPayment { convoke_mode: Some(_) }, GameBoard's mana-tappable iteration skipped creatures because their actions didn't classify as mana, and PermanentCard's click handler gated dispatch on isActivatable || canTapForMana, both false for convoke creatures. Net effect: the convoke prompt appeared but no creature could be tapped.

Classified TapForConvoke as a mana action so the same code path that routes land taps now routes convoke (and waterbend) taps. Also added the missing TapForConvoke variant to the hand-maintained GameAction union in adapter/types.ts.

3. Crafty Cutpurse — Token controller redirect replacement

Pre-fix the trigger parsed as Effect::Unimplemented, so even though the replacement pipeline could express the redirect it never got installed and opponent-controlled token creation resolved normally (violating CR 111.2).

Three architectural pieces:

  1. token_owner_redirect: Option<ControllerRef> on ReplacementDefinition. Pairs with the existing token_owner_scope: scope matches which CreateToken events fire, redirect rewrites the proposed owner on the modified event. You resolves to the source's controller; Opponent finds the first non-source-controller player (representable but not used by Magic). Threaded through create_token_applier so the shadowed owner flows into the final ApplyResult::Modified and into the Chatterfang-style additional_token_spec recursive event.
  2. replacement_targets understands TargetFilter::SelfRef (CR 201.5), so the Crafty Cutpurse self-install can anchor the replacement on its own source without depending on having explicit targets.
  3. Parser try_parse_token_controller_redirect recognizes "each token that would be created under control this turn is created under control instead" — built on nom combinators, accepts "each/any" + "an opponent's"/"your opponents'"/"your" on both sides, marks the replacement with expiry: Some(RestrictionExpiry::EndOfTurn) ("this turn").

Known limitation: the self-installed replacement lives on Cutpurse's replacement_definitions, which find_applicable_replacements only scans for Battlefield/Command zone objects. If Cutpurse leaves the battlefield mid-turn the redirect stops, which is technically incorrect ("this turn" should persist). A floating-replacement anchor would close that gap; out of scope here.

Review-cycle corrections folded in

  • CR 111.6 → CR 111.2 across the six sites that cited the wrong rule. CR 111.2 is the rule the redirect actually overrides ("the token enters the battlefield under that player's control"); CR 111.6 is about tokens being subject to permanent-affecting effects.
  • CR 109.5 → CR 201.5 for the SelfRef-anchoring comment in replacement_targets. CR 109.5 is about you/your pronoun resolution; CR 201.5 is the self-name reference rule.
  • to_ctrl alt ordering: bare "your" was first, so "your opponents'" would match the 4-byte prefix and strand " opponents'" before the next tag. Mirrored from_ctrl's longer-prefix-first order.
  • spec.controller follows redirected owner in create_token_applier. The apply path uses spec.controller (not owner) for combat::enter_attacking, so a redirected enters-attacking token (Goblin Rabblemaster class) would compute its defender against the original effect controller and end up attacking its new controller. Regression test exercises a tapped: true, enters_attacking: true Goblin under Cutpurse.

Verification

  • cargo test -p engine --lib: 7194 / 7194 pass (1 ignored).
  • cargo clippy --workspace --all-targets -- -D warnings: clean.
  • cargo fmt --all --check: clean.
  • Coverage regression vs main: 0 engine regressions, 0 coverage-honesty regressions; +2 cards gained (Crafty Cutpurse, Red Herring); swallowed-clause diagnostics down 1633 → 1624. 3 baseline-true flips are MTGJSON oracle-text rewordings (Fast, Very Cryptic Command, Artist Alley), not engine regressions.

Test plan

  • CI: rust fmt + clippy + workspace tests
  • CI: coverage-regression check vs preview baseline
  • CI: frontend lint + type-check (note: pre-existing useTransitions TS gap on App.tsx unrelated to this branch)
  • Manual: cast Witherbloom, the Balancer from the command zone, verify the Affinity-reduced cost is displayed on the commander card before mana is available
  • Manual: cast Sprout Swarm with Convoke, verify creatures show the mana-tap ring and can be tapped for the convoke cost
  • Manual: with Crafty Cutpurse on the battlefield, have an opponent cast Treasure-generating or Saproling-generating spells and verify the tokens enter under your control

🤖 Generated with Claude Code

AgilErck and others added 5 commits May 14, 2026 20:20
Three independent fixes for Witherbloom, the Balancer (and the cards
sharing each pattern):

1. Display cost for command-zone commanders and unaffordable hand spells.
   `legal_actions_full` previously populated `spell_costs` only for
   spells with a current `CastSpell` action — so a commander you can't
   yet afford fell back to the printed `{5}{B}{G}` in the UI instead of
   the Affinity-reduced cost. Added `display_spell_cost`, an engine-
   authoritative entry point that runs the full cost-modification
   pipeline (Affinity, ReduceCost/RaiseCost, commander tax, pending
   reductions) while suppressing situational checks (timing, mana,
   can't-cast statics). Threaded a `for_display` flag through
   `prepare_spell_cast_with_variant_override_inner` so cost calc lives
   in one shared pipeline.

2. Parser keyword: `Keyword::from_str("affinity for creatures")` fell
   through to `Keyword::Unknown(...)`, so the granted-keyword path on
   Witherbloom's static silently no-op'd at cast time (CR 702.41a).
   Added an "affinity for " prefix-strip branch alongside the existing
   "hexproof from " arm, routing through the same parse_affinity_type
   helper.

3. Parser affected filter: `parse_spells_have_keyword` collapsed
   `parse_type_phrase("Instant and sorcery")` (TargetFilter::Or) to
   `TypedFilter::card()`, broadening the granted keyword to every
   spell type (CR 113.3a violation, latent — masked by bug 2 but real).
   Now collects inner TypedFilters from both Typed and Or shapes,
   applies controller/zone/MV qualifiers to each, and re-wraps as
   TargetFilter::Or when there are multiple branches.

Also adds a passing regression test for Snuff Out (paid 4 life →
destroy target nonblack creature → target in graveyard end-to-end)
proving that bug report was a non-engine issue.

Regression tests added:
- parser::oracle_static::tests::
    static_instant_and_sorcery_spells_have_affinity_for_creatures
- game::casting::tests::
    affinity_for_creatures_on_commander_in_command_zone_reduces_generic
- game::casting::tests::
    witherbloom_grants_affinity_to_instant_and_sorcery_spells
- game::casting::tests::
    snuff_out_alt_cost_paid_resolves_destroy_on_chosen_target
- game::casting::tests::
    snuff_out_from_card_database_alt_cost_destroys_target
- ai_support::tests::
    spell_costs_include_commander_affinity_reduction_without_castability
- ai_support::tests::
    spell_costs_apply_granted_affinity_from_battlefield_static

cargo test -p engine --lib: 6319/6319 pass.
cargo clippy --all-targets -- -D warnings: clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Tapping a creature for convoke (CR 702.51a) pays the spell's mana cost,
but `isManaObjectAction` only matched `TapLandForMana` and Mana-typed
ActivateAbility actions. As a result:

- During `WaitingFor::ManaPayment { convoke_mode: Some(_) }`, GameBoard
  iterates `legalActionsByObject` and only populates `activatableObjectIds`
  for Priority — not ManaPayment. The mana-tappable iteration runs for
  ManaPayment too, but only added creatures whose actions classified as
  mana. So convoke creatures got neither ring.
- PermanentCard's click handler gated dispatch on `isActivatable ||
  canTapForMana`. Convoke creatures had both false, so the click fell
  through to selection/inspection.

Net effect: the convoke prompt appeared but no creature could be tapped.

Classified `TapForConvoke` as a mana action so the same code path that
routes land taps now routes convoke taps. For creatures producing more
than one mana option (e.g., a green creature can tap for {C} or {G}),
the multi-mana-choice modal flow already handles selection.

Also adds `TapForConvoke` to the hand-maintained `GameAction` union in
`adapter/types.ts` — it was missing entirely, so any TS code switching
on the action variant treated it as unreachable.

Regression test extends the existing `isManaObjectAction` suite to
require both Colorless and color-specific TapForConvoke actions classify
as mana.

pnpm test -- --run: 694 pass / 18 todo.
pnpm run type-check: clean.
pnpm lint: clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-fix the trigger parsed as `Effect::Unimplemented`, so even though
the replacement pipeline could express the redirect it never got
installed and opponent-controlled token creation resolved normally
(violating CR 111.6).

Three architectural pieces, in order of generality:

1. **`token_owner_redirect: Option<ControllerRef>` on
   `ReplacementDefinition`** (CR 111.6). Pairs with the existing
   `token_owner_scope` filter: `scope` matches which CreateToken events
   fire the replacement, `redirect` rewrites the proposed `owner` on
   the modified event. `You` resolves to the replacement source's
   controller; `Opponent` finds the first non-source-controller player
   (representable but not used by Magic). Threaded through
   `create_token_applier` so the shadowed `owner` flows into the final
   `ApplyResult::Modified` *and* into the Chatterfang-style
   `additional_token_spec` recursive event (so a Cutpurse + appender
   stack redirects both batches in lockstep — per CR rulings).

2. **`replacement_targets` understands `TargetFilter::SelfRef`**.
   Previously the helper only handled `Any` (use ability targets) and
   event-context filters via `resolve_event_context_target`. Crafty
   Cutpurse self-installs the replacement on its own source object
   (CR 109.5 anchors SelfRef to the ability source), so the trigger
   needed a path that doesn't depend on having explicit targets.

3. **Parser:
   `try_parse_token_controller_redirect`** recognizes
   "each token that would be created under <X> control this turn is
   created under <Y> control instead" — built on nom combinators,
   accepts "each/any" + "an opponent's"/"your opponents'"/"your" on
   both sides, marks the replacement `expires_at_eot: true` ("this
   turn"). Wired into the effect dispatcher alongside the damage
   replacement parsers.

Known limitation (acceptable for first pass; rare in actual play):
the self-installed replacement lives on Cutpurse's
`replacement_definitions`, which `find_applicable_replacements` only
scans for Battlefield/Command zone objects. If Cutpurse leaves the
battlefield mid-turn, the redirect stops — Magic-wise the effect
should persist "this turn" regardless. A floating-replacement anchor
(synthetic command-zone object marked transient-until-EOT) would close
that gap; out of scope here.

Regression coverage:
- `parser::oracle_effect::snapshot_tests::
    crafty_cutpurse_oracle_text_parses_to_token_controller_redirect`
- `game::effects::add_target_replacement::tests::
    crafty_cutpurse_self_install_redirects_opponent_tokens_to_controller`
- `game::effects::add_target_replacement::tests::
    crafty_cutpurse_does_not_redirect_own_tokens`

cargo test -p engine --lib: 6322/6322 pass.
cargo clippy --all-targets -- -D warnings: clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five corrections surfaced by /review-impl:

1. CR 111.6 → CR 111.2 (HIGH). The token controller redirect overrides
   CR 111.2 ("The token enters the battlefield under that player's
   control"), not CR 111.6 ("A token is subject to anything that affects
   permanents"). Fixed across the six sites in replacement.rs, ability.rs,
   parser/oracle_effect/mod.rs (parser + dispatcher + snapshot comment),
   and add_target_replacement.rs.

2. CR 109.5 → CR 201.5 (MED). SelfRef in `replacement_targets` anchors
   text-referring-to-the-object-by-name; that's CR 201.5, not the
   you/your pronoun rule.

3. CR 113.3a citation dropped (LOW). The parse_spells_have_keyword
   compound-type-prefix fix referenced CR 113.3a (spell-abilities
   resolution) for a static-affected-scope claim. The design comment
   stands without a wrong rule cite.

4. to_ctrl alt ordering (MED). `try_parse_token_controller_redirect`
   listed `tag("your")` before `tag("your opponents'")`, so
   "created under your opponents' control instead" matched the 4-byte
   "your" and stranded " opponents'" before the next tag. Mirrored
   from_ctrl's longer-prefix-first ordering.

5. spec.controller follows redirected owner (MED). create_token_applier
   redirected the proposed event's `owner` but left `spec.controller`
   pointing at the original effect controller. The apply path uses
   `spec.controller` (not `owner`) in combat::enter_attacking for the
   defending-player lookup, so an enters-attacking Goblin Rabblemaster
   token under Crafty Cutpurse would compute its defender against the
   original opponent and end up attacking its new controller. Now the
   spec controller follows the redirected owner whenever the redirect
   actually fires.

Regression test added:
- game::effects::add_target_replacement::tests::
    crafty_cutpurse_redirects_spec_controller_for_enters_attacking_token
  Existing tests extended to assert spec.controller alongside owner.

cargo test -p engine --lib: 6323/6323 pass.
cargo clippy -p engine --all-targets -- -D warnings: clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two upstream refactors landed during the 353-commit gap:

1. `ReplacementDefinition.expires_at_eot: bool` was unified into
   `expiry: Option<RestrictionExpiry>`. The end-of-turn cleanup in
   `turns.rs::expires_at_eot` predicate now matches
   `RestrictionExpiry::EndOfTurn` directly.

2. `TokenSpec`'s flat characteristic fields (display_name, power,
   toughness, core_types, subtypes, supertypes, colors, keywords) were
   pulled into a nested `characteristics: TokenCharacteristics` struct
   shared with `TokenPreset` and the debug `CreateToken` payload.

The Crafty Cutpurse parser, applier-side EOT flag, snapshot test, and
the three regression tests in add_target_replacement.rs now use the
new shapes. No behavior change.

cargo test -p engine --lib: 7194/7194 pass.
cargo clippy --workspace --all-targets -- -D warnings: clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces support for 'Affinity for [type]' cost reductions and 'Crafty Cutpurse' token redirection, refactoring the casting pipeline to provide display-ready costs and adding TargetFilter::SelfRef for replacements. Feedback highlights several style guide violations, including an R1 violation for using strip_prefix in parsing, an R2 violation for using a bool flag in casting logic, and an L1 violation regarding the Opponent redirect logic. The reviewer also noted duplicated castable zone logic and missing round-trip tests for the new TapForConvoke action.

Comment thread crates/engine/src/types/keywords.rs Outdated
Comment thread client/src/adapter/types.ts
Comment thread crates/engine/src/ai_support/mod.rs Outdated
Comment thread crates/engine/src/game/casting.rs Outdated
Comment thread crates/engine/src/game/replacement.rs Outdated
…used branch, test

- Replace `for_display: bool` parameter with typed `CastingMode` enum (`Actual` /
  `Display`) in `prepare_spell_cast_with_variant_override_inner` and all callers,
  eliminating the bool-flag R2 violation flagged by the reviewer.

- Replace `strip_prefix("affinity for ")` dispatch in `Keyword::from_str` with
  `split_once(' ')` routed through the existing `parse_affinity_type` path, keeping
  the grant-keyword form ("affinity for creatures") consistent with the colon-split
  dispatch path and removing the R1 strip_prefix-for-dispatch complaint.

- Remove the `ControllerRef::Opponent` arm from the token owner-redirect match in
  `replacement.rs`. No current card uses this form; the naive "first non-source player"
  implementation is wrong in multiplayer, so we fail-open (preserve original owner)
  and document why, per the L1 building-block violation note.

- Eliminate the manual `castable_zone` guard (duplicated zone logic) from the
  `spell_costs` loop in `ai_support/mod.rs`. `display_spell_cost` already returns
  `None` for non-castable objects, making the manual Hand/Command check redundant.
  Remove the now-unused top-level `Zone` import.

- Add a `TapForConvoke` action round-trip test to `protocol.test.ts`, exercising the
  new `GameAction` variant through the P2P wire encoding/decoding path.
- ai_support: drop `obj.controller != player` pre-filter on spell_costs
  walk. Foreign-cast permissions (Etali / Dire Fleet Daredevil / Light-Paws
  CastFromZone) attach to opponent-controlled objects; the controller
  filter silently dropped their display cost. `display_spell_cost` is the
  engine-authoritative source for eligibility — let it adjudicate. Replace
  with a cheap castable-zone pre-filter (Hand/Command/Exile/Graveyard/
  Library) for performance only.

- oracle_effect: restrict `try_parse_token_controller_redirect` to_ctrl
  to `your`. No printed card redirects token creation to an opponent and
  the runtime in replacement.rs does not implement that semantics —
  parser/runtime contract must stay symmetric.

- replacement: tighten comment on the non-`You` ControllerRef fallthrough
  now that the parser cannot produce `Opponent`. Drops the CR 109.4
  citation that was a stretch for the multiplayer reasoning.

- cardActionChoice: fix CR annotation — waterbend is CR 701.67, not
  bundled under 702.51a.
@matthewevans
matthewevans enabled auto-merge (squash) May 15, 2026 14:58
@matthewevans
matthewevans merged commit 260d6d0 into phase-rs:main May 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants