Skip to content

feat(parser): recognize "Players may spend mana as though it were mana of any color"#5558

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_players_spend_any_color
Jul 11, 2026
Merged

feat(parser): recognize "Players may spend mana as though it were mana of any color"#5558
matthewevans merged 1 commit into
phase-rs:mainfrom
minion1227:minion_players_spend_any_color

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

CR 609.4b

Bug

Mycosynth Lattice and Mycosynthwave read "Players may spend mana as though it were mana of any color" — the all-players form of the board-wide any-color concession. But the dispatcher only matched the controller form ("You may spend mana …") via a verbatim string == check, so the "Players may" subject dropped the entire static and no player gained the permission.

Fix

Replace the verbatim == match with a nom recognizer parse_board_wide_spend_any_color (mirroring the existing is_speed_unlock_sentence helper) that accepts both the "You may" and "Players may" subject prefixes on the bare form. Both map to the same SpendManaAsAnyColor static with affected: TargetFilter::Player, which check_static_ability already scopes to every player — so no runtime change is needed.

Why it's the right seam / minimal blast radius

  • Removes a prohibited verbatim-string dispatch (tp.lower.trim_end_matches('.') == "…") in favor of a combinator — the same idiom the sibling is_speed_unlock_sentence uses.
  • all_consuming keeps the recognizer to the bare form; the qualified tails ("… to activate abilities of X", "… to cast it") are consumed by earlier arms (try_parse_spend_any_color_to_activate_abilities, the type_change "to cast it" grant) and never reach here — verified unchanged.

Tests

  • static_players_may_spend_mana_as_any_color — "Players may …" → SpendManaAsAnyColor { .. } with affected: Some(TargetFilter::Player).
  • static_you_may_spend_mana_as_any_color_still_parses — regression: the "You may" form still parses to the same shape.
  • Full parser::oracle_static::tests (1047 passed, 0 failed), including the "… to activate abilities of creatures you control" activation-source-filtered case.

CR verification

  • CR 609.4b — spending mana as though it were another type/color — verified in docs/MagicCompRules.txt.

AI-contributor notes

  • Rules-correct: every player gains the any-color concession per CR 609.4b; the runtime already broadcasts TargetFilter::Player to all players.
  • Builds the class: Mycosynth Lattice + Mycosynthwave, and any future "Players may spend mana …" board-wide static.
  • Verified: live-parsed "Players may" / "You may" / "You may … to activate abilities of X" before/after; cargo fmt, CI-exact clippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static module are green.

🤖 Generated with Claude Code

…a of any color"

CR 609.4b: Mycosynth Lattice and Mycosynthwave read "Players may spend mana as
though it were mana of any color" — the all-players form of the board-wide
any-color concession. The dispatcher only matched the controller form ("You
may spend mana …") via a verbatim string `==` check, so the "Players may"
subject dropped the entire static and no player gained the permission.

Fix: replace the verbatim `==` match with a nom recognizer
`parse_board_wide_spend_any_color` (mirroring `is_speed_unlock_sentence`) that
accepts both the "You may" and "Players may" subject prefixes on the bare form
(no activation-source / spell-class qualifier tail). Both map to the same
`SpendManaAsAnyColor` static with `affected: TargetFilter::Player`, which
`check_static_ability` already scopes to every player — so no runtime change is
needed. The tailed forms ("… to activate abilities of X", "… to cast it") are
consumed by earlier arms and never reach this recognizer.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@minion1227
minion1227 requested a review from matthewevans as a code owner July 11, 2026 07:21

@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 a parser helper parse_board_wide_spend_any_color to match board-wide mana-spending concessions (such as Chromatic Orrery and Mycosynth Lattice) and replaces a verbatim string check. However, the feedback points out a critical rule fidelity issue: mapping both 'you may' and 'players may' to the same all-players concession violates the Magic Comprehensive Rules (CR 109.5 and CR 609.4b), as 'you may' should only apply to the controller. The reviewer suggests refactoring the parser to return the correct TargetFilter and properly propagate parser errors.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +100 to +113
fn parse_board_wide_spend_any_color(lower: &str) -> bool {
all_consuming(terminated(
preceded(
alt((
tag::<_, _, OracleError<'_>>("you may "),
tag("players may "),
)),
tag("spend mana as though it were mana of any color"),
),
opt(tag(".")),
))
.parse(lower)
.is_ok()
}

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.

high

[HIGH] Incorrect player scoping for Chromatic Orrery vs Mycosynth Lattice. Evidence: crates/engine/src/parser/oracle_static/dispatch.rs:100.

Why it matters: Mapping both "You may" (Chromatic Orrery) and "Players may" (Mycosynth Lattice) to TargetFilter::Player allows all players to spend mana as any color under Chromatic Orrery, violating CR 109.5 and CR 609.4b.

Suggested fix: Refactor the parser to return the correct TargetFilter (TargetFilter::Controller for "you may" and TargetFilter::Player for "players may"). Decompose the compound phrase into modular, reusable parsers for constituent parts (subject and action) to avoid verbatim string matching. Additionally, ensure unexpected parser errors (like nom::Err::Failure) are propagated instead of being swallowed by .ok().

Suggested change
fn parse_board_wide_spend_any_color(lower: &str) -> bool {
all_consuming(terminated(
preceded(
alt((
tag::<_, _, OracleError<'_>>("you may "),
tag("players may "),
)),
tag("spend mana as though it were mana of any color"),
),
opt(tag(".")),
))
.parse(lower)
.is_ok()
}
fn parse_board_wide_spend_any_color(lower: &str) -> Result<Option<TargetFilter>, nom::Err<OracleError<'_>>> {
let mut parser = all_consuming(terminated(
pair(
alt((
value(TargetFilter::Controller, tag("you")),
value(TargetFilter::Player, tag("players")),
)),
tag(" may spend mana as though it were mana of any color"),
),
opt(tag(".")),
));
match parser.parse(lower) {
Ok((_, (filter, _))) => Ok(Some(filter)),
Err(nom::Err::Error(_)) => Ok(None),
Err(e) => Err(e),
}
}
References
  1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Convenience shortcuts that get rules wrong are not simpler; they are wrong. (link)
  2. Avoid swallowing all errors (e.g., using .ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.
  3. Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main 824e7e03f897)

