Skip to content

Fix bare color-category & historic spell-cost modifiers dropping their filter (Herald of Kozilek, Ugin, Urza's Filter) - #6150

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
minion1227:minion_colorless_spell_cost
Jul 18, 2026
Merged

Fix bare color-category & historic spell-cost modifiers dropping their filter (Herald of Kozilek, Ugin, Urza's Filter)#6150
matthewevans merged 3 commits into
phase-rs:mainfrom
minion1227:minion_colorless_spell_cost

Conversation

@minion1227

Copy link
Copy Markdown
Contributor

Summary

Fixes the dropped-filter misparse for bare color-category & "historic" spell-cost modifiers.

"Colorless spells you cast cost {N} less to cast" (Herald of Kozilek, Ugin, the Ineffable, It That Heralds the End), "Multicolored spells cost {N} less to cast" (Urza's Filter), and "Historic spells you cast cost {N} less" (Jhoira's Familiar) all parsed with spell_filter: None — the color-category / historic restriction was silently dropped, so the modifier wrongly cheapened every spell instead of only the named category.

Root cause: the bare-word fallback in parse_cost_mod_spell_type_prefix (static_helpers.rs) hand-rolled only the five named colors via parse_named_color. A bare colorless/monocolored/multicolored matched neither that nor parse_bare_supertype_spell_filter, so the whole filter returned None. (The noun-bearing path — "Colorless creature spells" — already produced the correct ColorCount via parse_type_phrase.)

Fix (single-authority parameterization): route the bare-word subject through one parse_bare_spell_subject_filter authority that resolves the color word via the existing nom_filter::parse_color_property combinator, so the color-CATEGORY axis resolves identically to the noun-bearing path — colorless → ColorCount{EQ,0}, monocolored → {EQ,1}, multicolored → {GE,2}, named color → HasColor — plus historicFilterProp::Historic and the pre-existing bare-supertype path. This replaces the partial parse_named_color special-case with the complete color-property authority and composes with the trailing mana-value qualifier (It That Heralds the End → ColorCount + Cmc). Parse-only: FilterProp::ColorCount and Historic already exist and are evaluated by the spell-cost filter path.

Parse impact (before → after)

Card Oracle Before After
Herald of Kozilek / Ugin "Colorless spells you cast cost {N} less" None (all spells) ColorCount{EQ,0}
Urza's Filter "Multicolored spells cost {2} less" None ColorCount{GE,2}
It That Heralds the End "Colorless spells … with mana value 7 or greater cost {1} less" Cmc{GE,7} only (colorless dropped) ColorCount{EQ,0} + Cmc{GE,7}
Jhoira's Familiar "Historic spells you cast cost {1} less" None Historic

Files changed

  • crates/engine/src/parser/oracle_static/static_helpers.rs
  • crates/engine/src/parser/oracle_static/tests.rs
  • crates/engine/tests/integration/colorless_spell_cost_reduction.rs
  • crates/engine/tests/integration/main.rs
  • docs/parser-misparse-backlog.md

CR references

  • CR 105.2 — an object's colors; a colorless object has zero colors (ColorCount{EQ,0})
  • CR 205.4a — supertypes (the bare-supertype fallback)
  • CR 700.6 — "historic" = legendary supertype, artifact card type, or Saga subtype
  • CR 601.2f — cost determination (cost reductions apply here)

Implementation method (required)

Method: not-applicable — targeted single-function parser fix in an existing dense handler (static_helpers.rs). Implemented directly, then an independent fresh-context review-impl pass (§5.3) was run on the committed diff (result in Final review-impl below).

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high

Verification

  • Required checks ran clean, or the exact CI-owned alternative is stated below.

  • Gate A output below is for the current committed head.

  • Final review-impl below is clean for the current committed head.

  • Both anchors cite existing analogous code at the same seam.

  • cargo fmt --all — clean (exit 0)

  • cargo test -p engine --lib parser::oracle_static1195 passed; 0 failed (full parser module incl. snapshot tests)

  • cargo test -p engine --lib cost_mod_bare_color_category_and_historic_subject — passed (new parser unit test over the full bare-subject axis + named-color/supertype regression + MV composition)

  • cargo test -p engine --test integration colorless_spell_cost_reduction3 passed; 0 failed (AST + runtime cast-cost differential: colorless discounted, colored NOT)

  • cargo clippy -p engine --all-targets --features proptest — clean, exit 0, no warnings

  • Gate A coverage/semantic-audit (cargo coverage, cargo semantic-audit): not run locally — both read client/public/card-data.json, which is stale in this checkout, and the audit binaries currently drift against the card-data schema. CI owns these checks.

Gate A

Gate A PASS head=81509d05d8cabda8e8a957db03da50c7f07e5aff base=0d7783c4e93dc364761735663348bbc03b1a6300

Anchored on

  • crates/engine/src/parser/oracle_static/static_helpers.rs:46 (parse_bare_supertype_spell_filter) — the sibling bare-word fallback this new authority mirrors and composes.
  • crates/engine/src/parser/oracle_nom/filter.rs:432 (parse_color_property) — the pre-existing single color-property authority the fix routes through (colorless/monocolored/multicolored → ColorCount, named colors → HasColor).
  • crates/engine/src/game/filter.rs:3261 — the spell-cost filter path already evaluates FilterProp::ColorCount and FilterProp::Historic, so no runtime/type change was needed (the working noun-path "Colorless creature spells" resolves through the same authority).

Final review-impl

Final review-impl PASS head=81509d05d8cabda8e8a957db03da50c7f07e5aff

An independent fresh-context review-impl was run on the committed diff (handed only the unified diff + CLAUDE.md). Result: no defects. Confirmed: correct seam (the bare-subject fallback of parse_cost_mod_spell_type_prefix); genuine reuse of the single parse_color_property authority, behavior-preserving for the five named colors (parse_named_color still used elsewhere, not left dead); nom-mandate compliant (lower == "historic" is a single CR 700.6 game-term equality, not a verbatim Oracle-text match); all four CR citations real and authorizing; correct composition with the trailing mana-value qualifier (ColorCount{EQ,0} + Cmc{GE,7} for It That Heralds the End); no false positives (non-category words still fall through to None); mixed-case safe (the helper lowercases its own input); and the runtime cast-cost differential is genuinely revert-failing and non-vacuous (on revert the None filter matches every spell, dropping the {2}{R} spell from mv 3 to 2). Backlog hygiene verified internally consistent.

Claimed parse impact

  • Herald of Kozilek
  • Ugin, the Ineffable
  • Urza's Filter
  • It That Heralds the End
  • Jhoira's Familiar (parse of the cost line is fixed; the card retains a separate, unrelated unimplemented ability, so it stays in the backlog under root cause chore: update coverage stats and badges #23)

Validation Failures

None.

CI Failures

None.

…s to their filter (Herald of Kozilek, Ugin, Urza's Filter)

"Colorless spells you cast cost {N} less to cast" (Herald of Kozilek, Ugin,
the Ineffable, It That Heralds the End), "Multicolored spells cost {N} less
to cast" (Urza's Filter), and "Historic spells you cast cost {N} less"
(Jhoira's Familiar) all parsed with `spell_filter: None` — the color-category
/ historic restriction was silently dropped, so the modifier (mis)applied to
EVERY spell instead of only the named category.

Root cause: the bare-word fallback in `parse_cost_mod_spell_type_prefix`
(static_helpers.rs) hand-rolled only the five NAMED colors via
`parse_named_color`. A bare "colorless"/"monocolored"/"multicolored" matched
neither that nor `parse_bare_supertype_spell_filter`, so the whole filter
returned `None` (the noun-bearing path — "Colorless CREATURE spells" — already
produced the correct `ColorCount` via `parse_type_phrase`).

Fix: route the bare-word subject through a single `parse_bare_spell_subject_filter`
authority that resolves the color word via the existing
`nom_filter::parse_color_property` combinator — so the color-CATEGORY axis
resolves identically to the noun-bearing path (colorless → ColorCount{EQ,0},
monocolored → {EQ,1}, multicolored → {GE,2}, named color → HasColor) — plus
"historic" (FilterProp::Historic) and the pre-existing bare-supertype path. This
replaces the partial `parse_named_color` special-case with the complete
color-property authority; it composes with the trailing mana-value qualifier
(It That Heralds the End → ColorCount + Cmc). Parse-only: FilterProp::ColorCount
and Historic already exist and are evaluated by the spell-cost filter path.

CR 105.2 (object colors; colorless = zero colors) / CR 700.6 (historic) /
CR 205.4a (supertypes) / CR 601.2f (cost determination).

Tests: parser unit test over the full bare-subject axis (colorless/mono/multi/
historic + named-color & supertype regression + MV composition); runtime
cast-cost differential driving parse -> cast -> cost determination — a colorless
spell gets the {1} discount and a colored spell does NOT (revert-failing: before
the fix the filter was None and every spell was discounted).

Backlog: removed It That Heralds the End and Urza's Filter from root cause phase-rs#1
(both now parse fully clean); decremented the associated counts.

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

@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 resolves an issue where bare-word spell-subject filters for cost modifiers (such as "colorless", "historic", or "legendary") were being dropped, causing cost reductions to be incorrectly applied to all spells. It introduces parse_bare_spell_subject_filter to handle these cases and adds corresponding integration tests. Feedback on the changes suggests using nom combinators instead of verbatim string equality for parsing "historic" to adhere to architectural rules, and passing the trimmed, lowercased string to parse_bare_supertype_spell_filter to prevent potential parsing failures from leading or trailing whitespace.

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 thread crates/engine/src/parser/oracle_static/static_helpers.rs
Comment thread crates/engine/src/parser/oracle_static/static_helpers.rs Outdated
@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans matthewevans self-assigned this Jul 18, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 18, 2026

@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.

Approved after maintainer review: the cost-modifier filter now uses the existing color-property authority, preserves the historic/supertype paths, and the registered runtime regression distinguishes colorless from colored spells. Current-head CI and parse-diff evidence are clean.

@matthewevans
matthewevans added this pull request to the merge queue Jul 18, 2026
@matthewevans matthewevans removed their assignment Jul 18, 2026
Merged via the queue into phase-rs:main with commit 1bbf603 Jul 18, 2026
13 checks passed
@minion1227
minion1227 deleted the minion_colorless_spell_cost branch July 20, 2026 11:58
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