2 card(s) · static/SpendManaAsAnyColor · added: SpendManaAsAnyColor (affects=player)

Examples: Mycosynth Lattice, Mycosynthwave

2 card(s) · ability/unknown · removed: unknown

Examples: Mycosynth Lattice, Mycosynthwave

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Verdict: clean, functional fix — anti-verbatim-string refactor that correctly delivers the all-players concession. Holding only for CI.

✅ Clean

  • Right refactor. Replaces a verbatim == "you may spend mana as though it were mana of any color" string match with a parse_board_wide_spend_any_color nom recognizer (all_consuming(terminated(preceded(alt((tag("you may "), tag("players may "))), tag("spend mana as though it were mana of any color")), opt(tag("."))))), mirroring the existing is_speed_unlock_sentence helper. This is exactly the anti-verbatim-string mandate — the subject is now one alt() axis, so both Mycosynth Lattice/Mycosynthwave ("Players may") and the controller form share one recognizer.
  • Functional, not cosmetic. Verified the produced shape actually grants to every player: affected: Some(TargetFilter::Player)static_filter_matches (static_abilities.rs:1819) returns true for all player_id on the TargetFilter::Player arm ("All players match"). So opponents genuinely gain the any-color concession, which is the #5290-class bug being fixed.
  • CR 609.4b grep-verified verbatim ("allows a player to spend mana 'as though it were mana of any [type or color]'").
  • Discriminating testsstatic_players_may_spend_mana_as_any_color asserts the mode + affected == Some(TargetFilter::Player) (fails on the pre-fix drop), plus a static_you_may_...still_parses regression guard.

🟡 Non-blocking (pre-existing, not introduced here)

  • The return block is unchanged, so the bare "You may spend mana as though it were mana of any color." form still maps to affected: TargetFilter::Player (all players). For a genuinely controller-scoped bare "You may" card that would over-grant to opponents (TargetFilter::Controller would be the correct affected filter). In practice the tailed forms (Chromatic Orrery's "… to cast spells") are consumed by earlier arms and never reach here, so this is latent, not a regression. Worth a follow-up but out of scope for this PR.

Recommendation: approve as bug on settled green.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approve — clean anti-verbatim-string refactor; the board-wide any-color concession now reaches all players.

✅ Clean

  • Right seam / anti-verbatim. Replaces the == "you may spend mana…" verbatim string check with parse_board_wide_spend_any_color, a nom recognizer folding the you may / players may subject into a single alt() axis (mirrors is_speed_unlock_sentence). This is exactly the decomposition the parser mandate calls for — no card-name or full-sentence ==.
  • Functional, not cosmetic. affected: TargetFilter::Player routes through static_filter_matches → the "all players match" arm (static_abilities.rs:1819), so Mycosynth Lattice / Mycosynthwave genuinely grant every player the "spend mana as though any color" permission — the "Players may" subject no longer drops the entire static.
  • CR 609.4b grep-verified for the mana-spending permission.
  • Discriminating tests + a regression guard on the dropped-subject path.

🟡 Non-blocking (pre-existing, not a regression)

  • The unchanged return still maps a bare "You may" form to all-players scope. Latent in the prior code too; out of scope here.

Recommendation: approve and enqueue as bug. CI is fully green (both Rust shards, lint, coverage-gate, card-data) on the reviewed head.

@matthewevans matthewevans added the bug Bug fix label Jul 11, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 11, 2026
Merged via the queue into phase-rs:main with commit 2326c9d Jul 11, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_players_spend_any_color branch July 18, 2026 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants